-
Notifications
You must be signed in to change notification settings - Fork 1
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.
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
Feat: #167 일정 수정 모달의 일정 파일 삭제 기능 구현 #176
Conversation
Walkthrough이 변경 사항은 파일 삭제 기능을 구현하기 위해 여러 컴포넌트와 훅을 수정하고 새로운 API 엔드포인트를 추가하는 내용을 포함합니다. 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
src/services/taskService.ts (1)
175-193
: 잘 구현되었습니다. JSDoc 주석에 대한 작은 제안이 있습니다.
deleteTaskFile
함수의 구현이 잘 되었습니다. 기존 코드 스타일과 일관성이 있으며, 타입 안전성을 보장합니다. API 엔드포인트 구성도 적절합니다.JSDoc 주석을 더욱 개선하기 위해 반환값에 대한 설명을 추가하는 것이 좋겠습니다. 다음과 같이 수정할 수 있습니다:
* @param {TaskFile['fileId']} fileId - 일정 파일 ID * @param {AxiosRequestConfig} [axiosConfig={}] - axios 요청 옵션 설정 객체 - * @returns {Promise<AxiosResponse<void>>} + * @returns {Promise<AxiosResponse<void>>} 성공 시 비어있는 응답을 반환하는 Promise */이렇게 하면 함수의 반환값에 대한 이해가 더욱 명확해질 것입니다.
src/hooks/query/useTaskQuery.ts (1)
232-248
: 새로운useDeleteTaskFile
함수가 잘 구현되었습니다.이 함수는 PR의 목표인 일정 파일 삭제 기능을 구현하고 있으며, 파일의 다른 mutation 훅들과 일관된 패턴을 따르고 있습니다. 오류 및 성공 메시지도 다른 함수들과 일치하는 스타일로 작성되었습니다.
한 가지 제안사항:
- 파일 삭제 성공 시
taskFilesQueryKey
뿐만 아니라tasksQueryKey
도 무효화하는 것이 좋을 것 같습니다. 이렇게 하면 파일 삭제가 전체 태스크 정보에 미치는 영향을 UI에 즉시 반영할 수 있습니다.다음과 같이 수정을 고려해보세요:
const { toastSuccess, toastError } = useToast(); const queryClient = useQueryClient(); const taskFilesQueryKey = generateTaskFilesQueryKey(projectId, taskId); + const tasksQueryKey = generateTasksQueryKey(projectId); const mutation = useMutation({ mutationFn: (fileId: TaskFile['fileId']) => deleteTaskFile(projectId, taskId, fileId), onError: () => toastError('일정 파일 삭제에 실패 했습니다. 잠시후 다시 시도해주세요.'), onSuccess: () => { toastSuccess('일정 파일을 삭제 했습니다.'); queryClient.invalidateQueries({ queryKey: taskFilesQueryKey }); + queryClient.invalidateQueries({ queryKey: tasksQueryKey }); }, });src/components/modal/task/UpdateModalTask.tsx (3)
63-63
: 파일 삭제 mutation 훅 사용 승인 및 개선 제안
useDeleteTaskFile
훅의 초기화는 파일 삭제 기능 구현이라는 PR 목표와 일치합니다.projectId
와taskId
를 매개변수로 사용하여 올바른 작업 범위를 지정하고 있습니다.가독성 향상을 위해 변수명을 더 구체적으로 변경하는 것을 고려해보세요:
-const { mutate: deleteTaskFileMutate } = useDeleteTaskFile(projectId, taskId); +const { mutate: deleteTaskFileMutation } = useDeleteTaskFile(projectId, taskId);이렇게 하면 이 변수가 mutation 함수라는 것이 더 명확해집니다.
128-128
: 파일 삭제 핸들러 구현 승인 및 개선 제안
handleFileDeleteClick
함수가 새로운deleteTaskFileMutate
함수를 사용하도록 업데이트된 것은 적절합니다. 이는 PR의 목표와 일치하며 간결하게 구현되었습니다.다음과 같은 개선 사항을 고려해보세요:
- 사용자 확인: 실수로 인한 삭제를 방지하기 위해 삭제 전 사용자 확인을 추가하세요.
- 오류 처리: 삭제 작업 실패 시 사용자에게 알림을 제공하세요.
예시 구현:
const handleFileDeleteClick = async (fileId: string) => { if (window.confirm('정말로 이 파일을 삭제하시겠습니까?')) { try { await deleteTaskFileMutate(Number(fileId)); toastInfo('파일이 성공적으로 삭제되었습니다.'); } catch (error) { toastWarn('파일 삭제 중 오류가 발생했습니다. 다시 시도해주세요.'); console.error('File deletion error:', error); } } };이러한 변경으로 사용자 경험과 오류 처리가 개선될 것입니다.
141-143
: 로딩 상태에 대한 조건부 렌더링 개선 승인 및 최적화 제안데이터 로딩 중 스피너를 표시하는 조건부 렌더링 블록 추가는 사용자 경험을 크게 향상시킵니다. 이는 컴포넌트의 여러 섹션에서 일관되게 구현되어 있습니다.
코드 중복을 줄이기 위해 다음과 같은 최적화를 고려해보세요:
- 공통 로딩 컴포넌트 생성:
const LoadingWrapper = ({ isLoading, children }) => { if (isLoading) return <Spinner />; return <>{children}</>; };
- 이 컴포넌트를 사용하여 코드 간소화:
<LoadingWrapper isLoading={isStatusLoading || isTaskLoading}> <FormProvider {...methods}> {/* form content */} </FormProvider> <ModalFormButton formId="updateTaskForm" isCreate={false} onClose={handleClose} /> </LoadingWrapper> <hr className="my-20" /> <LoadingWrapper isLoading={isProjectUserRoleLoading || isAssigneeLoading}> <section> {/* SearchUserInput and AssigneeList */} </section> </LoadingWrapper> <hr className="my-20" /> <LoadingWrapper isLoading={isTaskFileLoading}> <FileDropZone {/* FileDropZone props */} /> </LoadingWrapper>이렇게 하면 코드 중복이 줄어들고 가독성이 향상됩니다.
Also applies to: 180-182, 199-201
src/mocks/services/taskServiceHandler.ts (1)
103-103
: 프로젝트 권한 확인 로직 구현 필요현재 JWT에서
userId
를 가져와 프로젝트 권한을 확인하는 로직이 필요합니다. 이 부분을 구현하여 인증된 사용자의 권한을 확인해야 합니다.이 로직을 구현하는 데 도움이 필요하시면 말씀해 주시면, 새로운 GitHub 이슈를 생성하거나 구현 방법을 제안해 드릴 수 있습니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- src/components/common/FileDropZone.tsx (1 hunks)
- src/components/modal/task/UpdateModalTask.tsx (4 hunks)
- src/hooks/query/useTaskQuery.ts (5 hunks)
- src/mocks/services/taskServiceHandler.ts (1 hunks)
- src/services/taskService.ts (1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/hooks/query/useTaskQuery.ts (1)
Learnt from: Seok93 PR: GU-99/grow-up-fe#160 File: src/hooks/query/useTaskQuery.ts:28-28 Timestamp: 2024-09-25T11:48:38.512Z Learning: 삭제 기능은 다른 PR에서 다룰 예정입니다.
🔇 Additional comments (8)
src/components/common/FileDropZone.tsx (1)
71-71
: 변경 사항이 적절해 보입니다.
fileId
를 문자열로 변환하는 것은 API와의 일관성을 유지하는 데 도움이 될 수 있습니다. 이는 PR의 목표와 일치하며 파일 삭제 기능의 안정성을 향상시킵니다.다만, 코드의 가독성을 더욱 높이기 위해 다음과 같은 작은 개선을 제안합니다:
- handleFileDeleteClick(fileId.toString()); + handleFileDeleteClick(String(fileId));
String()
함수를 사용하면 의도가 더 명확해지고,null
이나undefined
값에 대해서도 안전하게 처리할 수 있습니다.src/hooks/query/useTaskQuery.ts (4)
12-12
: LGTM: 새로운 import 문이 추가되었습니다.
deleteTaskFile
함수의 import 추가는 새로운useDeleteTaskFile
훅의 구현과 일치합니다. 이는 일정 파일 삭제 기능 구현이라는 PR의 목표에 부합합니다.
131-131
: 오류 메시지가 개선되었습니다.
toastError
호출의 오류 메시지가 문법적으로 더 자연스럽게 수정되었습니다. 이는 사용자 경험을 향상시키는 긍정적인 변화입니다.
201-204
: 오류 및 성공 메시지가 개선되었습니다.
toastError
와toastSuccess
호출의 메시지들이 문법적으로 더 자연스럽게 수정되었습니다. 이러한 변경은 사용자에게 더 자연스러운 메시지를 제공하여 사용자 경험을 향상시킵니다.
221-224
: 오류 및 성공 메시지가 개선되었습니다.
toastError
와toastSuccess
호출의 메시지들이 문법적으로 더 자연스럽게 수정되었습니다. 이러한 변경은 사용자에게 더 일관된 메시지를 제공하여 전반적인 사용자 경험을 향상시킵니다.src/components/modal/task/UpdateModalTask.tsx (3)
22-22
: 새로운 훅 import 추가 승인
useDeleteTaskFile
훅의 추가는 파일 삭제 기능 구현이라는 PR 목표와 일치합니다. 관련 훅들과 함께 적절히 배치되어 있습니다.
202-210
: FileDropZone 컴포넌트 props 변경 승인FileDropZone 컴포넌트에
onFileDeleteClick
prop이 추가된 것은 새로운 파일 삭제 기능과 일치합니다.handleFileDeleteClick
함수가 올바르게 FileDropZone 컴포넌트에 전달되고 있습니다.이 변경은 파일 삭제 기능을 효과적으로 구현하고 있으며, 컴포넌트 구조를 일관성 있게 유지하고 있습니다.
Line range hint
1-215
: 전체 변경 사항에 대한 종합 평가이 PR의 변경 사항들은 파일 삭제 기능 구현이라는 목표를 효과적으로 달성하고 있습니다. 주요 개선 사항은 다음과 같습니다:
- 파일 삭제 기능의 성공적인 구현
- 로딩 상태에 대한 개선된 사용자 경험
- 컴포넌트 구조의 일관성 유지
전반적으로 코드 품질이 우수하며, PR의 목표를 잘 달성하고 있습니다. 제안된 개선 사항들(오류 처리, 사용자 확인, 코드 중복 감소)을 고려하여 구현하면 더욱 견고한 코드가 될 것입니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
파일 삭제 시 splice
함수를 써주셔서, 해당 함수에 대해 다시 한번 공부해 보게 되었습니다! (아직까지 js 함수에 대해 확실히 숙지하고 있지 못하다는 생각이 드네요. 더 공부해 봐야겠어요.😂)
파일 업로드가 너무 까다로웠어서 그런지 상대적으로 삭제가 간단해(?) 보이는 매직이...ㅋㅋㅋ 이번 작업도 고생 많으셨습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
파일 삭제 기능 고생하셨습니다!
@Yoonyesol @ice-bear98 리뷰 감사합니다! |
PR Type
What kind of change does this PR introduce?
Related Issues
What does this PR do?
View