Repository navigation
refactor: TASK-21854 logic/UI separation — extract inline-state pages and monoliths onto features/ - #3007
Conversation
…es/ (TASK-21854) state machines and tier math move into hooks, four duplicated qr return branches collapse into parameterized views; route pages become thin wrappers per the features/home pattern. the code query fallback moves to nuqs (new-file ratchet). zero behavior change otherwise.
…ges onto features/ (TASK-21854) the 12-state card page, the recovery step machine and the signature repair flow move into flow hooks + view components per the features/home pattern; route pages become thin wrappers. zero behavior change.
…er and Create.request.link.view in place (TASK-21854) each monolith splits into a flow/lifecycle hook plus named subview components in sibling files; composition roots keep their paths and exports so importers are untouched. the request-link url params move to nuqs (new-file ratchet). zero behavior change otherwise.
…s/ (TASK-21854) the bridge bank deposit flow, the root country/method dispatch and the crypto deposit page move into flow hooks + views per the features/home pattern; routes become thin wrappers. url params move to nuqs (new-file ratchet); the home-drawer redirect keeps its hand-built cross-pathname url. native-build stub imports stay route-local. zero behavior change.
…ce (TASK-21854) the 20-useState claim machine and the 18-effect initial view move into useClaimFlow / useInitialClaimFlow hooks with split subviews; both composition roots keep their paths and exports. zero behavior change.
… in place (TASK-21854) data, derived-memo, interaction, fetch, rendering, force and camera concerns split into dedicated hooks; three jsx subviews extracted; effect registration order kept byte-identical. index.tsx stays the composition root with the same export and props. zero behavior change.
…(TASK-21854) the new files backing 6e6386e — useClaimFlow, useInitialClaimFlow, split subviews and the hook test suite; url params read via nuqs (new-file ratchet). the earlier pathspec commit missed untracked files.
|
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: Advanced 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 |
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)
|
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
…(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.
🖼 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. |
![chip-peanut-bot[bot]](https://avatars.githubusercontent.com/in/2915219?s=60&v=4){kind=small}
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.
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
…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.
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)
|
/chip review |
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
…to refactor/TASK-21854-logic-ui-separation
removeParamStep() clears the trigger through a raw window.history.replaceState that nuqs does not observe, so stepFromURL keeps reporting 'claim' for the rest of the mount — and fetchUser() during the claim replaces the user object, re-running the trigger effect mid-claim. That re-run fired a SECOND claim POST, which rejected against the already-claimed link and painted an error over the success screen. A consume-once ref (armed only when a step was actually present) swallows the re-run. The pinning test now stages the real prod sequence — a user identity swap after the first claim — instead of relying on a stable user object that made the double-fire unreachable in the harness.
…afe storage, one flow surface - useAddMoneyFlow: handleBack's if (countryFromQuery) branch was unreachable — every ?country= render path returns a component that owns its own back behavior, so the handler never runs with a country in the URL. Deleted, with its pinning test replaced by one that records why. - card/utils: the celebration gate read raw window.localStorage inside a render-time useState initializer; the bare accessor throws where site data is blocked. Now goes through safe-storage. - QrPayFormView read useWallet() right next to useQrPayFlow(), which already holds the wallet; the flow now surfaces balance so views read one object. Also dropped InvitesGraph/index.tsx's unused GraphMode re-export (types live in types.ts per the export rules).
|
Heads-up: I pushed 3 commits (
Checked and skipped, with reasons (from the review's remaining list):
Local gate before push: prettier clean, Note for both PRs: any push now re-runs |
|
/chip review |
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
The structural refactor is otherwise sound, but prior P1 remains: the checked-in history-kind enum is not generated by the current backend schema.
Findings
- MINOR · src/components/TransactionDetails/strategies/registry.ts:30 · Keep the history-kind enum reproducible from the backend
At this head, src/types/api.openapi.json and api.generated.ts still declarekindas a closed enum, so registry.ts line 30 relies on that union for an exhaustive strategy map. The current sibling backend still declares getHistoryEntrySchema.querystring.kind as Type.String() in src/routes/user/history.ts:235-240; regenerating from that contract therefore restoresstring, removes the exhaustiveness check, and proves the committed snapshot is not reproducible. This remains true after P1, even though the retargeted base now carries the same snapshot. Add the enum to the backend route schema and regenerate the frontend types, or keep a frontend-owned union until that backend contract is actually merged.
Inline anchors unavailable for 1 finding(s); the findings remain in this summary.
Checked clean
- Confirmed the detached worktree is clean at the supplied exact head, and the PR metadata, base ref, base SHA, and merge base all match.
- Reviewed the route-to-feature extractions for add-money bank and crypto, card and card recovery, QR claim, rewards, claim, request-link, KYC, perks, and InvitesGraph, including behavior-sensitive URL state, effect ordering, error paths, and focused regression coverage.
- Checked the final head changes: the claim step trigger is consumed once across a user refetch, the removed add-money back branch is unreachable under the route dispatcher, card celebration reads and writes use safe storage, and QR pay surfaces the same spendable balance already used for validation.
- Security and money pass found no new authorization, secret, destination, or amount-trust regression in the structural moves; the money-moving flow surface is still flagged for the dedicated security reviewer.
- Exact-head GitHub exposes only successful bot-approval and Vercel Preview Comments checks (plus a skipped IndexNow run); no exact-head unit, typecheck, lint, format, or native-export checks ran. The detached worktree has no installed test or typecheck dependencies, so local execution was unavailable; earlier ancestors had full green CI but do not replace exact-head coverage.
- Ran git diff --check with no whitespace errors.
Security review by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
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: 2eb307554031 · Context: repo, sibling-api · Took 24m
This branch was successfully deployed
2 participants
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.