refactor: TASK-21854 logic/UI separation — extract inline-state pages and monoliths onto features/ - #3007
Open
kushagrasarathe wants to merge 69 commits into
Open
Conversation
Named screen ids in the URL via nuqs (never indexes), entry guards with fallbacks for refresh/deep-link into a step whose prerequisites are gone, back owned by the stepper (backMap for non-linear flows, onExit on the first step). TASK-21816 / TASK-21665.
Board 17802:61539 anatomy; error is FieldError text only and replaces the helper — never an input border (DS call). react-hook-form is the expected state owner. Showcase at /dev/ds/primitives/field. TASK-21454.
…/withdraw - WithdrawFlowProvider mounts at the /withdraw layout, not app-global. Fresh entry IS the reset: the hand-written resetWithdrawFlow() compensation in home and send is gone (TASK-21203 / TASK-20806). - Root page: method → amount as ?step= named ids; the amount travels as ?amount= to every downstream route (TASK-21665); showAllWithdrawMethods becomes ?showAll= (kills the twin racing effects, TASK-21198). - features/ pattern: WithdrawRoot → useWithdrawRootFlow → dumb views; bank page → useBridgeOfframpFlow + WithdrawBankReviewView with ?step=review|success; crypto page steps recipient|review|success. - AddWithdrawRouterView deleted (only consumer was the withdraw page; its add branches were unreachable — add-money renders AddWithdrawCountriesList). - DynamicBankAccountForm decoupled from the withdraw context and the redux bankForm slice; fields render through the new Field component. - Amount-error gating: the banner yields to the limits card only when the card renders — crypto always shows a reason (TASK-21666). TASK-21816
…honest search placeholder - Manteca flow moves onto the shared URL stepper (amount | bank-details | review | success | failure) and honors ?amount= from the shared amount step — one amount entry, honored downstream (TASK-21664). Back from bank-details returns to the root amount step when the amount was seeded. - Withdraw network icons: the chain registry's curated raster logos win over chain-details.json SVG URLs, which next/image refuses without dangerouslyAllowSVG — ETH/OP/BNB rendered as initials (TASK-21667). - Token search placeholder stops promising address paste in all four locales — the field never supported it (TASK-21199). TASK-21816
- withdraw-states runs the REAL stepper + flow hook against the nuqs
testing adapter, so the URL contract itself is asserted: guard fallback
on ?step=amount without flow memory, ?amount= pre-fill and forwarding,
send-marker survival, TASK-21666 crypto error visibility.
- crypto-withdraw-confirm keeps its double-spend regression net on the
goTo('success') transition instead of setCurrentView.
- send/home drop the resetWithdrawFlow expectations — the compensation
they pinned is gone with the app-global context.
- WithdrawFlowContext test moves with the module to features/withdraw.
- deflake: the stepper's guard-redirect URL assertion waits out nuqs's
write throttle.
TASK-21816
…eld bank form Fixture routes may now carry their own query string (deep-linked flow steps — the URL stepper's whole point); the shots runner and the route-exists check split on '?'. TASK-21816 / TASK-21454
The crypto page's unmount cleanup called resetWithdrawFlow(), clearing selectedMethod on the intra-/withdraw back transition — the root amount guard then bounced ?step=amount to method selection. The cleanup now clears only crypto-transient state (charge, route, recipient, modals); the selection survives, and leaving /withdraw still resets everything by unmounting the provider. Regression test pins the cleanup contract. TASK-21816
…ut abandoned prepare drafts (TASK-21817)
Section 1 — history wire contract: IntentKind now derives from the
generated OpenAPI types (the BE declares its vocabulary on the
/history/{entryId} kind parameter), so the STRATEGIES registry is total
over the real wire enum and a BE kind addition becomes a compile error
instead of a row that falls through the fallback and renders as an
outgoing send. The four previously-unmapped kinds get honest strategies:
P2P_SEND behaves like DIRECT_TRANSFER (and joins the referral-nudge set),
REWARD_PAYOUT renders as the Peanut Rewards credit, INTERNAL_TRANSFER and
CHARGEBACK render as neutral debits (the BE's INFLOW_KINDS excludes
CHARGEBACK — it is not a credit). The FE-only 'OTHER' synthesis stays in
the fallback for kindless legacy rows only.
Section 2 — TASK-21815 follow-up: the prepare call no longer sends a
client-declared kind (the backend chooses it from the destination;
RainCollateralKind stays as FE-internal analytics vocabulary), and
abandoned drafts are backed out best-effort via the new
/rain/cards/withdraw/prepare/cancel: both spend-bundle hooks cancel on
their failure unwind, and useReturnExcessCollateral plus the Lock/Cancel
card modals cancel a signed-but-unconsumed draft. Every cancel is
fire-and-forget — the backend refuses while the Rain signature could
still execute and the TTL sweep is the guaranteed cleanup. Types
regenerated from the paired BE branch (kind enum, cancel route, prepare
without kind).
A dispute resolving in the user's favor arrives as kind=REFUND (the credit the card terms promise); CHARGEBACK is the clawback leg — INFLOW_KINDS excludes it on purpose. Recorded at the strategy so the sign is never 'corrected' into contradicting the ledger.
Kimi's finding on the review: a throw around /submit (or lockCard/ cancelCard) is execution-ambiguous — the withdrawal may have executed with the response lost — and while the backend's sig-expiry gate blocks most of that window, firing cancel there buys nothing and leans on the guard. useSpendBundle now tracks broadcastAttempted and cancels only when the failure provably precedes any broadcast (the useSignSpendBundle model); the returnExcess/lock/cancel-card back-outs are removed entirely — the probe-verified TTL sweep owns those. New useSpendBundle tests pin the three boundaries: charge-backed never cancels, pre-broadcast cancels once with the prep id, post-broadcast-attempt never cancels.
…s; chargeback follows the viewer entry The broadcastAttempted flag flipped before handleSendUserOpEncoded / tryMixedEphemeralSpend ran client setup and the second WebAuthn ceremony — a dismissed tap #2 was treated as execution-ambiguous and the draft leaked to the TTL sweep. Both helpers now expose onBroadcastAttempt, fired at the last line before the actual UserOp submission, and the catch adds one carve-out: a WebAuthn ceremony rejection is provably pre-broadcast even past the boundary (an unsigned op cannot submit). Tests pin a dismissed second ceremony (cancels) vs a bundler failure after the boundary (does not). CHARGEBACK now derives direction from the viewer's entry (userRole): RECIPIENT renders incoming, SENDER/BOTH/NONE the common clawback debit — the mapper's role and the strategy's sign can no longer disagree. Role-specific strategy tests added.
…cross the fallback Encoding now happens BEFORE the broadcast signal in both userop helpers — an encodeCalls failure is provably pre-broadcast. And a session-key attempt that crossed the boundary and fell through marks the draft ambiguous for good: the passkey fallback reuses the same prep (only one can execute), so even its own ceremony rejection must not cancel — the earlier broadcast may have landed. Test pins crossed-ephemeral + dismissed-fallback-ceremony → no cancel. The residual inside sendUserOperation (estimation/paymaster failures read as ambiguous) leaks a draft to the 30-min TTL sweep only — splitting sign from transport means decomposing the SDK client call, out of this PR's blast radius.
Both userop helpers now decompose prepare → sign → transport: estimation, paymaster work, and the signature complete before onBroadcastAttempt, and the final sendUserOperation receives the prepared request + signature with an empty fill-list — a pure eth_sendUserOperation. An estimation or paymaster rejection is therefore provably pre-broadcast and the draft backs out immediately instead of waiting for the TTL sweep. Encoding also moved ahead of the sending-state flag so a rejecting encoder cannot leave isSendingUserOp stuck true. Types resynced to the final BE contract (limit: integer >= 1).
The broadcast-boundary ordering is now regression-covered against the REAL helpers: both tryMixedEphemeralSpend and handleSendUserOpEncoded run with instrumented encode/prepare/sign/send stages, pinning that encode, preparation, and signature failures never fire onBroadcastAttempt while a transport rejection observes it exactly once, fired before the send. isIntentKind swaps for Object.hasOwn: walks the prototype chain, so a crafted receipt URL like ?kind=toString dispatched Object.prototype.toString as a strategy instead of falling back. Prototype keys are pinned rejected. Types resynced to the BE's bounded-limit contract.
dev is on a merge freeze for the release, so tech-debt work integrates on the tech-debt branch (same pattern as feat/design-system) and merges to dev after the release. Without these filter additions, PRs based on tech-debt get no Tests/preview/code-analysis runs.
…withdraw-url-stepper
…t (Chip review) - step-guards.ts: every ?step=success (and manteca's failure) now demands flow-local proof set only after the money operation — bank: confirm succeeded (completedTxHash); crypto: broadcast transaction identifier; manteca: submission outcome. A hand-edited URL or a refresh without proof falls back to a working step. Tampering regressions per flow. - amount-validation.ts: the bank submit handler revalidates the user-editable ?amount= synchronously — finite, positive, ≥ the $1 Bridge floor, within the displayed balance — and the normalized decimal string is what goes on the wire. TASK-21816
…ontract fix: TASK-21817 history kind vocabulary from generated types + TASK-21815 prepare follow-up
The step cursor is a named screen id in the URL (?screen=signup) on the
shared useFlowStepper — replacing the redux numeric index into the
runtime-filtered step array whose next/previous clamps silently
dead-ended (TASK-21404). history:'push' keeps the deliberate setup
contract: the browser/hardware Back button walks the steps; a step with
showBackButton=false is a point of no return — earlier screens' guards
bounce a history pop back. Entry stays owned by resolveSetupEntryStep
and REPLACES any stale ?screen= on fresh load. The pushState mirror
(useSetupStepUrlSync) is retired; its analytics live on in
useSetupStepAnalytics.
The setup slice dissolves by field:
- steps/direction/isLoading/username/residence → SetupFlowProvider,
mounted at the (setup) layout (flow-scoped, TASK-21816 pattern).
- inviteCode/inviteType → cookies (invite-stash) — they are written from
payment/claim/invite surfaces and read at registration, and the cookie
is the copy that already survived the PWA-install hop.
- showIosPwaInstallScreen → useIosPwaInstallGate (sessionStorage) — the
cross-layout latch (setup arms it, mobile-ui reads it).
- telegramHandle was dead state: no writer anywhere; dropped.
- useResidenceRestrictions reads the during-signup answer via
useOptionalSetupFlow (Profile/Home consumers have no provider).
useFlowStepper grows a history option ('replace' default per design.md;
'push' for this flow) and a per-call history override on goTo (entry
replaces). TASK-21460.
An unloaded balance is no longer headroom: validateBankOfframpAmount refuses with balanceLoading and the bank review submit stays disabled until the spendable balance is real, so an edited ?amount= can't reach createOfframp before the ceiling check exists. The Manteca ?amount= seed moves into useMantecaAmountSeed and only advances past the amount screen once the limits gate and a synchronous balance/minimum validator pass for the seeded amount — the effect-set balanceErrorMessage lags a render, so the gate asks the live balance instead. Price-lock and handleWithdraw re-check balance + async limits right before the money operations as the last line of defense. The seeding, gating, back-to-root, and Try-again re-arm behavior is unit tested (useMantecaAmountSeed.test.tsx).
…ubmit handler (Chip round 4) The crypto withdraw page now validates and normalizes the user-editable ?amount= before any request/charge row is persisted (same-chain USDC had no floor, so 0 and malformed values sailed past the Rhino-only minimum and persisted records that could never sign). The USD amount the records were created for is pinned to the charge id, and the confirm leg broadcasts the pinned amount — re-validated against the live balance — so a URL edit between review and confirm can no longer move a different amount on-chain than the records say. The route quote pins the same way. The bank submit handler is a plain function again: its useCallback deps were all lifetime-stable, so the memo froze the first render's proceedWithOfframp — a click after capabilities/balance resolved ran the stale gate-loading no-op forever (dead button until remount). New tests: useBridgeOfframpFlow.test.tsx runs the real hook under the nuqs adapter (create→send→confirm with the normalized amount, the frozen-closure regression — verified failing against the old memoized handler — and over-balance/below-minimum/malformed tamper cases); crypto-withdraw-confirm.test.tsx gains the setup-persistence and broadcast-pin cases.
…460-setup-url-stepper
… gates (Chip round 5) The bank submit revalidation enforced a flat $1 while the amount step enforces per-rail minimums (GB £3, MX 50 MXN) — an edited ?amount= could bypass them. The conversion moves into bankWithdrawMinUsd (shared by the amount step and the submit re-check: one conversion, two enforcement points), validateBankOfframpAmount takes the destination minimum, and for GB/MX the submit stays disabled until the FX rate behind the minimum loads (isSubmitReady) rather than under-enforcing. New manteca-withdraw-gates.test.tsx renders the real Manteca page and covers the submit-time gates: a limits verdict that flips to blocking (or a balance that unloads) on review bounces to the amount step with no signSpend / withdrawWithSignedTx call; all-clear fires the withdraw once with the locked priceLockCode; the price-lock boundary bounces the same way. Writing it caught a real wiring bug: the blanket mount-reset ran AFTER the ?amount= seed's effects and clobbered the seeded amounts — the hand-off silently died on entry. The reset now registers before the seed hook, so a fresh mount clears state first and the seed arms on clean state.
…460-setup-url-stepper
…t Manteca money boundaries (Chip round 6)
The UK country record is { id: 'GBR', iso2: 'GB' } and the FX account
ternary keyed on id — so the £3 minimum silently converted through the
EUR rate. countryIso2 now reads iso2 (falling back to id), matching the
amount step's derivation, and the GB submit test uses the real 'GBR'
record and asserts the GBP rate is the one requested.
Both Manteca money boundaries (price lock, handleWithdraw) now also ask
the synchronous live-balance validator instead of relying on the
effect-set balanceErrorMessage, which lags a render — a balance that
drops while the user sits on review bounces to the amount step without
signing or submitting. Covered in manteca-withdraw-gates.test.tsx.
…460-setup-url-stepper
…o success amount (Chip round 7) ?amount=1e21 survived the seed's bare parseFloat check, normalized to '1e+21', and crashed the live-balance validator's parseUnits call before any gate could render. Parsing now goes through parseUsdAmount — the fail-closed plain-decimal parser the bank/crypto validators already used — in both seed effects, with exponential cases in the hook tests. The crypto success screen and the WITHDRAW_COMPLETED analytics read a new executedAmountUsd (set from the charge-pinned broadcast amount at completion) instead of the still-editable ?amount= — a post-execution URL edit could forge the displayed receipt. A new review setup clears the previous attempt's transactionHash, so the success-step guard only admits execution proof from the current attempt. New WithdrawMethodView test pins what the deleted AddWithdrawRouterView test covered: a saved Manteca account forwards destination + isSavedAccount into /withdraw/manteca, a saved bank account advances without navigating, and the crypto row sets the method with no router.push (the pre-amount-push redirect-guard regression).
…460-setup-url-stepper
Editing ?amount= after a completed bank offramp left completedTxHash satisfying the success guard while the screen rendered the edited number — a forged confirmation. The hook now stores executedAmountUsd (the validated amount the offramp moved) alongside the completion proof and the success screen renders it; a regression drives the real hook, completes a withdrawal, edits the URL amount through the nuqs setter, and asserts the pinned amount survives.
…460-setup-url-stepper
Contributor
|
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: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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: 7255.9 → 8961.47 (+1705.57) 🆕 New findings (1391)
…and 1371 more. ✅ Resolved (894)
…and 874 more. 📈 Painscore deltas (top movers)
|
Contributor
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
The page becomes a thin nuqs entry keyed on the scan; state lives in QrPayFlowContext, behavior in useQrPayFlowController (mounted once, so no effect runs per-view), the KYC gate and hold-to-claim in their own hooks, and each screen in a no-prop view. View selection is a derived precedence ladder (deriveQrPayView) instead of eleven early returns — same trick classifyScanOutcome already used one level down. The full state-matrix suite runs unchanged through the page entry; only its URL harness moved to the nuqs testing adapter. (TASK-21457)
…(review F1) the nuqs shim rebuilt the params in fixed insertion order, flipping precedence when both campaignTag and campaign are present and dropping duplicate keys. resolve campaignTag in the allowlisted view via the live useSearchParams object (original url-order semantics) and hand it to the hook.
… for the relocation The dismissal capture defers one tick and a remount cancels it, so StrictMode's mount/unmount/mount cannot fire a phantom reward_claim_dismissed — the new hook test locks that in. The ds-lint baseline rises only because the old page's markup moved verbatim into views/ scope (inlineStyle 21→24, nonDsClassesInViews 16→19, hoverNoActiveFiles 44→45): relocation, not new drift; flagged in the PR body per the design.md migration rule.
Contributor
🖼 Visual diff — 11 screens moved14 of 66 shots changed · 52 identical · baseline
new screens (2)
job summary · before/after/diff images — artifact Fixture screenshots, no backend. Advisory — this check never blocks a merge. Posted from the default branch by ds-shots-comment.yml; the report it renders is untrusted data. |
kushagrasarathe
marked this pull request as ready for review
September 7, 2026 03:36
kushagrasarathe
requested review from
0xkkonrad,
Hugo0,
abalinda,
innolope-dev and
jjramirezn
as code owners
September 7, 2026 03:36
![chip-peanut-bot[bot]](https://avatars.githubusercontent.com/in/2915219?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
Clean at the supplied head. The extraction preserves the reviewed claim, deposit, card, KYC, request-link, rewards, QR, and invites-graph behavior, and the final campaign-query fix restores the original ordered search-parameter semantics.
Findings
- MINOR · src/components/Claim/Link/Initial.view.tsx:32 · [claude-opus] Claim campaign-wire regression fixed in this PR is still unpinned by any test
The final commit (cbb96c0, "review F1") is the one non-verbatim change in this PR, and it sits on a money path.useInitialClaimFlowforwardscampaignTagtoinvitesApi.acceptInvite(inviteCode, EInviteType.PAYMENT_LINK, campaignTag)(useInitialClaimFlow.ts:307-311) and toclaimLink/claimLinkXchain(:422, :433) — it decides which badge campaign a claim is attributed to and mutates account/badge state. The intermediate commit rebuilt the query string from nuqs values into a freshURLSearchParams, which flipped precedence when bothcampaignandcampaignTagwere present and dropped duplicate keys; the fix re-resolves it from the liveuseSearchParams()object at Initial.view.tsx:31-32 and passes it in at :75.
Nothing pins that wiring. badge-campaign-context.test.ts tests the helper (url-order precedence, repeated keys, CSV splitting) and would have passed throughout — the helper was never wrong, only its input was. Initial.view.cashoutLimit.test.tsx and claim-states.test.tsx mock useSearchParams: () => new URLSearchParams() (empty) and assert nothing about campaign. claim-campaign-isolation.test.ts forwards a single opaque tag and so cannot distinguish the two orderings. Every other flow hook extracted in this PR got a __tests__/useXFlow.test.ts; useInitialClaimFlow.ts (1085 lines, the largest money-moving extraction) is the only one that did not.
Exact untested case: render InitialClaimLinkView with ?campaign=a&campaignTag=b and assert the tag reaching claimLink is a (URL order wins, not insertion order), plus ?campaign=a&campaign=b asserting the duplicate key is not dropped. Both were wrong before the fix and both would silently mis-attribute a badge award today if the wiring regressed again.
Checked clean
- Verified the exact head, supplied base, merge base, trusted author, and dev target.
- Checked exact-head CI; all required checks completed successfully.
- Compared the claim and initial-claim extractions with the base, including URL restoration, campaign precedence, cross-chain quote generation, stale-result guards, and claim action wiring.
- Compared the add-money bank, crypto, and root flows with the base, including scalar query parsing, returnTo validation, history behavior, amount validation, and the Manteca-before-Bridge mount gate.
- Checked card, recovery, perk, QR, rewards, request-link, and Sumsub hook/view boundaries for lost state, reordered effects, missing abort cleanup, and miswired callbacks.
- Checked the invites-graph decomposition for preserved filtering, fetch, force, camera, interaction, rendering, overlay, and cleanup wiring.
- Ran security and slop passes over the changed trust boundaries, caller-controlled query values, external SDK lifecycle, dead-code risk, and duplicated behavior; no actionable regression survived the adversarial pass.
Security review: did not run — openrouter-http-402. This review is one reviewer short.
Third opinion by claude-opus: 1 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: cbb96c080bc0 · Context: repo · Took 13m
…finding) renders InitialClaimLinkView with ordered/duplicated legacy campaign params and asserts the tag handed to useInitialClaimFlow follows url order — the exact regression the cbb96c0 fix reverted.
Contributor
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
No actionable defects remain at this head. The prior campaign-wire regression is fixed and now pinned by a view-to-hook regression test.
Findings
- MINOR · src/components/Claim/Link/useInitialClaimFlow.ts:1029 · [claude-opus] Auto-claim trigger switched from useSearchParams to nuqs with no test
In the extraction of Initial.view.tsx into useInitialClaimFlow.ts, the post-auth auto-claim trigger stopped readinguseSearchParams().get('step')/.get('method')(old Initial.view.tsx:1028,1038) and now reads them through nuqsuseQueryStates(useInitialClaimFlow.ts:59-73). Everything else in this hook moved verbatim, but this is a genuine change of input source on the one branch that fires an on-chain claim with no user gesture:stepFromURL === 'claim' && isPeanutWalletcallshandleClaimLink(false, true), andstepFromURL === 'regional-claim'restores the Manteca method and flips into the regional claim flow.
The exact untested case: a user returning from the auth/KYC redirect to /claim?step=claim (the FINANCIAL_REDIRECT that src/hooks/tests/post-auth-redirect-consumers.test.tsx and src/services/tests/post-auth-redirect.test.ts both produce) — no test asserts that the claim view consumes it. The effect also calls removeParamStep(), which mutates the URL via a raw window.history.replaceState in useClaimLink.tsx:532; nothing pins that the nuqs-sourced stepFromURL clears in response, and a value that fails to clear leaves a claim-firing effect armed across later dep changes. The producer half of this contract is tested; the consumer half is not. Note that the new hook tests added elsewhere in this PR (useClaimFlow.test.tsx, useBridgeBankFlow.test.ts, useCardFlow.test.ts) cover their own hooks — useInitialClaimFlow has no hook test, only Initial.view.cashoutLimit.test.tsx, which exercises the bank/IBAN branch and never sets step.
Fix: add a useInitialClaimFlow (or Initial.view) test using withNuqsTestingAdapter with searchParams: '?step=claim' asserting handleClaimLink is invoked once for a peanut-wallet user, and a ?step=regional-claim&method=pix case asserting setRegionalMethodType('pix') + setClaimToMercadoPago(true); assert the claim is not re-fired after removeParamStep.
Checked clean
- Confirmed the detached worktree head and merge base exactly match the supplied head and base SHAs.
- Re-checked prior P1: InitialClaimLinkView now resolves the legacy campaign from live URLSearchParams, and the added test covers URL-order precedence, duplicate keys, and the no-param case; exact-head unit CI passed.
- Reviewed the production diff with moved-code detection, including claim, add-money, card and recovery, request, perk, QR, rewards, KYC, and route-wrapper seams; no behavior-changing omission or incorrect wiring survived.
- Verified the InvitesGraph extraction preserves effect registration order and the data, interaction, rendering, force, and camera handoffs.
- Reviewed URL-state and trust-boundary deltas: nuqs reads retain the relevant string/null contracts, add-money return targets remain sanitized, and QR/KYC security behavior is unchanged by the extraction.
- Exact-head unit, typecheck, eslint, format, DS lint, native export, analyze, deploy preview, and required aggregate checks passed; advisory ds-shots was still in progress.
- The focused Jest command was not runnable locally because this detached worktree has no installed Jest binary; exact-head unit CI provides the execution result.
Security review: did not run — openrouter-http-402. This review is one reviewer short.
Third opinion by claude-opus: 1 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: ea5c93d1b051 · Context: repo · Took 10m
…s new home the INLINE_STYLE_ALLOW entry pointed at qr-pay/page.tsx, which no longer holds the hold button — the exemption moves with it to QrPaySuccessView, undoing the inlineStyle baseline bump. the nonDsClassesInViews +3 was misattributed: it is withdraw's min-h-[inherit] from the tech-debt merge, not qr-pay relocation — the exemption log now says so.
…t (chip finding) real useInitialClaimFlow under withNuqsTestingAdapter: step=claim fires the claim exactly once at the service boundary, step=regional-claim routes to manteca without touching the on-chain claim, and a cleared step param never re-fires.
…-split # Conflicts: # src/components/Setup/Views/Residence.tsx
Contributor
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
Clean at the pinned head. The two supplied claim-flow findings are fixed by focused tests that exercise the real nuqs consumer and the live legacy campaign-wire resolution.
Checked clean
- Confirmed the detached worktree head, merge base, trusted author, and dev base exactly match the supplied review coordinates.
- Verified P1 is fixed: the exact-head auto-claim suite drives useInitialClaimFlow through the nuqs testing adapter and covers claim, regional-claim, and cleared-step behavior at the claim service boundary.
- Verified P2 is fixed: InitialClaimLinkView resolves the campaign tag from live URL-ordered search params, with focused coverage for key order, duplicate keys, and absence.
- Reviewed the claim, add-money, card, QR, rewards, KYC, request-link, and invites-graph extractions for state, effect ordering, navigation, failure paths, and moved trust boundaries; no reachable regression remained.
- Exact-head required CI passed, including the aggregate gate, unit suite, typecheck, lint, format, native export, and deploy preview. Advisory ds-shots failed during its duplicate Next build without an actionable diagnostic; no visual test ran, while the independent build paths passed.
Security review: did not run — openrouter-http-402. This review is one reviewer short.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 5c41c933b640 · Context: repo · Took 15m
…ng qr-pay branch (stacked-PR order: #3008 -> tech-debt, this on top)
kushagrasarathe
changed the base branch from
dev
to
feat/TASK-21457-qr-pay-split
September 7, 2026 08:37
Contributor
Author
|
/chip review |
Contributor
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
Clean at the pinned head. P1 is fixed by adapter-backed coverage of the post-auth auto-claim and regional-claim URL contracts; all required exact-head CI checks passed.
Findings
- MINOR · src/components/TransactionDetails/strategies/registry.ts:30 · [claude-opus] Committed OpenAPI snapshot declares a /history/{entryId} kind enum the merged backend does not
TASK-21817 replaces the hand-written IntentKind union withpaths['/history/{entryId}']['get']['parameters']['query']['kind'](src/components/TransactionDetails/strategies/registry.ts:30) and makes STRATEGIES aRecord<IntentKind, …>so that a backend enum addition becomes a compile error. That guarantee rests entirely on thekindparameter carrying an 18-value enum, which this PR adds to src/types/api.openapi.json (and api.generated.ts:5603).
The pinned peanut-api-ts policy branch does not declare that enum: src/routes/user/history.ts:234-239 has kind: Type.String() with the comment "Loose string type — invalid kinds fall through to 404", and no Type.Union/enum for these values exists anywhere in src/. Since pnpm gen:api:live curls a running backend, this snapshot was captured against a backend build that has the tightening — almost certainly the paired peanut-api-ts PR that I cannot see in this checkout. I am not claiming api-ts breaks: nothing here changes what the frontend sends, and the values themselves match the Prisma TransactionIntentKind enum (prisma/schema.prisma:1393-1410) plus the synthetic PERK_REWARD exactly, so runtime behaviour is correct today. Worth noting that the same checkout does contain the TASK-21815 half (/rain/cards/withdraw/prepare/cancel at routes/rain/withdraw.ts:931, and prepare accepting-and-ignoring a client kind at withdraw.ts:418-421), so the absence of the enum is a real asymmetry rather than a stale checkout.
What goes wrong if the backend half does not land: the next pnpm gen:api:live regenerates kind as plain string, IntentKind widens to string, Record<string, TransactionStrategy> accepts any subset of keys, and the compile-time tripwire the comment at registry.ts:20-28 advertises disappears silently — an added backend kind then falls through to intentFallback and renders as an outgoing send, exactly TASK-21403's failure shape. check:api will not catch it: it regenerates from the committed JSON, not from the live backend.
Fix: land the peanut-api-ts side (the enum on the getHistoryEntrySchema querystring) before or with this merge, and reference it in the PR description so the pairing is explicit.
Checked clean
- Re-evaluated P1 in useInitialClaimFlow and Initial.view.autoClaim.test.tsx: the real nuqs-backed flow now covers ?step=claim, regional method restoration, the claim service boundary, param removal, and no re-fire.
- Compared the extracted claim, add-money, card/card-recovery, request-link, QR-claim, KYC, rewards, and invites-graph flow boundaries with the supplied base, including error, retry, URL-state, signing, and transaction paths.
- Inspected the restack merge and nuqs provider integration for textual conflicts or missing runtime context; no changed behavior was established.
- Exact-head unit, typecheck, eslint, format, native-export, ds-lint, analysis, deploy-preview, authorship, and aggregate CI checks succeeded; advisory ds-shots was still in progress.
- A local focused Jest rerun was unavailable because this detached worktree has no installed Jest dependency; exact-head unit CI completed successfully instead.
Security review: did not run — openrouter-http-402. This review is one reviewer short.
Third opinion by claude-opus: 1 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 9fc23ccfa56a · Context: repo, ci · Took 10m
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
2 participants
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
Structural remainder of DS phase 2 (TASK-21854): pages and monolith views that interleave state machines with markup move onto the
features/pattern (page → flow hook → views), so they can compose the DS recipes. Zero behavior change — logic, hook order, effect dep arrays, markup, classNames and copy are moved verbatim.Extracted onto
features/(routes become thin wrappers):card(848 loc, 12 useState / 9 useEffect) →features/cardcard-recovery,fix-card-signature→ own feature foldersadd-money/[country]/bank(638 loc),add-moneyroot,add-money/crypto→features/add-moneyqr/[code](4 duplicated return branches collapsed into parameterized views) →features/qr-coderewards(tier math out of JSX) →features/rewardsDecomposed in place (shared components — path + export unchanged):
Global/InvitesGraph/index.tsx2,514 → 401 loc (7 hooks + 3 subviews, effect registration order byte-identical)Claim/Claim.tsx543 → 149,Claim/Link/Initial.view.tsx1,260 → 241 (useClaimFlow,useInitialClaimFlow)Home/PerkClaimModal.tsx,Kyc/SumsubKycWrapper.tsx,Request/link/views/Create.request.link.view.tsxNot in scope (deliberate): qr-pay (TASK-21457, separate branch) · withdraw/* + setup (live on
tech-debtvia #2917/#2949 — extraction rides on the URL stepper) · recover-funds / recover-wallet / JoinWaitlistPage / AddWithdrawRouterView (touched ontech-debt, deferred to avoid conflicts) · notifications (deleted on dev, intentional) · profile/backup (already thin on dev).Task
TASK-21854 — https://app.notion.com/p/3c783811757981198e14cd65877d4412
Design notes / accepted trade-offs
useSearchParamsor hand-built query pushes. Read-only params moved 1:1 ontouseQueryState/useQueryStates(samestring | nullcontract). Two deltas worth review attention:useAddMoneyFlowbank-method push now writes params via the nuqs setter — a shallow client-side URL update whererouter.pushdid a full Next navigation; renders identically on this client page. Cross-pathname redirects (/home?drawer=add) keep hand-built URLs (allowed).router.pushrebuilt the query from scratch; no caller passes any.withNuqsTestingAdapter.useCreateRequestLink; deadconfirmstep in card-recovery; the post-auth auto-claim effect has no once-guard — its only double-fire protection isremoveParamStepclearing the URL, see the note inInitial.view.autoClaim.test.tsx).Review convergence (Chip)
Initial.view.campaignWire.test.tsx(url-order precedence, duplicate keys, no-param).Initial.view.autoClaim.test.tsxpins the?step=claimsingle-fire at the claim service boundary, the?step=regional-claim&method=pixManteca route, and no-refire after the param clears.Risks / breaking changes
QA
npm run typecheck, full jest suite,npm run build.add-money-states2,013-line matrix,claim-states,SumsubKycWrapperregression suite) pass unmodified except for the nuqs testing-adapter wiring.Stack (updated 2026-09-07)
Base retargeted
dev→feat/TASK-21457-qr-pay-split(#3008). Stacked order: #3008 merges totech-debtfirst (carrying dev + qr-pay), then this auto-retargets totech-debt. The branch merged #3008's branch in (9fc23ccfa, textually clean; typecheck + 519 suites/6,297 tests + prettier + ds-lint + build all green after the merge). Do not merge this before #3008.