Repository navigation
review: re-land the eight fixes stranded on the closed 08-26 back-merge - #2894
Merged
Merged
Conversation
(cherry picked from commit 5b20452)
startAuthentication ran bare, so a step-up racing the native passkey shim install could silently never settle (same class as login, TASK-21782). Route it through guardPasskeyCeremony and map guard failures to StepUpError with the curated classifyPasskeyError copy so callers render sane text. (cherry picked from commit b74a5e4)
tookTooLong, notReady, deviceState and interrupted landed in es-419 without es-AR counterparts, so Argentine users got tuteo forms for these errors. (cherry picked from commit 9172826)
Login failures surfaced the hardcoded English PASSKEY_LOGIN_MESSAGES copy at SetupPasskey and InvitesPage even though the same strings exist translated in the setup.* catalog. The thrown PasskeyError already carries its code, so map known codes to their catalog keys at the render sites and keep the English message as the fallback for codes without a translated equivalent. (cherry picked from commit 5c54f47)
robots.ts derived IS_PRODUCTION_DOMAIN from BASE_URL, whose production fallback opened indexing on any environment with the env unset. Extract the raw-env fail-closed derivation layout.tsx already used into seo-route-policy.js (isProductionDomain) and use it in both. (cherry picked from commit 39f3336)
…l on submitted closes The capture+fetchUser effect in useMultiPhaseKycFlow re-fired on every dep change while the status sat on a stale REJECTED/ACTION_REQUIRED, duplicating the posthog event and the user refetch on resubmit rounds. It now tracks the previous status like the sibling transition effect in useSumsubKycFlow. The multi-level ACTION_REQUIRED deferral also replayed on ANY close. A close after the user submitted (level-2 done, SDK on "documents submitted") now carries submitted: true from the wrapper and consumes the deferred transition in both hooks, while an abandoned close still replays it once. (cherry picked from commit 2c19373)
(cherry picked from commit b3ed020)
innolope-dev
requested review from
0xkkonrad,
abalinda and
kushagrasarathe
as code owners
August 31, 2026 19:44
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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: Essentials Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Contributor
Code-analysis diffPainscore total: 7144.46 → 7147.28 (+2.82) 🆕 New findings (34)
…and 14 more. ✅ Resolved (34)
…and 14 more. |
Contributor
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
![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 — changes requested
Request changes: abandoning an unfinished follow-up level is misclassified as a completed submission, suppressing the real ACTION_REQUIRED state. The other reviewed fixes and CI are clean.
Findings
- MAJOR · src/components/Kyc/SumsubKycWrapper.tsx:368 · Do not consume ACTION_REQUIRED after only level 1 was submitted
In a multi-level session, the first level's submit sets hasSubmittedRef before Sumsub opens level 2. If ACTION_REQUIRED then arrives for that follow-up and the user closes without submitting it, this line sends {submitted: true}; both KYC hooks consume the deferred status, so the real action-required transition and user-store refresh are suppressed, and an open progress modal can remain stuck. Track submission for the currently required level (or forward and deduplicate every logical submit event, consuming ACTION_REQUIRED only when that event fires while the follow-up is required) so closing an unfinished follow-up remains an abandonment.
Checked clean
- Exact detached head, supplied base SHA, merge base, trusted author, PR number, and dev base ref all matched.
- CI at the exact head completed successfully, including unit, typecheck, eslint, format, analyze, and ds-shots.
- SEO production-domain gating was checked for unset, empty, preview, spoofed-host, and trailing-slash inputs; the shared gate remains fail-closed.
- Passkey lazy Sentry loading, translated PasskeyError mapping, and the step-up ceremony shim/timeout guard were checked without another actionable defect.
- The backend bridge-uplift response and Bridge follow-up workflow were cross-checked; treating EU bridge-uplift as multi-level matches the API contract.
- Security pass found no new secret exposure, injection path, privilege expansion, or unsafe domain trust.
- No issue comments or review comments were read.
Second opinion skipped: daily-budget-reached.
Exact head: 514f23ff0945 · Context: repo, mono, github
Chip flagged the `submitted: true` half of the previous commit: in a multi-level session the first level's submit sets hasSubmittedRef before Sumsub opens level 2, so a close that abandons an unfinished follow-up was reported as a completed submission. Both hooks then consumed the deferred ACTION_REQUIRED, suppressing the real transition and the user-store refresh and leaving an open progress modal stuck. The wrapper has no signal that separates the two cases: it subscribes to no step/level event, and a second onApplicantSubmitted is deduped as the idCheck twin rather than read as a new level. Rather than guess at the SDK's per-level semantics, drop the manual-close consumption entirely and restore the always-replay behaviour dev has today. handleSdkComplete keeps its consumption — an SDK-signalled completion is unambiguous, and it is the path the in-session resubmit actually takes. The edge-triggered capture (prevCapturedStatusRef), which is the other half of the previous commit and the one that stops duplicate KYC_REJECTED events on resubmit rounds, is unaffected. Telling an abandoned follow-up from a finished one needs a real level signal from the SDK; that is left for a follow-up rather than inferred here.
Contributor
There was a problem hiding this comment.
Chip review — changes requested
The web manual-close regression is fixed, but the same deferred ACTION_REQUIRED is still consumed when a native multi-level session is manually closed after Level 1.
Findings
- MAJOR · src/hooks/useMultiPhaseKycFlow.ts:325 · Do not consume native Level-1 exits
On Capacitor, SumsubNativeSdk records Pending as submitted and then calls onComplete even when launch() later resolves Initial because the user manually closed; the existing native test pins exactly that Pending-to-Initial behavior. In an EU/LATAM multi-level session, Level 1 can reach Pending, the follow-up becomes ACTION_REQUIRED, and exiting before Level 2 finishes invokes this callback. This line consumes the deferred status, and originalHandleSdkComplete consumes it in the base hook too, so the rejection/store refresh never runs and the progress modal stays stale. The web onClose() fix does not cover this driver. Route ambiguous multi-level native exits through onClose, or consume only on a terminal/final-level signal, and add the corresponding native regression test.
Checked clean
- Earlier finding: fixed for the cited web manual-close path, but still present through the native driver and re-reported at the current changed hook line.
- Exact head and merge base matched the supplied SHAs; trusted PR metadata matched repository, author, head, and dev base.
- Bridge-uplift multi-level routing was checked against the current backend initiate and follow-up status contracts.
- Passkey ceremony guarding, localized PasskeyError rendering, and lazy Sentry loading were reviewed without another actionable defect.
- Shared production-domain robots and root-metadata policy fail closed for unset, preview, and lookalike origins.
- All exact-head CI checks completed successfully, including unit, typecheck, format, eslint, design-system, analysis, and preview checks.
- Correctness, security, adversarial failure paths, and slop were reviewed across all 19 changed files.
Second opinion skipped: daily-budget-reached.
Exact head: 4e9d14e35e99 · Context: repo, product
Chip found the same defect class through the second driver. On Capacitor, SumsubNativeSdk counts Pending as submitted and resolves launch() on close whatever the user did, so in a multi-level session a Level-1 submit followed by backing out of Level 2 fires onComplete exactly like a real completion — the existing native test pins that Pending-then-Initial behaviour. The previous commit's web-side onClose fix does not reach that path. The consumption this drops is the one the re-landed commit ADDED: mirroring the base hook's consume into this hook's capture effect. Without it the capture and the user-store refresh run on that exit, which is what dev does today; with it they were suppressed and the applicant sat on a stale progress modal. The base hook's own consume in originalHandleSdkComplete is pre-existing on dev and carries the same ambiguity — it suppresses the transition effect on a native Level-1 exit. Left alone here rather than widened into a behaviour change this PR cannot claim was reviewed; it wants a terminal/final-level signal from the SDK, which is the same follow-up the web side needs. The edge-triggered capture, the actual point of the original commit, is intact.
Contributor
There was a problem hiding this comment.
Chip review — changes requested
One repeated major KYC defect remains: the outer native-exit capture was fixed, but the inner status transition still consumes the same ambiguous Level-1 exit and can leave the progress modal stale.
Findings
- MAJOR · src/hooks/useMultiPhaseKycFlow.ts:331 · Do not consume native Level-1 exits in the inner handler
On native, SumsubNativeSdk records the Level-1 Pending event as submitted and calls this completion path even when launch closes as Initial after the user backs out of Level 2. This still invokes originalHandleSdkComplete(), whose base handler advances prevStatusRef to ACTION_REQUIRED and opens the progress modal. The new outer test passes because capture/fetchUser now run, but the inner transition that would close that modal is still consumed, so the applicant can remain on stale verifying UI. Propagate a final-level/ambiguous-native-exit signal and skip the base consumption for that case; add an assertion that the progress modal closes.
Checked clean
- Earlier useMultiPhaseKycFlow finding: STILL PRESENT in the inner transition path; the outer capture consumption was removed, but originalHandleSdkComplete still consumes ACTION_REQUIRED.
- Earlier SumsubKycWrapper finding: FIXED; post-submit manual closes no longer claim submission and both hooks replay the deferred ACTION_REQUIRED.
- Exact-head CI: unit, typecheck, eslint, and format succeeded; ds-shots was still running when checked.
- Passkey ceremony guard, translated PasskeyError rendering, SEO production-domain gate, and associated tests were inspected without another actionable defect.
- A local targeted Jest run was unavailable because the detached worktree has no node_modules; exact-head unit CI succeeded instead.
Second opinion skipped: daily-budget-reached.
Exact head: 07844fc9b6a7 · Context: repo
Closes the native half of the deferral defect at its source rather than at the consumer. SumsubNativeSdk trusted `hasSubmitted` alone, but in a multi-level session that flag only proves SOME level was submitted: Pending fires after Level 1 with the questionnaire still outstanding, so backing out of Level 2 resolved launch() as Initial and still reported a completion. Both flow hooks then consumed the deferred ACTION_REQUIRED and left the applicant on a stale verifying modal. The plugin dismisses after the LAST level, so for multi-level a close whose own status is not itself a submitted one is the user leaving mid-workflow. Single level is unchanged and still trusts `hasSubmitted` — one submit IS the flow there, and the closing status genuinely does read Initial after a real submission, which the existing test pins. The level the user did finish still reaches the funnel: an abandoned exit fires onSubmitted before onClose, so dropping the bogus onComplete does not cost the KYC_SUBMITTED signal. SumsubSdkProps carried isMultiLevel and onSubmitted already and the wrapper spreads both, so no call site changes. Why here and not in the hook: this is the driver that cannot tell the two exits apart, and fixing it in the hook would have meant weakening a consume that is correct for every unambiguous completion. It also settles the pre-existing base hook consume for this path — the reason that one was left alone last round is that only the native driver could distinguish the case, and now it does.
Contributor
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
Clean: the native multi-level Level-1 exit now routes through onClose, fixing both repeated findings; no other actionable defect remains.
Checked clean
- Review of 07844fc: FIXED. A multi-level Pending event followed by an Initial launch result now emits onSubmitted and onClose, bypassing the inner completion handler that consumes deferred ACTION_REQUIRED.
- Review of 4e9d14e: FIXED. The same native Level-1 abandonment path is now classified as a manual close, while final submitted statuses still complete.
- KYC status deferral, rejection capture edge triggering, manual-close replay, native result routing, and EU bridge-uplift multi-level behavior were checked against the UI and backend contracts.
- Passkey ceremony guarding, lazy Sentry loading, translated PasskeyError rendering, and setup locale parity were inspected without an actionable defect.
- SEO production-domain sharing between root metadata and robots.ts fails closed for unset, preview, and lookalike origins.
- Exact-head CI completed successfully: unit, typecheck, eslint, format, design-system checks, analysis, preview deployment, and aggregate ci-success.
- A local targeted Jest run was unavailable because the detached worktree has no node_modules; exact-head unit CI succeeded instead.
Second opinion skipped: daily-budget-reached.
Exact head: 105963f03342 · Context: repo, peanut-api-ts, provider-docs
This branch was successfully deployed
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.
Re-lands the review fixes that were reviewed on #2830 and lost when that back-merge was closed in favour of the 08-28 one (#2874). The replacement carried
main's history but not these commits, so none of them are ondevormaintoday — verified per commit by greppingorigin/devfor its added lines before opening this.Cherry-picked with
-x, so each commit keeps its original author, message and a pointer back to the original SHA.What's in it
1a3e02080d280129f0e06setup.passkeyerror keys815c00cPasskeyErrorcodesfd90272a8da766robots.tsclosed with the shared production-domain check7e33fb7KYC_REJECTEDcapture, consume the deferral on submitted closes514f23fbridge-uplift)Where
devhad moved onFive applied clean. The rest needed a decision, and one commit was only half-valid:
robots.ts—devhad already fixed the fail-closed bug inline, but left the derivation duplicated inlayout.tsx. Kept the commit'sisProductionDomainextraction so the two can't drift apart again — that drift is how the bug happened. Took the commit's test file: it is a strict superset ofdev's (table-driven over unset/empty/staging/peanut.me.evil.example, plus a trailing-slash case), anddev's lazyrequire/resetModulesdance is unnecessary now that the check reads the env at call time.d953b345split — that commit droppedgetUnderlyingFetchas unimported and removed a stale font mock. The first half is no longer true: feat(native): restore the transport canary, failure-only, iOS included #2871 restored the transport canary andnative-canary.tsimports it again (caught bytypecheck). Only the font-mock half is re-landed, asfd90272; the export stays.SumsubKycWrapper.test.tsx— purely additive.devadded multi-level resubmission tests, the commit added a post-submit-close test; both kept.sumsub.types.ts—dev'sKycActionTypealready hadbridge-upliftand has since gainedrails-unavailable. Keptdev's union; the commit'suseSumsubKycFlowhunks applied clean.Two mocks in the new
useMultiPhaseKycFlowsuite also needed updating fordev's current surface —@/utils/capacitornow needsisAndroidNative(the mascot module calls it), and the@/app/actions/sumsubmock has to spreadrequireActualso the hook'sisTerminalActionCodebranch gets the real helper instead ofundefined. That is the same partial-mock pattern the siblinguseSumsubKycFlowsuite documents; without it every initiate silently bailed. Folded into7e33fb7rather than left as a follow-up.Verification
pnpm typecheckclean;prettier --checkclean on all touched files; the 8 touched suites pass (121 tests), plus the i18n parity suites (messages,locale-bridge,proxy-locale,friendly-error) for the es-AR change — 109 tests.