Merged
Conversation
feat: Added Email/Password signin and signup
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds email/password signup, signin, and password-reset flows across Firebase helpers, the AuthCard UI (multi-path auth + state/redirects), and the User model; Navbar hides auth CTAs on auth pages and Get Started links to /signup. Changes
Sequence DiagramsequenceDiagram
participant User
participant AuthCard
participant Firebase as "Firebase Auth"
participant DB as "MongoDB"
participant Context as "AuthContext"
User->>AuthCard: submit email + password (+ displayName) / request reset
AuthCard->>Firebase: signUpWithEmail / signInWithEmail / resetPassword
Firebase-->>AuthCard: returns User / ack reset
AuthCard->>DB: syncUserWithDB(user, 'email')
DB-->>AuthCard: user persisted
AuthCard->>Context: update auth state
Context-->>AuthCard: auth state confirmed
AuthCard-->>User: show success / redirect to dashboard
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/Navbar.tsx (1)
53-67:⚠️ Potential issue | 🟡 MinorPoint the primary CTA at
/signup.With the new email sign-up flow in this PR,
Get Startedstill routes unauthenticated users to/login, so the main navbar CTA drops new users on the wrong screen.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Navbar.tsx` around lines 53 - 67, Update the primary CTA in components/Navbar.tsx so unauthenticated users are routed to the signup flow: change the href on the "Get Started" Link (the second Link inside the JSX branch where isAuthPage ? null : !user) from "/login" to "/signup" while keeping the same className and styling; ensure the "Sign In" Link remains pointing to "/login" and only the "Get Started" Link is updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/AuthCard.tsx`:
- Around line 57-63: The redirect effect currently triggers on raw auth state
(useEffect using authLoading and currentUser); change it to wait for a
successful DB sync flag instead: introduce or use an existing "userSynced" /
"syncSuccess" boolean set by syncUserWithDB (or set on successful resolution
inside the sign-in flow), then update the useEffect condition to require
!authLoading && currentUser && userSynced before calling
router.push('/dashboard') and include userSynced in the dependency array;
likewise, update the sign-in success/unmount logic (the handler around
syncUserWithDB referenced at lines ~153-163) to set that same flag only on
successful sync and gate any unmount/next-step behavior on it so failures don’t
cause premature navigation or unmounting of AuthCard.
- Around line 91-116: The frontend now passes provider: 'email' from
handleEmailSubmit via syncUserWithDB but the backend model and API only allow
'google' | 'github', causing sync failures; update the backend contract to
include 'email' by: adding 'email' to the provider union/type in
src/lib/db/models/User.ts (the User model/type), and updating
app/api/user/route.ts to accept and validate 'email' in the upsert/write logic
so posted provider values from signInWithEmail/signUpWithEmail are persisted
without error. Ensure any type guards, DB upsert fields, and tests are adjusted
accordingly.
In `@src/lib/firebase/auth.ts`:
- Around line 79-84: signUpWithEmail creates the Firebase user first then
updates the profile, so if updateProfile(res.user, { displayName }) throws the
new account remains and future signups will error; catch errors from
updateProfile (and any post-create steps) and call deleteUser(res.user) to
remove the partially created account before rethrowing the error (use the
created user returned by createUserWithEmailAndPassword), ensuring you still
check auth presence and propagate the original error after cleanup.
---
Outside diff comments:
In `@components/Navbar.tsx`:
- Around line 53-67: Update the primary CTA in components/Navbar.tsx so
unauthenticated users are routed to the signup flow: change the href on the "Get
Started" Link (the second Link inside the JSX branch where isAuthPage ? null :
!user) from "/login" to "/signup" while keeping the same className and styling;
ensure the "Sign In" Link remains pointing to "/login" and only the "Get
Started" Link is updated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd91d81b-a365-4783-bd41-0ad93d39f116
📒 Files selected for processing (3)
components/AuthCard.tsxcomponents/Navbar.tsxsrc/lib/firebase/auth.ts
feat: add email/password auth, fix navbar redirect, harden signup rol…
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
components/AuthCard.tsx (1)
61-69:⚠️ Potential issue | 🟠 MajorA sync failure still gets treated like an authenticated success path.
The effect now waits during the in-flight attempt, but Line 174 still stops rendering the card as soon as
currentUserbecomes truthy, beforeuserSyncedis true. IfsyncUserWithDB()then fails, the error is stored in a component that returnsnull, and a later remount still redirects on raw auth state.🧭 Suggested direction
const [successMessage, setSuccessMessage] = useState<string | null>(null); const [userSynced, setUserSynced] = useState(false); + const [authFlowStarted, setAuthFlowStarted] = useState(false); useEffect(() => { - if (!authLoading && currentUser && !signingInRef.current) { + if (!authLoading && currentUser && (!authFlowStarted || userSynced)) { router.push('/dashboard'); } - if (userSynced) { - router.push('/dashboard'); - } - }, [authLoading, currentUser, userSynced, router]); + }, [authLoading, currentUser, userSynced, authFlowStarted, router]); setLoading(true); signingInRef.current = true; + setAuthFlowStarted(true); ... setIsLoadingEmail(true); signingInRef.current = true; + setAuthFlowStarted(true); ... - if (currentUser) return null; + if (currentUser && (!authFlowStarted || userSynced)) return null;Also applies to: 173-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/AuthCard.tsx` around lines 61 - 69, The redirect logic in the AuthCard effect and the render gating currently treats a truthy currentUser as success before sync completes, so change the useEffect and any render-return that hides the card to wait for userSynced (and no sync error) as well: only call router.push('/dashboard') when !authLoading && currentUser && userSynced && !signingInRef.current, and ensure any conditional that returns null (stopping render) also checks that syncUserWithDB succeeded (no sync error flag) so a failed sync doesn't cause a remount/instant redirect on raw auth state; update references to AuthCard, useEffect, signingInRef, currentUser, userSynced, syncUserWithDB and the sync error state accordingly.
🧹 Nitpick comments (1)
components/AuthCard.tsx (1)
139-160: Add a real pending state for password-reset submits.This flow never flips a reset-specific loading flag, so the button stays enabled during slow responses and can fire multiple reset emails.
⏳ Suggested change
const [isLoadingGitHub, setIsLoadingGitHub] = useState(false); const [isLoadingEmail, setIsLoadingEmail] = useState(false); + const [isResettingPassword, setIsResettingPassword] = useState(false); ... - const isLoading = isLoadingGoogle || isLoadingGitHub || isLoadingEmail; + const isLoading = isLoadingGoogle || isLoadingGitHub || isLoadingEmail || isResettingPassword; ... const handlePasswordReset = useCallback(async (e: React.FormEvent) => { e.preventDefault(); setSignInError(null); setSuccessMessage(null); if (!email.trim()) { setSignInError('Please enter your email address.'); return; } + setIsResettingPassword(true); try { await resetPassword(email); setSuccessMessage('Password reset email sent! Check your inbox.'); setShowResetPassword(false); } catch (error) { setSignInError( error instanceof Error ? error.message : 'Failed to send reset email.' ); + } finally { + setIsResettingPassword(false); } }, [email]);Also applies to: 220-223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/AuthCard.tsx` around lines 139 - 160, The password-reset flow lacks a dedicated loading flag; update handlePasswordReset to set and clear a "resetting" state (e.g., isResetting / setIsResetting) around the async call to resetPassword so the UI can disable the reset button while pending and prevent duplicate submissions, and mirror the same pattern in the other reset handler referenced (lines 220-223); ensure you setIsResetting(true) before awaiting resetPassword, setIsResetting(false) in both success and catch branches (or finally), and tie the reset button's disabled/aria-busy props to isResetting while still calling setSignInError/setSuccessMessage and setShowResetPassword as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/AuthCard.tsx`:
- Around line 100-137: The signup flow can leave a partially-created Firebase
auth user if syncUserWithDB fails; hoist the user variable out of the try so it
remains visible in the catch, and when mode === 'signup' and user was created
but syncUserWithDB throws, perform cleanup by deleting the just-created auth
account (or at minimum sign the user out) before surfacing the error; update
handleEmailSubmit to declare let user: User | null outside the try, check for a
created user in the catch, call the appropriate cleanup (deleteUser or signOut
via your auth SDK), then rethrow or setSignInError, ensuring signingInRef and
setIsLoadingEmail are still cleared in the finally block.
---
Duplicate comments:
In `@components/AuthCard.tsx`:
- Around line 61-69: The redirect logic in the AuthCard effect and the render
gating currently treats a truthy currentUser as success before sync completes,
so change the useEffect and any render-return that hides the card to wait for
userSynced (and no sync error) as well: only call router.push('/dashboard') when
!authLoading && currentUser && userSynced && !signingInRef.current, and ensure
any conditional that returns null (stopping render) also checks that
syncUserWithDB succeeded (no sync error flag) so a failed sync doesn't cause a
remount/instant redirect on raw auth state; update references to AuthCard,
useEffect, signingInRef, currentUser, userSynced, syncUserWithDB and the sync
error state accordingly.
---
Nitpick comments:
In `@components/AuthCard.tsx`:
- Around line 139-160: The password-reset flow lacks a dedicated loading flag;
update handlePasswordReset to set and clear a "resetting" state (e.g.,
isResetting / setIsResetting) around the async call to resetPassword so the UI
can disable the reset button while pending and prevent duplicate submissions,
and mirror the same pattern in the other reset handler referenced (lines
220-223); ensure you setIsResetting(true) before awaiting resetPassword,
setIsResetting(false) in both success and catch branches (or finally), and tie
the reset button's disabled/aria-busy props to isResetting while still calling
setSignInError/setSuccessMessage and setShowResetPassword as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7440cca7-f33d-4928-83ab-1448db126fcb
📒 Files selected for processing (4)
components/AuthCard.tsxcomponents/Navbar.tsxsrc/lib/db/models/User.tssrc/lib/firebase/auth.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- components/Navbar.tsx
Comment on lines
+129
to
+144
| export const resetPassword = async (email: string) => { | ||
| if (!auth) throw new Error("Firebase Auth is not initialized."); | ||
| try { | ||
| await sendPasswordResetEmail(auth, email); | ||
| } catch (error) { | ||
| const authError = error as AuthError; | ||
| console.error("Password reset error:", { | ||
| code: authError.code, | ||
| message: authError.message, | ||
| }); | ||
| if (authError.code === 'auth/user-not-found') { | ||
| throw new Error("No account found with this email."); | ||
| } | ||
| throw new Error(authError.message || "Failed to send reset email"); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Avoid revealing whether an email is registered from the reset flow.
This returns a distinct user-facing outcome when no account exists, which makes email enumeration trivial from the reset screen. Keep the external response the same for existent and nonexistent addresses, and switch the success copy in components/AuthCard.tsx to something generic like “If an account exists, you’ll receive a reset link.”
🔐 Suggested change
export const resetPassword = async (email: string) => {
if (!auth) throw new Error("Firebase Auth is not initialized.");
try {
await sendPasswordResetEmail(auth, email);
} catch (error) {
const authError = error as AuthError;
console.error("Password reset error:", {
code: authError.code,
message: authError.message,
});
- if (authError.code === 'auth/user-not-found') {
- throw new Error("No account found with this email.");
- }
- throw new Error(authError.message || "Failed to send reset email");
+ if (authError.code === 'auth/user-not-found') {
+ return;
+ }
+ throw new Error("Failed to send reset email");
}
};📝 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.
Suggested change
| export const resetPassword = async (email: string) => { | |
| if (!auth) throw new Error("Firebase Auth is not initialized."); | |
| try { | |
| await sendPasswordResetEmail(auth, email); | |
| } catch (error) { | |
| const authError = error as AuthError; | |
| console.error("Password reset error:", { | |
| code: authError.code, | |
| message: authError.message, | |
| }); | |
| if (authError.code === 'auth/user-not-found') { | |
| throw new Error("No account found with this email."); | |
| } | |
| throw new Error(authError.message || "Failed to send reset email"); | |
| } | |
| }; | |
| export const resetPassword = async (email: string) => { | |
| if (!auth) throw new Error("Firebase Auth is not initialized."); | |
| try { | |
| await sendPasswordResetEmail(auth, email); | |
| } catch (error) { | |
| const authError = error as AuthError; | |
| console.error("Password reset error:", { | |
| code: authError.code, | |
| message: authError.message, | |
| }); | |
| if (authError.code === 'auth/user-not-found') { | |
| return; | |
| } | |
| throw new Error("Failed to send reset email"); | |
| } | |
| }; |
fix(auth): handle signup sync failures, fix redirect race, add reset …
There was a problem hiding this comment.
♻️ Duplicate comments (1)
components/AuthCard.tsx (1)
63-71:⚠️ Potential issue | 🟠 MajorPrevent premature/blocked redirects by managing
syncFailedin provider flows.
handleProviderSignInnever resetssyncFailedat start and never sets it on failure. That leaves redirect behavior inconsistent: provider sync failures can still navigate, and stale failures can block later success paths.Suggested fix
const handleProviderSignIn = useCallback(async ( signInFn: () => Promise<User | null>, setLoading: (v: boolean) => void, provider: 'google' | 'github' ) => { setSignInError(null); setSuccessMessage(null); + setSyncFailed(false); setLoading(true); signingInRef.current = true; try { const user = await signInFn(); if (!user) { setSignInError(`Failed to sign in with ${provider}. Please try again.`); return; } await syncUserWithDB(user, provider); setUserSynced(true); } catch (error) { + setSyncFailed(true); setSignInError( error instanceof Error ? error.message : `Failed to sign in with ${provider}. Please try again.` ); } finally { signingInRef.current = false; setLoading(false); } }, []);Also applies to: 73-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/AuthCard.tsx` around lines 63 - 71, The redirect logic in AuthCard's useEffect is inconsistent because handleProviderSignIn never resets syncFailed at the start nor sets it on failure; update handleProviderSignIn to clear syncFailed (set false) immediately when starting a provider sign-in (and set signingInRef appropriately), and on any sync error set syncFailed = true (and stop signingInRef), so the useEffect conditions (checking syncFailed, userSynced, currentUser, signingInRef) behave deterministically; ensure the same reset/set behavior is applied to other provider flows referenced in this file so stale syncFailed flags don't block later successful sign-ins or allow premature redirects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@components/AuthCard.tsx`:
- Around line 63-71: The redirect logic in AuthCard's useEffect is inconsistent
because handleProviderSignIn never resets syncFailed at the start nor sets it on
failure; update handleProviderSignIn to clear syncFailed (set false) immediately
when starting a provider sign-in (and set signingInRef appropriately), and on
any sync error set syncFailed = true (and stop signingInRef), so the useEffect
conditions (checking syncFailed, userSynced, currentUser, signingInRef) behave
deterministically; ensure the same reset/set behavior is applied to other
provider flows referenced in this file so stale syncFailed flags don't block
later successful sign-ins or allow premature redirects.
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.
Summary by CodeRabbit
New Features
Improvements