refactor(fx): consume shared backend rate policy - #2607
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesThe change moves display-rate fetching to the backend FX rate rollout
API contract expansion
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant useExchangeRate
participant fetchDisplayRate
participant PeanutAPI
Client->>useExchangeRate: Request display rate
useExchangeRate->>fetchDisplayRate: Fetch normalized currency pair
fetchDisplayRate->>PeanutAPI: GET /fx/rate without authentication
PeanutAPI-->>fetchDisplayRate: Validated rate or 400/404/503 response
fetchDisplayRate-->>useExchangeRate: Rate or typed error
useExchangeRate-->>Client: Display rate result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code-analysis diffPainscore total: 7079 → 7078.73 (-0.27) 🆕 New findings (376)
…and 356 more. ✅ Resolved (363)
…and 343 more. 📈 Painscore deltas (top movers)
|
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/utils/__tests__/fx.utils.test.ts (1)
74-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
implausibly small ratefixture does not isolateMIN_DISPLAY_RATE.
'0.0000000000000000001'has 19 decimal places.PLAIN_DECIMALallows at most 18, so the payload is rejected by the precision rule before the numeric bound is reached. The lower bound stays untested.Use a value with 18 decimals that is still below
1e-18.♻️ Proposed fixture change
- ['implausibly small rate', { ...validResponse, rate: '0.0000000000000000001' }], + // 18 decimals, so PLAIN_DECIMAL passes and MIN_DISPLAY_RATE is the rule under test. + ['implausibly small rate', { ...validResponse, rate: '0.000000000000000000' }],Note:
'0.000000000000000000'parses to0, which is belowMIN_DISPLAY_RATE. Pick any 18-decimal string under1e-18that suits the intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/__tests__/fx.utils.test.ts` at line 74, Update the “implausibly small rate” fixture in the rate validation tests to use a numeric string with exactly 18 decimal places that remains below MIN_DISPLAY_RATE, so it passes PLAIN_DECIMAL precision validation and specifically exercises the lower-bound check.src/utils/fx.utils.ts (1)
76-80: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout to the FX request.
fetchDisplayRateruns on the Next.js server for/api/exchange-rateand in native clients.apiFetchsupportstimeoutMs, but this call omits it. Ifapi.peanut.mestalls, the route handler holds the request until the platform limit.
src/utils/demo-api.tsalready uses a 10-second timeout for the native FX passthrough. Use the same bound here.♻️ Proposed fix to bound the outbound call
const response = await apiFetch(`/fx/rate?${query.toString()}`, { method: 'GET', includeAuth: false, credentials: 'omit', + timeoutMs: 10_000, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/fx.utils.ts` around lines 76 - 80, Update the apiFetch call in fetchDisplayRate to pass the timeoutMs option using the existing 10-second bound from src/utils/demo-api.ts, while preserving the current GET method, authentication, and credential settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/proxy.ts`:
- Around line 57-60: Update the exchange-rate route handler in
src/app/api/exchange-rate/route.ts so every 400, 404, and 500 error response
explicitly sets Cache-Control to no-store, while preserving the existing
s-maxage/stale-while-revalidate policy on successful responses.
---
Nitpick comments:
In `@src/utils/__tests__/fx.utils.test.ts`:
- Line 74: Update the “implausibly small rate” fixture in the rate validation
tests to use a numeric string with exactly 18 decimal places that remains below
MIN_DISPLAY_RATE, so it passes PLAIN_DECIMAL precision validation and
specifically exercises the lower-bound check.
In `@src/utils/fx.utils.ts`:
- Around line 76-80: Update the apiFetch call in fetchDisplayRate to pass the
timeoutMs option using the existing 10-second bound from src/utils/demo-api.ts,
while preserving the current GET method, authentication, and credential
settings.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b23aed96-a4b0-4245-93bd-4d9dcb5a89a6
⛔ Files ignored due to path filters (1)
src/types/api.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing (18)
docs/api-types.mdnext.config.jssrc/__tests__/proxy.test.tssrc/app/(mobile-ui)/add-money/[country]/bank/page.tsxsrc/app/api/exchange-rate/__tests__/route.test.tssrc/app/api/exchange-rate/route.tssrc/app/m/[slug]/MerchantLandingPage.tsxsrc/hooks/useExchangeRate.tssrc/proxy.tssrc/types/api.openapi.jsonsrc/utils/__tests__/api-fetch.test.tssrc/utils/__tests__/demo-api.test.tssrc/utils/__tests__/fx.utils.test.tssrc/utils/__tests__/sentry.utils.test.tssrc/utils/api-fetch.tssrc/utils/demo-api.tssrc/utils/fx.utils.tssrc/utils/sentry.utils.ts
💤 Files with no reviewable changes (1)
- next.config.js
|
Addressed all CodeRabbit findings in 5ade3d4: error responses are explicitly no-store, FX calls have a 10s timeout, and the lower-bound fixture now passes decimal-shape validation before exercising the numeric bound. Focused 47 tests, typecheck, changed ESLint, and formatting pass. |
|
@coderabbitai review |
|
|
Read #2607 together with peanut-api-ts#1277, since they ship as a pair. Direction looks right — it finishes the single display-rate path #2601 started, and the policy belongs behind one endpoint. Notes below, roughly in the order I would deal with them. All display-rate, no money movement. Worth a decision before merge
Smaller
How this pairs with #1275 / #2601Two direct overlaps.
Rest of the FX module reads well. |
The freshness check compared backend timestamps against the DEVICE clock with
a five-minute tolerance. A phone more than five minutes out rejected every
response the backend could send — permanently, and there is no local fallback
left to catch it. Device-clock comparisons now allow six hours; the checks
that actually bound staleness compare two backend timestamps and stay tight.
Sentry now skips 503 on /fx/rate. It means a provider leg is momentarily
absent, which peanut-api already reports with the upstream cause attached;
reporting it client-side multiplies one incident by every mounted hook and
its retries, and buries the signal that can be acted on.
Also gives /fx/rate a real demo-api handler. Without one a failed passthrough
fell to defaultShape and answered 200 {} — a contract violation dressed as a
success. A canned rate is not possible (handlers never see the query string,
and the validator rejects a mismatched pair), so it answers 503.
Retargeted from main to dev, so dev's locale redirect and native authReady gate had to land alongside the FX changes. proxy.ts: kept dev's locale block and Vary reasoning, carried over the /api/exchange-rate cache exemption. api-fetch.ts: authReady() now runs only when includeAuth is true. A public rate read sends no token either way, so making it queue behind auth hydration on a native cold start would delay it for nothing.
|
Peanut team has taken this branch over. Retargeted Addressed the client-side items from @abalinda's review: Drifted device clock (review point 3) — this was the sharpest one. The freshness check compared backend timestamps against the device clock with a 5-minute tolerance, so a phone more than 5 minutes out rejected every response the backend could send, permanently, with no local fallback left to catch it. Device-clock comparisons now allow 6 hours. The checks that actually bound staleness compare two backend-stamped times and stay tight, so this costs correctness nothing. Tests cover ±3h. 503 in the Sentry skip list — a 503 means a provider leg is momentarily absent, which peanut-api already reports with the upstream cause attached. Reporting it client-side multiplies one incident by every mounted hook and its retries (three on the merchant page alone) and buries the actionable backend signal.
Merge resolution worth a look: Deploy order is not optional — backend first. This PR drops All green, including |
3 participants
Summary
GET /fx/rate.Retry-After, avoid retrying deterministic/rate-limit outcomes, and keep expected outcomes out of Sentry.Why the UI should consume the same API
Split needs the backend contract, but moving the UI too is what makes that contract trustworthy: the production UI dogfoods the exact pair endpoint and validation rules that Split depends on. It removes a second implementation of provider/reference fallback, so a future provider, cache, or policy change cannot silently diverge between products.
This is not intended to change the rate a user sees. The API preserves the UI's existing atomic policy: use provider rates only when both legs are covered; otherwise use reference rates for both legs. PLN→EUR therefore remains reference/reference, with no hybrid source.
The actual operational change is that refresh/cache ownership moves to Peanut API. First-party clients call the public endpoint without cookies or bearer credentials; the legacy
/api/exchange-ratewrapper remains for compatibility with a bounded five-minute success cache and no cached failures.Rollout / review focus
reference_pair.QA
git diff --checkpass. Repository-wide ESLint remains advisory/red at its pre-existing 978-problem baseline; the only touched files it names are unchangedany/CommonJS lines in the existing Sentry utility/test.main; the final merge was conflict-free.Screenshots: N/A (no intended visible UI change).