refactor: 로그인 상태 확인 전 페이지 이동 문제 수정 #286
Conversation
areumH
added
✔︎pull requests
Walkthrough이 풀 리퀘스트는 토큰 새로 고침 상태 관리를 개선하기 위한 변경 사항을 포함하고 있습니다. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🧹 Nitpick comments (2)
src/store/auth.ts (1)
26-28: 리듀서 네이밍 개선 제안setRefreshing 리듀서의 이름이 다소 모호할 수 있습니다.
다음과 같이 더 명확한 이름을 사용하는 것을 제안합니다:
- setRefreshing: (state, action: PayloadAction<boolean>) => { + setTokenRefreshingStatus: (state, action: PayloadAction<boolean>) => { state.isRefreshing = action.payload; },src/App.tsx (1)
53-53: 토큰 갱신 중 UI 렌더링 방지토큰 갱신 중에는 null을 반환하여 UI 렌더링을 방지하는 것은 좋은 접근입니다. 하지만 사용자 경험 개선이 필요합니다.
토큰 갱신 중임을 사용자에게 알리는 로딩 인디케이터를 추가하는 것을 제안합니다:
- if (isTokenRefreshing) return null; + if (isTokenRefreshing) { + return ( + <div className="flex items-center justify-center h-screen"> + <CircularProgress /> + </div> + ); + }Also applies to: 65-66
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/App.tsx(3 hunks)src/components/Routes/ProtectedRoutes.tsx(0 hunks)src/hooks/common/useTokenRefresh.ts(1 hunks)src/store/auth.ts(2 hunks)
💤 Files with no reviewable changes (1)
- src/components/Routes/ProtectedRoutes.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: storybook
🔇 Additional comments (3)
src/hooks/common/useTokenRefresh.ts (1)
16-19: 토큰이 존재할 때 불필요한 API 호출 방지토큰이 이미 존재하는 경우 불필요한 API 호출을 방지하고 즉시 refreshing 상태를 false로 설정하는 최적화가 잘 되어있습니다.
src/store/auth.ts (2)
6-6: 토큰 갱신 상태 추적을 위한 상태 추가TokenState 인터페이스에 isRefreshing 상태를 추가하고 초기값을 true로 설정한 것은 적절합니다. 이를 통해 앱 초기 로딩 시 토큰 갱신 프로세스를 명확하게 추적할 수 있습니다.
Also applies to: 11-11
20-20: 토큰 설정/삭제 시 자동 상태 업데이트토큰이 설정되거나 삭제될 때 자동으로 isRefreshing을 false로 설정하는 것은 상태 일관성 유지에 도움이 됩니다.
Also applies to: 24-24
| } finally { | ||
| dispatch(tokenActions.setRefreshing(false)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
토큰 갱신 상태 관리 개선 필요
finally 블록에서 refreshing 상태를 false로 설정하는 것은 좋지만, 에러 발생 시에도 동일하게 처리되어 사용자에게 잘못된 상태를 보여줄 수 있습니다.
다음과 같이 에러 처리를 개선하는 것을 제안합니다:
} catch (error) {
dispatch(tokenActions.deleteToken());
+ throw error; // 상위에서 에러 처리할 수 있도록 전파
} finally {
dispatch(tokenActions.setRefreshing(false));
}📝 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.
| } finally { | |
| dispatch(tokenActions.setRefreshing(false)); | |
| } catch (error) { | |
| dispatch(tokenActions.deleteToken()); | |
| throw error; // 상위에서 에러 처리할 수 있도록 전파 | |
| } finally { | |
| dispatch(tokenActions.setRefreshing(false)); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/Routes/ProtectedRoutes.tsx (1)
6-6: Props 타입과 로직이 개선되었습니다.boolean 플래그 대신 토큰 존재 여부를 직접 확인하는 방식으로 변경된 것이 더 명확하고 안전합니다. 하지만 몇 가지 개선사항을 제안드립니다:
token타입에 대한 명시적인 상수 정의가 있으면 좋을 것 같습니다.- 리다이렉션 시 현재 경로를 state로 저장하여 로그인 후 원래 페이지로 돌아올 수 있게 하면 UX가 개선될 것 같습니다.
다음과 같이 수정하는 것을 제안드립니다:
import React from 'react'; import { PATH } from '@/constants/path'; -import { Navigate, Outlet } from 'react-router-dom'; +import { Navigate, Outlet, useLocation } from 'react-router-dom'; + +const TOKEN_TYPE = 'string | undefined' as const; type Props = { - token: string | undefined; + token: typeof TOKEN_TYPE; }; const ProtectedRoutes = ({ token }: Props) => { - return token ? <Outlet /> : <Navigate to={PATH.LOGIN} />; + const location = useLocation(); + return token ? ( + <Outlet /> + ) : ( + <Navigate to={PATH.LOGIN} state={{ from: location }} replace /> + ); };Also applies to: 9-10
src/App.tsx (2)
27-27: 상태 관리 로직이 개선되었습니다.Redux 상태 관리가 더 명확해졌습니다. 하지만 몇 가지 개선사항을 제안드립니다:
- 선택자 사용 시 메모이제이션을 적용하면 좋을 것 같습니다.
- 타입 안전성을 위해 선택자의 반환 타입을 명시적으로 정의하면 좋을 것 같습니다.
// auth.selectors.ts import { createSelector } from '@reduxjs/toolkit'; import { RootState } from '../store'; export const selectAuth = (state: RootState) => state.auth; export const selectAccessToken = createSelector( [selectAuth], (auth): string | undefined => auth.accessToken ); export const selectIsRefreshing = createSelector( [selectAuth], (auth): boolean => auth.isRefreshing );Also applies to: 52-53
65-66: 토큰 갱신 중 UI 처리가 개선되었습니다.토큰 갱신 중에 빈 화면을 보여주는 것이 잘못된 상태를 보여주는 것보다 낫습니다. 하지만 사용자 경험을 위해 다음과 같은 개선을 제안드립니다:
- 로딩 인디케이터를 추가하여 사용자에게 진행 상황을 알려주면 좋을 것 같습니다.
- 토큰 갱신이 실패할 경우에 대한 처리도 추가하면 좋을 것 같습니다.
+ import { CircularProgress } from '@mui/material'; + import { styled } from '@mui/material/styles'; + + const LoadingContainer = styled('div')({ + display: 'flex', + justifyContent: 'center', + alignItems: 'center', + height: '100vh', + }); + - if (isTokenRefreshing) return null; + if (isTokenRefreshing) { + return ( + <LoadingContainer> + <CircularProgress /> + </LoadingContainer> + ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/App.tsx(4 hunks)src/components/Routes/ProtectedRoutes.tsx(1 hunks)
🔇 Additional comments (1)
src/App.tsx (1)
99-99: 보호된 라우트 구성이 개선되었습니다.토큰 기반의 인증 확인이 잘 구현되었습니다. 하지만 몇 가지 보안 관련 개선사항을 제안드립니다:
- 토큰 만료 시간을 확인하여 만료된 토큰으로 인한 불필요한 API 호출을 방지하면 좋을 것 같습니다.
- 민감한 작업(예: 프로필 변경)에 대해서는 추가적인 인증 체크를 고려해보면 좋을 것 같습니다.
토큰 검증 로직을 다음과 같이 추가하는 것을 제안드립니다:
// hooks/useTokenValidation.ts import { useEffect } from 'react'; import { useNavigate } from 'react-router-dom'; import { PATH } from '@/constants/path'; import { parseJwt } from '@/utils/jwt'; export const useTokenValidation = (token?: string) => { const navigate = useNavigate(); useEffect(() => { if (!token) return; const tokenData = parseJwt(token); if (!tokenData || tokenData.exp * 1000 < Date.now()) { navigate(PATH.LOGIN); } }, [token, navigate]); };✅ Verification successful
토큰 검증 로직 개선 제안이 적절합니다
현재 구현된 인증 시스템에 대한 검토 결과, 제안하신 개선사항이 보안성 강화에 실질적인 도움이 될 것으로 판단됩니다. 현재 구현에는 다음과 같은 부분이 누락되어 있습니다:
- 토큰 만료 시간 검증
- 중요 작업에 대한 추가 보안 검증
제안하신
useTokenValidation훅 구현은 이러한 보안 취약점을 효과적으로 보완할 수 있을 것으로 보입니다.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for JWT-related utilities and token validation echo "=== Searching for JWT utilities ===" rg "parseJwt|jwt" -t ts -t tsx echo -e "\n=== Looking for ProtectedRoutes implementation ===" ast-grep --pattern 'const ProtectedRoutes = $_' echo -e "\n=== Searching for token validation logic ===" rg "token.*expir|expir.*token" -t ts -t tsx echo -e "\n=== Looking for authentication hooks ===" fd "use.*auth|use.*token" -e ts -e tsxLength of output: 933
Script:
#!/bin/bash echo "=== Examining useTokenRefresh hook ===" cat src/hooks/common/useTokenRefresh.ts echo -e "\n=== Examining useAuth hook ===" cat src/hooks/login/useAuth.ts echo -e "\n=== Looking for token types and interfaces ===" rg "interface.*Token|type.*Token" -t tsLength of output: 2053
src/hooks/common/useTokenRefresh.ts
Outdated
| await queryClient.invalidateQueries({ | ||
| queryKey: ['members'], | ||
| }); |
There was a problem hiding this comment.
이 부분은 재발급 받은 useMemberQuery를 무효화하는 작업이라 흐름 상 새로운 발급 받은 token에 대해서 새로 useMemberQuery가 실행되는게 적절해보이는데 삭제하신 이유가 있을까요?
There was a problem hiding this comment.
전에 원준님께서 헤더의 회원 정보 api 관련으로 말씀하셨던 부분을 이 부분으로 착각했네요,,,,😲
062f176 에서 다시 되돌려놓았습니다!!!!!!!!
| const ProtectedRoutes = ({ token }: Props) => { | ||
| return token ? <Outlet /> : <Navigate to={PATH.LOGIN} />; |
There was a problem hiding this comment.
토큰 값을 가지고 분기를 나눈게 좋은 처리인거 같습니다!
| @@ -61,6 +62,8 @@ const App: React.FC = () => { | |||
| }, [location.search, navigate]); | |||
| useTokenRefresh(); | |||
|
|
|||
| if (isTokenRefreshing) return null; | |||
There was a problem hiding this comment.
혹시 이 부분에서 null이 반환된 이후에는 수동으로 새로고침을 해줘야하나요? 아니면 자동으로 리렌더링이 발생하나요?
There was a problem hiding this comment.
로컬에서 테스트해본 결과 페이지가 정상적으로 잘 떠서 로그인 후에 새로고침해도 자동 리렌더링될 것 같아요!!
|
| @@ -3,10 +3,12 @@ import { PayloadAction, createSlice } from '@reduxjs/toolkit'; | |||
|
|
|||
| interface TokenState { | |||
| accessToken: string | undefined; | |||
| isRefreshing: boolean; | |||
There was a problem hiding this comment.
전역 상태에 토큰 재발급 상태 관리를 통해 재발급 중임을 명확히 할 수 있어서 좋은 방식인거 같습니다!
@alwubin 어느 방식이 더 좋은 방식일지 고민이 되네요🥹 스피너를 추가하게 된다면 디자이너, pm님과 간단히 의논이 필요할 것 같기도 하네요!! 그 전에 원준님께서 제안주신 방식에 대해서도 프론트끼리 상의해봐도 좋을 것 같아용👾👾 |
💡 작업 내용
💡 자세한 설명
📗 참고 자료 (선택)
📢 리뷰 요구 사항 (선택)
🚩 후속 작업 (선택)
✅ 셀프 체크리스트
closes #281
Summary by CodeRabbit
Summary by CodeRabbit
새로운 기능
버그 수정
리팩터링