Improve email handling in OTP verification flow (fix for Windows) #1120
Conversation
WalkthroughAdds email fallback logic in the verify-OTP form to read email from URL when prop is absent, updating API calls, sign-in, and UI text to use the derived value. The page removes session-based redirects and now guards only on the presence of searchParams.email, redirecting to /login if missing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant P as VerifyOTP Page
participant F as VerifyOTP Form
participant A as Auth/API
U->>P: Navigate to /verify-otp?email=...
P->>P: Check searchParams.email
alt email missing
P-->>U: redirect /login
else email present
P->>F: Render with email from searchParams
F->>F: Derive verifyEmail (prop or URL)
U->>F: Enter OTP and submit
F->>A: Verify OTP with verifyEmail
A-->>F: Success/Failure
opt Resend flow
U->>F: Click Resend
F->>A: Sign in / resend with verifyEmail
A-->>F: Result
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/app/(org)/verify-otp/page.tsx (1)
14-14: Remove unnecessary optional chaining.The optional chaining on
searchParamsis unnecessary since it's awaited from a Promise on line 12 and will always resolve to an object (empty{}if no params). The check should be!searchParams.emailinstead.Apply this diff:
- if (!searchParams?.email) { + if (!searchParams.email) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/app/(org)/verify-otp/form.tsx(5 hunks)apps/web/app/(org)/verify-otp/page.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for all client-side server state and data fetching in the web app
Web mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData rather than broad invalidations
Client code should use useEffectQuery/useEffectMutation and useRpcClient from apps/web/lib/EffectRuntime.ts; do not create ManagedRuntime inside components
Files:
apps/web/app/(org)/verify-otp/form.tsxapps/web/app/(org)/verify-otp/page.tsx
apps/web/app/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer Server Components for initial data in the Next.js App Router and pass initialData to client components
Files:
apps/web/app/(org)/verify-otp/form.tsxapps/web/app/(org)/verify-otp/page.tsx
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
apps/web/app/(org)/verify-otp/form.tsxapps/web/app/(org)/verify-otp/page.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; leverage shared types from packages
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.
Files:
apps/web/app/(org)/verify-otp/form.tsxapps/web/app/(org)/verify-otp/page.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/web/app/(org)/verify-otp/form.tsxapps/web/app/(org)/verify-otp/page.tsx
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
On the client, always use
useEffectQueryoruseEffectMutationfrom@/lib/EffectRuntime; never callEffectRuntime.run*directly in components.
Files:
apps/web/app/(org)/verify-otp/form.tsxapps/web/app/(org)/verify-otp/page.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (4)
apps/web/app/(org)/verify-otp/form.tsx (4)
90-90: Consistent with verifyEmail usage.The change correctly uses
verifyEmailand maintains proper encoding. However, if the email fallback logic (lines 30-39) is removed as recommended, this should revert to using the
116-116: LGTM! Magic number extracted.Good refactor removing the inline comment and extracting the magic number to a named constant.
127-127: Consistent with verifyEmail usage.The change correctly uses
verifyEmailin the resend flow. However, if the email fallback logic (lines 30-39) is removed as recommended, this should revert to using the
175-175: Consistent with verifyEmail usage.The change correctly displays
verifyEmailin the UI. However, if the email fallback logic (lines 30-39) is removed as recommended, this should revert to using the
| let verifyEmail = email; | ||
| if (!email && typeof window !== 'undefined') { | ||
| const urlParams = new URLSearchParams(window.location.search); | ||
| const urlEmail = urlParams.get('email'); | ||
| if (urlEmail) { | ||
| verifyEmail = urlEmail; | ||
| } else { | ||
| router.push('/login'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: SSR hydration mismatch and imperative redirect in render.
This code has several critical issues:
- Hydration mismatch: Accessing
window.location.searchduring render in a client component causes server/client HTML mismatch since server-rendered output won't have the URL param logic. - Race condition:
router.push('/login')is async but execution continues, potentially using an undefinedverifyEmail. - Render-phase side effect: Router navigation should occur in
useEffect, not during render. - Redundant guard: The parent page component already redirects when
emailis missing, making this fallback unnecessary.
Recommended approach: Remove this client-side fallback entirely since page.tsx line 14-16 already guards the email requirement. The email prop will always be present when this component renders.
If fallback is needed for some edge case, use this pattern:
- let verifyEmail = email;
- if (!email && typeof window !== 'undefined') {
- const urlParams = new URLSearchParams(window.location.search);
- const urlEmail = urlParams.get('email');
- if (urlEmail) {
- verifyEmail = urlEmail;
- } else {
- router.push('/login');
- }
- }
+ const searchParams = useSearchParams();
+ const verifyEmail = email || searchParams.get('email') || '';
+
+ useEffect(() => {
+ if (!verifyEmail) {
+ router.push('/login');
+ }
+ }, [verifyEmail, router]);However, given the page-level guard, this entire block should be removed and email used directly.
🤖 Prompt for AI Agents
In apps/web/app/(org)/verify-otp/form.tsx around lines 30 to 39, remove the
client-side fallback that reads window.location.search and calls router.push
during render; instead use the incoming email prop directly (the page guard
ensures it is always present). Do not access window or call router.push in
render; if you absolutely need a client-side fallback for an edge case,
implement it with useEffect and local state (read URLSearchParams and then set
state or navigate), but prefer simply deleting this block so no SSR hydration
mismatch, race condition, or render-phase side effects remain.
Refactored OTP verification to retrieve email from URL parameters if not provided, and redirect to login if missing. Updated API calls and UI to use the resolved email. Removed unused session check from the OTP page.
On Windows I was unable to sign in on Chrome. This seems to have done the trick.
Summary by CodeRabbit