[FE-Feat] 로그인 화면 및 기능 구현 #154
Conversation
executeRequest -> executeFetch
WalkthroughThis pull request introduces several enhancements to the frontend codebase. Updates include extending the ESLint configuration with a new plugin, adding related dependencies, and refactoring an import path. A new Google login API function and its associated TypeScript interfaces are introduced to support authentication. In addition, new styling and components for the login page are added, along with lazy-loaded routes for login and OAuth redirection. Utility functions for HTTP requests and route management are also implemented, and the routing system is enhanced with React Query integration. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant LandingPage
participant LoginPage
participant "Google Login API" as API
participant Browser
User->>LandingPage: Clicks "Google Login"
LandingPage->>LoginPage: Navigate to LoginPage route
User->>LoginPage: Clicks "Login with Google"
LoginPage->>API: Call requestGoogleLoginUrl()
API-->>LoginPage: Return Google login URL
LoginPage->>Browser: Initiate redirect using useTransition
sequenceDiagram
autonumber
participant User
participant "OAuth Redirect Route" as Route
participant "Route Utils" as Utils
participant Browser
User->>Route: Hit /oauth/redirect/ route
Route->>Utils: Get last visited path
Utils-->>Route: Return last route (or '/')
Route->>Browser: Navigate (replace history)
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 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: 5
🧹 Nitpick comments (8)
frontend/src/utils/route/index.ts (2)
1-7: Add error handling for sessionStorage operations.Consider adding error handling for cases where sessionStorage is unavailable (e.g., in private browsing mode) and path validation.
const LAST_VISITED_PATH_KEY = 'lastVisitedPath'; export const setLastRoutePath = (path: string) => { + if (!path) { + return; + } + try { sessionStorage.setItem(LAST_VISITED_PATH_KEY, path); + } catch (error) { + console.warn('Failed to save last route path:', error); + } };
7-7: Add error handling for getLastRoutePath.Similarly, add error handling for the getter function.
-export const getLastRoutePath = () => sessionStorage.getItem(LAST_VISITED_PATH_KEY); +export const getLastRoutePath = () => { + try { + return sessionStorage.getItem(LAST_VISITED_PATH_KEY); + } catch (error) { + console.warn('Failed to retrieve last route path:', error); + return null; + } +};frontend/src/pages/LoginPage/index.css.ts (1)
8-8: Fix typo in color variable name.The color variable name contains a typo:
Neturalshould beNeutral.- border: `1px solid ${vars.color.Ref.Netural[400]}`, + border: `1px solid ${vars.color.Ref.Neutral[400]}`,frontend/src/routes/__root.tsx (2)
23-28: Consider improving route path management.
- Extract route paths as constants to avoid magic strings
- Add error handling for the subscription callback
+const EXCLUDED_ROUTES = ['/login', '/oauth/redirect'] as const; + history.subscribe((subArgs) => { - const pathname = subArgs.location.pathname; - if (pathname !== '/login' && pathname !== '/oauth/redirect') { - setLastRoutePath(pathname); - } + try { + const pathname = subArgs.location.pathname; + if (!EXCLUDED_ROUTES.includes(pathname)) { + setLastRoutePath(pathname); + } + } catch (error) { + console.error('Failed to update last route path:', error); + } });
20-20: Consider configuring QueryClient options.The default QueryClient configuration might not be optimal for your use case. Consider configuring retry attempts, cache time, etc.
-const queryClient = new QueryClient(); +const queryClient = new QueryClient({ + defaultOptions: { + queries: { + retry: 1, + staleTime: 5 * 60 * 1000, // 5 minutes + }, + }, +});frontend/src/pages/LandingPage/index.tsx (1)
37-39: Add error handling to navigation.Consider adding error handling to catch and handle navigation failures.
const handleClickGoogleLogin = () => { - navigate({ to: '/login' }); + try { + navigate({ to: '/login' }); + } catch (error) { + console.error('Failed to navigate to login page:', error); + // Consider showing a user-friendly error message + } };frontend/src/utils/fetch/index.ts (1)
69-74: Consider adding TypeScript generics to request methods.The request methods could benefit from TypeScript generics for better type safety of response data.
export const request = { - get: (endpoint: string) => executeFetch('GET', endpoint), - post: (endpoint: string, body?: BodyInit) => executeFetch('POST', endpoint, body), - put: (endpoint: string, body?: BodyInit) => executeFetch('PUT', endpoint, body), - delete: (endpoint: string) => executeFetch('DELETE', endpoint), + get: <T = unknown>(endpoint: string) => executeFetch('GET', endpoint) as Promise<T>, + post: <T = unknown>(endpoint: string, body?: BodyInit) => + executeFetch('POST', endpoint, body) as Promise<T>, + put: <T = unknown>(endpoint: string, body?: BodyInit) => + executeFetch('PUT', endpoint, body) as Promise<T>, + delete: <T = unknown>(endpoint: string) => executeFetch('DELETE', endpoint) as Promise<T>, };frontend/package.json (1)
24-24: LGTM! Consider version management strategy.The addition of React Query and its ESLint plugin is appropriate for managing server state and API requests, which aligns well with the PR objectives. A few suggestions for version management:
- Consider using exact versions (
5.66.0instead of^5.66.0) for better dependency predictability- Ensure the React Query version matches across all @TanStack packages
Also applies to: 43-43
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlfrontend/src/components/Icon/png/google-login-icon.pngis excluded by!**/*.png
📒 Files selected for processing (15)
frontend/eslint.config.js(2 hunks)frontend/package.json(2 hunks)frontend/src/components/Input/MultiInput.tsx(1 hunks)frontend/src/features/login/api/index.ts(1 hunks)frontend/src/features/login/model/index.ts(1 hunks)frontend/src/layout/GlobalNavBar/index.tsx(1 hunks)frontend/src/pages/LandingPage/index.tsx(2 hunks)frontend/src/pages/LoginPage/index.css.ts(1 hunks)frontend/src/pages/LoginPage/index.tsx(1 hunks)frontend/src/routeTree.gen.ts(9 hunks)frontend/src/routes/__root.tsx(2 hunks)frontend/src/routes/login/index.lazy.tsx(1 hunks)frontend/src/routes/oauth.redirect/index.lazy.tsx(1 hunks)frontend/src/utils/fetch/index.ts(1 hunks)frontend/src/utils/route/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/src/layout/GlobalNavBar/index.tsx
- frontend/src/components/Input/MultiInput.tsx
🔇 Additional comments (7)
frontend/src/features/login/model/index.ts (1)
2-8: Well-structured TypeScript interfaces!The interfaces are clear, focused, and follow TypeScript best practices with descriptive names and appropriate types.
frontend/src/routes/login/index.lazy.tsx (1)
1-19: LGTM! Clean implementation of lazy-loaded login route.The implementation follows best practices with:
- Proper use of lazy loading for optimized performance
- Clean component composition
- Correct route configuration
frontend/src/pages/LoginPage/index.css.ts (1)
13-21: Consider documenting the z-index value.The z-index value of 9998 is quite high. Consider:
- Documenting why this specific value was chosen
- Using a z-index variable from theme if available
- Verifying there are no conflicts with other overlays
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for other high z-index values that might conflict rg -n "z-?index.*999" --type cssLength of output: 183
Action Required: Document and Verify the High z-index Value
The
backdropStyleinfrontend/src/pages/LoginPage/index.css.tsuses a z-index of 9998 without any explanation. Please:
- Document why the value 9998 was chosen.
- Consider using a centralized z-index variable from the theme if one exists.
- Manually verify that there are no conflicts with other overlays. (Note: The initial ripgrep command didn’t return results because it filtered by CSS file types, but since this file uses a
.css.tsextension, a broader search is needed.)To aid further investigation, you can run the following shell script to search across all file types for high z-index values:
#!/bin/bash echo "Searching for high z-index values (9000+) across the repository:" rg -n -e "z[-]?index\s*[:=]\s*9[0-9]{3,}" .This will help ensure no other overlays use conflicting high z-index values.
frontend/src/pages/LoginPage/index.tsx (1)
12-21: LGTM! Good use of React Query and useTransition.The implementation correctly handles the login flow by:
- Using React Query for managing the login request state
- Using useTransition to handle the redirect smoothly
- Properly handling the success case by redirecting to the provided URL
frontend/eslint.config.js (1)
3-3: LGTM! Good addition of React Query linting.The integration of @tanstack/eslint-plugin-query will help maintain consistent React Query usage across the codebase.
Also applies to: 13-13
frontend/src/routeTree.gen.ts (1)
26-27: Verify the lazy-loaded route configurations.The route configurations look correct, but please verify:
- The existence of the lazy-loaded components at the specified paths
- The correct parent-child relationships in the route hierarchy
Also applies to: 43-47, 55-61
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Check if the lazy-loaded components exist fd -t f "index.lazy.tsx" "frontend/src/routes/(login|oauth.redirect)"Length of output: 199
Below is an updated verification script that splits the search into the two expected directories and also checks the route configuration in the auto-generated file. Please execute these commands and manually verify that:
• The lazy-loaded component file (typically named
index.lazy.tsx) exists in the expected directories:
-frontend/src/routes/login
-frontend/src/routes/oauth/redirect• The
frontend/src/routeTree.gen.tsfile registers these routes correctly (i.e. includes entries for/login/and/oauth/redirect/with the proper hierarchy).Run the following script:
Once these commands are executed and you confirm that the components and route configurations are correctly in place, we can conclude that the lazy-loaded route configurations are set up as expected.
🏁 Script executed:
[run_scripts] #!/bin/bash echo "Checking for lazy-loaded component in /login route:" fd -t f "index.lazy.tsx" "frontend/src/routes/login" echo "------------------------------" echo "Checking for lazy-loaded component in /oauth/redirect route:" fd -t f "index.lazy.tsx" "frontend/src/routes/oauth/redirect" echo "------------------------------" echo "Verifying routeTree entries for '/login/' in routeTree.gen.ts:" rg "/login/" frontend/src/routeTree.gen.ts echo "------------------------------" echo "Verifying routeTree entries for '/oauth/redirect/' in routeTree.gen.ts:" rg "/oauth/redirect/" frontend/src/routeTree.gen.tsLength of output: 1862
Lazy-Loaded Routes Verification: Verify OAuth Redirect Component Path
- The lazy-loaded component for the
/login/route exists atfrontend/src/routes/login/index.lazy.tsx.- The auto-generated route configuration for
/oauth/redirect/exists infrontend/src/routeTree.gen.ts, but the expected lazy component was not found underfrontend/src/routes/oauth/redirect/index.lazy.tsx. This indicates that either the directory/file is missing or it’s located elsewhere.Please manually verify that the OAuth redirect lazy component exists at the expected path or update the route configuration if its location has changed.
frontend/package.json (1)
24-24: Verify the @tanstack/react-query version.The addition of React Query is a good choice for managing server state and API requests. However, let's verify if this is the latest stable version.
✅ Verification successful
🌐 Web query:
What is the latest stable version of @tanstack/react-query?💡 Result:
The latest stable version of
@tanstack/react-queryis v5.66.0, released on January 31, 2025[1]. This version includes updates across all framework implementations (React, Solid, Svelte, Vue) and associated packages like devtools and persistence utilities[1][2][6].Key details:
- Package updates: Affects
@tanstack/react-query,@tanstack/react-query-devtools, and other framework-specific implementations[1]- Release frequency: Regular updates with minor/patch versions like v5.65.1 (Jan 28) and v5.64.2 (Jan 19) preceding this release[1]
- Compatibility: Requires React v18+ and modern browsers (Chrome ≥91, Safari ≥15, etc.)[4]
For installation:
npm i @tanstack/[email protected]The package has seen over 266k weekly downloads of its devtools companion package at time of release[6].
Citations:
- 1: https://github.com/TanStack/query/releases
- 2: https://tanstack.com/blog/announcing-tanstack-query-v5
- 3: https://cloudsmith.com/navigator/npm/@tanstack/react-query
- 4: https://tanstack.com/query/latest/docs/framework/react/installation
- 5: https://www.npmjs.com/package/@tanstack/react-query-devtools?activeTab=versions
- 6: https://www.npmjs.com/package/@tanstack/react-query-devtools
- 7: https://www.npmjs.com/package/react-query
- 8: https://tanstack.com/query/v4/docs/framework/react/guides/migrating-to-react-query-4
- 9: https://github.com/TanStack/table/releases
- 10: https://tanstack.com/query/latest/docs/framework/react/overview
@tanstack/react-query version is up-to-date.
The latest stable version is indeed v5.66.0, which matches the version specified in the package.json.
| export const requestGoogleLoginUrl = async (): Promise<RequestGoogleLoginUrlResponse> => { | ||
| const response = await request.get('/api/v1/google'); | ||
| return response; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for failed requests.
The function should handle potential API failures gracefully.
export const requestGoogleLoginUrl = async (): Promise<RequestGoogleLoginUrlResponse> => {
+ try {
const response = await request.get('/api/v1/google');
return response;
+ } catch (error) {
+ console.error('Failed to request Google login URL:', error);
+ throw new Error('Failed to initialize Google login. Please try again later.');
+ }
};📝 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 requestGoogleLoginUrl = async (): Promise<RequestGoogleLoginUrlResponse> => { | |
| const response = await request.get('/api/v1/google'); | |
| return response; | |
| }; | |
| export const requestGoogleLoginUrl = async (): Promise<RequestGoogleLoginUrlResponse> => { | |
| try { | |
| const response = await request.get('/api/v1/google'); | |
| return response; | |
| } catch (error) { | |
| console.error('Failed to request Google login URL:', error); | |
| throw new Error('Failed to initialize Google login. Please try again later.'); | |
| } | |
| }; |
| const Redirect = () => { | ||
| const navigate = useNavigate(); | ||
| const lastPath = getLastRoutePath(); | ||
| navigate({ | ||
| to: lastPath || '/', | ||
| replace: true, | ||
| }); | ||
|
|
||
| return <div></div>; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add loading state and error handling.
The component should handle loading and error states gracefully, and provide visual feedback to users.
const Redirect = () => {
const navigate = useNavigate();
const lastPath = getLastRoutePath();
+ const [error, setError] = useState<string | null>(null);
+
+ useEffect(() => {
+ try {
navigate({
to: lastPath || '/',
replace: true,
+ }).catch(err => {
+ console.error('Navigation failed:', err);
+ setError('Failed to redirect. Please try again.');
});
+ } catch (err) {
+ console.error('Navigation failed:', err);
+ setError('Failed to redirect. Please try again.');
+ }
+ }, [navigate, lastPath]);
+
+ if (error) {
+ return <div className="error-message">{error}</div>;
+ }
- return <div></div>;
+ return <div className="loading-spinner">Redirecting...</div>;
};📝 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 Redirect = () => { | |
| const navigate = useNavigate(); | |
| const lastPath = getLastRoutePath(); | |
| navigate({ | |
| to: lastPath || '/', | |
| replace: true, | |
| }); | |
| return <div></div>; | |
| }; | |
| const Redirect = () => { | |
| const navigate = useNavigate(); | |
| const lastPath = getLastRoutePath(); | |
| const [error, setError] = useState<string | null>(null); | |
| useEffect(() => { | |
| try { | |
| navigate({ | |
| to: lastPath || '/', | |
| replace: true, | |
| }).catch(err => { | |
| console.error('Navigation failed:', err); | |
| setError('Failed to redirect. Please try again.'); | |
| }); | |
| } catch (err) { | |
| console.error('Navigation failed:', err); | |
| setError('Failed to redirect. Please try again.'); | |
| } | |
| }, [navigate, lastPath]); | |
| if (error) { | |
| return <div className="error-message">{error}</div>; | |
| } | |
| return <div className="loading-spinner">Redirecting...</div>; | |
| }; |
| export const executeFetch = async ( | ||
| method: Method, | ||
| endpoint: string, | ||
| body?: BodyInit, | ||
| options?: RequestOptions, | ||
| ) => { | ||
| const fetchOptions = buildFetchOptions(options); | ||
|
|
||
| try { | ||
| const response = await fetch(BASE_URL + endpoint, { | ||
| method: method, | ||
| body: JSON.stringify(body), | ||
| ...fetchOptions, | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`HTTP Error: ${response.status}`); | ||
| } | ||
|
|
||
| const data = await response.json(); | ||
| return data; | ||
| } catch (error) { | ||
| throw new Error(`Network Error : ${error}`); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in executeFetch.
The current error handling could be improved by:
- Parsing error responses for better error messages
- Adding retry logic for network errors
- Handling non-JSON responses
export const executeFetch = async (
method: Method,
endpoint: string,
body?: BodyInit,
options?: RequestOptions,
) => {
const fetchOptions = buildFetchOptions(options);
try {
const response = await fetch(BASE_URL + endpoint, {
method: method,
body: JSON.stringify(body),
...fetchOptions,
});
if (!response.ok) {
- throw new Error(`HTTP Error: ${response.status}`);
+ const errorData = await response.json().catch(() => null);
+ throw new Error(
+ `HTTP Error ${response.status}: ${
+ errorData?.message || response.statusText
+ }`
+ );
}
+ const contentType = response.headers.get('content-type');
+ if (contentType?.includes('application/json')) {
const data = await response.json();
return data;
+ }
+ return response.text();
} catch (error) {
- throw new Error(`Network Error : ${error}`);
+ if (error instanceof Error) {
+ throw error;
+ }
+ throw new Error(`Network Error: ${String(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.
| export const executeFetch = async ( | |
| method: Method, | |
| endpoint: string, | |
| body?: BodyInit, | |
| options?: RequestOptions, | |
| ) => { | |
| const fetchOptions = buildFetchOptions(options); | |
| try { | |
| const response = await fetch(BASE_URL + endpoint, { | |
| method: method, | |
| body: JSON.stringify(body), | |
| ...fetchOptions, | |
| }); | |
| if (!response.ok) { | |
| throw new Error(`HTTP Error: ${response.status}`); | |
| } | |
| const data = await response.json(); | |
| return data; | |
| } catch (error) { | |
| throw new Error(`Network Error : ${error}`); | |
| } | |
| }; | |
| export const executeFetch = async ( | |
| method: Method, | |
| endpoint: string, | |
| body?: BodyInit, | |
| options?: RequestOptions, | |
| ) => { | |
| const fetchOptions = buildFetchOptions(options); | |
| try { | |
| const response = await fetch(BASE_URL + endpoint, { | |
| method: method, | |
| body: JSON.stringify(body), | |
| ...fetchOptions, | |
| }); | |
| if (!response.ok) { | |
| const errorData = await response.json().catch(() => null); | |
| throw new Error( | |
| `HTTP Error ${response.status}: ${ | |
| errorData?.message || response.statusText | |
| }` | |
| ); | |
| } | |
| const contentType = response.headers.get('content-type'); | |
| if (contentType?.includes('application/json')) { | |
| const data = await response.json(); | |
| return data; | |
| } | |
| return response.text(); | |
| } catch (error) { | |
| if (error instanceof Error) { | |
| throw error; | |
| } | |
| throw new Error(`Network Error: ${String(error)}`); | |
| } | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/pages/LoginPage/index.tsx (1)
45-64: Enhance button accessibility.The button could benefit from improved accessibility features.
Add ARIA attributes and loading state indicator:
const GoogleLoginButton = ({ onClick, disabled }: { onClick: () => void; disabled: boolean }) => ( <Flex align='center' as='button' + aria-label="Google로 시작하기" + aria-busy={disabled} className={googleLoginButtonStyle} disabled={disabled} gap={200} justify='center' onClick={onClick} width='full' > <img alt='Google 로그인 아이콘' height='20px' src={googleLoginIcon} width='20px' /> - <Text color={vars.color.Ref.Netural[400]} typo='t3'>Google로 시작하기</Text> + <Text color={vars.color.Ref.Netural[400]} typo='t3'> + {disabled ? '로그인 중...' : 'Google로 시작하기'} + </Text> </Flex> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/package.json(2 hunks)frontend/src/pages/LoginPage/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/package.json
🔇 Additional comments (2)
frontend/src/pages/LoginPage/index.tsx (2)
1-11: LGTM! Well-organized imports.The imports are properly organized and the Google login icon is correctly imported as a module.
33-33: Consider making the modal's open state dynamic.The modal is always open (
isOpen={true}). Consider managing its state if there are cases where it should be hidden.Could you clarify if there are any scenarios where the modal should be hidden? If not, we could remove the
isOpenprop entirely since it's always true.
| const googleLogin = useMutation({ | ||
| mutationFn: requestGoogleLoginUrl, | ||
| onSuccess: ({ url }) => { | ||
| startRedirectTransition(async () => { | ||
| window.location.href = url; | ||
| }); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Add error handling for failed login attempts.
The mutation doesn't handle API failures. Users won't receive feedback if the login request fails.
Add error handling:
const googleLogin = useMutation({
mutationFn: requestGoogleLoginUrl,
onSuccess: ({ url }) => {
startRedirectTransition(async () => {
window.location.href = url;
});
},
+ onError: (error) => {
+ // Handle error (e.g., show error message to user)
+ console.error('Login failed:', 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.
| const googleLogin = useMutation({ | |
| mutationFn: requestGoogleLoginUrl, | |
| onSuccess: ({ url }) => { | |
| startRedirectTransition(async () => { | |
| window.location.href = url; | |
| }); | |
| }, | |
| }); | |
| const googleLogin = useMutation({ | |
| mutationFn: requestGoogleLoginUrl, | |
| onSuccess: ({ url }) => { | |
| startRedirectTransition(async () => { | |
| window.location.href = url; | |
| }); | |
| }, | |
| onError: (error) => { | |
| // Handle error (e.g., show error message to user) | |
| console.error('Login failed:', error); | |
| }, | |
| }); |
Redirect 라우트 내에서, useEffect 밖에서 navigate를 하고 있었기 때문에 발생한 문제였음. 컴포넌트가 렌더링 완료되기 전 상태가 업데이트되면, 그로 인해 무한 루프가 발생할 수 있기 때문에 상태 업데이트는 항상 렌더링이 완료된 시점에 수행해야 함
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/pages/LoginPage/index.tsx (1)
13-18:⚠️ Potential issueAdd loading and error handling to the mutation.
The mutation lacks loading state indication and error handling, which affects user experience.
Apply this diff:
const googleLogin = useMutation({ mutationFn: requestGoogleLoginUrl, onSuccess: ({ url }) => { window.location.href = url; }, + onError: (error) => { + console.error('Login failed:', error); + // Add error notification/toast here + }, });
🧹 Nitpick comments (2)
frontend/src/pages/LoginPage/index.tsx (2)
29-29: Make Modal's isOpen state dynamic.The Modal is always open (isOpen={true}). Consider managing this with state for better control.
Apply this diff:
+import { useState } from 'react'; + const LoginPage = () => { + const [isModalOpen, setIsModalOpen] = useState(true); + return ( <div className={backdropStyle}> <Modal description={...} - isOpen={true} + isOpen={isModalOpen} + onClose={() => setIsModalOpen(false)} ... >
41-59: Enhance button accessibility.The button could benefit from additional accessibility attributes.
Apply this diff:
const GoogleLoginButton = ({ onClick }: { onClick: () => void }) => ( <Flex align='center' as='button' + aria-label='Google로 로그인' + role='button' + tabIndex={0} + onKeyPress={(e) => e.key === 'Enter' && onClick()} className={googleLoginButtonStyle} gap={200} justify='center'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/pages/LoginPage/index.tsx(1 hunks)frontend/src/routes/oauth.redirect/index.lazy.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/routes/oauth.redirect/index.lazy.tsx
| onSuccess: ({ url }) => { | ||
| window.location.href = url; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use React Router's navigation instead of direct window.location modification.
Direct window.location modification breaks the browser's history stack. Consider using React Router's navigation methods.
Apply this diff:
+import { useNavigate } from 'react-router-dom';
+
const LoginPage = () => {
+ const navigate = useNavigate();
const googleLogin = useMutation({
mutationFn: requestGoogleLoginUrl,
onSuccess: ({ url }) => {
- window.location.href = url;
+ navigate(url);
},
});📝 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.
| onSuccess: ({ url }) => { | |
| window.location.href = url; | |
| }, | |
| import React from 'react'; | |
| import { useMutation } from 'react-query'; | |
| import { useNavigate } from 'react-router-dom'; | |
| const LoginPage = () => { | |
| const navigate = useNavigate(); | |
| const googleLogin = useMutation({ | |
| mutationFn: requestGoogleLoginUrl, | |
| onSuccess: ({ url }) => { | |
| navigate(url); | |
| }, | |
| }); | |
| // ...rest of the LoginPage component code | |
| return ( | |
| // JSX code for LoginPage | |
| ); | |
| }; | |
| export default LoginPage; |
hamo-o
left a comment
hamo-o
left a comment
There was a problem hiding this comment.
굿굿 수고하셨어요! 같이 문제점들 고쳐서 어프룹합니당
#️⃣ 연관된 이슈>
📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)
로그인 구현
fetch util 모듈 작성
fetch를 한 단계 추상화한
request객체를 구현했습니다.현재는 기본적인
get,post,put,delete메서드만 간단히 구현되어 있습니다.🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요
백엔드 base url은 프로젝트 루트에
.env.local파일을 생성하고, 내부에 아래와 같은 형식으로 작성해주시면 됩니다.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Style
Chores