[refactor] 로그인 상태 관리 수정 #279
Conversation
|
Warning Rate limit exceeded@BellYun has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 30 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. 📒 Files selected for processing (3)
WalkthroughThe changes integrate a centralized authentication management system. A new route is added in Changes
Sequence Diagram(s)AuthCallback FlowsequenceDiagram
participant U as User
participant A as AuthCallback
participant S as AuthStore
participant SR as Server
U->>A: Navigate to /Auth
A->>S: Execute checkAuth()
S->>SR: GET /user
SR-->>S: User data or error
alt Authenticated
S-->>A: Return user data
A->>U: Redirect to "/"
else Authentication Failed
S-->>A: Return error
A->>A: Display error message
end
Axios Interceptor Token RefreshsequenceDiagram
participant C as Client (axios)
participant S as AuthStore
participant SR as Server
C->>SR: API Request
SR-->>C: 401 Unauthorized
C->>C: Check retry flag
C->>SR: POST /reissue (token refresh)
alt Token refresh successful
SR-->>C: New token
C->>SR: Retry original API Request
SR-->>C: API Response
else Token refresh failed
SR-->>C: Error response
C->>S: Call logout()
S-->>C: Logout complete
end
Suggested reviewers
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 (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (8)
frontend/src/pages/AuthCallback.tsx (1)
1-41: Provide a re-login option for a better user experience.The component cleanly handles authentication checks and error states. For a smoother user flow, consider rendering a small “Re-login” link or button in case of errors, so users can easily navigate back to a login page. Otherwise, this callback logic looks solid.
frontend/src/store/authStore.ts (2)
23-23: Remove console.log statements.These debug statements should be removed before merging to production.
- console.log(get().hasChecked); if (get().hasChecked) return; try { const response = await axiosInstance.get("/user"); set({ isAuthenticated: true, userData: response.data?.result, hasChecked: true, }); - console.log(response);Also applies to: 33-33
34-37: Enhance error handling.The error handling could be more robust:
- In
checkAuth: Consider logging the error details before throwing- In
logout: The error is logged but silently ignored, which could lead to inconsistent state} catch (error) { + console.error("Authentication check failed:", error); set({ isAuthenticated: false, userData: null, hasChecked: true }); throw error; } }, logout: async () => { try { await axiosInstance.post("/logout"); set({ isAuthenticated: false, userData: null, hasChecked: false }); } catch (error) { console.error("로그아웃 실패!", error); + throw error; // Propagate the error to handle it in the UI }Also applies to: 43-45
frontend/src/App.tsx (1)
30-30: Consider consistent route path naming.The path
/Authuses PascalCase while other routes use camelCase or lowercase. Consider using a consistent naming convention across routes.- <Route path="/Auth" element={<AuthCallback />} /> + <Route path="/auth" element={<AuthCallback />} />frontend/src/utils/Token.tsx (1)
2-4: Clean up unused code and imports.
- Remove commented-out code as it's now handled by the Zustand store
- Remove unused imports and variables:
axiosfrom "./axiosInstance"useStatefrom "react"useLoginStatus, useUserInfofrom "../store/LoginStore"BASE_URLconstantimport { Outlet, Navigate } from "react-router-dom"; -import axios from "./axiosInstance"; -import { useEffect, useState } from "react"; -import { useLoginStatus, useUserInfo } from "../store/LoginStore"; +import { useEffect } from "react"; import useAuthStore from "../store/authStore"; -const BASE_URL = import.meta.env.VITE_API_BASE_URL; function ProtectedRoute() { - // const [isAuthenticated, setIsAuthenticated] = useState<boolean | null>(null); - // const { setLoginStatus } = useLoginStatus(); - // const { setUserData } = useUserInfo(); const { checkAuth, isAuthenticated } = useAuthStore(); useEffect(() => { - // const checkAuth = async () => { - // try { - // const response = await axios.get(`${BASE_URL}/user`, { - // withCredentials: true, // HTTP-Only 쿠키 포함 - // }); - - // // 응답 코드가 USER_200일 경우 인증 성공 - // if (response.data?.code === "USER_200") { - // setIsAuthenticated(true); - // setLoginStatus(1); - // setUserData(response.data?.result); - // } else { - // setIsAuthenticated(false); - // setLoginStatus(0); - // } - // } catch (error) { - // console.error("인증 요청 실패: ", error); - // setIsAuthenticated(false); - // setLoginStatus(0); - // } - // }; - // checkAuth(); checkAuth(); }, []);Also applies to: 7-7, 10-37
🧰 Tools
🪛 GitHub Check: build (20.x)
[failure] 2-2:
'axios' is defined but never used
[failure] 3-3:
'useState' is defined but never used
[failure] 4-4:
'useLoginStatus' is defined but never used
[failure] 4-4:
'useUserInfo' is defined but never usedfrontend/src/components/comment/CommentSection.tsx (1)
80-80: Simplify the ternary expression.The ternary expression for the
disabledprop can be simplified.- disabled={isLogin ? false : true} + disabled={!isLogin}frontend/src/components/common/Navbar.tsx (2)
10-10: Remove unused constant.The
BASE_URLconstant is defined but never used in this file.-const BASE_URL = import.meta.env.VITE_API_BASE_URL;🧰 Tools
🪛 GitHub Check: build (20.x)
[failure] 10-10:
'BASE_URL' is assigned a value but never used
14-14: Consider translating Korean comments for consistency.For better maintainability and collaboration, consider translating Korean comments to English:
- const [searchText, setSearchText] = useState<string>(""); // 검색어 상태 관리 + const [searchText, setSearchText] = useState<string>(""); // Search text state management - {/* 검색 바 */} + {/* Search Bar */}Also applies to: 68-68
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/src/App.tsx(2 hunks)frontend/src/api/axios.config.ts(2 hunks)frontend/src/components/MyInfoPage/UserInfo.tsx(1 hunks)frontend/src/components/comment/CommentSection.tsx(3 hunks)frontend/src/components/common/Navbar.tsx(3 hunks)frontend/src/pages/AuthCallback.tsx(1 hunks)frontend/src/store/authStore.ts(1 hunks)frontend/src/utils/Token.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Check: build (20.x)
frontend/src/components/comment/CommentSection.tsx
[warning] 41-41:
React Hook useEffect has a missing dependency: 'isAuthenticated'. Either include it or remove the dependency array
frontend/src/utils/Token.tsx
[failure] 7-7:
'BASE_URL' is assigned a value but never used
frontend/src/components/common/Navbar.tsx
[failure] 10-10:
'BASE_URL' is assigned a value but never used
🔇 Additional comments (5)
frontend/src/api/axios.config.ts (1)
6-6: Consolidate auth store usage.Great job importing the
useAuthStoremodule. This ensures a consistent approach to authentication state across the codebase.frontend/src/components/MyInfoPage/UserInfo.tsx (1)
2-5: Switching touseAuthStoreimproves maintainability.Replacing
useUserInfowithuseAuthStorealigns this component with the new centralized auth store. This keeps user data fetching consistent across the app.frontend/src/components/common/Navbar.tsx (3)
8-8: LGTM! Good job integrating zustand for auth state management.The integration of authStore aligns well with the PR objectives of centralizing login state management.
16-20: LGTM! Clean implementation of auth state management.The destructured auth store values and useEffect implementation look good. The dependency on checkAuth ensures proper auth state synchronization.
93-103: LGTM! Clean implementation of conditional rendering based on auth state.The conditional rendering using
isAuthenticatedand displaying theuservalue from the auth store is implemented correctly.
| //재발급 로직 | ||
| axiosInstance.interceptors.response.use( | ||
| (response) => response, | ||
| async (error) => { | ||
| const originalRequest = error.config; | ||
|
|
||
| if (error.response?.status === 401 && !originalRequest._retry) { | ||
| originalRequest._retry = true; | ||
| try { | ||
| // 토큰 갱신 요청 | ||
| await axiosInstance.post(BASE_URL + "/reissue"); | ||
| return axiosInstance(originalRequest); | ||
| } catch (error) { | ||
| const logout = useAuthStore.getState().logout; | ||
| logout(); | ||
| return Promise.reject(error); | ||
| } | ||
| } | ||
| } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Propagate errors for non-401 responses.
When an error is not 401, the interceptor currently returns undefined, which may result in unhandled rejections. Explicitly return a rejected promise to surface non-401 errors back to the caller.
Apply this diff to ensure consistent error propagation:
axiosInstance.interceptors.response.use(
(response) => response,
async (error) => {
const originalRequest = error.config;
+ // If it's not an unretired 401, just reject and propagate the error.
+ if (!(error.response?.status === 401 && !originalRequest._retry)) {
+ return Promise.reject(error);
+ }
if (error.response?.status === 401 && !originalRequest._retry) {
originalRequest._retry = true;
try {
await axiosInstance.post(BASE_URL + "/reissue");
return axiosInstance(originalRequest);
} catch (error) {
const logout = useAuthStore.getState().logout;
logout();
return Promise.reject(error);
}
}
}
);📝 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.
| //재발급 로직 | |
| axiosInstance.interceptors.response.use( | |
| (response) => response, | |
| async (error) => { | |
| const originalRequest = error.config; | |
| if (error.response?.status === 401 && !originalRequest._retry) { | |
| originalRequest._retry = true; | |
| try { | |
| // 토큰 갱신 요청 | |
| await axiosInstance.post(BASE_URL + "/reissue"); | |
| return axiosInstance(originalRequest); | |
| } catch (error) { | |
| const logout = useAuthStore.getState().logout; | |
| logout(); | |
| return Promise.reject(error); | |
| } | |
| } | |
| } | |
| ); | |
| //재발급 로직 | |
| axiosInstance.interceptors.response.use( | |
| (response) => response, | |
| async (error) => { | |
| const originalRequest = error.config; | |
| // If it's not an unretired 401, just reject and propagate the error. | |
| if (!(error.response?.status === 401 && !originalRequest._retry)) { | |
| return Promise.reject(error); | |
| } | |
| if (error.response?.status === 401 && !originalRequest._retry) { | |
| originalRequest._retry = true; | |
| try { | |
| // 토큰 갱신 요청 | |
| await axiosInstance.post(BASE_URL + "/reissue"); | |
| return axiosInstance(originalRequest); | |
| } catch (error) { | |
| const logout = useAuthStore.getState().logout; | |
| logout(); | |
| return Promise.reject(error); | |
| } | |
| } | |
| } | |
| ); |
| useEffect(() => { | ||
| if (!isAuthenticated) { | ||
| setIsLogin(false); | ||
| } else { | ||
| setIsLogin(true); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
Add missing dependency to useEffect.
The useEffect hook is missing isAuthenticated in its dependency array, which could lead to stale state.
useEffect(() => {
if (!isAuthenticated) {
setIsLogin(false);
} else {
setIsLogin(true);
}
- }, []);
+ }, [isAuthenticated]);📝 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(() => { | |
| if (!isAuthenticated) { | |
| setIsLogin(false); | |
| } else { | |
| setIsLogin(true); | |
| } | |
| }, []); | |
| useEffect(() => { | |
| if (!isAuthenticated) { | |
| setIsLogin(false); | |
| } else { | |
| setIsLogin(true); | |
| } | |
| }, [isAuthenticated]); |
🧰 Tools
🪛 GitHub Check: build (20.x)
[warning] 41-41:
React Hook useEffect has a missing dependency: 'isAuthenticated'. Either include it or remove the dependency array
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/common/Navbar.tsx (1)
91-91: Simplify boolean comparison.The explicit comparison
isAuthenticated === trueis redundant and can be simplified.- {isAuthenticated === true ? ( + {isAuthenticated ? (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/common/Navbar.tsx(3 hunks)frontend/src/utils/Token.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/utils/Token.tsx
🔇 Additional comments (4)
frontend/src/components/common/Navbar.tsx (4)
8-8: Good addition of the authStore import.The import of the centralized authStore aligns with the PR objective of refactoring login state management to use Zustand.
14-14: Clean implementation of authStore.Using destructured values from the authStore is a clean approach to access authentication state. This effectively replaces the previous local state management for login status and username.
100-100:❓ Verification inconclusive
Verify user property type.
The display has changed from
userNametouser. Ensure thatusercontains the appropriate string value to display the username.
🏁 Script executed:
#!/bin/bash # Check the type and structure of the user property in authStore rg -A 5 -B 5 "user(\s|:)" --type ts src/store/authStore.tsLength of output: 123
Action Required: Verify Authentication Store Reference and User Property Type
The review comment points out that the display value in the Navbar has been changed from
userNametouserand instructs verifying that theuserproperty contains the proper string for displaying the username. The initial grep search forsrc/store/authStore.tsreturned a "No such file or directory" error, suggesting that either the file has been moved/renamed or the property is defined elsewhere.Please:
- Verify the correct file that defines the
userproperty. The above script searches the repository for any TypeScript files named "authStore". If the authentication store file has been relocated or renamed (e.g., toauth.ts), update references accordingly.- Confirm that the
userproperty indeed holds a string value suitable for display in the Navbar.Once you have located the authentication store file and verified that
useris correctly typed, the review comment can be considered addressed.
🏁 Script executed:
#!/bin/bash # Search for any TypeScript files with "authStore" in their name anywhere in the repository. echo "Searching for authStore-related files:" fd -t f "authStore" --extension ts echo "" # Additionally, search for files under src/store that mention an authentication property echo "Searching for auth-related files in src/store:" fd -t f -e ts src/store | grep -i auth echo "" # Search for 'user' property definitions in the store folder for better context. echo "Searching for 'user' property definitions in src/store:" rg -A 5 -B 5 -t ts "user\s*:" src/storeLength of output: 538
Action: Verify "user" Property Type in Auth Store
The Navbar now displays the
userproperty instead ofuserName. We found the authentication store file atfrontend/src/store/authStore.ts, but our initial search didn’t locate the direct definition of theuserproperty. Please confirm that within this file theuserproperty is declared and that it holds a string value appropriate for display in the Navbar. You may need to update its type or ensure it is correctly set if it was intended to replaceuserName.
16-18:❓ Verification inconclusive
Verify memoization of checkAuth.
The dependency array correctly includes
checkAuth, but this might cause the effect to run on every render if the function isn't memoized in the authStore.
🏁 Script executed:
#!/bin/bash # Let's check if checkAuth is memoized in the authStore rg -A 10 -B 10 "checkAuth" --type ts src/store/authStore.tsLength of output: 122
Below is a shell script to search the entire repository for any definitions or memoization patterns related to
checkAuth(e.g. usage ofuseCallback) so we can determine whether it’s properly memoized. This should help us verify whether its inclusion in the dependency array may lead to unintended re-renders:Action Required: Verify
checkAuthMemoizationThe
useEffecthook infrontend/src/components/common/Navbar.tsxdepends oncheckAuth. Since the previous search againstsrc/store/authStore.tsdid not return any results, it's unclear whethercheckAuthis memoized. Please run the above script to locate its definition and confirm that it’s memoized (for example, usinguseCallback). IfcheckAuthisn’t memoized, it might be recreated on each render, causing the effect to run more often than expected.
- If
checkAuthis not memoized: Wrap it using a memoization hook (e.g.,useCallback) in its definition.- If it is memoized elsewhere: Please add a brief comment clarifying that memoization occurs in its definition to aid future reviews.
🏁 Script executed:
[run_scripts] #!/bin/bash echo "Searching for function definitions of checkAuth and memoization patterns ..." # Look for function declarations named checkAuth rg -n "function checkAuth\s*\(" --type ts || true # Look for const declarations (possibly arrow functions) rg -n "const checkAuth\s*=" --type ts || true # Look for memoization via useCallback wrapping checkAuth rg -n "useCallback.*checkAuth" --type ts || trueLength of output: 596
ACTION: Verify the Memoization of
checkAuthThe
useEffectinfrontend/src/components/common/Navbar.tsxcallscheckAuthand depends on it. Our search shows thatcheckAuthis derived from the auth store via:const checkAuth = useAuthStore((state) => state.checkAuth);found in
frontend/src/pages/AuthCallback.tsx. No explicit memoization (e.g., wrapping withuseCallback) was detected. This raises the possibility that ifcheckAuthisn’t maintained as a stable reference by the store, the effect in Navbar might execute on every render.
- Verify: Ensure the auth store returns a stable reference for
checkAuth.- Action: If it isn’t already stable, consider memoizing
checkAuth(e.g., wrapping it inuseCallback) so that the dependency does not inadvertently trigger repeated effect executions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/components/common/Navbar.tsx (1)
91-91: Boolean comparison can be simplified.Since
isAuthenticatedis already a boolean value, the explicit comparison withtrueis redundant.- {isAuthenticated === true ? ( + {isAuthenticated ? (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/components/common/Navbar.tsx(3 hunks)frontend/tsconfig.app.tsbuildinfo(1 hunks)frontend/tsconfig.node.tsbuildinfo(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/tsconfig.node.tsbuildinfo
- frontend/tsconfig.app.tsbuildinfo
🧰 Additional context used
🪛 GitHub Actions: CI
frontend/src/components/common/Navbar.tsx
[error] 100-100: TypeScript error TS2552: Cannot find name 'user'. Did you mean 'User'?
🔇 Additional comments (1)
frontend/src/components/common/Navbar.tsx (1)
8-14: Good architectural change to centralized state management.The shift from local state management to Zustand-based auth store follows good software design principles by centralizing authentication logic. This makes the authentication state accessible across the application and simplifies the component implementation.
|
|
||
| function Navbar() { | ||
| const navigate = useNavigate(); | ||
| const [searchText, setSearchText] = useState<string>(""); // 검색어 상태 관리 | ||
| const { setSearchName } = useSearchStore(); | ||
| const { loginStatus, setLoginStatus } = useLoginStatus(); | ||
| const [userName, setUserName] = useState<string>(""); | ||
| const { checkAuth, isAuthenticated } = authStore(); |
There was a problem hiding this comment.
Missing destructured user variable from authStore.
The code is trying to use a variable named user on line 100, but it's not being destructured from the authStore. According to the pipeline failure, this is causing a TypeScript error.
- const { checkAuth, isAuthenticated } = authStore();
+ const { checkAuth, isAuthenticated, user } = authStore();📝 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.
| const { checkAuth, isAuthenticated } = authStore(); | |
| const { checkAuth, isAuthenticated, user } = authStore(); |
변경 사항
로그인 상태 관리 로직을 zustand 내부로 위치를 옮겼습니다.
로그인 상태를 사용하고 싶다면 zustand를 활용해서 사용하면 됩니다.
Summary by CodeRabbit