refactor(fx): consume shared backend rate policy - #2607
Draft
0xkkonrad wants to merge 7 commits into
Draft
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 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 |
Contributor
Code-analysis diffPainscore total: 6346.7 → 6346.27 (-0.43) 🆕 New findings (375)
…and 355 more. ✅ Resolved (362)
…and 342 more. 📈 Painscore deltas (top movers)
|
Contributor
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
Contributor
Author
|
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. |
Contributor
Author
|
@coderabbitai review |
Contributor
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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).