fix(ui): remove ticket parameters on sign up continue - #9255
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: b6716cf The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-google-signin
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
📝 WalkthroughWalkthroughThe sign-up completion flow now removes Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/ui/src/components/SignUp/__tests__/SignUpStart.test.tsx`:
- Around line 510-529: Restore the original window.history object after the test
that defines the minimal history mock, ensuring cleanup runs even when the test
fails. Update the setup/teardown surrounding the window.history
Object.defineProperty and preserve the existing window.location cleanup
behavior.
🪄 Autofix (Beta)
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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: ba7e1ff5-47ed-487e-8426-0a48b8d3c2ee
📒 Files selected for processing (3)
.changeset/quiet-tickets-continue.mdpackages/ui/src/components/SignUp/SignUpContinue.tsxpackages/ui/src/components/SignUp/__tests__/SignUpStart.test.tsx
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)clerk/clerk-ios(auto-detected)clerk/clerk-android(auto-detected)clerk/cli(auto-detected)
| Object.defineProperty(window, 'location', { | ||
| configurable: true, | ||
| value: { | ||
| get href() { | ||
| return currentUrl.href; | ||
| }, | ||
| get search() { | ||
| return currentUrl.search; | ||
| }, | ||
| }, | ||
| }); | ||
| Object.defineProperty(window, 'history', { | ||
| configurable: true, | ||
| value: { | ||
| state: undefined, | ||
| replaceState: vi.fn((_state, _title, url) => { | ||
| currentUrl = new URL(url, currentUrl); | ||
| }), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for cleanup/restoration of window.location or window.history in this test file
rg -n "afterEach|afterAll|beforeEach" -A5 packages/ui/src/components/SignUp/__tests__/SignUpStart.test.tsx
rg -n "window.location|window.history" -B2 -A2 packages/ui/src/components/SignUp/__tests__/SignUpStart.test.tsxRepository: clerk/javascript
Length of output: 1862
🏁 Script executed:
#!/bin/bash
sed -n '500,640p' packages/ui/src/components/SignUp/__tests__/SignUpStart.test.tsxRepository: clerk/javascript
Length of output: 5422
🏁 Script executed:
#!/bin/bash
rg -n "afterEach|afterAll|beforeEach|window\.history|window\.location" packages/ui/src/components/SignUp/__tests__/SignUpStart.test.tsxRepository: clerk/javascript
Length of output: 652
Restore the window.history stub after this test.
window.location is reset later for the captcha cases, but window.history stays replaced with the minimal mock here, so any later test that expects the real History API can inherit the stubbed object and behave inconsistently.
🤖 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/ui/src/components/SignUp/__tests__/SignUpStart.test.tsx` around
lines 510 - 529, Restore the original window.history object after the test that
defines the minimal history mock, ensuring cleanup runs even when the test
fails. Update the setup/teardown surrounding the window.history
Object.defineProperty and preserve the existing window.location cleanup
behavior.
API Changes Report
Summary
No API Changes DetectedAll packages have stable APIs with no detected changes. Report generated by Break Check Last ran on |
| handleComplete: () => | ||
| clerk.setActive({ | ||
| handleComplete: () => { | ||
| removeClerkQueryParam('__clerk_ticket'); |
There was a problem hiding this comment.
This cleanup only runs when signUp.update() completes immediately. completeSignUpFlow forwards the ticket into email/phone verification and protect-check routes, whose terminal handlers call setActive without removing it, so those flows can still remount and attempt to consume an already-used ticket. Please apply the cleanup to every terminal sign-up completion path (preferably via a shared helper) and cover a continue → verification → activation case.
~ 🤖
There was a problem hiding this comment.
I was a bit nervous to impact ~all the sign up routes, but I feel like removing the tickets universally should be safe. So I updated to move the removal logic into completeSignUp instead of forcing each caller to individually remove the parameters.
There was a problem hiding this comment.
We don't HAVE to, I just wanted to call it out. Feel free to ignore for now.
2 participants
Description
This PR updates the
handleCompletecallback onSignUpContonueto remove Clerk's ticket query parameter before callingsetActive. This prevents a remount attempting to consume the already consumed ticket.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change