fixed unnecessary log in option after getting logged in (#205) #219
Conversation
|
@Amnyadav is attempting to deploy a commit to the bunty's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for submitting your pull request! 🙌 We'll review it as soon as possible. In the meantime, please ensure that your changes align with our CONTRIBUTING.md. If there are any specific instructions or feedback regarding your PR, we'll provide them here. Thanks again for your contribution! 😊 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
frontend/src/components/Pages/Login.jsx (2)
37-38: Consider simplifying token storageThe token storage implementation is correct, but it can be simplified:
Consider applying this change:
-const token=result.token; -localStorage.setItem("token",JSON.stringify(token)); +localStorage.setItem("token", result.token);This change eliminates the need for JSON stringification, as the token is likely already a string. It also improves readability and reduces the risk of parsing errors when retrieving the token later.
Line range hint
1-105: Enhance security, error handling, and user experienceWhile the overall implementation is solid, consider the following improvements:
Security Enhancement:
Instead of storing the entire token in localStorage, consider storing only a session identifier and keeping the token in memory. This approach reduces the risk of XSS attacks.// After successful login const sessionId = generateSessionId(); // Implement this function localStorage.setItem("sessionId", sessionId); sessionStorage.setItem("token", result.token);Improved Error Handling:
Provide more specific error messages based on the API response. This will help users understand and resolve issues more easily.if (!response.ok) { const errorMessage = result.message || 'Login failed'; if (response.status === 401) { throw new Error('Invalid email or password. Please try again.'); } else if (response.status === 429) { throw new Error('Too many login attempts. Please try again later.'); } else { throw new Error(errorMessage); } }Password Visibility Toggle:
Add a button to toggle password visibility, improving user experience.const [showPassword, setShowPassword] = useState(false); // In the JSX, replace the password input with: <div className="relative w-full"> <input className="input w-full h-10 rounded-md border-2 border-black bg-beige shadow-[4px_4px_0px_0px_black] text-[15px] font-semibold text-[#323232] p-2.5 focus:outline-none focus:border-[#2d8cf0] placeholder-[#666] placeholder-opacity-80" name="password" placeholder="Password" type={showPassword ? "text" : "password"} onChange={(e) => handleChange(e)} /> <button type="button" className="absolute inset-y-0 right-0 pr-3 flex items-center" onClick={() => setShowPassword(!showPassword)} > {showPassword ? "Hide" : "Show"} </button> </div>These changes will significantly improve the security, user experience, and error handling of your login component.
frontend/src/components/Shared/Navbar.jsx (5)
Line range hint
5-5: Remove unusedisloggedInstate variableThe state variable
isloggedInis no longer used in the component. Removing it will clean up the code.Apply this diff to remove the unused state variable:
-const [isloggedIn, setisloggedIn] = useState(false);
40-40: Remove commented-out code related toisloggedInThe commented-out line setting
isloggedIntofalseis no longer needed and can be removed to clean up the code.Apply this diff to remove the unused code:
-// setisloggedIn(false); // Set isLoggedIn to false on confirmation
41-41: Improve comment for clarity and grammarConsider rephrasing the comment for better clarity and proper grammar.
Apply this diff to update the comment:
-//managing log in , logout using jwt tokens +// Managing login and logout using JWT tokens
44-45: Add missing semicolons for consistencyLines 44 and 45 are missing semicolons at the end of the statements. Adding them ensures consistency and prevents potential issues.
Apply this diff to add the missing semicolons:
setIsModalOpen(false); // Close the modal -setIsMenuOpen(false) // after getting logged out close the menu if it is open +setIsMenuOpen(false); // After logging out, close the menu if it is open -navigate("/login")//navigate to login after get logged out +navigate("/login"); // Navigate to login after logging out
Line range hint
104-109: Refactor duplicated code for login/logout buttons into a reusable componentThe logic for rendering the login/logout buttons in both the desktop menu (lines 104-109) and the mobile menu (lines 183-189) is duplicated. Consider creating a reusable component or function to reduce duplication and improve maintainability.
Example: Create an
AuthButtoncomponentFirst, create a new component within your
Navbar.jsxfile or separate it into its own file:const AuthButton = ({ baseTextColorClass, hoverTextColorClass }) => { const navigate = useNavigate(); const handleLogoutClick = () => { setIsModalOpen(true); }; return token ? ( <button className={`${baseTextColorClass} ${hoverTextColorClass} px-4 py-1 rounded-md border-2 border-black bg-beige shadow-[4px_4px_0px_0px_black] font-semibold text-[#323232]`} type="button" onClick={handleLogoutClick} > LOGOUT </button> ) : ( <button className={`${baseTextColorClass} ${hoverTextColorClass} px-4 py-1 rounded-md border-2 border-black bg-beige shadow-[4px_4px_0px_0px_black] font-semibold text-[#323232]`} type="button" onClick={() => navigate('/login')} > LOGIN </button> ); };Then, replace the duplicated code in the desktop menu (line 104) and mobile menu (line 183) with the new
AuthButtoncomponent:Desktop Menu:
{/* Existing code */} <div className="hidden md:flex font-semibold Poppins text-lg"> - {token ? ( - <button /* ... */> - LOGOUT - </button> - ) : ( - <button /* ... */> - LOGIN - </button> - )} + <AuthButton baseTextColorClass={baseTextColorClass} hoverTextColorClass={hoverTextColorClass} /> </div>Mobile Menu:
{/* Existing code */} {isMenuOpen && ( <div /* ... */> <div className="px-4 pt-4 pb-4 space-y-2"> {/* Menu items */} - {token ? ( - <button /* ... */> - Log Out - </button> - ) : ( - <button /* ... */> - Log In - </button> - )} + <AuthButton baseTextColorClass={mobileMenuBaseTextColorClass} hoverTextColorClass="hover:bg-amber-300 hover:text-black" /> </div> </div> )}This refactoring reduces code duplication and makes the authentication button easier to manage.
Also applies to: 183-189
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- frontend/src/components/Pages/Login.jsx (1 hunks)
- frontend/src/components/Shared/Navbar.jsx (4 hunks)
🧰 Additional context used
|
@Amnyadav resolve code rabit suggestions |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
frontend/package.json (1)
25-25: LGTM! Consider adding a comment for clarity.The addition of the
js-cookiepackage (version ^3.0.5) is appropriate for handling JWT tokens as mentioned in the PR objectives. This change aligns well with the goal of improving user authentication.Consider adding a brief comment in the
package.jsonfile to explain the purpose of this new dependency. For example:{ ... "dependencies": { ... + // Used for handling JWT tokens for authentication "js-cookie": "^3.0.5", ... }, ... }This can help other developers understand why this package was added without needing to refer to the PR description.
frontend/src/components/Pages/Login.jsx (1)
37-40: Approved: Token storage moved to cookies, but minor formatting issue.The change to store the authentication token in cookies instead of local storage is a good security improvement. The cookie is correctly set with an expiration time and the secure flag.
However, there's a minor formatting issue:
Consider adjusting the indentation to match the rest of the function:
- Cookies.set('authToken',result.token,{ - expire:'1h', - secure:true - }); + Cookies.set('authToken', result.token, { + expire: '1h', + secure: true + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- frontend/package.json (1 hunks)
- frontend/src/components/Pages/Login.jsx (2 hunks)
- frontend/src/components/Shared/Navbar.jsx (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/Shared/Navbar.jsx
🧰 Additional context used
🔇 Additional comments (2)
frontend/src/components/Pages/Login.jsx (2)
5-5: LGTM: Cookie library import added correctly.The import statement for the Cookies library is correctly placed and follows proper naming conventions.
Line range hint
1-108: Verify consistency of authentication flow across the application.The change to use cookies for token storage is a good improvement. However, to ensure a consistent authentication flow:
- Verify that the logout functionality in other components (e.g., Navbar) has been updated to remove the cookie instead of clearing local storage.
- Check if any other components that previously accessed the token from local storage have been updated to read from cookies.
This will help maintain a consistent and secure authentication system throughout the application.
To assist in this verification, you can run the following script:
This script will help identify any remaining localStorage usage for authentication, verify cookie usage in other components, and locate logout functionality that may need updating.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
RamakrushnaBiswal
added
bug
RamakrushnaBiswal
added
level2
fixed unnecessary log in option after getting logged in .
now you got the log out option there instead of log in.
i have used jwt token to get to know if user is log in or not based no that i select what option i have to show .
Summary by CodeRabbit
New Features
Bug Fixes
Style