Refector/onError 전역처리 #164
Conversation
WalkthroughThis pull request introduces significant changes to error handling across various API functions and hooks in the codebase. Specifically, it removes try-catch blocks from several asynchronous functions in Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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}
| method: 'DELETE', | ||
| url: `groups/${groupId}/member/${memberUserId}`, | ||
| }); | ||
| } |
There was a problem hiding this comment.
코드 리뷰를 해 드리겠습니다.
-
getGroup함수에서 try-catch 구문이 제거되어 있습니다. 이 부분은 오류 처리를 위해 존재했으므로, try-catch 구문을 유지하는 것이 좋습니다. -
postGroup함수에서 body 변수를 선언한 후에 response.data를 반환하는 부분이 중복되어 있습니다. response.data를 직접 반환하는 것이 더 깔끔할 것입니다. -
postInviteGroup함수와getInviteGroup함수에서도 try-catch 구문이 제거되어 있습니다. 마찬가지로 오류 처리를 위해 try-catch 구문을 유지하는 것이 바람직합니다. -
postTaskList함수와getTasks함수에서도 body 변수를 선언한 후에 response.data를 반환하는 부분이 중복되어 있습니다. 응답 데이터를 바로 반환하는 것이 더 간결합니다. -
deleteMember함수에서도 try-catch 구문이 제거되어 있습니다. 오류 처리를 위해 try-catch 구문을 다시 추가하는 것이 좋습니다.
이상입니다. 수정해야 할 부분과 개선할 점은 위와 같습니다. 감사합니다.
| }, | ||
| }); | ||
| return response; | ||
| }; |
There was a problem hiding this comment.
해당 코드는 postTaskList와 deleteTaskList 함수를 포함하는 모듈입니다.
postTaskList 함수에서는 groupId와 name을 매개변수로 받아서 해당 그룹의 할 일 목록을 생성하는 POST 요청을 보냅니다. 그러나 에러 발생 시에 에러를 throw 하는 부분에서 좀 더 구체적인 에러 메시지를 전달할 수 있도록 수정하는 것이 좋겠습니다.
deleteTaskList 함수에서는 groupId와 taskListId를 매개변수로 받아서 해당 그룹의 특정 할 일 목록을 삭제하는 DELETE 요청을 보냅니다. 마찬가지로 에러 처리 부분에서 조금 더 구체적인 에러 메시지를 전달할 수 있도록 수정하는 것이 좋습니다.
두 함수 모두 axiosInstance를 통해 HTTP 요청을 보내고, 응답에서 data를 반환하고 있습니다. HTTP 요청의 성공 여부에 따라 적절한 처리가 이루어지고 있습니다.
추가적으로, 함수 이름이 postTaskList와 deleteTaskList로 작명되어 있는데, 함수의 동작이 생성과 삭제를 담당하는 것으로 보아, 함수 이름을 보다 직관적으로 수정하는 것이 좋을 수도 있습니다.
전체적으로 코드는 깔끔하게 작성되어 있으며, 주요 기능에 대한 문제점은 보이지 않습니다. 함께 고려해 볼 부분은 에러 처리 시 에러 메시지의 구체화와 함수 이름의 명확성 등입니다.
| variant: 'destructive', | ||
| }); | ||
| }; | ||
| }; |
There was a problem hiding this comment.
이 코드 패치는 에러 처리에 대한 전역 핸들러 함수를 정의하는 것으로 보입니다.
개선 제안:
- AxiosError 인터페이스의 경우, AxiosError 인터페이스 대신 Error로 대체하는 것이 더 좋을 수 있습니다. 이렇게하면 다른 HTTP 클라이언트를 사용하거나 교체해야할 경우 유연성이 높아질 수 있습니다.
- 에러 메시지가 '알 수 없는 에러가 발생했습니다.'로 설정되어 있지만, 구체적인 에러 메시지를 제공하는 것이 사용자에게 더 도움이 될 수 있습니다. 예외 처리 로직을 추가하여 더 적합한 메시지를 제공하는 것이 좋습니다.
- 'destructive'와 같이 구체적인 variant를 사용하는 대신, 다양한 예외 유형에 대한 다양한 처리 방법을 구현할 수 있다면 더 좋을 것입니다.
이러한 개선 사항을 고려하여 코드를 수정하면 더욱 견고하고 사용자 친화적인 전역 에러 핸들러가 될 것입니다.
| onError: (error) => handleError(error, '게시글 삭제에 실패하였습니다.'), | ||
| }); | ||
| }; | ||
|
|
There was a problem hiding this comment.
코드 리뷰:
- 오류 처리 관련 사항:
- 현재 코드에서
onError콜백에 toast를 사용하여 오류 메시지를 표시하고 있습니다. 그러나useGlobalErrorHandler훅을 통해 전역적인 오류 처리를 하는 방법을 사용하고 있습니다.useGlobalErrorHandler를 사용하여 오류 처리를 일관되게 관리할 수 있으므로,onError콜백에서 직접 toast를 사용하는 대신useGlobalErrorHandler를 통해 처리하는 것이 더 나은 방법일 수 있습니다.
- 개선 제안:
onError콜백 내용을 중복해서 사용하고 있습니다.useUpdateArticleMutation및useDeleteArticleMutation함수에서 동일한onError핸들러를 사용하고 있습니다. 이러한 중복을 제거하고 코드의 가독성을 향상시키기 위해 공통된 로직을 함수로 추출하여 재사용하는 방식을 고려해 볼 수 있습니다.
- 잠재적 리스크:
- 코드에서
articleId가 어디서 오는지 명확히 확인할 필요가 있습니다. 현재updateArticle()및deleteArticle()함수에 전달되는articleId가 어디에서 가져오는 것인지 주석 등을 추가하여 명시적으로 표현하면 좋을 것 같습니다.
이러한 점들을 고려하여 코드를 개선할 수 있을 것으로 보입니다.
| }, | ||
| onError: (error) => handleError(error, '댓글 수정을 실패했습니다.'), | ||
| }); | ||
| }; |
There was a problem hiding this comment.
코드 리뷰를 해보겠습니다.
-
useGlobalErrorHandler를 추가했는데, 이것은 에러 핸들링을 위한 좋은 아이디어입니다. 이제 모든 오류 메시지를 일관된 방식으로 처리할 수 있게 되었습니다.
-
하지만, useAddComment, useUpdateComment, useDeleteComment 함수에서 toast를 사용하는 대신 handleError 함수를 사용하도록 수정했습니다. 하지만, useDeleteComment 함수의 onError에서 '댓글 수정을 실패했습니다.' 라고 되어 있는데, 댓글 삭제 실패 메시지가 아닌 댓글 수정 실패 메시지로 되어 있습니다. 이 부분을 수정해야 합니다.
-
useAddComment, useUpdateComment, useDeleteComment 함수에서 중복으로 handleError를 선언하고 있습니다. 이 부분을 함수나 변수로 추출하여 코드 중복을 제거하는 것이 좋습니다.
위의 점을 고려하여 코드를 수정하면 더욱 효율적이고 가독성 좋은 코드로 개선할 수 있을 것입니다.
| }, | ||
| onError: (error) => handleError(error, '해당 멤버를 삭제할 수 없습니다.'), | ||
| }); | ||
| }; |
There was a problem hiding this comment.
이 코드는 global error handling을 개선하기 위해 useGlobalErrorHandler 훅을 도입했으며, 각각의 mutation 함수에서 onError 핸들러를 통해 error를 처리하는 방식입니다. 이로써 각 mutation 함수에서 error를 처리하는 코드의 중복을 줄일 수 있고, 전역적인 에러 핸들링을 위해 useGlobalErrorHandler를 통해 통합적으로 관리할 수 있습니다.
개선 제안:
- handleError 함수 내부에서 error 객체의 message에 직접 접근하는 대신, error를 받아서 error.message를 반환하는 방식으로 변경할 수 있습니다. 이렇게 하면 error 객체 구조에 변화가 생겨도 해당 부분을 수정할 필요가 없어집니다.
- onError 핸들러의 처리 함수가 각각 toast를 호출하는 형태로 통일되어 있습니다. 이를 보다 일관된 방식으로 통합하고자 한다면 handleError 함수 내부에서 toast를 호출하도록 변경할 수 있습니다.
- 코드 중복을 줄이기 위해 useGlobalErrorHandler를 사용하는 부분을 최대한 활용하여 통일된 에러 핸들링 방식을 유지하는 것이 좋습니다.
전반적으로 global error handling이 잘 적용되어 있고, 에러 처리 부분을 개선하려는 노력이 보이는 좋은 코드입니다.
| }, | ||
| onError: (error) => handleError(error, '목록생성 실패.'), | ||
| }); | ||
| }; |
There was a problem hiding this comment.
이 코드 페치에서는 useGlobalErrorHandler가 새로 추가되었고, 모든 에러 처리 코드가 해당 핸들러로 대체되었습니다. 이는 코드의 가독성을 높이고 중복을 줄일 수 있습니다. 단 하나의 중요한 문제는 useDeleteTaskList 함수의 onError 핸들러에서 '목록생성 실패'라고 잘못 표기되어 있는 것입니다. 이것은 '목록삭제 실패'여야 합니다. 필요한 경우 이러한 표기 오류를 수정하고 각 핸들러에 대한 주석을 추가하여 코드를 더 명확하게 할 수 있습니다.
| }, | ||
| onError: (error) => handleError(error, '할 일 삭제를 실패했습니다.'), | ||
| }); | ||
| }; |
There was a problem hiding this comment.
코드 리뷰를 해보겠습니다.
-
코드 상단에 새로운 import 문이 추가되었습니다. useGlobalErrorHandler 훅을 가져와 사용하고 있습니다. 이는 코드에 새로운 기능이 추가되었음을 나타냅니다.
-
useAddTask, useUpdateTask, useUpdateTaskStatus, useDeleteTask 함수에서 이제 handleError 함수를 사용하고 있습니다. 이전에는 onError 콜백 내에서 error가 처리되었지만, 이제는 handleError 함수로 에러 처리 로직을 이동하여 코드의 중복을 줄였습니다. 이는 코드의 가독성과 유지보수성을 향상시킬 수 있습니다.
-
각각의 onError 콜백 내용이 handleError 함수로 이동되었으며, 에러 메시지가 추가되었습니다. 이것은 사용자에게 더 구체적인 에러 메시지를 제공하여 사용성을 높이는 좋은 접근 방식입니다.
-
코드에 대부분의 수정이 onError 콜백 내용을 handleError 함수로 이동시키는 것이므로, 전반적으로 코드 가독성이 향상되었으며, 중복 코드가 줄어들었습니다.
개선 제안:
- handleError 함수가 에러 메시지 외에도 추가적인 에러 처리 로직이 필요하다면 handleError 함수를 확장하여 처리하는 방법을 고려할 수 있습니다.
- 추가적인 에러 핸들링이나 에러 로깅이 필요한 경우에 대비하여 handleError 함수를 보다 강력하고 유연하게 만들 수 있습니다.
이상입니다. 혹시 추가 질문이 있거나 다른 문의 사항이 있으면 언제든지 물어봐주세요. 감사합니다.
| onError: (error) => handleError(error, '비밀번호 재설정을 실패했습니다.'), | ||
| }); | ||
| }; | ||
|
|
There was a problem hiding this comment.
useGlobalErrorHandler를 사용하여 오류 처리를 단순화하고 중복을 제거했기 때문에 좋은 변경이라고 생각됩니다. 그러나 onError 함수 내에서도 toast를 사용하여 에러를 표시하고 있습니다. handleError 함수 내에서 에러 메시지를 표시하거나 에러 처리를 완료하고 불필요한 toast 메시지를 피할 수 있습니다. 추가적으로, onError 함수에서 에러 처리가 많이 중복되어 있으므로 중복을 줄이기 위해 handleError 함수를 사용하여 onError 함수에서 불필요한 코드를 제거할 수 있습니다. 이런식으로 코드를 더 간결하게 유지할 수 있습니다.
![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
🧹 Outside diff range and nitpick comments (5)
src/hooks/useGlobalErrorHandler.ts (1)
8-8: Consider implementing i18n for error messagesThe hardcoded Korean error message should be externalized to support internationalization.
src/queries/taskList.queries.ts (1)
Line range hint
13-19: Consider using global error handler for validationThe validation error uses direct toast calls while other errors use the global handler. Consider unifying the approach:
mutationFn: (params: { groupId: number; name: string }) => { if (!params.name.trim()) { - toast({ - title: '목록생성 실패', - description: '할일 목록명을 작성해주세요.', - variant: 'destructive', - }); - return Promise.reject(new Error('할일 목록명을 작성해주세요.')); + throw new Error('할일 목록명을 작성해주세요.'); }src/queries/comments.queries.ts (1)
Line range hint
34-92: Consider standardizing success notification patternThe success notification pattern is inconsistent across mutations:
useAddComment: No success toastuseUpdateComment: Uses toast with title onlyuseDeleteComment: Uses toast with title onlyConsider standardizing the success notification pattern across all comment mutations for consistency.
src/queries/article.queries.ts (2)
Line range hint
143-182: Consider adding error handling to like/unlike mutations.The
useLikeArticleMutationanduseUnlikeArticleMutationoperations could fail due to network issues or server errors. Consider adding error handling to provide feedback to users.Apply this pattern to both mutations:
export const useLikeArticleMutation = (searchQuery: string, orderBy: string) => { const queryClient = useQueryClient(); + const handleError = useGlobalErrorHandler(); return useMutation({ mutationFn: (articleId: number) => likeArticle(articleId), onSuccess: (_, articleId) => { // ... existing success handling }, + onError: (error) => handleError(error, '게시글 좋아요에 실패하였습니다.'), }); };
22-22: Fix ESLint max-len warning.Consider moving the import to a new line to comply with the max-len rule.
-// eslint-disable-next-line max-len -import { useGlobalErrorHandler } from '@/hooks/useGlobalErrorHandler'; +import { useGlobalErrorHandler } from + '@/hooks/useGlobalErrorHandler';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/apis/groups.api.ts(3 hunks)src/apis/taskList.api.ts(1 hunks)src/hooks/useGlobalErrorHandler.ts(1 hunks)src/queries/article.queries.ts(5 hunks)src/queries/comments.queries.ts(5 hunks)src/queries/groups.queries.ts(8 hunks)src/queries/taskList.queries.ts(3 hunks)src/queries/tasks.queries.ts(7 hunks)src/queries/users.queries.ts(4 hunks)
🔇 Additional comments (11)
src/apis/taskList.api.ts (1)
5-13: 🛠️ Refactor suggestion
Improve type safety and response handling in postTaskList
While removing local error handling aligns with the PR objectives, we should ensure type safety:
- const response = await axiosInstance<TaskList>({
+ const { data } = await axiosInstance<{ data: TaskList }>({
method: 'POST',
url: `/groups/${groupId}/task-lists`,
data: { name },
headers: {
'Content-Type': 'application/json',
},
});
- return response.data;
+ return data;Likely invalid or redundant comment.
src/queries/taskList.queries.ts (1)
32-32: Consider implementing error type checking
The global error handler accepts both AxiosError and Error types, but there's no validation of error types in the onError callbacks.
Let's verify error handling in other mutation hooks:
Consider adding error type validation:
- onError: (error) => handleError(error, '목록생성 실패'),
+ onError: (error: unknown) => {
+ if (error instanceof Error || axios.isAxiosError(error)) {
+ handleError(error, '목록생성 실패');
+ }
+ },Also applies to: 52-52
src/queries/users.queries.ts (2)
48-48: LGTM: Error handling refactored correctly in useSendResetPasswordEmail
The implementation correctly uses the global error handler while maintaining the success toast notification.
Also applies to: 57-58
64-64: LGTM: Error handling refactored correctly in useResetPassword
The implementation correctly uses the global error handler while maintaining the success toast notification.
Also applies to: 73-73
src/queries/comments.queries.ts (2)
34-34: LGTM: Error handling refactored correctly in useAddComment
The implementation correctly uses the global error handler.
Also applies to: 49-49
56-56: LGTM: Error handling refactored correctly in useUpdateComment
The implementation correctly uses the global error handler while maintaining the success toast notification.
Also applies to: 67-67
src/queries/groups.queries.ts (2)
11-11: LGTM! Good practice implementing a global error handler.
The addition of useGlobalErrorHandler import aligns well with the PR objective of centralizing error handling.
38-38: Consistent implementation of global error handling across mutations.
The error handling implementation follows a consistent pattern across all mutations:
- Initialize
handleErrorat the beginning of each mutation - Use contextual error messages specific to each operation
- Maintain proper error handling in the mutation chain
This approach improves maintainability and provides consistent error feedback to users.
Also applies to: 50-50, 58-58, 70-70, 77-77, 91-91, 98-98, 110-110, 133-133, 145-145
src/queries/tasks.queries.ts (2)
Line range hint 43-76: LGTM! Consistent error handling in task mutations.
The implementation of global error handling in task mutations maintains consistency with the rest of the codebase while preserving the specific error messages for each operation.
Also applies to: 83-108, 191-209
Line range hint 114-161: Verify error handling in optimistic update flow.
The useUpdateTaskStatus mutation implements optimistic updates with rollback capability. While the global error handler is integrated, we should verify that the error handling sequence maintains the proper order:
- Rollback the optimistic update
- Show the error message
✅ Verification successful
Error handling sequence in optimistic update is correctly implemented
Looking at the implementation, the error handling in useUpdateTaskStatus follows the correct sequence:
- The
onErrorcallback first checks and restores the previous state usingcontext?.previousTasks - Only after the rollback, it calls
handleErrorto show the error message
This is the only optimistic update implementation in the codebase that requires rollback handling, as other mutations (useUpdateTask, useAddTask) use simple invalidation patterns instead.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other optimistic update implementations to ensure consistent error handling
ast-grep --pattern 'onMutate: async ($params) => {
$$$
await queryClient.cancelQueries($$$)
$$$
const previous$_ = queryClient.getQueryData($$$)
$$$
}'
Length of output: 163
Script:
#!/bin/bash
# Let's try a different pattern to search for optimistic updates and error handling patterns
rg -U "onMutate.*\n.*\{[\s\S]*?onError.*\n.*\{[\s\S]*?\}" --type ts
Length of output: 6796
src/queries/article.queries.ts (1)
Line range hint 99-116: LGTM! Proper implementation of global error handling.
The article update and delete mutations correctly implement the global error handler with appropriate error messages.
Also applies to: 126-140
| if (axios.isAxiosError(error)) { | ||
| errorMessage = error.response?.data?.message as string; | ||
| } else { | ||
| errorMessage = error.message; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety in error handling
The current implementation has potential type safety issues:
- The type assertion on
error.response?.data?.message as stringcould be unsafe - No type narrowing for the response data structure
Consider implementing a type-safe approach:
- if (axios.isAxiosError(error)) {
- errorMessage = error.response?.data?.message as string;
+ if (axios.isAxiosError<{ message: string }>(error)) {
+ errorMessage = error.response?.data?.message ?? errorMessage;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
Specify return type for deleteTaskList
The function should specify its return type for better type safety.
-export const deleteTaskList = async (groupId: number, taskListId: number) => {
+export const deleteTaskList = async (groupId: number, taskListId: number): Promise<void> => {
const response = await axiosInstance({
method: 'DELETE',
url: `/groups/${groupId}/task-lists/${taskListId}`,
headers: {
'Content-Type': 'application/json',
},
});
- return response;
+ return;📝 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({ | |
| method: 'DELETE', | |
| url: `/groups/${groupId}/task-lists/${taskListId}`, | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| }); | |
| return response; | |
| export const deleteTaskList = async (groupId: number, taskListId: number): Promise<void> => { | |
| const response = await axiosInstance({ | |
| method: 'DELETE', | |
| url: `/groups/${groupId}/task-lists/${taskListId}`, | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| }); | |
| return; |
| variant: 'destructive', | ||
| }); | ||
| }, | ||
| onError: (error) => handleError(error, '목록생성 실패.'), |
There was a problem hiding this comment.
Fix incorrect error message in useDeleteTaskList
The error message for deletion failure shows "목록생성 실패." (list creation failed) instead of a deletion-related message.
- onError: (error) => handleError(error, '목록생성 실패.'),
+ onError: (error) => handleError(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.
| onError: (error) => handleError(error, '목록생성 실패.'), | |
| onError: (error) => handleError(error, '목록삭제 실패'), |
| const response = await axiosInstance<GetGroupResponse>({ | ||
| method: 'GET', | ||
| url: `groups/${id}`, | ||
| }); | ||
| return response.data; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent error handling pattern across API functions
While removing try-catch blocks aligns with centralizing error handling, there are inconsistencies:
- Some functions (like
patchGroup) still retain their error handling - The
deleteMemberfunction doesn't return the response data, unlike other functions
Consider:
- Applying the same pattern across all functions
- Adding response data return for
deleteMember:
export async function deleteMember(groupId: number, memberUserId: number) {
- await axiosInstance({
+ const response = await axiosInstance({
method: 'DELETE',
url: `groups/${groupId}/member/${memberUserId}`,
});
+ return response.data;
}Also applies to: 20-31, 58-69, 81-89, 93-101, 105-108
| export const useDeleteComment = () => { | ||
| const queryClient = useQueryClient(); | ||
| const { toast } = useToast(); | ||
| const handleError = useGlobalErrorHandler(); |
There was a problem hiding this comment.
Fix incorrect error message in useDeleteComment
The error message incorrectly states "댓글 수정을 실패했습니다." (comment modification failed) for a deletion operation.
- onError: (error) => handleError(error, '댓글 수정을 실패했습니다.'),
+ onError: (error) => handleError(error, '댓글 삭제를 실패했습니다.'),Also applies to: 92-92
작업 내용
useGlobalErrorHandler로 에러 처리를 분리하여 에러 처리의 공통 로직이 한 곳에 집중되게 하여 코드의 가독성을 높이고 유지보수가 쉬워지도록 하였습니다.onError로직을 매번 새로 정의할 필요 없이, 에러 발생 시 제목(title)만 전달하면 재사용할 수 있습니다!axios.isAxiosError체크 등)을 모든 컴포넌트에 중복 작성하지 않아도 됩니다.리뷰 요구사항(선택)
관련 이슈