Refector/글로벌에러바운더리 #163
Refector/글로벌에러바운더리 #163
Conversation
WalkthroughThe pull request introduces several changes primarily focused on error handling and routing within the application. A new dependency, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant GlobalBoundary
participant ErrorFallback
participant API
User->>App: Interacts with UI
App->>GlobalBoundary: Wraps components
GlobalBoundary->>API: Makes API call
API-->>GlobalBoundary: Returns response
alt Error occurs
GlobalBoundary->>ErrorFallback: Displays error message
ErrorFallback->>User: Shows retry or redirect options
end
Assessment against linked issues
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 (
|
✅ Deploy Preview for coworkers-colla ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }, | ||
| "node_modules/react-hook-form": { | ||
| "version": "7.53.0", | ||
| "resolved": "https://registry.npmjs.org/react-hook-form/-/react-hook-form-7.53.0.tgz", |
There was a problem hiding this comment.
react-error-boundary 라이브러리의 버전이 package.json 파일에 추가되었습니다. 이 라이브러리의 의존성 및 peerDependencies도 확인되었습니다. 코드 변경은 안전해 보이며, 개선 제안 사항은 없습니다. 이 코드는 안정적인 상태로 보입니다.
| "react-error-boundary": "^4.1.2", | ||
| "react-hook-form": "^7.53.0", | ||
| "react-intersection-observer": "^9.13.1", | ||
| "tailwind-merge": "^2.5.4", |
There was a problem hiding this comment.
이 코드 패치를 살펴보겠습니다.
- 버그 리스크:
- 새로 추가된 "react-error-boundary" 패키지의 버전이 "^4.1.2"로 지정되어 있습니다. 이 패키지의 안정성과 호환성을 고려하여 최신 버전으로 업데이트하는 것이 좋습니다.
- 개선 제안:
- 코드 상단에 주석을 추가하여 해당 코드 패치에 대한 목적 또는 변경 내용을 설명하는 것이 좋습니다.
- 패키지 버전 관리를 효율적으로 하기 위해 패키지 의존성을 정확히 관리하고 최신 버전으로 업데이트하는 것이 좋습니다.
- 코드 품질을 높이기 위해 변수 및 함수의 명칭을 명확하고 의미있는 이름으로 변경하는 것이 좋습니다.
이상입니다. 부족한 점이 있거나 추가로 도움이 필요하시면 말씀해 주세요. 감사합니다!
| method: 'DELETE', | ||
| url: `groups/${groupId}/member/${memberUserId}`, | ||
| }); | ||
| } |
There was a problem hiding this comment.
-
트랜잭션에 대한 처리가 누락되어 있습니다. 예외가 발생할 경우 롤백되지 않는 것이 문제가 될 수 있습니다. 각 기능을 논리적인 그룹으로 묶어 트랜잭션 처리를 추가하는 것이 좋습니다.
-
에러 처리가 너무 간단합니다. 더 구체적이고 유용한 에러 메시지를 제공하는 것이 사용자 친화적이며 디버깅에 도움이 될 수 있습니다.
-
주석이 부족합니다. 코드의 의도와 각 함수가 하는 일에 대한 주석을 추가하여 코드의 가독성을 높일 수 있습니다.
-
코드 중복이 있습니다. 중복된 코드 부분을 함수로 분리하여 재사용성을 높일 수 있습니다.
-
테스트 코드가 없습니다. 각 함수에 대한 유닛 테스트를 작성하여 코드의 품질을 향상시킬 수 있습니다.
| }, | ||
| }); | ||
| return response; | ||
| }; |
There was a problem hiding this comment.
- postTaskList 함수와 deleteTaskList 함수는 거의 동일한 패턴을 가지고 있습니다. 중복을 방지하기 위해 중복 코드를 줄일 수 있습니다.
- error에 대한 더 구체적인 에러 메시지를 제공하는 것이 유용할 수 있습니다. 예를 들어, "할 일 목록 생성 실패"와 같은 메시지를 제공하면 어떤 부분에서 문제가 발생했는지 더 쉽게 파악할 수 있습니다.
- try-catch 블록에서 발생하는 예외에 대한 처리가 좀 더 구체적이어야 합니다. 특정 예외 유형에 따른 다른 처리를 할 수 있도록 수정하는 것이 좋을 수 있습니다.
| router.push(`/teams/${taskList.groupId}/tasks`); | ||
| e.stopPropagation(); | ||
| }; | ||
|
|
There was a problem hiding this comment.
이 코드 패치에서 우리가 변경한 부분은 router.push 메소드에서 경로를 수정한 것뿐입니다. 이 변경은 /teams 경로를 추가하여 경로의 일관성을 유지하고 문제없이 작동할 것으로 예상됩니다.
버그나 위험은 발견되지 않았습니다. 또한 개선 제안사항은 현재 코드에서 주요한 요소로 보이지 않습니다. 이 변경은 안전하게 병합될 수 있습니다.
| <Link href={`/teams/${m.groupId}`}> | ||
| <Menu.Trigger | ||
| className="flex" | ||
| menuId={GROUP_MENU} |
There was a problem hiding this comment.
Link 컴포넌트의 href 속성은 변경되었습니다. 이 변경된 코드는 "/teams" 경로로 이동하도록 되어 있습니다. 해당 변경은 제대로 동작할 것으로 예상됩니다. 코드 리뷰에서 잠재적 오류나 개선점이 발견되지는 않았습니다.
| href={`/teams/${groupId}/editteam`} | ||
| className="p-4 hover:bg-tertiary" | ||
| > | ||
| 수정하기 |
There was a problem hiding this comment.
변경된 코드 패치는 Link 요소에서 href 속성값이 변경되었습니다.
이전에는 /groupId로 설정되어 있었지만, 변경 후에는 /teams/groupId로 설정되었습니다.
또한, 두 번째 Link 요소도 비슷한 방식으로 변경되었습니다. /groupId/editteam에서 /teams/groupId/editteam로 변경되었습니다.
개선 제안:
- 변경된 URL 경로에 대한 주석을 추가하여 다른 개발자들이 코드를 이해하기 쉽도록 도와줍니다.
- classNames을 이용하여 스타일을 관리하고 있습니다. 이유가 명확하지 않으면 의미가 명확한 Tailwind CSS 클래스명으로 전환하면 더 깔끔한 코드를 유지할 수 있을 것입니다.
- 가능한 경우, 중복 링크 제거를 고려해보세요. 같은 링크가 여러 곳에 포함되어 있는 경우, 공통된 부분을 한 곳으로 통합해 중복을 방지할 수 있습니다.
| </SessionProvider> | ||
| </GlobalBoundary> | ||
| ); | ||
| } |
There was a problem hiding this comment.
이 코드 패치는 전체적으로 안정적인 변화를 보여줍니다. 가장 큰 변경은 새로운 GlobalBoundary 컴포넌트의 추가입니다. 이 컴포넌트는 에러 경계를 전역으로 처리하는 역할을 합니다.
이 코드 변경 사항에서 주목할 점은 현재 SessionProvider, QueryProvider, OAuthProvider, 및 RootLayout 컴포넌트들이 GlobalBoundary 컴포넌트로 감싸졌다는 것입니다. 이것은 에러 처리를 좀 더 효율적으로 관리할 수 있게 해줍니다.
개선 제안은 없으며, 버그도 발견되지 않았습니다. 전반적으로 이 코드 패치는 안정적이고 향후 유지보수에 도움이 될 것으로 보입니다. 이렇게 안전한 코드 변경을 식별하고 위험을 줄이는 것은 좋은 개발 관행입니다.
|
|
||
| router.replace( | ||
| { | ||
| query: { |
There was a problem hiding this comment.
위의 코드 패치를 간단히 살펴보겠습니다.
-
handleSearchKeyDown함수에서 Enter 키가 눌렸을 때만setSearchQuery(searchValue);가 호출되도록 되어 있습니다. 이 부분은 잘 처리되어 있습니다. -
그러나
router.replace메소드를 호출할 때query속성을 수정하는 것으로 보입니다. 이것은 쿼리 파라미터 값을 변경하는 동작이므로, 이게 의도된 동작이라면 괜찮지만, 변경하지 않아야 할 경우에는 조심해야 합니다. -
코드 블록의 줄이 급격하게 떨어지지 않도록 공백이 있으면 더 가독성이 좋을 것입니다.
-
무난하게 작성된 코드로 보이지만, 주석이나 추가 설명이 없어서 전체적인 기능에 대한 이해가 어려울 수 있습니다. 주석을 추가해서 코드의 목적을 명확하게 나타낸다면 좋을 것입니다.
-
코드에 대한 단위 테스트가 있으면 코드를 더욱 견고하게 할 수 있습니다. 테스트 코드를 작성하는 것은 코드의 품질 향상에 도움이 될 수 있습니다.
| throwOnError: true, | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
이 코드 패치는 makeQueryClient 함수 내에 queries 객체에 두 가지 새로운 속성을 추가하는 것 같습니다. 새로운 retry 속성은 0으로 설정되어 있어 재시도를 하지 않음을 의미하고, throwOnError 속성은 true로 설정되어 error가 throw되도록 처리됩니다. 이러한 변경사항은 요청에 대해 캐시되어 있는 정보를 사용하고, 에러가 발생할 경우 적절하게 처리하기 위한 것으로 보입니다. 이에 대한 추가 개선사항은 현재 없어 보입니다. 코드 리뷰 결과는 위와 같습니다.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (8)
src/components/common/error/ErrorBoundary.tsx (1)
3-3: Define proper types for ErrorFallback props.Ensure type safety by defining proper interface for ErrorFallback component.
// Add to ErrorFallback.tsx interface ErrorFallbackProps { error: Error; resetErrorBoundary: () => void; errorType?: 'unauthorized' | 'not_found' | 'api_error' | 'unknown'; }src/pages/_app.tsx (1)
15-25: LGTM! Proper implementation of global error boundary.The GlobalBoundary is correctly placed as the outermost wrapper, which will catch errors from all child components and API calls.
Consider adding error reporting configuration at this level:
- <GlobalBoundary> + <GlobalBoundary + onError={(error) => { + // Add error reporting service integration + console.error('Application Error:', error); + }} + >src/providers/QueryProvider.tsx (1)
14-15: LGTM! Consider adding error logging.The configuration changes align well with the Error Boundary implementation:
retry: 0prevents prolonged loading statesthrowOnError: trueenables Error Boundary to catch query errorsConsider adding a global error logger in the default options:
defaultOptions: { queries: { staleTime: 60 * 1000, retry: 0, throwOnError: true, + onError: (error) => { + // Log errors to your monitoring service + console.error('Query Error:', error); + }, }, },src/components/TaskList/TaskLists.tsx (1)
Line range hint
49-51: Add error handling for task deletionThe handleTaskDelete function should handle potential errors and show appropriate feedback to users.
Consider adding error handling:
const handleTaskDelete = () => { + // Show loading state deleteTask.mutate( { groupId: taskList.groupId, taskListId: taskList.id }, + { + // Configure to work with error boundary + throwOnError: true + } ); };src/pages/teams/[id]/index.tsx (4)
94-94: Remove trailing slash from routeThe trailing slash in the route path is unnecessary and should be removed for consistency with Next.js routing conventions.
- router.push(`/teams/${group?.id}/editteam/`); + router.push(`/teams/${group?.id}/editteam`);
108-108: Add fallback title for SEOWhile optional chaining prevents runtime errors, provide a fallback title for SEO purposes when the group name is undefined.
- <title>{group?.name} 팀 페이지 - Coworkers</title> + <title>{group?.name ? `${group.name} 팀 페이지` : '팀 페이지'} - Coworkers</title>
193-193: Improve members count displayThe current implementation might show "0" when members is undefined. Consider making this more explicit.
- ({group?.members.length}개) + ({group?.members?.length ?? 0}개)
243-243: Add fallback text for dialog titleConsider adding a fallback text when the group name is undefined for better user experience.
- <DialogTitle> {group?.name} </DialogTitle> + <DialogTitle> {group?.name ?? '팀'} </DialogTitle>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
package.json(1 hunks)src/apis/groups.api.ts(3 hunks)src/apis/taskList.api.ts(1 hunks)src/components/TaskList/TaskLists.tsx(1 hunks)src/components/common/Badge.tsx(1 hunks)src/components/common/error/ErrorBoundary.tsx(1 hunks)src/components/common/error/ErrorFallback.tsx(1 hunks)src/components/layouts/ui/header/ui/header.menu.tsx(1 hunks)src/components/layouts/ui/header/ui/header.nav.tsx(1 hunks)src/components/layouts/ui/membership/MembershipItem.tsx(2 hunks)src/pages/_app.tsx(2 hunks)src/pages/boards/index.tsx(1 hunks)src/pages/teams/[id]/index.tsx(7 hunks)src/providers/QueryProvider.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/common/Badge.tsx
🔇 Additional comments (11)
src/components/common/error/ErrorBoundary.tsx (1)
1-4: LGTM! Clean and necessary imports.
The imports are well-organized and include all required dependencies for error boundary implementation.
src/components/layouts/ui/header/ui/header.menu.tsx (1)
38-38: Verify routing structure consistency.
The URL structure has been updated to include the '/teams/' prefix. Let's ensure this change is consistent across the application.
✅ Verification successful
The previous search patterns might have been too specific. Let's try with broader patterns to catch different variations of route definitions and understand the routing structure better.
URL structure with '/teams/' prefix is consistently implemented
Based on the search results, the routing structure using '/teams/' prefix is consistently implemented across the codebase:
- All team-related routes in components use the '/teams/${groupId}' pattern
- Found in multiple components:
MembershipItem.tsxheader.menu.tsxheader.nav.tsxTaskLists.tsxteams/[id]/index.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining direct groupId routes that might need updating
rg -g '*.{ts,tsx}' "href=.*?/\$\{.*?groupId\}" --no-filename
# Search for the new teams route pattern to verify consistency
rg -g '*.{ts,tsx}' "href=.*?/teams/\$\{.*?groupId\}" --no-filename
Length of output: 129
Script:
#!/bin/bash
# Search for routes containing groupId with more flexible pattern
rg -g '*.{ts,tsx}' "groupId.*?/edit" -B 2 -A 2
# Search for any route patterns with teams prefix
rg -g '*.{ts,tsx}' "/teams/" -B 2 -A 2
# Look for router configuration files
fd -e ts -e tsx "route|router" --exec head -n 20 {}
Length of output: 3356
package.json (1)
41-41: LGTM: react-error-boundary addition
The addition of react-error-boundary aligns with the PR objective of implementing global error handling. Version 4.1.2 is the latest stable version with no known security vulnerabilities.
src/components/layouts/ui/membership/MembershipItem.tsx (1)
35-35: Verify route changes across the application
The route paths have been updated to use the /teams prefix. While these changes are consistent with the route restructuring mentioned in the summary, we should verify that all related routes and navigation logic have been updated accordingly.
Also applies to: 67-67
✅ Verification successful
Route changes are consistent across the codebase
The verification shows that all relevant route references using groupId consistently use the /teams prefix. Found in:
- MembershipItem.tsx: Main link and edit team link
- header.nav.tsx: Team navigation link
- header.menu.tsx: Edit team link
No instances of the old route pattern without /teams prefix were found, confirming the changes are properly synchronized across the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining old route patterns and verify new route usage
# Check for old route patterns
echo "Checking for old route patterns..."
rg -g '*.{ts,tsx}' "href={\`/\$\{.*?groupId\}\`}"
# Check new route pattern usage
echo "Checking new route pattern usage..."
rg -g '*.{ts,tsx}' "href={\`/teams/\$\{.*?groupId\}\`}"
Length of output: 497
Script:
#!/bin/bash
# Description: Check for route patterns with improved regex
# Check for old route patterns without teams prefix
echo "Checking for old route patterns..."
rg -g '*.{ts,tsx}' 'href=.*?/[^t][^e][^a][^m][^s].*?groupId'
# Check new route pattern usage
echo "Checking new route pattern usage..."
rg -g '*.{ts,tsx}' 'href=.*?/teams.*?groupId'
# Check for any router definitions or route configurations
echo "Checking router configurations..."
rg -g '*.{ts,tsx}' 'createBrowserRouter|RouteObject|path:'
Length of output: 1018
src/apis/groups.api.ts (1)
12-16: 🛠️ Refactor suggestion
Verify error boundary configuration and add retry settings
The removal of local error handling aligns with the PR objective of centralizing error handling using Error Boundary. However, there are a few considerations:
- The PR mentions setting
retry: 0to prevent automatic retries, but this configuration is not visible in the API calls. - Type safety could be improved by using more specific error types with the Error Boundary.
Consider adding the following improvements:
- Add retry configuration to prevent automatic retries:
const queryClient = new QueryClient({
defaultOptions: {
queries: {
retry: 0,
throwOnError: true
}
}
});- Enhance type safety with custom error types:
type ApiError = {
status: number;
message: string;
};
// Example usage in one of the functions
export const getGroup = async (id: number) => {
try {
const response = await axiosInstance<GetGroupResponse>({
method: 'GET',
url: `groups/${id}`,
});
return response.data;
} catch (error) {
throw error as ApiError;
}
};Also applies to: 20-31, 58-69, 81-89, 93-101, 105-108
src/pages/boards/index.tsx (1)
Line range hint 34-43: Consider error boundary integration for search-related API calls
While the URL update logic is correct, ensure that any API calls triggered by search parameter changes are properly integrated with the new global error boundary.
Let's verify the search-related API calls:
Consider implementing a loading state and error handling for search operations to provide better user feedback.
src/components/layouts/ui/header/ui/header.nav.tsx (2)
Line range hint 34-47: Consider integrating with global error boundary
The component has local error handling with toast notifications. Consider whether this should be integrated with the new global error boundary for consistency.
Let's check error handling patterns:
✅ Verification successful
Toast notifications complement global error boundary, no changes needed
The current implementation is actually following good practices:
- The global error boundary is properly set up at the root level to catch unhandled errors and render fallback UI
- The toast notifications in the header component handle expected mutation errors, providing immediate feedback for specific user actions
- This is a complementary approach where:
- Expected errors (like failed API calls) are handled gracefully with toast messages
- Unexpected errors are caught by the error boundary as a safety net
The current implementation maintains both user-friendly feedback for expected errors and robust error handling for unexpected issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling patterns in mutation hooks
ast-grep --pattern 'useEffect($_) {
if (isError) {
$$$
}
}'
Length of output: 69
Script:
#!/bin/bash
# Let's search for error boundary usage and error handling patterns more broadly
echo "=== Searching for ErrorBoundary components ==="
rg -l "ErrorBoundary"
echo -e "\n=== Searching for error handling with toast notifications ==="
rg "toast.*error|isError.*toast" -A 2
echo -e "\n=== Searching for error handling patterns in useEffect ==="
rg "useEffect.*isError" -A 3
Length of output: 1069
Script:
#!/bin/bash
# Let's examine the error boundary implementation and how it's used
echo "=== Error Boundary Implementation ==="
cat src/components/common/error/ErrorBoundary.tsx
echo -e "\n=== Error Boundary Usage in _app.tsx ==="
cat src/pages/_app.tsx
Length of output: 1590
111-111: Verify URL structure consistency across the application
The URL structure change to /teams/${m.groupId} aligns with the new routing pattern.
Let's verify URL pattern consistency:
✅ Verification successful
Let me try a different search pattern to find team-related URL patterns:
Let me try another search pattern to find team-related routes and URLs:
URL pattern /teams/${id} is consistently used across the codebase
The search results confirm that the URL structure change to /teams/${m.groupId} aligns with the application's routing patterns. All team-related routes consistently use the /teams/ prefix followed by the group ID:
- Team details:
/teams/${id} - Edit team:
/teams/${id}/editteam - Team tasks:
/teams/${id}/tasks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all team-related route patterns
rg -g '!*.{json,md}' -A 2 "['\"`]/teams?/.*?['\"`]"
Length of output: 104420
Script:
#!/bin/bash
# Search for team-related route patterns and Link components with team URLs
rg -g '*.{ts,tsx,js,jsx}' -A 2 "(path|href).*?['\"].*?/teams?/.*?['\"]"
Length of output: 77
Script:
#!/bin/bash
# Search for any occurrences of /team or /teams in the codebase
rg -g '*.{ts,tsx,js,jsx}' "/teams?/"
# Also search for route definitions
rg -g '*.{ts,tsx,js,jsx}' "route.*team"
Length of output: 980
src/components/TaskList/TaskLists.tsx (1)
44-44: Verify URL structure consistency
The URL structure change to /teams/${taskList.groupId}/tasks aligns with the new routing pattern.
Let's verify task-related route patterns:
✅ Verification successful
Let me try another search pattern to find task-related routes:
Let me search for more route patterns to verify consistency:
URL structure is consistent with the codebase
The URL pattern /teams/${taskList.groupId}/tasks aligns with other team-related routes in the codebase:
/teams/[id]/editteamfor team editing/teams/[id]for team details/teams/[id]/tasksfor team tasks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all task-related route patterns
rg -g '!*.{json,md}' -A 2 "['\"`]/teams/.*/tasks?['\"`]"
Length of output: 100812
Script:
#!/bin/bash
# Search for task-related route patterns more specifically
rg -g '!*.{json,md}' -A 2 "router.*push.*teams.*tasks"
Length of output: 253
Script:
#!/bin/bash
# Search for all team/task related routes and navigation
rg -g '!*.{json,md}' "(/teams/|router.*teams)" -A 2
Length of output: 1520
src/pages/teams/[id]/index.tsx (2)
233-233: LGTM!
Good use of optional chaining with empty array fallback for the members prop.
39-39: Verify React Query's error handling configuration
The removal of local error handling aligns with using global error boundaries. However, ensure that the throwOnError option is properly configured in the React Query client to enable error propagation to the error boundary.
✅ Verification successful
React Query is properly configured for error boundary integration
The React Query client is correctly configured with throwOnError: true in the default options at src/providers/QueryProvider.tsx. This ensures that query errors will be propagated to the error boundary as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for React Query client configuration
rg -A 10 'QueryClient|throwOnError'
Length of output: 22839
| export default function GlobalBoundary({ | ||
| children, | ||
| }: { | ||
| children: React.ReactNode; | ||
| }) { | ||
| return ( | ||
| <QueryErrorResetBoundary> | ||
| {({ reset }) => ( | ||
| <ErrorBoundary onReset={reset} FallbackComponent={ErrorFallback}> | ||
| {children} | ||
| </ErrorBoundary> | ||
| )} | ||
| </QueryErrorResetBoundary> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing error boundary configuration.
While the basic setup is good, consider these improvements for more robust error handling:
- Add retry configuration as mentioned in PR objectives:
- Implement error classification for different API errors
export default function GlobalBoundary({
children,
}: {
children: React.ReactNode;
}) {
return (
<QueryErrorResetBoundary>
{({ reset }) => (
- <ErrorBoundary onReset={reset} FallbackComponent={ErrorFallback}>
+ <ErrorBoundary
+ onReset={reset}
+ FallbackComponent={ErrorFallback}
+ onError={(error) => {
+ // Log errors to your error reporting service
+ console.error('Error caught by boundary:', error);
+ }}
+ fallbackRender={({ error, resetErrorBoundary }) => (
+ <ErrorFallback
+ error={error}
+ resetErrorBoundary={resetErrorBoundary}
+ // Pass error type based on classification
+ errorType={classifyError(error)}
+ />
+ )}
+ >
{children}
</ErrorBoundary>
)}
</QueryErrorResetBoundary>
);
}
+// Add error classification utility
+function classifyError(error: unknown) {
+ if (axios.isAxiosError(error)) {
+ switch (error.response?.status) {
+ case 401: return 'unauthorized';
+ case 404: return 'not_found';
+ default: return 'api_error';
+ }
+ }
+ return 'unknown';
+}Committable suggestion skipped: line range outside the PR's diff.
| const response = await axiosInstance({ | ||
| method: 'DELETE', | ||
| url: `/groups/${groupId}/task-lists/${taskListId}`, | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }); | ||
| return response; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply consistent error handling pattern.
Apply the same retry configuration and type validation pattern to deleteTaskList.
export const deleteTaskList = async (groupId: number, taskListId: number) => {
const response = await axiosInstance({
method: 'DELETE',
url: `/groups/${groupId}/task-lists/${taskListId}`,
headers: {
'Content-Type': 'application/json',
},
+ retry: 0,
+ retryDelay: 1000,
});
+
+ // Validate response status
+ if (response.status !== 204) {
+ throw new Error('Unexpected response status');
+ }
return response;
};Committable suggestion skipped: line range outside the PR's diff.
| const response = await axiosInstance<TaskList>({ | ||
| method: 'POST', | ||
| url: `/groups/${groupId}/task-lists`, | ||
| data: { name }, | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }); | ||
| return response.data; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type validation and retry configuration.
While removing local error handling is good for centralization, consider these improvements:
+import { AxiosError } from 'axios';
export const postTaskList = async (groupId: number, name: string) => {
const response = await axiosInstance<TaskList>({
method: 'POST',
url: `/groups/${groupId}/task-lists`,
data: { name },
headers: {
'Content-Type': 'application/json',
},
+ // Implement retry behavior mentioned in PR objectives
+ retry: 0,
+ retryDelay: 1000,
});
+
+ // Validate response type
+ if (!isTaskList(response.data)) {
+ throw new Error('Invalid response format');
+ }
return response.data;
};
+// Type guard
+function isTaskList(data: unknown): data is TaskList {
+ return (
+ typeof data === 'object' &&
+ data !== null &&
+ 'id' in data &&
+ 'name' in data
+ );
+}Committable suggestion skipped: line range outside the PR's diff.
| <p className="mb-10 text-2xl mob:text-xl-semibold"> | ||
| {redirected ? '로그인 후 시도해주세요!' : error.response?.data.message} | ||
| </p> |
There was a problem hiding this comment.
Improve error message handling.
The current implementation might display raw API error messages or fail when the response structure is different.
Consider adding error message fallbacks and sanitization:
- <p className="mb-10 text-2xl mob:text-xl-semibold">
- {redirected ? '로그인 후 시도해주세요!' : error.response?.data.message}
- </p>
+ <p className="mb-10 text-2xl mob:text-xl-semibold">
+ {redirected ? '로그인 후 시도해주세요!' : (
+ error.response?.data.message ||
+ '일시적인 오류가 발생했습니다. 다시 시도해 주세요.'
+ )}
+ </p>📝 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.
| <p className="mb-10 text-2xl mob:text-xl-semibold"> | |
| {redirected ? '로그인 후 시도해주세요!' : error.response?.data.message} | |
| </p> | |
| <p className="mb-10 text-2xl mob:text-xl-semibold"> | |
| {redirected ? '로그인 후 시도해주세요!' : ( | |
| error.response?.data.message || | |
| '일시적인 오류가 발생했습니다. 다시 시도해 주세요.' | |
| )} | |
| </p> |
| if ( | ||
| axios.isAxiosError(error) && | ||
| error.response?.status === 401 && | ||
| !redirected | ||
| ) { | ||
| router.push('/signin'); // 로그인 페이지로 리다이렉트 | ||
| setRedirected(true); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Prevent potential memory leaks and improve redirect handling.
The current implementation has potential issues with cleanup and redirect handling.
Consider wrapping the redirect logic in useEffect:
- if (
- axios.isAxiosError(error) &&
- error.response?.status === 401 &&
- !redirected
- ) {
- router.push('/signin');
- setRedirected(true);
- return;
- }
+ useEffect(() => {
+ let mounted = true;
+ if (
+ axios.isAxiosError(error) &&
+ error.response?.status === 401 &&
+ !redirected
+ ) {
+ router.push('/signin').then(() => {
+ if (mounted) {
+ setRedirected(true);
+ }
+ });
+ }
+ return () => {
+ mounted = false;
+ };
+ }, [error, redirected, router]);📝 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.
| if ( | |
| axios.isAxiosError(error) && | |
| error.response?.status === 401 && | |
| !redirected | |
| ) { | |
| router.push('/signin'); // 로그인 페이지로 리다이렉트 | |
| setRedirected(true); | |
| return; | |
| } | |
| useEffect(() => { | |
| let mounted = true; | |
| if ( | |
| axios.isAxiosError(error) && | |
| error.response?.status === 401 && | |
| !redirected | |
| ) { | |
| router.push('/signin').then(() => { | |
| if (mounted) { | |
| setRedirected(true); | |
| } | |
| }); | |
| } | |
| return () => { | |
| mounted = false; | |
| }; | |
| }, [error, redirected, router]); |
| <div className="flex h-screen flex-col items-center justify-center"> | ||
| <div className="flex flex-col items-center"> | ||
| <h1 | ||
| className="relative mt-[50px] text-[10rem] text-brand-primary | ||
| mob:text-[6rem]" | ||
| > | ||
| <span className="relative inline-block animate-bounce404-1">E</span> | ||
| <span className="relative inline-block animate-bounce404-2">R</span> | ||
| <span className="relative inline-block animate-bounce404-1">R</span> | ||
| <span className="relative inline-block animate-bounce404-2">O</span> | ||
| <span className="relative inline-block animate-bounce404-3">R</span> | ||
| </h1> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility for error display.
The animated error text might not be accessible to screen readers and could be distracting for some users.
Consider these accessibility improvements:
- <div className="flex h-screen flex-col items-center justify-center">
+ <div
+ role="alert"
+ aria-live="assertive"
+ className="flex h-screen flex-col items-center justify-center"
+ >
<div className="flex flex-col items-center">
- <h1
+ <h1
+ aria-label="Error"
className="relative mt-[50px] text-[10rem] text-brand-primary
mob:text-[6rem]"
>📝 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.
| <div className="flex h-screen flex-col items-center justify-center"> | |
| <div className="flex flex-col items-center"> | |
| <h1 | |
| className="relative mt-[50px] text-[10rem] text-brand-primary | |
| mob:text-[6rem]" | |
| > | |
| <span className="relative inline-block animate-bounce404-1">E</span> | |
| <span className="relative inline-block animate-bounce404-2">R</span> | |
| <span className="relative inline-block animate-bounce404-1">R</span> | |
| <span className="relative inline-block animate-bounce404-2">O</span> | |
| <span className="relative inline-block animate-bounce404-3">R</span> | |
| </h1> | |
| </div> | |
| <div | |
| role="alert" | |
| aria-live="assertive" | |
| className="flex h-screen flex-col items-center justify-center" | |
| > | |
| <div className="flex flex-col items-center"> | |
| <h1 | |
| aria-label="Error" | |
| className="relative mt-[50px] text-[10rem] text-brand-primary | |
| mob:text-[6rem]" | |
| > | |
| <span className="relative inline-block animate-bounce404-1">E</span> | |
| <span className="relative inline-block animate-bounce404-2">R</span> | |
| <span className="relative inline-block animate-bounce404-1">R</span> | |
| <span className="relative inline-block animate-bounce404-2">O</span> | |
| <span className="relative inline-block animate-bounce404-3">R</span> | |
| </h1> | |
| </div> |
| const response = await axiosInstance<Task[]>({ | ||
| method: 'GET', | ||
| url: `groups/${id}/tasks`, | ||
| data: { date }, | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }); | ||
| return response.data; |
There was a problem hiding this comment.
Verify data parameter usage in getTasks
The getTasks function is using data parameter for a GET request, which is unusual. GET requests typically use query parameters instead.
Consider refactoring to use query parameters:
export async function getTasks(id: number, date: string) {
const response = await axiosInstance<Task[]>({
method: 'GET',
url: `groups/${id}/tasks`,
- data: { date },
+ params: { date },
headers: {
'Content-Type': 'application/json',
},
});
return response.data;
}📝 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 response = await axiosInstance<Task[]>({ | |
| method: 'GET', | |
| url: `groups/${id}/tasks`, | |
| data: { date }, | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| }); | |
| return response.data; | |
| const response = await axiosInstance<Task[]>({ | |
| method: 'GET', | |
| url: `groups/${id}/tasks`, | |
| params: { date }, | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| }); | |
| return response.data; |
| const handleTaskClick = (e: React.MouseEvent) => { | ||
| setSelectedTaskList(taskList); | ||
| router.push(`/${taskList.groupId}/tasks`); | ||
| router.push(`/teams/${taskList.groupId}/tasks`); | ||
| e.stopPropagation(); | ||
| }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add loading states and integrate with error boundary
The task-related mutations (handleTaskClick, handleTaskDelete) should be integrated with the global error boundary and include loading states for better user experience.
Consider adding loading states and ensuring mutations are configured to work with the error boundary:
const handleTaskClick = (e: React.MouseEvent) => {
+ // Show loading state
setSelectedTaskList(taskList);
router.push(`/teams/${taskList.groupId}/tasks`);
e.stopPropagation();
};Committable suggestion skipped: line range outside the PR's diff.
| taskLists={group?.taskLists || []} | ||
| groupId={id!.toString()} | ||
| isMember={isMember} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace non-null assertion with safe fallback
The non-null assertion (!) on id could lead to runtime errors. Consider providing a safe fallback or early return when id is undefined.
<TaskLists
taskLists={group?.taskLists || []}
- groupId={id!.toString()}
+ groupId={id?.toString() ?? ''}
isMember={isMember}
/>📝 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.
| taskLists={group?.taskLists || []} | |
| groupId={id!.toString()} | |
| isMember={isMember} | |
| taskLists={group?.taskLists || []} | |
| groupId={id?.toString() ?? ''} | |
| isMember={isMember} |
작업 내용
페이지 내부 에러(API)에서 데이터 표현(useQuery)시 발생하는 에러를 전역으로 핸들링하였습니다.
Error Boundary를 구현하기 위해 React Query의 throwOnError 옵션을 사용해줬습니다.
에러 발생 시 재시도(자동 api 재요청)를 여러번 발생시켜 길어지는 로딩 현상을 해결하기 위해 React Query의 retry 옵션을 0으로 지정했습니다.
QueryErrorResetBoundary 컴포넌트가 제공하는 쿼리 에러 상태 초기화 역할을 하는 매개변수를 사용하여 에러 발생시 사용자가 버튼을 누르면 API를 재요청하도록 하였습니다.
자세한 설명과 내용은 아래 정리한 블로그를 봐주시면 감사하겠습니다 !
에러 핸들링
에러 바운더리
스크린샷(선택)
관련 이슈
Summary by CodeRabbit
New Features
GlobalBoundary, for managing errors in the application.ErrorFallback, to provide user feedback during errors.Improvements
Bug Fixes
Chores