[Fix] 경험 등록/수정/조회 페이지 리팩토링 전 버그 수정#157
Conversation
📝 WalkthroughWalkthrough폼 제출 시 중복 방지 기능을 추가하고, API 에러 메시지를 상수화했으며, 쿼리 키의 오타를 수정했습니다. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
🚀 빌드 결과✅ 린트 검사 완료 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/experience-detail/model/use-actions.ts (1)
110-131: 🧹 Nitpick | 🔵 Trivial
isSubmitting플래그가 stuck 상태로 남을 수 있는 edge case가 있어요.현재 구조에서
setIsSubmitting(true)이후mutate()호출 시, 성공/실패 콜백에서만 플래그가 해제돼요. 하지만 다음 상황에서 플래그가true로 남을 수 있어요:
- 컴포넌트가 언마운트되어 콜백이 호출되지 않는 경우
- 네트워크 요청이 취소되는 경우
이 PR의 범위가 pre-refactor 버그 픽스이므로, 현재 구현으로도 대부분의 이중 제출 시나리오를 방지할 수 있어요. 하지만 향후 리팩토링 시 다음 패턴을 고려해보세요:
onSettled콜백을 사용하여 성공/실패 모두에서 플래그 해제- 또는 페이지 진입 시
isSubmitting을 초기화하는 로직 추가🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/experience-detail/model/use-actions.ts` around lines 110 - 131, setIsSubmitting(true) can remain true if mutate callbacks never run (e.g., component unmount or request cancel); update the mutate calls for createMutation and patchMutation so they use their onSettled (or pass an options object with onSettled) to always call setIsSubmitting(false) and handle showSaveError on failure, and/or add a cleanup on component unmount to reset isSubmitting; specifically adjust createMutation.mutate(...) and patchMutation.mutate(...) use to supply onSettled and onError handlers rather than relying only on existing success/failure callbacks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/experience-detail/ui/experience-form/experience-form.tsx`:
- Around line 40-44: The submit Button in the rightSlot currently only disables
via isSubmitting; update the Button render logic in experience-form.tsx (the
rightSlot block referencing Button, submit, and isSubmitting) to also provide an
explicit loading state/label so users know submission is in progress—e.g., when
isSubmitting is true render the Button with a loading prop or show a spinner and
change its text to something like "제출중…" (ensure onClick remains wired to submit
and button stays disabled while loading for accessibility).
---
Outside diff comments:
In `@src/features/experience-detail/model/use-actions.ts`:
- Around line 110-131: setIsSubmitting(true) can remain true if mutate callbacks
never run (e.g., component unmount or request cancel); update the mutate calls
for createMutation and patchMutation so they use their onSettled (or pass an
options object with onSettled) to always call setIsSubmitting(false) and handle
showSaveError on failure, and/or add a cleanup on component unmount to reset
isSubmitting; specifically adjust createMutation.mutate(...) and
patchMutation.mutate(...) use to supply onSettled and onError handlers rather
than relying only on existing success/failure callbacks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba746711-e8b3-4bcc-a192-d589e8d08255
📒 Files selected for processing (5)
src/features/experience-detail/config/messages.tssrc/features/experience-detail/model/use-actions.tssrc/features/experience-detail/ui/experience-form/experience-form.tsxsrc/pages/experience-detail/experience-detail-page.tsxsrc/shared/api/config/query-key.ts
| rightSlot={ | ||
| <Button variant="primary" size="small" onClick={submit}> | ||
| <Button variant="primary" size="small" onClick={submit} disabled={isSubmitting}> | ||
| 작성완료 | ||
| </Button> | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
선택적 개선: 제출 중 상태를 사용자에게 더 명확히 전달할 수 있어요.
현재 버튼이 비활성화되지만, 사용자에게 "제출 중"임을 더 명확히 알려주면 UX가 향상될 수 있어요.
💡 제출 중 상태 표시 예시
<Button variant="primary" size="small" onClick={submit} disabled={isSubmitting}>
- 작성완료
+ {isSubmitting ? "저장 중..." : "작성완료"}
</Button>📝 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.
| rightSlot={ | |
| <Button variant="primary" size="small" onClick={submit}> | |
| <Button variant="primary" size="small" onClick={submit} disabled={isSubmitting}> | |
| 작성완료 | |
| </Button> | |
| } | |
| rightSlot={ | |
| <Button variant="primary" size="small" onClick={submit} disabled={isSubmitting}> | |
| {isSubmitting ? "저장 중..." : "작성완료"} | |
| </Button> | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/experience-detail/ui/experience-form/experience-form.tsx` around
lines 40 - 44, The submit Button in the rightSlot currently only disables via
isSubmitting; update the Button render logic in experience-form.tsx (the
rightSlot block referencing Button, submit, and isSubmitting) to also provide an
explicit loading state/label so users know submission is in progress—e.g., when
isSubmitting is true render the Button with a loading prop or show a spinner and
change its text to something like "제출중…" (ensure onClick remains wired to submit
and button stays disabled while loading for accessibility).
odukong
left a comment
odukong
left a comment
There was a problem hiding this comment.
쿼리키 작성할 때 제가 졸았나봅니다..... 꼼꼼한 수정 감사합니다 !! 덕분에 API 호출 시에 잘못된 쿼리 키 설정으로 생길 수 있는 오류를 방지할 수 있을 것 같아요 !!🫰🏻
에러 메시지 데이터 (message.ts의 API)는 서버에서 직접적으로 받아온 에러 코드와 (프론트에서 매핑한) 에러 메시지를 보여주는 방식이 실제 에러와 대응되고 화면에는 적절한 메시지를 보여줄 수 있어 안정적이라고 생각해요. 그렇지만 현재 api 호출부와 에러 관리 체계가 아직 안정적이지 않다고 생각해서(아마 추후 리팩토링을 추가적으로 진행하지 않을까합니다..ㅜ) 지금처럼 message.ts에서 에러메시지 관리를 처리하는 것이 적절한 방향이라고 생각합니다!
추가적으로 리팩토링 흐름에 대한 부분에 고민이 많을 것 같아서 간단하게나마 의견 남겨보자면, 흐름 자체는 좋다고 생각합니다. 다만 한 기능에 대해 리팩터링의 규모가 비교적 세분화되어있다고 생각해요.
개발 아티클을 보면서 인상깊었던 말 중에 하나인데 구현하는데 이런 원칙을 가지라는 것을 본 적이 있어요.
"작동하게 만들고, 올바르게 만들고, 빠르게 만들어라"
우선 경험 등록,수정,조회에 대한 기능은 정상적으로 작동하기 때문에 제일 첫 번째 조건은 만족되었고, 다음 진행되어야 할 부분은 올바르게 만들어라!! 라고 생각합니다.
올바르게 만들기 위한 단계의 리팩토링은 우선적으로 두 가지 부분은 고려하여 진행하면 좋을 것 같아요.
1. 중복 코드 제거 및 모듈 구조 정리
2. zustand 스토어 구조 개선
=> 스토어의 변화에 따른 경험 작성에 엮여있는 상태를 사용하는 코드에 대한 리팩토링 함께 진행 (PR 2, 4, 5)
현재 경험 등록/수정/조회 기능은 결합도 분리를 위해서 분리된 모듈 파일이나 사용하고 있는 상태들이 많습니다. 그 만큼 함께 관리해야 하는 파일도 많아졌다고 생각이 들어 코드에 대한 적절한 가지치기와 상태에 대한 리팩토링을 함께 진행하는 것이 좋을 것 같다는 생각이 듭니다. (PR 4에 작성해주신 것처럼요!)
어느 정도의 가지치기가 완료가 되었다면, 그 때 유진님이 생각하기에 지금의 코드를 더 효율적으로 만들기 위해 적용하면 좋을 방법들을 코드에 녹여내어 보면, 비교적 쉽게 리팩토링을 진행할 수 있을 것 같아요. 이 단계에서 빠르게 만들어라.. 이건 아마 사용자 경험이나 최적화, DX적인 부분에 대한 개선을 의미하겠죠..? (PR 6, PR 7과 같은 부분)
이렇게 크게 두 흐름으로 나누어 리팩토링을 진행 흐름으로 정리를 해봤는데 천천히 코드 분석을 하다 보면 유진님에게 맞는 리팩토링 흐름이 보일 거라 생각해요! 제가 말한 방식이 정답은 아니기 때문에 리팩토링할 때 참고 용도로 보시면 될 것 같습니당
앞으로의 리팩토링도 아자아자아잣 화이팅 o((>ω< ))o
p.s. 쓰다보니.. 되게 명언충.. 같이 되었네요..
hummingbbird
left a comment
hummingbbird
left a comment
There was a problem hiding this comment.
지난 회의 때 이야기했던 부분들은 수빈이가 위 리뷰에서 잘 정리해주어서 어푸만 남기겠습니다 ~~ 작업 파이팅하고 고민되는 지점은 언제든 쉐어해주세요 !! 😙😙
| export const aiReportsQueryKey = { | ||
| all: () => ["aiReports"], | ||
| list: (page: number, keyword?: string) => [ | ||
| ...experienceQueryKey.all(), |
There was a problem hiding this comment.
오 맞아요 나도 여기 보면서 수정 필요하다고 생각했는데! 굿굿 ~
qowjdals23
left a comment
qowjdals23
left a comment
There was a problem hiding this comment.
이번 pr은 리팩토링 전에 기준선 먼저 정리해두는 의미가 큰 수정이라고 느껴졌고,
query key 수정이나 submit 방지, 메시지 상수화도 지금 시점에 잘 필요한 정리였던 것 같습니다 !
"작동하게 만들고, 올바르게 만들고, 빠르게 만들어라"
위에서 좋은 의견이 너무너무 잘 정리되어 있어서 저도 어푸만 남기고 갑니다 ~~
너무 수고 많으셨고 !!! 이후 리팩토링도 모두 화이팅입니다~~ 🤩
|
리뷰 꼼꼼하게 남겨주셔서 감사합니다!! 수빈님이 말씀해주신 것처럼, 현재는 message.ts에서 에러 메시지를 관리하고 추후 리팩토링을 통해 개선해나가는 방향 이해했습니다! :) 리팩토링 흐름 관련해서도
이 기준을 잘 기억하면서, 적어주신 리팩토링 과정 참고해서 우선 중복 코드/모듈 구조 정리와 상태 관리 개선을 중심으로 코드 정합성과 구조를 먼저 다듬는 방향으로 진행해보겠습니다! 리팩토링 순서에 대해 고민이 많았는데, 정리해주신 흐름 덕분에 방향을 잡는 데 큰 도움이 되었습니다 감사합니다 👍 그 이후에는 말씀해주신 것처럼 UX, 성능, DX까지 확장하는 흐름으로 이어가보겠습니다. 전체적으로 방향을 어떻게 가져가야 할지 기준이 잡힌 것 같아서 말씀해주신 내용 참고해서 리팩토링 단계적으로 진행해보겠습니다 🙇 |
4 participants
✏️ Summary
close [Fix] 경험 등록/수정/조회 페이지 리팩토링 전 버그 수정 #156
경험 등록/수정/삭제 페이지 리팩토링 전에, 선행 버그 수정 작업을 진행했습니다.
Query Key 오타 및 잘못된 Query Key 참조를 수정해 캐시 키가 의도한 도메인 기준으로 동작하도록 바로잡았습니다.
제출 중 중복 요청이 발생하지 않도록 더블 submit 방지 로직을 추가하고, 에러 메시지 하드코딩 문자열을 상수로 분리해 메시지 관리 일관성을 높였습니다.
📑 Tasks
✔️ Query Key 오타 및 잘못된 참조 수정
experienceQueryKey.all()에서"experiene"로 작성되어 있던 오타를"experience"로 수정했습니다.aiReportsQueryKey.list()가experienceQueryKey.all()을 참조하고 있던 문제를aiReportsQueryKey.all()참조로 수정했습니다.query-key.ts파일 내에서 발견된 단순 버그들이라, 리팩토링 전에 먼저 바로잡아 이후 캐시 무효화 및 조회 흐름 기준선을 명확히 하고자 했습니다.✔️ 경험 등록/수정 시 더블 submit 방지
submit함수 진입 시점에isSubmitting상태를 즉시 확인하는 guard를 추가해, 제출 중 중복 호출이 발생하면 바로 return 하도록 처리했습니다.작성완료버튼에disabled={isSubmitting}을 적용해, 사용자가 제출 중 상태를 시각적으로 인지할 수 있도록 했습니다.✔️ 에러 메시지 매직 스트링 상수화
experience-detail-page.tsx에 직접 하드코딩되어 있던"경험 데이터를 불러오는데 실패했습니다."문자열을EXPERIENCE_MESSAGES.API.FETCH_FAILED상수로 분리했습니다.messages.ts로 일원화해, 이후 문구 수정이나 에러 정책 확장 시 관리가 쉬운 구조로 정리했습니다.👀 To Reviewer
messages.ts기반 메시지 관리 방향이 적절한지 위주로 봐주시면 감사하겠습니다!📸 Screenshot
🔔 ETC