[FE-Feat] 로그인 state 추적하여 헤더에 아바타 혹은 로그인 버튼 표시 #231
Conversation
|
Warning Rate limit exceeded@dioo1461 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 46 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. 📥 CommitsReviewing files that changed from the base of the PR and between 0cf85c494c07d165adfe164a7c3c46b063e59bb9 and ab0fa7a. 📒 Files selected for processing (10)
WalkthroughThis pull request introduces a new user API module alongside related model and query utilities. It implements a new API method to fetch the current user information and validates the response using a new schema. Related authentication utilities are refactored to use a constant for the access token key. In addition, the GlobalNavBar component is updated to conditionally render a user avatar instead of a login link, simplifying the layout in both landing and login pages. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as GlobalNavBar
participant Query as useUserInfoQuery
participant Auth as getAccessToken
participant API as userApi.getUserInfo
participant Server as /api/v1/user/current
UI->>Query: Render & trigger user query
Query->>Auth: Retrieve accessToken
Auth-->>Query: Return accessToken
Query->>API: Call getUserInfo()
API->>Server: GET /api/v1/user/current
Server-->>API: Return user data
API->>API: Validate data with UserInfoSchema
API-->>Query: Return parsed user info
Query-->>UI: Provide user data for display (UserAvatar)
Possibly related PRs
Suggested labels
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 (
|
![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 (4)
frontend/src/features/user/api/keys.ts (1)
2-3: Consider implementing the TODO to enhance security.The current implementation exposes the raw access token in the query key. While functionally correct, implementing the suggested hash would improve security by preventing the raw token from being exposed in debugging tools and logs.
Consider using a one-way hash function like SHA-256. Example implementation:
-export const userInfoQueryKey = (accessToken: string | null) => ['userInfo', accessToken]; +export const userInfoQueryKey = (accessToken: string | null) => ['userInfo', accessToken ? sha256(accessToken) : null];frontend/src/utils/auth/index.ts (1)
3-4: Add type safety and error handling for localStorage operations.The current implementation assumes localStorage operations will always succeed. Consider adding type safety and error handling:
-export const getAccessToken = () => localStorage.getItem(ACCESS_TOKEN_KEY); +export const getAccessToken = (): string | null => { + try { + return localStorage.getItem(ACCESS_TOKEN_KEY); + } catch (error) { + console.error('Failed to access localStorage:', error); + return null; + } +}; -export const isLogin = () => localStorage.getItem(ACCESS_TOKEN_KEY) !== null; +export const isLogin = (): boolean => { + try { + return localStorage.getItem(ACCESS_TOKEN_KEY) !== null; + } catch (error) { + console.error('Failed to check login status:', error); + return false; + } +};frontend/src/features/user/api/queries.ts (1)
8-11: Enhance React Query configuration with essential options.The query configuration could benefit from additional options to improve user experience and error handling:
+import type { UserInfo } from '../model'; -export const useUserInfoQuery = () => useQuery({ +export const useUserInfoQuery = () => useQuery<UserInfo, Error>({ queryKey: userInfoQueryKey(getAccessToken()), queryFn: () => userApi.getUserInfo(), + retry: 2, + staleTime: 5 * 60 * 1000, // 5 minutes + cacheTime: 30 * 60 * 1000, // 30 minutes + refetchOnWindowFocus: true, + onError: (error) => { + console.error('Failed to fetch user info:', error); + }, });frontend/src/layout/GlobalNavBar/index.tsx (1)
35-39: Consider using theme tokens for gap values.The gap value of 300 seems arbitrary. Consider:
- Using theme tokens for consistent spacing
- Ensuring the layout is responsive
<Flex align='center' direction='row' - gap={300} + gap={theme.spacing.lg} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between e42137b and 0cf85c494c07d165adfe164a7c3c46b063e59bb9.
📒 Files selected for processing (8)
frontend/src/features/user/api/index.ts(1 hunks)frontend/src/features/user/api/keys.ts(1 hunks)frontend/src/features/user/api/queries.ts(1 hunks)frontend/src/features/user/model/index.ts(1 hunks)frontend/src/layout/GlobalNavBar/index.tsx(2 hunks)frontend/src/routes/landing/index.tsx(1 hunks)frontend/src/routes/login/index.lazy.tsx(1 hunks)frontend/src/utils/auth/index.ts(1 hunks)
🔇 Additional comments (5)
frontend/src/routes/login/index.lazy.tsx (1)
8-8: LGTM! Clean and consistent with the new GlobalNavBar implementation.The removal of LoginLink from GlobalNavBar is appropriate as the login state is now handled internally by the GlobalNavBar component.
frontend/src/routes/landing/index.tsx (1)
8-8: LGTM! Clean and consistent with the new GlobalNavBar implementation.The removal of LoginLink from GlobalNavBar is appropriate as the login state is now handled internally by the GlobalNavBar component.
frontend/src/features/user/model/index.ts (2)
10-13: LGTM! Well-structured schema for user info.The UserInfoSchema is appropriately defined with necessary fields for avatar display and includes proper URL validation for the picture field.
15-16: LGTM! Clean type definitions.Types are correctly inferred from the Zod schemas, providing type safety throughout the application.
frontend/src/layout/GlobalNavBar/index.tsx (1)
34-44: LGTM! Clean conditional rendering based on login state.The implementation correctly handles the display of either UserAvatar or LoginLink based on the login state.
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and type safety for API response.
The API call should handle potential network errors and invalid responses. Also, consider adding explicit return type:
+import { UserInfo } from '../model';
export const userApi = {
- getUserInfo: async () => {
+ getUserInfo: async (): Promise<UserInfo> => {
try {
const response = await request.get('/api/v1/user/current');
const parsedData = UserInfoSchema.parse(response);
return parsedData;
+ } catch (error) {
+ if (error instanceof Error) {
+ console.error('Failed to fetch user info:', error.message);
+ }
+ throw 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.
| getUserInfo: async () => { | |
| const response = await request.get('/api/v1/user/current'); | |
| const parsedData = UserInfoSchema.parse(response); | |
| return parsedData; | |
| }, | |
| import { UserInfo } from '../model'; | |
| export const userApi = { | |
| getUserInfo: async (): Promise<UserInfo> => { | |
| try { | |
| const response = await request.get('/api/v1/user/current'); | |
| const parsedData = UserInfoSchema.parse(response); | |
| return parsedData; | |
| } catch (error) { | |
| if (error instanceof Error) { | |
| console.error('Failed to fetch user info:', error.message); | |
| } | |
| throw error; | |
| } | |
| }, | |
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance loading and error states in UserAvatar component.
The current loading and error messages are not user-friendly. Consider:
- Using a loading skeleton or spinner for better UX
- Implementing proper error handling with retry mechanism
- Adding error boundaries for graceful fallback
Example implementation:
const UserAvatar = () => {
const { data, isPending } = useUserInfoQuery();
- if (isPending) return <div>pending ...</div>;
- if (!data) return <div>user data is undefined or null</div>;
+ if (isPending) return <Avatar.Skeleton size='lg' />;
+ if (!data) {
+ return (
+ <ErrorBoundary fallback={<Avatar.Fallback size='lg' />}>
+ <RetryButton onClick={() => queryClient.invalidateQueries({ queryKey: ['userInfo'] })} />
+ </ErrorBoundary>
+ );
+ }
return (
<Avatar imageUrls={[data.picture]} size='lg' />
);
};📝 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 UserAvatar = () => { | |
| const { data, isPending } = useUserInfoQuery(); | |
| if (isPending) return <div>pending ...</div>; | |
| if (!data) return <div>user data is undefined or null</div>; | |
| return ( | |
| <Avatar imageUrls={[data.picture]} size='lg' /> | |
| ); | |
| }; | |
| const UserAvatar = () => { | |
| const { data, isPending } = useUserInfoQuery(); | |
| if (isPending) return <Avatar.Skeleton size='lg' />; | |
| if (!data) { | |
| return ( | |
| <ErrorBoundary fallback={<Avatar.Fallback size='lg' />}> | |
| <RetryButton onClick={() => queryClient.invalidateQueries({ queryKey: ['userInfo'] })} /> | |
| </ErrorBoundary> | |
| ); | |
| } | |
| return ( | |
| <Avatar imageUrls={[data.picture]} size='lg' /> | |
| ); | |
| }; |
hamo-o
left a comment
hamo-o
left a comment
There was a problem hiding this comment.
굿굿 이거 CSS만 수정 부탁드립니다 (디자인에서는 버튼끼리는 갭 없어용)
dioo1461
force-pushed
the
feature/fe/header-avatar
branch
from
9b618db to
ab0fa7a
Compare
ab0fa7a |
#️⃣ 연관된 이슈>
📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)
로그인 상태에 따라, 헤더에 아바타 혹은 로그인 버튼을 표시합니다.
로그인 상태
🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요
Summary by CodeRabbit
New Features
Refactor