Leave the sign-up flow when it completes with an assertion - #65
Conversation
📝 WalkthroughWalkthroughThe sign-up flow now blocks repeated submissions after completion. Non-redirection responses with assertions now redirect to ChangesSign-up completion behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react/src/components/presentation/auth/SignUp/__tests__/SignUp.test.tsx (1)
65-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the
afterSignUpUrlredirect.This test disables redirects and only verifies
onComplete. It does not execute the changed branch inSignUp.handleComplete.Add a test with
shouldRedirectAfterSignUp={true}andafterSignUpUrl. Verify that an assertion-bearing non-redirection completion navigates to that URL.🤖 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 `@packages/react/src/components/presentation/auth/SignUp/__tests__/SignUp.test.tsx` around lines 65 - 83, The existing SignUp completion test only verifies onComplete and does not cover the redirect branch in SignUp.handleComplete. Add a separate test configuring shouldRedirectAfterSignUp={true} and an afterSignUpUrl, submit an assertion-bearing non-redirection completion, and verify navigation occurs to the configured URL.
🤖 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.
Nitpick comments:
In
`@packages/react/src/components/presentation/auth/SignUp/__tests__/SignUp.test.tsx`:
- Around line 65-83: The existing SignUp completion test only verifies
onComplete and does not cover the redirect branch in SignUp.handleComplete. Add
a separate test configuring shouldRedirectAfterSignUp={true} and an
afterSignUpUrl, submit an assertion-bearing non-redirection completion, and
verify navigation occurs to the configured URL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: baaff14b-c72d-4771-bcd4-a66b48cf264f
📒 Files selected for processing (3)
packages/react/src/components/presentation/auth/SignUp/BaseSignUp.tsxpackages/react/src/components/presentation/auth/SignUp/SignUp.tsxpackages/react/src/components/presentation/auth/SignUp/__tests__/SignUp.test.tsx
A completed sign-up carrying an assertion skipped the afterSignUpUrl redirect, on the grounds that the caller had handled navigation. Consumers that pass afterSignUpUrl and no onComplete, such as the Gate sign-up page, therefore had nothing navigate for them: the finished step stayed on screen with its submit action live, and clicking it started a new sign-up flow. SignIn has no such guard and always navigates on completion. Honour afterSignUpUrl for these completions too. shouldRedirectAfterSignUp remains the opt-out for consumers that navigate themselves, and one that passes no afterSignUpUrl is still never redirected. Also latch the completion in BaseSignUp so the step rendered while the redirect is in flight cannot be submitted again. The execution id is spent by then, and re-submitting would begin a fresh flow. Refs thunder-id/thunderid#4795
0ab43f3 to
4a57ca8
Compare
|
Thanks — valid catch, fixed in You were right that the existing tests disabled redirects and never executed the changed branch. I had claimed in the PR description that the redirect was untestable because these tests run in a real browser and assigning A URL differing from the current one only by its fragment is a same-document navigation: I verified it actually discriminates rather than just passing: with the I have updated the PR description to drop the claim that this branch could not be covered. |
2 participants
Purpose
Fixes a sign-up flow that completes on the server but never finishes in the UI.
When a sign-up completes with an assertion,
ThunderIDReactClient.signUp()stores the session and emitssign-in, which is what triggers the/users/mecall people have noticed.SignUp.handleCompletethen skipped theafterSignUpUrlredirect for exactly those responses:The comment justified it as "the SDK stored the session and the caller handled navigation". That premise does not hold for a consumer that passes
afterSignUpUrland noonComplete— the ThunderID Gate sign-up page, for one. Nothing navigated,BaseSignUp.handleSubmitreturned without touchingcurrentFlow, and the finished step stayed on screen with a live submit action. Clicking it posted an execute request that began a whole new sign-up flow.SignInhas no equivalent guard: it always navigates on completion (redirectUrl || afterSignInUrl), so sign-up was the outlier.Approach
Drop the
!assertioncondition. A stored session is a reason to leave the flow, not to stay on it. The existing controls still apply:shouldRedirectAfterSignUpis the opt-out for consumers that navigate themselves, and one that passes noafterSignUpUrlis never redirected here.Redirects are not instantaneous, so the completed step remains interactive for a moment.
BaseSignUpnow latches the completion and refuses further submissions — the execution id is spent by then, and re-submitting starts a fresh flow.Display-only completions are untouched: they return before the latch and never reach
handleComplete.Behaviour change worth calling out
A consumer that passes both
afterSignUpUrlandonComplete, relying on the assertion case not redirecting, will now be redirected. SettingshouldRedirectAfterSignUp={false}restores the previous behaviour and is the documented way to express "I navigate myself".Related Issues
Related PRs
Checklist
SignUp.test.tsx: a completion carrying an assertion reachesonComplete, and a second submit after completion issues no further flow request.Security checks
Verification
@thunderid/react: 67 tests pass,typecheckclean, no new lint errors on the touched files. Verified manually end to end against a local ThunderID instance with the server side fix applied: the flow completes,/users/mefires as the session hydration step, Gate redirects away, and no trailing execute request is made.The redirect branch is covered by pointing
afterSignUpUrlat a fragment of the current URL, which is a same-document navigation and so observable without the test page reloading. Confirmed the test fails when the!assertionguard is restored.