Logout API Integration #57
Conversation
|
Warning Rate limit exceeded@vidyaaKhandekar has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 31 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve modifications to several components and services related to user authentication and logout functionality. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Navbar
participant AuthService
participant Home
User->>Navbar: Click Logout
Navbar->>AuthService: Call logoutUser()
AuthService->>localStorage: Remove tokens
AuthService-->>Navbar: Return success
Navbar->>User: Show success toast
Navbar->>Home: Redirect to home page
Home->>User: Refresh page
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (6)
src/components/common/layout/Navbar.tsx (1)
35-50: Add loading state during logoutThe logout process should indicate to users that an action is in progress, preventing multiple clicks and improving user experience.
const Navbar: React.FC<{ isMenu?: boolean }> = ({ isMenu = true }) => { const [success] = useState<string>(""); + const [isLoggingOut, setIsLoggingOut] = useState(false); const navigate = useNavigate(); const { t } = useTranslation(); const toast = useToast(); const handleLogout = async () => { + if (isLoggingOut) return; + setIsLoggingOut(true); try { const response = await logoutUser(); // ... rest of the code } catch (error) { // ... error handling } finally { + setIsLoggingOut(false); } };src/screens/auth/SignIn.tsx (1)
Line range hint
35-51: Consider removing redundant token stateThe token state appears unnecessary since tokens are already stored in localStorage. Consider removing it unless needed for specific UI rendering.
- const [token, setToken] = useState(); const handleLogin = async () => { try { setLoading(true); const response = await loginUser({ username, password }); if (response) { toast({ title: t("SIGNIN_LOGGEDIN_SUCCESSFULLY"), status: "success", duration: 3000, isClosable: true, }); - setToken(response); localStorage.setItem("authToken", response.data.access_token); localStorage.setItem("refreshToken", response.data.refresh_token); navigate(0); }🧰 Tools
🪛 eslint
[error] 54-54: 'error' is defined but never used.
(@typescript-eslint/no-unused-vars)
src/screens/Home.tsx (1)
53-54: Improve error loggingConsider using a proper error logging service instead of console.log for better error tracking in production.
} catch (error) { - console.log(error); + // TODO: Implement proper error logging service + console.error('Logout failed:', error); toast({src/services/auth/auth.tsx (3)
24-25: Remove debugging console.log statementProduction code should not contain console.log statements. Consider using proper logging mechanisms if logging is required.
- console.log("response.data", response.data);
54-55: Consider centralizing token managementToken management (storage and removal) is scattered across the codebase. Consider creating a dedicated token management utility to handle these operations consistently.
Example utility:
// utils/tokenManager.ts export const TokenManager = { clearTokens: () => { localStorage.removeItem("authToken"); localStorage.removeItem("refreshToken"); }, getTokens: () => ({ accessToken: localStorage.getItem("authToken"), refreshToken: localStorage.getItem("refreshToken") }) };
Line range hint
38-67: Enhance security with proper error handling and token validationWhile the logout implementation is generally secure, consider these security enhancements:
- Validate token format before making the API call
- Clear tokens even if the API call fails
- Add proper error typing for better error handling
Example implementation:
export const logoutUser = async () => { try { const tokens = TokenManager.getTokens(); if (!tokens.accessToken || !tokens.refreshToken) { throw new Error("Missing authentication tokens"); } await axios.post( `${apiBaseUrl}/auth/logout`, { access_token: tokens.accessToken, refresh_token: tokens.refreshToken, }, { headers: { "Content-Type": "application/json", }, } ); return true; } catch (error) { // Clear tokens even if API call fails console.error("Logout failed:", error); } finally { TokenManager.clearTokens(); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/components/common/layout/Navbar.tsx(1 hunks)src/screens/Home.tsx(3 hunks)src/screens/auth/SignIn.tsx(3 hunks)src/services/auth/auth.tsx(4 hunks)
🧰 Additional context used
🪛 eslint
src/services/auth/auth.tsx
[error] 2-2: 'useNavigate' is defined but never used.
(@typescript-eslint/no-unused-vars)
| const handleLogout = async () => { | ||
| localStorage.removeItem("authToken"); | ||
| navigate("/"); | ||
| navigate(0); | ||
| // localStorage.removeItem("authToken"); | ||
|
|
||
| // keycloak.logout({ redirectUri: window.location.origin }); |
There was a problem hiding this comment.
Consider clearing local tokens regardless of API response
For security reasons, it's recommended to clear local tokens even if the API call fails. This ensures the user's session is terminated locally even in case of network issues.
const handleLogout = async () => {
+ const clearLocalTokens = () => {
+ localStorage.removeItem("authToken");
+ localStorage.removeItem("refreshToken");
+ };
try {
const response = await logoutUser();
if (response) {
+ clearLocalTokens();
navigate("/");
navigate(0);
}
} catch (error) {
+ clearLocalTokens();
console.log(error);
toast({Committable suggestion skipped: line range outside the PR's diff.
| @@ -63,6 +66,7 @@ const SignIn: React.FC = () => { | |||
| const handleBack = () => { | |||
| navigate("/"); | |||
| }; | |||
| console.log("response", token); | |||
There was a problem hiding this comment.
Remove sensitive data logging
Console logging of authentication tokens poses a security risk as it exposes sensitive information in browser logs.
- console.log("response", token);| @@ -1,10 +1,11 @@ | |||
| import React, { useContext, useEffect, useState } from "react"; | |||
| import { Box, VStack } from "@chakra-ui/react"; | |||
| import { Box, useToast, VStack } from "@chakra-ui/react"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the results, I can now generate the final response:
Logout implementation is consistent but needs cleanup
The logout implementation using logoutUser service is consistent across components, but there's a commented-out token removal in Navbar that should be cleaned up:
src/components/common/layout/Navbar.tsx: Remove commented code forlocalStorage.removeItem("authToken")- Main logout logic in
src/services/auth/auth.tsxcorrectly removes both tokens - Both
Home.tsxandNavbar.tsxcomponents handle logout consistently with proper navigation
🔗 Analysis chain
Verify consistent logout implementation
Ensure that the logout implementation is consistent across all components using the logoutUser service.
Also applies to: 8-8
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of logoutUser and localStorage token removal
echo "Checking logoutUser usage:"
rg "logoutUser" -A 5
echo "\nChecking localStorage token removal:"
rg "localStorage\.removeItem\(.*Token" -A 2
Length of output: 2632
| const handleConsent = async () => { | ||
| setConsentSaved(!consentSaved); | ||
| keycloak.logout(); | ||
| localStorage.removeItem("authToken"); | ||
| try { | ||
| const response = await logoutUser(); | ||
| if (response) { | ||
| navigate("/"); | ||
| navigate(0); | ||
| } | ||
| } catch (error) { | ||
| console.log(error); | ||
| toast({ | ||
| title: "Logout failed", | ||
| status: "error", | ||
| duration: 3000, | ||
| isClosable: true, | ||
| description: "Try Again", | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Separate consent and logout logic
The handleConsent function now handles logout functionality, which violates the Single Responsibility Principle. Consider separating these concerns.
- const handleConsent = async () => {
+ const handleConsentClose = () => {
setConsentSaved(!consentSaved);
+ };
+
+ const handleLogout = async () => {
try {
const response = await logoutUser();
if (response) {
+ localStorage.removeItem("authToken");
+ localStorage.removeItem("refreshToken");
navigate("/");
navigate(0);
}
} catch (error) {
console.log(error);
toast({
title: "Logout failed",
status: "error",
duration: 3000,
isClosable: true,
description: "Try Again",
});
}
};Then update the dialogue component:
{consentSaved && (
<CommonDialogue
isOpen={consentSaved}
- onClose={handleConsent}
+ onClose={handleConsentClose}
termsAndConditions={termsAndConditions}
handleDialog={saveConsent}
/>
)}Committable suggestion skipped: line range outside the PR's diff.
| export const logoutUser = async () => { | ||
| const accessToken = localStorage.getItem("authToken"); | ||
| const refreshToken = localStorage.getItem("refreshToken"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null checks and type safety for localStorage tokens
The tokens retrieved from localStorage should be checked for null values and properly typed.
- const accessToken = localStorage.getItem("authToken");
- const refreshToken = localStorage.getItem("refreshToken");
+ const accessToken = localStorage.getItem("authToken");
+ const refreshToken = localStorage.getItem("refreshToken");
+
+ if (!accessToken || !refreshToken) {
+ return Promise.reject(new Error("Missing authentication tokens"));
+ }📝 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.
| export const logoutUser = async () => { | |
| const accessToken = localStorage.getItem("authToken"); | |
| const refreshToken = localStorage.getItem("refreshToken"); | |
| export const logoutUser = async () => { | |
| const accessToken = localStorage.getItem("authToken"); | |
| const refreshToken = localStorage.getItem("refreshToken"); | |
| if (!accessToken || !refreshToken) { | |
| return Promise.reject(new Error("Missing authentication tokens")); | |
| } |
src/services/auth/auth.tsx
Outdated
| @@ -1,5 +1,5 @@ | |||
| import axios from "axios"; | |||
| import { removeToken } from "./asyncStorage"; | |||
| import { useNavigate } from "react-router-dom"; | |||
There was a problem hiding this comment.
Remove unused import and maintain service layer architecture
The useNavigate hook import is unused and should be removed. Additionally, React hooks should not be used in service files as they are meant for React components only.
-import { useNavigate } from "react-router-dom";🧰 Tools
🪛 eslint
[error] 2-2: 'useNavigate' is defined but never used.
(@typescript-eslint/no-unused-vars)
|
Logout API Integration
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation