fix: logging in from video page #1082
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughIntroduces a two-step email → OTP sign-in flow in the video comment AuthOverlay, adds a new OtpForm component with verify/resend logic and rate-limiting, and refines UI text, sizing, and layout in the login and verify-otp forms. Removes the embedded Google sign-in path from the overlay. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant AO as AuthOverlay (StepOne)
participant NA as next-auth signIn("email")
participant M as Mailer
participant OF as OtpForm
participant API as /api/auth/callback/email
participant S as Session/Router
U->>AO: Submit email
AO->>NA: signIn("email", { email })
NA-->>AO: Ok (email sent)
AO->>U: Show "Email sent" (step = 2)
NA-->>M: Trigger email with code
M-->>U: Deliver 6-digit code
U->>OF: Enter/paste 6-digit code
OF->>API: Verify(email, token)
API-->>OF: Success
OF->>S: Refresh session/router
OF->>AO: Close modal
rect rgba(200,230,255,0.45)
note right of OF: Resend (client 30s rate-limit)
U->>OF: Click Resend
OF->>NA: signIn("email", { email })
NA-->>OF: Ok (update lastResendTime)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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: 3
🧹 Nitpick comments (1)
apps/web/app/s/[videoId]/_components/OtpForm.tsx (1)
52-52: Consider extracting mutation call to avoid auto-submission edge case.Auto-triggering verification when pasting 6 digits could be unexpected for users who might want to review the pasted code first.
Consider either removing the auto-submit or adding a small delay to give users time to review:
- if (index + value.length >= 5) handleVerify.mutate(undefined); + // Let users review the pasted code before auto-submitting + // if (index + value.length >= 5) handleVerify.mutate(undefined);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/app/(org)/login/form.tsx(3 hunks)apps/web/app/(org)/verify-otp/form.tsx(3 hunks)apps/web/app/s/[videoId]/_components/AuthOverlay.tsx(2 hunks)apps/web/app/s/[videoId]/_components/OtpForm.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)/login/form.tsxapps/web/app/(org)/verify-otp/form.tsxapps/web/app/s/[videoId]/_components/OtpForm.tsxapps/web/app/s/[videoId]/_components/AuthOverlay.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)/login/form.tsxapps/web/app/(org)/verify-otp/form.tsxapps/web/app/s/[videoId]/_components/OtpForm.tsxapps/web/app/s/[videoId]/_components/AuthOverlay.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)/login/form.tsxapps/web/app/(org)/verify-otp/form.tsxapps/web/app/s/[videoId]/_components/OtpForm.tsxapps/web/app/s/[videoId]/_components/AuthOverlay.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)/login/form.tsxapps/web/app/(org)/verify-otp/form.tsxapps/web/app/s/[videoId]/_components/OtpForm.tsxapps/web/app/s/[videoId]/_components/AuthOverlay.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)/login/form.tsxapps/web/app/(org)/verify-otp/form.tsxapps/web/app/s/[videoId]/_components/OtpForm.tsxapps/web/app/s/[videoId]/_components/AuthOverlay.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)/login/form.tsxapps/web/app/(org)/verify-otp/form.tsxapps/web/app/s/[videoId]/_components/OtpForm.tsxapps/web/app/s/[videoId]/_components/AuthOverlay.tsx
🧬 Code graph analysis (2)
apps/web/app/(org)/verify-otp/form.tsx (1)
packages/ui/src/components/icons/LogoBadge.tsx (1)
LogoBadge(1-28)
apps/web/app/s/[videoId]/_components/AuthOverlay.tsx (1)
packages/ui/src/components/icons/LogoBadge.tsx (1)
LogoBadge(1-28)
⏰ 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). (4)
- GitHub Check: Vercel Agent Review
- 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 (9)
apps/web/app/(org)/login/form.tsx (3)
160-161: LGTM! Size class standardization improves maintainability.Using the predefined
size-12class instead of explicit width/height values (w-[72px] h-[72px]) is more maintainable and ensures consistency across the UI.
462-471: LGTM! Fragment removal improves code clarity.Removing the unnecessary fragment wrapper simplifies the JSX structure without affecting functionality.
482-482: Copy improvement enhances clarity.The simplified message "Please enter your email." is more concise and clearer than the previous longer version.
apps/web/app/(org)/verify-otp/form.tsx (3)
159-159: LGTM! Consistent size class usage.Using
size-12here maintains consistency with the login form changes.
163-168: LGTM! Typography adjustments improve visual hierarchy.The reduced heading size (
text-xl) and body text (text-sm) create better visual balance for the OTP verification form.
171-194: LGTM! Flexible input layout improves responsiveness.The switch from fixed dimensions to flexible sizing (
flex-1with fixed height) creates a more responsive layout that adapts better to different screen sizes.apps/web/app/s/[videoId]/_components/AuthOverlay.tsx (2)
33-48: LGTM! Well-structured state reset for back navigation.The back button properly resets all relevant state when returning to the email input step, ensuring a clean user experience.
88-107: LGTM! Terms and privacy notice properly positioned.Moving the legal text outside the form steps ensures it's always visible regardless of the current authentication step.
apps/web/app/s/[videoId]/_components/OtpForm.tsx (1)
73-99: Verify server-side rate limiting on/api/auth/callback/email
The OTP verification mutation currently calls the NextAuth callback with no throttling, allowing unlimited retry attempts—ensure the endpoint is rate-limited server-side (e.g. via middleware or NextAuth customization) and optionally add a small client-side delay to hinder brute-force retries.
|
|
||
| <p className="mt-6 text-xs text-center text-gray-9"> | ||
| By verifying your email, you acknowledge that you have both read and | ||
| By entering your email, you acknowledge that you have both read and |
There was a problem hiding this comment.
Minor inconsistency in copy text.
The text says "By entering your email" but this is the OTP verification form where users are entering a code, not their email. Consider updating to "By verifying your email" to match the context.
- By entering your email, you acknowledge that you have both read and
+ By verifying your email, you acknowledge that you have both read and📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| By entering your email, you acknowledge that you have both read and | |
| By verifying your email, you acknowledge that you have both read and |
🤖 Prompt for AI Agents
In apps/web/app/(org)/verify-otp/form.tsx around line 224, the copy reads "By
entering your email" which is inconsistent for the OTP verification form; change
the wording to "By verifying your email" (or similar context-appropriate
phrasing) so the message matches that users are entering a verification code
rather than an email, and update any adjacent punctuation/capitalization to
match existing style.
| onClick={() => { | ||
| handleVerify.mutate(undefined); | ||
| }} |
There was a problem hiding this comment.
Redundant mutation call in onClick handler.
The button's onClick handler calls handleVerify.mutate(undefined) which is redundant since clicking the button will already trigger the mutation.
The mutation is already bound to the button click, so this explicit call is unnecessary.
🤖 Prompt for AI Agents
In apps/web/app/s/[videoId]/_components/OtpForm.tsx around lines 176 to 178, the
button's onClick handler calls handleVerify.mutate(undefined) redundantly;
remove that explicit call so the button relies on its existing mutation binding
(or form submit handler) to trigger the verify mutation. If any side-effect is
required on click, replace the mutate call with only that side-effect; otherwise
delete the onClick prop entirely to avoid duplicate mutation invocations.
Summary by CodeRabbit
New Features
UI/UX