Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 12 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
"react": "^18",
"react-day-picker": "^8.10.1",
"react-dom": "^18",
"react-error-boundary": "^4.1.2",
"react-hook-form": "^7.53.0",
"react-intersection-observer": "^9.13.1",
"tailwind-merge": "^2.5.4",

Choose a reason for hiding this comment

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

이 코드 패치를 살펴보겠습니다.

  1. 버그 리스크:
  • 새로 추가된 "react-error-boundary" 패키지의 버전이 "^4.1.2"로 지정되어 있습니다. 이 패키지의 안정성과 호환성을 고려하여 최신 버전으로 업데이트하는 것이 좋습니다.
  1. 개선 제안:
  • 코드 상단에 주석을 추가하여 해당 코드 패치에 대한 목적 또는 변경 내용을 설명하는 것이 좋습니다.
  • 패키지 버전 관리를 효율적으로 하기 위해 패키지 의존성을 정확히 관리하고 최신 버전으로 업데이트하는 것이 좋습니다.
  • 코드 품질을 높이기 위해 변수 및 함수의 명칭을 명확하고 의미있는 이름으로 변경하는 것이 좋습니다.

이상입니다. 부족한 점이 있거나 추가로 도움이 필요하시면 말씀해 주세요. 감사합니다!

Expand Down
122 changes: 49 additions & 73 deletions src/apis/groups.api.ts
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;
};

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;
Comment on lines +93 to +101
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

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

}

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}`,
});
}

Choose a reason for hiding this comment

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

  1. 트랜잭션에 대한 처리가 누락되어 있습니다. 예외가 발생할 경우 롤백되지 않는 것이 문제가 될 수 있습니다. 각 기능을 논리적인 그룹으로 묶어 트랜잭션 처리를 추가하는 것이 좋습니다.

  2. 에러 처리가 너무 간단합니다. 더 구체적이고 유용한 에러 메시지를 제공하는 것이 사용자 친화적이며 디버깅에 도움이 될 수 있습니다.

  3. 주석이 부족합니다. 코드의 의도와 각 함수가 하는 일에 대한 주석을 추가하여 코드의 가독성을 높일 수 있습니다.

  4. 코드 중복이 있습니다. 중복된 코드 부분을 함수로 분리하여 재사용성을 높일 수 있습니다.

  5. 테스트 코드가 없습니다. 각 함수에 대한 유닛 테스트를 작성하여 코드의 품질을 향상시킬 수 있습니다.

42 changes: 17 additions & 25 deletions src/apis/taskList.api.ts
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;
Comment on lines +5 to +13
Copy link

Choose a reason for hiding this comment

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

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

};

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

Choose a reason for hiding this comment

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

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

};

Choose a reason for hiding this comment

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

  1. postTaskList 함수와 deleteTaskList 함수는 거의 동일한 패턴을 가지고 있습니다. 중복을 방지하기 위해 중복 코드를 줄일 수 있습니다.
  2. error에 대한 더 구체적인 에러 메시지를 제공하는 것이 유용할 수 있습니다. 예를 들어, "할 일 목록 생성 실패"와 같은 메시지를 제공하면 어떤 부분에서 문제가 발생했는지 더 쉽게 파악할 수 있습니다.
  3. try-catch 블록에서 발생하는 예외에 대한 처리가 좀 더 구체적이어야 합니다. 특정 예외 유형에 따른 다른 처리를 할 수 있도록 수정하는 것이 좋을 수 있습니다.

2 changes: 1 addition & 1 deletion src/components/TaskList/TaskLists.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ function TaskItem({ taskList, taskListColor, isMember }: TaskItemProps) {

const handleTaskClick = (e: React.MouseEvent) => {
setSelectedTaskList(taskList);
router.push(`/${taskList.groupId}/tasks`);
router.push(`/teams/${taskList.groupId}/tasks`);
e.stopPropagation();
};

Choose a reason for hiding this comment

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

이 코드 패치에서 우리가 변경한 부분은 router.push 메소드에서 경로를 수정한 것뿐입니다. 이 변경은 /teams 경로를 추가하여 경로의 일관성을 유지하고 문제없이 작동할 것으로 예상됩니다.
버그나 위험은 발견되지 않았습니다. 또한 개선 제안사항은 현재 코드에서 주요한 요소로 보이지 않습니다. 이 변경은 안전하게 병합될 수 있습니다.

Comment on lines 42 to 47
Copy link

Choose a reason for hiding this comment

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

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

Expand Down
10 changes: 5 additions & 5 deletions src/components/common/Badge.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@ function Badge(props: BadgeProps): JSX.Element {
const no = count === 0 && left === 0;
const badgeClass = `flex items-center gap-1 rounded-2xl
bg-primary pb-1 pl-2 pr-2 pt-1 shadow-md`;
const ongoingSrc = 'icons/Progress_ongoing.svg';
const checkSrc = 'icons/Progress_done.svg';
const bestSrc = 'icons/Best.svg';
const doneSrc = 'icons/Check_lightGreen.svg';
const xSrc = 'icons/X.svg';
const ongoingSrc = '../icons/Progress_ongoing.svg';
const checkSrc = '../icons/Progress_done.svg';
const bestSrc = '../icons/Best.svg';
const doneSrc = '../icons/Check_lightGreen.svg';
const xSrc = '../icons/X.svg';

let imageSrc;
let altText;

Choose a reason for hiding this comment

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

이 코드 패치는 파일 경로를 수정하는 것으로 보입니다. 이동한 파일이 있는 디렉토리 경로를 고려하여 상대 경로를 사용하고 있습니다. 이러한 변경은 존재하는 파일을 참조할 때 필요합니다. 이 코드 패치는 안전한 변경으로 보입니다. 단, 이동한 파일이나 경로가 다시 변경될 수 있으므로 이후에도 업데이트가 필요할 수 있습니다.

Expand Down
19 changes: 19 additions & 0 deletions src/components/common/error/ErrorBoundary.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { QueryErrorResetBoundary } from '@tanstack/react-query';
import { ErrorBoundary } from 'react-error-boundary';
import ErrorFallback from './ErrorFallback';

export default function GlobalBoundary({
children,
}: {
children: React.ReactNode;
}) {
return (
<QueryErrorResetBoundary>
{({ reset }) => (
<ErrorBoundary onReset={reset} FallbackComponent={ErrorFallback}>
{children}
</ErrorBoundary>
)}
</QueryErrorResetBoundary>
);
}

Choose a reason for hiding this comment

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

해당 코드 패치는 QueryErrorResetBoundary와 ErrorBoundary를 사용하여 전역 오류 처리를 담당하는 GlobalBoundary 컴포넌트를 정의하고 있습니다. 코드의 주요 기능은 자식 컴포넌트(children)를 감싸고 있으며 오류가 발생할 경우 ErrorFallback 컴포넌트를 표시하는 것입니다.

개선 제안:

  1. 코드 내에서 React를 import하지 않았습니다. React를 import 하고 사용하는 것이 좋습니다.
  2. 각 라이브러리나 컴포넌트에 대한 주석이 없으므로 코드의 의도를 파악하기 어려울 수 있습니다. 주석을 추가하여 코드를 이해하기 쉽도록 할 수 있습니다.
  3. ErrorFallback 컴포넌트를 임포트하는 부분이 누락되어 있습니다. 필요한 경우 해당 컴포넌트를 추가해야 합니다.
  4. QueryErrorResetBoundary와 ErrorBoundary를 사용하는 방법에 대한 충분한 이해가 없다면 해당 라이브러리의 문서를 참고하여 올바르게 사용하는 것이 좋습니다.

버그 위험:

  1. 필요한 라이브러리나 컴포넌트가 import되지 않은 경우 오류가 발생할 수 있습니다. 모든 필요한 라이브러리와 컴포넌트가 올바르게 임포트되었는지 확인해야 합니다.
  2. ErrorFallback 컴포넌트를 사용하는데 필요한 정보나 동작이 누락되어 있는 경우 오류가 발생할 수 있습니다. ErrorFallback 컴포넌트를 올바르게 구현하고 사용하는 것이 중요합니다.

Comment on lines +5 to +19
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider enhancing error boundary configuration.

While the basic setup is good, consider these improvements for more robust error handling:

  1. Add retry configuration as mentioned in PR objectives:
  2. 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.

62 changes: 62 additions & 0 deletions src/components/common/error/ErrorFallback.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import Button, {
ButtonBackgroundColor,
ButtonBorderColor,
ButtonPadding,
ButtonStyle,
TextColor,
TextSize,
} from '@/components/common/Button/Button';

import axios from 'axios';
import { useRouter } from 'next/router';
import { useState } from 'react';
import { FallbackProps } from 'react-error-boundary';

export default function ErrorFallback({
error,
resetErrorBoundary,
}: FallbackProps) {
const [redirected, setRedirected] = useState(false);
const router = useRouter();
if (
axios.isAxiosError(error) &&
error.response?.status === 401 &&
!redirected
) {
router.push('/signin'); // 로그인 페이지로 리다이렉트
setRedirected(true);
return;
}
Comment on lines +21 to +29
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

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

return (
<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>
Comment on lines +31 to +43
Copy link

Choose a reason for hiding this comment

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

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

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

<p className="mb-10 text-2xl mob:text-xl-semibold">
{redirected ? '로그인 후 시도해주세요!' : error.response?.data.message}
</p>
Comment on lines +44 to +46
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

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


<Button
textSize={TextSize.Large}
buttonStyle={ButtonStyle.Box}
textColor={TextColor.White}
buttonBackgroundColor={ButtonBackgroundColor.None}
buttonBorderColor={ButtonBorderColor.LightGray}
buttonPadding={ButtonPadding.Large}
className="border-2"
onClick={() => resetErrorBoundary()}
>
{redirected ? '로그인하기' : '재시도하기'}
</Button>
</div>
);
}

Choose a reason for hiding this comment

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

가장 먼저 확인해야 할 사항은 코드에서 사용된 외부 라이브러리 및 컴포넌트의 import 부분입니다. 코드 상단의 import 구문을 살펴보면 Button 컴포넌트 등 다양한 요소들이 import 되어 있습니다. 이 부분은 올바르게 불러오고 있는지 확인할 필요가 있습니다.

또한, axios와 같은 외부 라이브러리를 사용할 때 오류 처리 부분에 대해서도 주목해야 합니다. 현재 코드에서는 axios를 통해 에러 발생 시 401 상태코드에 대한 처리가 있는데, 이 부분이 올바로 동작하는지 확인이 필요합니다.

그리고 페이지 레이아웃 및 디자인에 대한 부분도 코드 리뷰에서 중요한 부분입니다. 현재 코드에서는 에러 발생 시 보여지는 화면의 디자인 및 레이아웃이 구현되어 있습니다. 이 부분이 사용자에게 적절한 정보와 시각적 효과를 전달하는지 확인이 필요합니다.

마지막으로, 버튼 클릭 시 동작하는 함수인 resetErrorBoundary()에 대한 처리도 신중하게 검토해야 합니다. 이 함수가 예상대로 동작하여 사용자 경험에 영향을 미치지 않는지 확인이 필요합니다.

이 외에도 코드 내에 개선할 점이나 버그 가능성이 있는 부분을 발견하거나 추가적인 피드백이 있다면 언제든지 공유해주세요. 함께 논의하고 보완하는 데 도움이 될 것입니다.

2 changes: 1 addition & 1 deletion src/components/layouts/ui/header/ui/header.menu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export function HeaderMenu({
className="flex items-center justify-center"
>
<Link
href={`/${membership.groupId}/editteam`}
href={`/teams/${membership.groupId}/editteam`}
className="p-4 hover:bg-tertiary"
>
수정하기

Choose a reason for hiding this comment

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

이 코드 패치의 주된 변경점은 href 속성의 경로입니다. 예전에는 /groupId/editteam 경로로 이동하고 있었지만, 이제는 /teams/groupId/editteam 경로로 변경되었습니다.

이 변경으로 인해 두 가지 주요 사항을 확인할 필요가 있습니다. 첫 번째로, 모든 관련된 경로와 링크들이 제대로 수정되었는지 확인해야 합니다. 두 번째로, groupId가 유효한 값인지 체크해야 합니다. 경로 관련한 코드와 이동하는 컴포넌트에서 groupId의 유효성을 확인하는 코드가 필요할 수도 있습니다.

개선 제안으로는 groupId가 유효한 값인지 더 강화한 조건문을 추가하여 안전하게 처리할 수 있도록 하는 것이 좋습니다. 또한, 경로 관련해서 테스트 케이스를 추가하여 변경사항이 예상대로 동작하는지 확인하는 것이 좋습니다. 최종적으로 코드 리뷰를 진행하며 모든 경로가 올바르게 수정되었는지 확인하는 것이 중요합니다.

Expand Down
2 changes: 1 addition & 1 deletion src/components/layouts/ui/header/ui/header.nav.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ export default function HeaderNav() {
{memberships &&
memberships.map((m) => (
<li key={m.groupId} className="relative flex items-center">
<Link href={`/${m.groupId}`}>
<Link href={`/teams/${m.groupId}`}>
<Menu.Trigger
className="flex"
menuId={GROUP_MENU}

Choose a reason for hiding this comment

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

Link 컴포넌트의 href 속성은 변경되었습니다. 이 변경된 코드는 "/teams" 경로로 이동하도록 되어 있습니다. 해당 변경은 제대로 동작할 것으로 예상됩니다. 코드 리뷰에서 잠재적 오류나 개선점이 발견되지는 않았습니다.

Expand Down
4 changes: 2 additions & 2 deletions src/components/layouts/ui/membership/MembershipItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export const MembershipItem = memo(
<ImageIcon width={32} height={32} />
)}
<Link
href={`/${groupId}`}
href={`/teams/${groupId}`}
type="button"
className={cn([
'w-36 overflow-hidden text-ellipsis whitespace-nowrap',
Expand Down Expand Up @@ -64,7 +64,7 @@ export const MembershipItem = memo(
className="flex items-center justify-center"
>
<Link
href={`/${groupId}/editteam`}
href={`/teams/${groupId}/editteam`}
className="p-4 hover:bg-tertiary"
>
수정하기

Choose a reason for hiding this comment

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

변경된 코드 패치는 Link 요소에서 href 속성값이 변경되었습니다.
이전에는 /groupId로 설정되어 있었지만, 변경 후에는 /teams/groupId로 설정되었습니다.
또한, 두 번째 Link 요소도 비슷한 방식으로 변경되었습니다. /groupId/editteam에서 /teams/groupId/editteam로 변경되었습니다.

개선 제안:

  1. 변경된 URL 경로에 대한 주석을 추가하여 다른 개발자들이 코드를 이해하기 쉽도록 도와줍니다.
  2. classNames을 이용하여 스타일을 관리하고 있습니다. 이유가 명확하지 않으면 의미가 명확한 Tailwind CSS 클래스명으로 전환하면 더 깔끔한 코드를 유지할 수 있을 것입니다.
  3. 가능한 경우, 중복 링크 제거를 고려해보세요. 같은 링크가 여러 곳에 포함되어 있는 경우, 공통된 부분을 한 곳으로 통합해 중복을 방지할 수 있습니다.

Expand Down
Loading
Loading