Skip to content

Refector/onError 전역처리 #164

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Injaeeee merged 1 commit into from
Jan 20, 2025
Merged

Refector/onError 전역처리 #164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 49 additions & 73 deletions src/apis/groups.api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,34 +9,26 @@ import { Task } from '@/types/tasks.types';
import { axiosInstance } from './_axiosInstance';

export const getGroup = async (id: number) => {
try {
const response = await axiosInstance<GetGroupResponse>({
method: 'GET',
url: `groups/${id}`,
});
return response.data;
} catch (error) {
throw new Error('팀 정보를 가져오는 데 실패했습니다.');
}
const response = await axiosInstance<GetGroupResponse>({
method: 'GET',
url: `groups/${id}`,
});
return response.data;
Comment on lines +12 to +16
Copy link

@coderabbitai coderabbitai bot Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Inconsistent error handling pattern across API functions

While removing try-catch blocks aligns with centralizing error handling, there are inconsistencies:

  1. Some functions (like patchGroup) still retain their error handling
  2. The deleteMember function doesn't return the response data, unlike other functions

Consider:

  1. Applying the same pattern across all functions
  2. 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 postGroup = async ({ name, image }: PostGroupRequest) => {
try {
const response = await axiosInstance.post<GetGroupResponse>(
'groups',
{ name, image },
{
headers: {
'Content-Type': 'application/json',
},
}
);
const body = response.data;
const response = await axiosInstance.post<GetGroupResponse>(
'groups',
{ name, image },
{
headers: {
'Content-Type': 'application/json',
},
}
);
const body = response.data;

return body;
} catch (error) {
throw new Error('팀 생성하는 데 실패했습니다.');
}
return body;
};

export const patchGroup = async ({ id, name, image }: UpdateGroupRequest) => {
Expand All @@ -63,22 +55,18 @@ export async function postInviteGroup({
userEmail,
token,
}: InviteGroupRequest) {
try {
const response = await axiosInstance.post<string>(
'groups/accept-invitation',
{ userEmail, token },
{
headers: {
'Content-Type': 'application/json',
},
}
);
const body = response.data;
const response = await axiosInstance.post<string>(
'groups/accept-invitation',
{ userEmail, token },
{
headers: {
'Content-Type': 'application/json',
},
}
);
const body = response.data;

return body;
} catch (error) {
throw new Error('그룹 참여에 실패했습니다.');
}
return body;
}

export const getInviteGroup = async (id: number) => {
Expand All @@ -90,44 +78,32 @@ export const getInviteGroup = async (id: number) => {
};

export const postTaskList = async (groupId: number, name: string) => {
try {
const response = await axiosInstance<TaskList>({
method: 'POST',
url: `/groups/${groupId}/task-lists`,
data: { name },
headers: {
'Content-Type': 'application/json',
},
});
return response.data;
} catch (error) {
throw new Error('할 일 목록를 생성하는 데 실패했습니다.');
}
const response = await axiosInstance<TaskList>({
method: 'POST',
url: `/groups/${groupId}/task-lists`,
data: { name },
headers: {
'Content-Type': 'application/json',
},
});
return response.data;
};

export async function getTasks(id: number, date: string) {
try {
const response = await axiosInstance<Task[]>({
method: 'GET',
url: `groups/${id}/tasks`,
data: { date },
headers: {
'Content-Type': 'application/json',
},
});
return response.data;
} catch (error) {
throw new Error('할 일을 불러오는 데 실패했습니다.');
}
const response = await axiosInstance<Task[]>({
method: 'GET',
url: `groups/${id}/tasks`,
data: { date },
headers: {
'Content-Type': 'application/json',
},
});
return response.data;
}

export async function deleteMember(groupId: number, memberUserId: number) {
try {
await axiosInstance({
method: 'DELETE',
url: `groups/${groupId}/member/${memberUserId}`,
});
} catch (error) {
throw new Error('멤버 삭제에 실패했습니다.');
}
await axiosInstance({
method: 'DELETE',
url: `groups/${groupId}/member/${memberUserId}`,
});
}
Copy link

@github-actions github-actions bot Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 리뷰를 해 드리겠습니다.

  1. getGroup 함수에서 try-catch 구문이 제거되어 있습니다. 이 부분은 오류 처리를 위해 존재했으므로, try-catch 구문을 유지하는 것이 좋습니다.

  2. postGroup 함수에서 body 변수를 선언한 후에 response.data를 반환하는 부분이 중복되어 있습니다. response.data를 직접 반환하는 것이 더 깔끔할 것입니다.

  3. postInviteGroup 함수와 getInviteGroup 함수에서도 try-catch 구문이 제거되어 있습니다. 마찬가지로 오류 처리를 위해 try-catch 구문을 유지하는 것이 바람직합니다.

  4. postTaskList 함수와 getTasks 함수에서도 body 변수를 선언한 후에 response.data를 반환하는 부분이 중복되어 있습니다. 응답 데이터를 바로 반환하는 것이 더 간결합니다.

  5. deleteMember 함수에서도 try-catch 구문이 제거되어 있습니다. 오류 처리를 위해 try-catch 구문을 다시 추가하는 것이 좋습니다.

이상입니다. 수정해야 할 부분과 개선할 점은 위와 같습니다. 감사합니다.

42 changes: 17 additions & 25 deletions src/apis/taskList.api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,32 +2,24 @@ import { TaskList } from '@/types/tasklist.types';
import { axiosInstance } from './_axiosInstance';

export const postTaskList = async (groupId: number, name: string) => {
try {
const response = await axiosInstance<TaskList>({
method: 'POST',
url: `/groups/${groupId}/task-lists`,
data: { name },
headers: {
'Content-Type': 'application/json',
},
});
return response.data;
} catch (error) {
throw new Error('할 일 목록를 생성하는 데 실패했습니다.');
}
const response = await axiosInstance<TaskList>({
method: 'POST',
url: `/groups/${groupId}/task-lists`,
data: { name },
headers: {
'Content-Type': 'application/json',
},
});
return response.data;
};

export const deleteTaskList = async (groupId: number, taskListId: number) => {
try {
const response = await axiosInstance({
method: 'DELETE',
url: `/groups/${groupId}/task-lists/${taskListId}`,
headers: {
'Content-Type': 'application/json',
},
});
return response;
} catch (error) {
throw new Error('');
}
const response = await axiosInstance({
method: 'DELETE',
url: `/groups/${groupId}/task-lists/${taskListId}`,
headers: {
'Content-Type': 'application/json',
},
});
return response;
Comment on lines +17 to +24
Copy link

@coderabbitai coderabbitai bot Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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;

};
Copy link

@github-actions github-actions bot Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 코드는 postTaskList와 deleteTaskList 함수를 포함하는 모듈입니다.

postTaskList 함수에서는 groupId와 name을 매개변수로 받아서 해당 그룹의 할 일 목록을 생성하는 POST 요청을 보냅니다. 그러나 에러 발생 시에 에러를 throw 하는 부분에서 좀 더 구체적인 에러 메시지를 전달할 수 있도록 수정하는 것이 좋겠습니다.

deleteTaskList 함수에서는 groupId와 taskListId를 매개변수로 받아서 해당 그룹의 특정 할 일 목록을 삭제하는 DELETE 요청을 보냅니다. 마찬가지로 에러 처리 부분에서 조금 더 구체적인 에러 메시지를 전달할 수 있도록 수정하는 것이 좋습니다.

두 함수 모두 axiosInstance를 통해 HTTP 요청을 보내고, 응답에서 data를 반환하고 있습니다. HTTP 요청의 성공 여부에 따라 적절한 처리가 이루어지고 있습니다.

추가적으로, 함수 이름이 postTaskList와 deleteTaskList로 작명되어 있는데, 함수의 동작이 생성과 삭제를 담당하는 것으로 보아, 함수 이름을 보다 직관적으로 수정하는 것이 좋을 수도 있습니다.

전체적으로 코드는 깔끔하게 작성되어 있으며, 주요 기능에 대한 문제점은 보이지 않습니다. 함께 고려해 볼 부분은 에러 처리 시 에러 메시지의 구체화와 함수 이름의 명확성 등입니다.

21 changes: 21 additions & 0 deletions src/hooks/useGlobalErrorHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { useToast } from '@/hooks/useToast';
import axios, { AxiosError } from 'axios';

export const useGlobalErrorHandler = () => {
const { toast } = useToast();

return (error: AxiosError | Error, title: string) => {
let errorMessage = '알 수 없는 에러가 발생했습니다.';

if (axios.isAxiosError(error)) {
errorMessage = error.response?.data?.message as string;
} else {
errorMessage = error.message;
}
Comment on lines +10 to +14
Copy link

@coderabbitai coderabbitai bot Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve type safety in error handling

The current implementation has potential type safety issues:

  1. The type assertion on error.response?.data?.message as string could be unsafe
  2. 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.

toast({
title,
description: errorMessage,
variant: 'destructive',
});
};
};
Copy link

@github-actions github-actions bot Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 코드 패치는 에러 처리에 대한 전역 핸들러 함수를 정의하는 것으로 보입니다.

개선 제안:

  1. AxiosError 인터페이스의 경우, AxiosError 인터페이스 대신 Error로 대체하는 것이 더 좋을 수 있습니다. 이렇게하면 다른 HTTP 클라이언트를 사용하거나 교체해야할 경우 유연성이 높아질 수 있습니다.
  2. 에러 메시지가 '알 수 없는 에러가 발생했습니다.'로 설정되어 있지만, 구체적인 에러 메시지를 제공하는 것이 사용자에게 더 도움이 될 수 있습니다. 예외 처리 로직을 추가하여 더 적합한 메시지를 제공하는 것이 좋습니다.
  3. 'destructive'와 같이 구체적인 variant를 사용하는 대신, 다양한 예외 유형에 대한 다양한 처리 방법을 구현할 수 있다면 더 좋을 것입니다.

이러한 개선 사항을 고려하여 코드를 수정하면 더욱 견고하고 사용자 친화적인 전역 에러 핸들러가 될 것입니다.

18 changes: 5 additions & 13 deletions src/queries/article.queries.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
UpdateArticleParams,
} from '@/types/dto/requests/article.request.types';
// eslint-disable-next-line max-len
import { useGlobalErrorHandler } from '@/hooks/useGlobalErrorHandler';
import { toast } from '@/hooks/useToast';
import {
ArticleCommentListResponse,
Expand Down Expand Up @@ -95,7 +96,7 @@ export const useUpdateArticleMutation = (
orderBy: string
) => {
const queryClient = useQueryClient();

const handleError = useGlobalErrorHandler();
return useMutation({
mutationFn: (data: UpdateArticleParams) => updateArticle(articleId, data),
onSuccess: () => {
Expand All @@ -112,12 +113,7 @@ export const useUpdateArticleMutation = (
title: '게시글 수정에 성공하였습니다.',
});
},
onError: () => {
toast({
title: '게시글 수정에 실패하였습니다.',
variant: 'destructive',
});
},
onError: (error) => handleError(error, '게시글 수정에 실패하였습니다.'),
});
};

Expand All @@ -127,6 +123,7 @@ export const useDeleteArticleMutation = (
orderBy: string
) => {
const queryClient = useQueryClient();
const handleError = useGlobalErrorHandler();
return useMutation({
mutationFn: () => deleteArticle(articleId),
onSuccess: () => {
Expand All @@ -140,12 +137,7 @@ export const useDeleteArticleMutation = (
title: '게시글 삭제에 성공하였습니다.',
});
},
onError: () => {
toast({
title: '게시글 삭제에 실패하였습니다.',
variant: 'destructive',
});
},
onError: (error) => handleError(error, '게시글 삭제에 실패하였습니다.'),
});
};

Copy link

@github-actions github-actions bot Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 리뷰:

  1. 오류 처리 관련 사항:
  • 현재 코드에서 onError 콜백에 toast를 사용하여 오류 메시지를 표시하고 있습니다. 그러나 useGlobalErrorHandler 훅을 통해 전역적인 오류 처리를 하는 방법을 사용하고 있습니다. useGlobalErrorHandler를 사용하여 오류 처리를 일관되게 관리할 수 있으므로, onError 콜백에서 직접 toast를 사용하는 대신 useGlobalErrorHandler를 통해 처리하는 것이 더 나은 방법일 수 있습니다.
  1. 개선 제안:
  • onError 콜백 내용을 중복해서 사용하고 있습니다. useUpdateArticleMutation 및 useDeleteArticleMutation 함수에서 동일한 onError 핸들러를 사용하고 있습니다. 이러한 중복을 제거하고 코드의 가독성을 향상시키기 위해 공통된 로직을 함수로 추출하여 재사용하는 방식을 고려해 볼 수 있습니다.
  1. 잠재적 리스크:
  • 코드에서 articleId가 어디서 오는지 명확히 확인할 필요가 있습니다. 현재 updateArticle() 및 deleteArticle() 함수에 전달되는 articleId가 어디에서 가져오는 것인지 주석 등을 추가하여 명시적으로 표현하면 좋을 것 같습니다.

이러한 점들을 고려하여 코드를 개선할 수 있을 것으로 보입니다.

Expand Down
29 changes: 7 additions & 22 deletions src/queries/comments.queries.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
getComments,
updateComment,
} from '@/apis/comments.api';
import { useGlobalErrorHandler } from '@/hooks/useGlobalErrorHandler';
import { useToast } from '@/hooks/useToast';
import { commentsQueryKeys } from '@/queries/keys/comments.keys';
import {
Expand All @@ -30,7 +31,7 @@ export const useGetComments = (params: GetCommentsRequest) => {

export const useAddComment = () => {
const queryClient = useQueryClient();
const { toast } = useToast();
const handleError = useGlobalErrorHandler();
return useMutation<AddCommentResponse, Error, AddCommentRequest>({
mutationFn: (params: AddCommentRequest) => addComment(params),
onSuccess: (_, params) => {
Expand All @@ -45,19 +46,14 @@ export const useAddComment = () => {
}),
});
},
onError: (error) => {
toast({
title: '댓글 추가를 실패했습니다.',
description: error.message,
variant: 'destructive',
});
},
onError: (error) => handleError(error, '댓글 추가를 실패했습니다.'),
});
};

export const useUpdateComment = () => {
const queryClient = useQueryClient();
const { toast } = useToast();
const handleError = useGlobalErrorHandler();
return useMutation<UpdateCommentResponse, Error, UpdateCommentRequest>({
mutationFn: (params: UpdateCommentRequest) => updateComment(params),
onSuccess: (_, params) => {
Expand All @@ -68,19 +64,14 @@ export const useUpdateComment = () => {
title: '댓글을 수정했습니다.',
});
},
onError: (error) => {
toast({
title: '댓글 수정을 실패했습니다.',
description: error.message,
variant: 'destructive',
});
},
onError: (error) => handleError(error, '댓글 수정을 실패했습니다.'),
});
};

export const useDeleteComment = () => {
const queryClient = useQueryClient();
const { toast } = useToast();
const handleError = useGlobalErrorHandler();
Copy link

@coderabbitai coderabbitai bot Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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

return useMutation<DeleteCommentResponse, Error, DeleteCommentRequest>({
mutationFn: (params: DeleteCommentRequest) => deleteComment(params),
onSuccess: (_, params) => {
Expand All @@ -98,12 +89,6 @@ export const useDeleteComment = () => {
title: '댓글을 삭제했습니다.',
});
},
onError: (error) => {
toast({
title: '댓글 삭제를 실패했습니다.',
description: error.message,
variant: 'destructive',
});
},
onError: (error) => handleError(error, '댓글 수정을 실패했습니다.'),
});
};
Copy link

@github-actions github-actions bot Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 리뷰를 해보겠습니다.

  1. useGlobalErrorHandler를 추가했는데, 이것은 에러 핸들링을 위한 좋은 아이디어입니다. 이제 모든 오류 메시지를 일관된 방식으로 처리할 수 있게 되었습니다.

  2. 하지만, useAddComment, useUpdateComment, useDeleteComment 함수에서 toast를 사용하는 대신 handleError 함수를 사용하도록 수정했습니다. 하지만, useDeleteComment 함수의 onError에서 '댓글 수정을 실패했습니다.' 라고 되어 있는데, 댓글 삭제 실패 메시지가 아닌 댓글 수정 실패 메시지로 되어 있습니다. 이 부분을 수정해야 합니다.

  3. useAddComment, useUpdateComment, useDeleteComment 함수에서 중복으로 handleError를 선언하고 있습니다. 이 부분을 함수나 변수로 추출하여 코드 중복을 제거하는 것이 좋습니다.

위의 점을 고려하여 코드를 수정하면 더욱 효율적이고 가독성 좋은 코드로 개선할 수 있을 것입니다.

Loading
Loading