Added Toast for Login and signup #45
Conversation
WalkthroughAdds react-toastify usage across App, SignIn, and SignUp; replaces console logs with toast.success/toast.error for auth outcomes; SignIn useEffect now returns the onAuthStateChanged unsubscribe; SignUp uses Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App
participant SignIn
participant SignUp
participant Auth
participant Toast
participant Router
rect rgba(230,240,255,0.6)
note over App: Global ToastContainer added
end
User->>SignIn: submit credentials / click Google
SignIn->>Auth: signInWithEmailAndPassword / signInWithPopup
alt success
Auth-->>SignIn: userCredential
SignIn->>Toast: toast.success(...)
SignIn->>Router: navigate(...)
else error
Auth-->>SignIn: error(code)
SignIn->>Toast: toast.error(getAuthErrorMessage(code))
end
User->>SignUp: submit signup form
SignUp->>Auth: createUserWithEmailAndPassword(email, values.password)
alt success
Auth-->>SignUp: userCredential
SignUp->>Toast: toast.success(...)
SignUp->>Router: navigate(/signin) after toast onClose
else error
Auth-->>SignUp: error(code)
SignUp->>Toast: toast.error(getSignUpErrorMessage(code))
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
♻️ Duplicate comments (1)
src/pages/SignIn.js (1)
71-72: Duplicate ToastContainer: use single instance at app root.This is the same issue as in
SignUp.js(line 60). You have multipleToastContainerinstances across different page components. As mentioned in theSignUp.jsreview, best practice is to renderToastContaineronce at the application root.See the detailed recommendation in the
SignUp.jsreview (lines 59-60) for how to refactor this.Based on learnings.
🧹 Nitpick comments (5)
src/pages/SignUp.js (3)
4-6: Optional: CSS import is redundant in react-toastify v11.The library auto-injects its stylesheet in v11.x, so the explicit CSS import on line 6 is optional. You can remove it unless you need explicit control over CSS injection timing.
Apply this diff if you want to remove the redundant import:
-import 'react-toastify/dist/ReactToastify.css'; // Important: Import the CSSBased on learnings.
23-31: Simplify toast configuration using defaults.The toast options are verbose and duplicated. Since you're already using
<ToastContainer />(line 60), you can configure defaults there and simplify individual toast calls.Apply this diff to simplify:
// 3. Call the success toast - toast.success("Account created successfully!", { - position: "top-right", - autoClose: 5000, - hideProgressBar: false, - closeOnClick: true, - pauseOnHover: true, - draggable: true, - progress: undefined, - }); + toast.success("Account created successfully!");// 3. Call the error toast - toast.error(error.message, { - position: "top-right", - autoClose: 5000, - hideProgressBar: false, - closeOnClick: true, - pauseOnHover: true, - draggable: true, - progress: undefined, - }); + toast.error(error.message);Then configure defaults on the
ToastContainerat line 60:- <ToastContainer /> + <ToastContainer + position="top-right" + autoClose={5000} + hideProgressBar={false} + closeOnClick + pauseOnHover + draggable + />This matches the simpler approach used in
SignIn.jsand reduces code duplication.Also applies to: 40-47
59-60: Consider using a single ToastContainer at app root.You're adding
ToastContainerin bothSignUp.jsandSignIn.js(line 72). Best practice is to renderToastContaineronce at the application root (e.g., inApp.js) rather than in individual page components. This ensures consistent configuration and avoids potential conflicts.Move the
ToastContainerto your app's root component:In
App.js(or your root component):import { ToastContainer } from 'react-toastify'; import 'react-toastify/dist/ReactToastify.css'; function App() { return ( <> <ToastContainer position="top-right" autoClose={5000} hideProgressBar={false} closeOnClick pauseOnHover draggable /> {/* Your routes and other components */} </> ); }Then remove the
ToastContainerfrom bothSignUp.jsandSignIn.js. The toast calls (toast.success(),toast.error()) will still work because the container is rendered globally.Based on learnings.
src/pages/SignIn.js (2)
12-14: Optional: CSS import is redundant in react-toastify v11.The library auto-injects its stylesheet in v11.x, so the explicit CSS import on line 14 is optional. You can remove it unless you need explicit control over CSS injection timing.
Apply this diff if you want to remove the redundant import:
-import 'react-toastify/dist/ReactToastify.css'; // Import the CSSBased on learnings.
28-36: LGTM with minor note on comment clarity.The empty dependency array is correct here—you want the auth state listener to be set up once on mount. The
Navigatefunction fromuseNavigateis stable across renders, so omitting it is safe. The comment on line 36 could be more confident: "should likely" → "should" or "is correct".Optional: Update the comment for clarity:
- }, []); // Note: The dependency array should likely be empty here + }, []); // Empty dependency array: set up auth listener once on mount
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonand included by nonepackage.jsonis excluded by none and included by none
📒 Files selected for processing (2)
src/pages/SignIn.js(3 hunks)src/pages/SignUp.js(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/pages/SignIn.js (3)
src/pages/SignUp.js (2)
Navigate(17-17)auth(16-16)src/firebase.js (2)
auth(33-33)auth(33-33)src/components/Form/FormContainer.js (1)
FormContainer(4-40)
src/pages/SignUp.js (1)
src/firebase.js (2)
auth(33-33)auth(33-33)
🔇 Additional comments (4)
src/pages/SignUp.js (1)
20-20: Correct fix: using the right password field.Good catch! Using
values.passwordinstead ofvalues.confirmPasswordfor account creation is the correct implementation.src/pages/SignIn.js (3)
45-52: Clean toast implementation.The simplified toast calls (without verbose configuration objects) are much cleaner than the approach in
SignUp.js. This is a good pattern.
57-66: LGTM!Consistent toast implementation for email/password sign-in. The user feedback flow is clear and the code is maintainable.
70-70: Nice text improvement.The title change from "Sign in for an account!" to "Sign in to your account!" is more natural and grammatically correct.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/pages/SignIn.js (1)
53-63: Same consideration for user-friendly error messages applies here.Similar to the Google sign-in handler, consider mapping Firebase error codes to user-friendly messages for better UX in the email/password sign-in flow.
🧹 Nitpick comments (5)
src/pages/SignUp.js (3)
19-35: Good fix: onClose callback prevents memory leak.The use of
onClose: () => Navigate("/signin")on line 25 properly addresses the previous concern about potential memory leaks from setTimeout. The navigation now occurs only after the toast completes, and no cleanup is needed if the component unmounts.Minor: Consider removing the empty lines at 22, 24, 26, and 34 for cleaner formatting.
56-56: Consider centralizing ToastContainer at the app level.Since the SignIn page (per the AI summary) also renders its own
ToastContainer, you may have multiple instances. Best practice is to renderToastContaineronce at the application root (e.g., inApp.jsor a layout component) rather than in individual pages. This prevents potential conflicts and ensures consistent toast behavior across the app.If you move the
ToastContainerto your app root, remove it from this component:- <ToastContainer />Then add it once in your main App component:
// In App.js or root component import { ToastContainer } from 'react-toastify'; function App() { return ( <> <ToastContainer /> {/* Your routes and other components */} </> ); }
4-5: Remove CSS import and centralize ToastContainer
- Delete
import 'react-toastify/dist/ReactToastify.css';(v11+ auto-injects styles).- Move
<ToastContainer>from SignUp.js into your root App component so notifications render globally.src/pages/SignIn.js (2)
13-13: Remove CSS import for react-toastify v11.x.React-toastify v11.x automatically injects styles when
ToastContaineris rendered, so the CSS import is no longer required.Based on learnings
Apply this diff:
-import 'react-toastify/dist/ReactToastify.css';
42-51: Consider user-friendly error messages.Firebase error messages (e.g.,
auth/popup-closed-by-user,auth/invalid-email) may be too technical for end users. Consider mapping common Firebase errors to user-friendly messages for better UX.Example:
const SignInWithGoogle = () => { const provider = new GoogleAuthProvider(); signInWithPopup(auth, provider) .then(() => { toast.success("Successfully logged in!"); }) .catch((err) => { const friendlyMessage = getFriendlyErrorMessage(err.code) || err.message; toast.error(friendlyMessage); }); }; // Helper function const getFriendlyErrorMessage = (errorCode) => { const errorMessages = { 'auth/popup-closed-by-user': 'Sign-in cancelled. Please try again.', 'auth/network-request-failed': 'Network error. Please check your connection.', 'auth/too-many-requests': 'Too many attempts. Please try again later.', }; return errorMessages[errorCode]; };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/pages/SignIn.js(2 hunks)src/pages/SignUp.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/pages/SignIn.js (2)
src/firebase.js (2)
auth(33-33)auth(33-33)src/components/Form/FormContainer.js (1)
FormContainer(4-40)
src/pages/SignUp.js (1)
src/firebase.js (2)
auth(33-33)auth(33-33)
🔇 Additional comments (2)
src/pages/SignUp.js (1)
36-47: LGTM: Error handling is appropriate.The error toast configuration is well-structured, and the 5-second autoClose duration is appropriate for error messages (giving users more time to read than the 2-second success message).
src/pages/SignIn.js (1)
66-66: LGTM! Improved title text.The title change from "Sign in for an account!" to "Sign in to your account!" is grammatically correct and more natural.
src/pages/SignIn.js
Outdated
| useEffect(() => { | ||
| onAuthStateChanged(auth, (user) => { | ||
| const unsubscribe = onAuthStateChanged(auth, (user) => { | ||
| if (user) { | ||
| Navigate("/main"); | ||
| } | ||
| }); | ||
| // eslint-disable-next-line | ||
| }, [auth]); | ||
|
|
||
| return () => unsubscribe(); | ||
| }, [Navigate]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use empty dependency array for mount-only effect.
The Navigate function from useNavigate() is stable and doesn't require inclusion in the dependency array. This effect should run once on mount to set up the auth state listener.
Apply this diff:
useEffect(() => {
const unsubscribe = onAuthStateChanged(auth, (user) => {
if (user) {
Navigate("/main");
}
});
return () => unsubscribe();
- }, [Navigate]);
+ }, []); 📝 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.
| useEffect(() => { | |
| onAuthStateChanged(auth, (user) => { | |
| const unsubscribe = onAuthStateChanged(auth, (user) => { | |
| if (user) { | |
| Navigate("/main"); | |
| } | |
| }); | |
| // eslint-disable-next-line | |
| }, [auth]); | |
| return () => unsubscribe(); | |
| }, [Navigate]); | |
| useEffect(() => { | |
| const unsubscribe = onAuthStateChanged(auth, (user) => { | |
| if (user) { | |
| Navigate("/main"); | |
| } | |
| }); | |
| return () => unsubscribe(); | |
| }, []); |
🤖 Prompt for AI Agents
In src/pages/SignIn.js around lines 28 to 36, the useEffect currently lists
[Navigate] as a dependency causing unnecessary re-runs; change the dependency
array to an empty array ([]) so the auth state listener is registered only once
on mount. Keep the onAuthStateChanged unsubscribe return as-is and do not
include Navigate in the dependency list since useNavigate() returns a stable
function.
| return ( | ||
| <FormContainer Icon={LoginIcon} title="Sign in for an account!"> | ||
| <FormContainer Icon={LoginIcon} title="Sign in to your account!"> | ||
| <ToastContainer position="top-right" autoClose={5000} hideProgressBar={false} /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move ToastContainer to app root level.
Rendering ToastContainer inside individual page components can lead to multiple container instances when navigating between pages (e.g., SignIn ↔ SignUp), potentially causing duplicate toasts or inconsistent behavior.
Recommended approach:
- Remove
ToastContainerfrom this component (and fromSignUp.js) - Add a single
ToastContainerat the app root level (typically inApp.jsorindex.js)
Example in App.js:
import { ToastContainer } from 'react-toastify';
function App() {
return (
<>
<ToastContainer
position="top-right"
autoClose={5000}
hideProgressBar={false}
/>
{/* Your routes and components */}
</>
);
}Then you can call toast.success() or toast.error() from any component without needing to render ToastContainer in each one.
🤖 Prompt for AI Agents
In src/pages/SignIn.js around line 67, the ToastContainer is rendered inside the
SignIn page which can produce multiple instances and duplicate toasts when
navigating; remove the ToastContainer from this file (and similarly from
SignUp.js) and add a single ToastContainer at the app root (e.g., App.js or
index.js) with the same props (position="top-right", autoClose={5000},
hideProgressBar={false}) so all components can call toast.* without rendering
their own container.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/pages/SignIn.js (1)
28-36: Use empty dependency array for mount-only effect.The
Navigatefunction fromuseNavigate()is stable and doesn't require inclusion in the dependency array. This effect should run once on mount to set up the auth state listener.Apply this diff:
useEffect(() => { const unsubscribe = onAuthStateChanged(auth, (user) => { if (user) { Navigate("/main"); } }); return () => unsubscribe(); - }, [Navigate]); + }, []);
🧹 Nitpick comments (1)
src/pages/SignIn.js (1)
12-13: Remove ToastContainer import and consider removing CSS import.Since
ToastContaineris now rendered at the app root level (inApp.js), you only need thetoastimport here. Additionally, react-toastify v11 auto-injects CSS, making the manual CSS import optional.Based on learnings
Apply this diff:
-import { ToastContainer, toast } from 'react-toastify'; -import 'react-toastify/dist/ReactToastify.css'; +import { toast } from 'react-toastify';
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/App.js(2 hunks)src/pages/SignIn.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/SignIn.js (2)
src/firebase.js (2)
auth(33-33)auth(33-33)src/components/Form/FormContainer.js (1)
FormContainer(4-40)
🔇 Additional comments (2)
src/App.js (1)
13-19: LGTM: Global ToastContainer placement is correct.Once the import is added, this global ToastContainer placement follows best practices and aligns with the recommended approach from previous reviews. The configuration props are appropriate for typical toast notifications.
src/pages/SignIn.js (1)
66-66: LGTM: Title text improved.The updated title "Sign in to your account!" is grammatically correct and clearer than the previous version.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/pages/SignUp.js (1)
13-24: Consider consolidating error message helpers.The
getSignUpErrorMessagehelper provides good UX by mapping Firebase error codes to user-friendly messages. However, SignIn.js likely has a similargetAuthErrorMessagehelper. Consider extracting these into a shared utility file (e.g.,utils/firebaseErrors.js) to reduce duplication and maintain consistency across authentication flows.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/pages/SignIn.js(2 hunks)src/pages/SignUp.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/SignIn.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/SignUp.js (1)
src/firebase.js (2)
auth(33-33)auth(33-33)
🔇 Additional comments (4)
src/pages/SignUp.js (4)
4-5: LGTM!The react-toastify imports are correct and follow the library's standard usage pattern.
32-32: LGTM!Using
values.password(rather thanconfirmPassword) is correct. TheconfirmPasswordfield is solely for validation purposes, whilepasswordis the actual credential for account creation.
34-37: LGTM!The navigation pattern using
onCloseavoids the previous memory leak concern withsetTimeout. The toast will complete its autoClose cycle before navigation, providing a smooth UX.
40-46: LGTM!The error handling correctly maps Firebase errors to user-friendly messages and displays them via toast, while still logging the original error for debugging purposes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/pages/SignUp.js (3)
4-4: Remove unused ToastContainer import.The
ToastContainercomponent is no longer rendered in this file (correctly removed as per previous review feedback, sinceApp.jsprovides the global container). The import should be cleaned up.Apply this diff:
-import { ToastContainer, toast } from 'react-toastify'; +import { toast } from 'react-toastify';
5-5: Optional: CSS import is redundant in react-toastify v11.In react-toastify v11, the CSS is automatically injected by default, so this manual import is technically redundant. However, it's harmless and some teams prefer explicit imports for clarity.
Based on learnings.
If you prefer to rely on the auto-injection, you can remove this line:
-import 'react-toastify/dist/ReactToastify.css';
13-24: Well-structured error message mapping.The
getSignUpErrorMessagehelper provides clear, user-friendly error messages for common Firebase auth errors. This is good UX and follows best practices for error handling.If this pattern is used in other authentication flows (e.g., SignIn), consider extracting this to a shared utility file like
utils/authErrors.jsto avoid duplication.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/SignUp.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/SignUp.js (1)
src/firebase.js (2)
auth(33-33)auth(33-33)
🔇 Additional comments (3)
src/pages/SignUp.js (3)
32-32: Correct use of password field.Good fix! Now correctly uses
values.passwordinstead ofvalues.confirmPasswordfor account creation, which aligns with Firebase's createUserWithEmailAndPassword API expectations.
34-37: Excellent navigation pattern with toast.The implementation correctly uses
autoClose: 2000with anonClosecallback to navigate after the toast is dismissed. This addresses the previous memory leak concern from the setTimeout pattern and ensures the user sees the success message before navigation.
40-46: Proper error handling with user-friendly messages.The error flow correctly maps Firebase errors to user-friendly messages via the helper function and displays them in a toast. Logging the full error to console is also helpful for debugging.
|
@ctrixcode can u merge now...i resolved all conflicts |
|
@coderabbitai review PR now |
|
@ctrixcode: I'll review the PR now and provide feedback on the toast notification implementation for login and signup functionality. ✅ Actions performedReview triggered.
|
|
@ctrixcode i think no more imprvments sugggestd by Coderabbit |
|
Why removed auth from deps? @Saikiran-Sugurthi |
2 participants
Done with adding Toasts for Signup and SignIn ... i have just worked on frontend and have not disturbed any backend code ...so the toast is showing firebase error on my local machine..it works fine on deployement ..
Summary by CodeRabbit
New Features
Bug Fixes
Chores