Repository navigation
Fix(host): 공연 수정뷰 QA 반영 - #354
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough뮤테이션에 쿼리 무효화(onSuccess + invalidateQueries)가 추가되고, 이벤트 수정 흐름이 모달 확인 핸들러에서 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/host/src/pages/event-edit/event-edit.tsx`:
- Around line 63-67: The submitEdit flow must catch network errors and surface
user feedback: wrap the body of submitEdit (which builds formData via
serializeUpdateFestivalFormData and calls await
updateMutation.mutateAsync(formData) then
navigate(NAV_PATH.noticeList(festivalId))) in a try/catch, on success navigate
as before, and on error show a user-facing toast or error message and avoid
closing any confirmation modal; also ensure the modal's onConfirm awaits
submitEdit (do not fire-and-forget or use implicit void) so the modal stays open
until submitEdit resolves/rejects. Use updateMutation.mutateAsync, submitEdit,
serializeUpdateFestivalFormData, navigate and NAV_PATH.noticeList identifiers
when editing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fec9cab1-07ff-4847-9116-b1ddcd543afa
📒 Files selected for processing (2)
apps/host/src/features/event-edit/use-event-edit.tsapps/host/src/pages/event-edit/event-edit.tsx
Sohyunnnn
left a comment
Sohyunnnn
left a comment
There was a problem hiding this comment.
빠른 수정 감사합니다!!
코멘트 남겨두었는데 확인 부탁드립니다
해당 PR 확인하다가 발견했는데, 디자인 시안을 보면 모달은 description 없이 설계된 것 같은데 현재 서비스에는 description이 포함된 버전으로 구현되어 있더라고요! |
jin-evergreen
left a comment
jin-evergreen
left a comment
There was a problem hiding this comment.
QA 관련 사항들 빠르게 반영해주셨네요!
코멘트 하나 확인해주시고, 다 같이 논의해보면 좋을 것 같아요 👀
고생 많으셨습니다👍👍
| navigate(NAV_PATH.noticeList(festivalId)); | ||
| }, | ||
| }); | ||
| await updateMutation.mutateAsync(formData); |
There was a problem hiding this comment.
작성해주신 PR을 확인한 후, async/await를 활용하여 흐름을 더 명확하게 하신 점은 좋은 방향성인 것 같아요!
mutateAsync는 익숙하지 않아서, 조금 찾아보니 mutateAsync는 오류를 수동으로 잡아야 한다고 해요. 그러나 현재는 관련한 로직이 없어서, 만약 mutateAsync를 계속 유지한다면 try/catch문을 활용할 필요가 있어보여요.
다만 조금 더 근본적으로 mutateAsync가 반드시 필요할까에 대해서는 고민이 필요할 것 같아요. 여러 뮤테이션을 실행하는 등 Promise를 직접 제어해야 하는 상황이 아니라면 mutate 사용을 권장하기도 하고, mutate의 콜백 방식을 통해서도 같은 흐름을 보장할 수 있을 것 같아요!
const submitEdit = (values: EventFormSubmitValues) => {
const formData = serializeUpdateFestivalFormData(values);
updateMutation.mutate(formData, {
onSuccess: () => {
// invalidateQueries가 끝난 뒤에 실행
navigate(NAV_PATH.noticeList(festivalId));
},
onError: () => {
// 토스트 등 에러 로직
},
});
};하지만 개인적인 의견일수도 있기에, 이 부분에 대해서는 다른 분들의 의견도 궁금해요!
아래 자료 중간에 mutate 또는 mutateAsync 섹션이 현재 PR 내용과 밀접한 부분인 것 같아서, 함께 확인해보면 좋을 것 같습니다!
['Mastering Mutations in React Query' 번역 아티클]
https://codingmax.net/courses/ko-react-query/section01/lec0013
There was a problem hiding this comment.
처음에는 수정 요청 이후의 흐름을 async/await로 한번에 읽을 수 있도록 mutateAsync를 사용했었는데, 말씀해 주신 것처럼 이번 케이스가 여러 뮤테이션을 조합해야 하는 등의 복잡한 상황이 아니기도 하고, mutate를 사용하는 것이 React Query에서 더 일반적으로 사용하는 패턴에 가까운 것 같아 mutate로 정리하는 쪽으로 반영했어요!
기존에 submitEdit에서 mutation을 처리하고 있던 것을 수정 모달의 확인 버튼에서 mutate를 호출하도록 변경했는데, 한번 더 확인해 주시면 감사하겠습니다.
좋은 리뷰 감사합니다!! 🙇♀️🙇♀️
There was a problem hiding this comment.
기존 mutateAsync를 mutate로 정리하신 부분 확인했습니다!
추가로 말씀해주신 것처럼, mutate 관련 로직이 모달 JSX 내부로 들어가게 수정해주신 것 같은데 이유가 궁금합니다!
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/host/src/pages/event-edit/event-edit.tsx (1)
63-67:⚠️ Potential issue | 🟠 MajorMust:
mutateAsync실패 경로를 처리하고, 성공 시에만 모달을 닫도록 순서를 바꿔 주세요.Line 66에서
mutateAsync가 실패하면 reject되는데, 현재는 확인 클릭 후 모달이 즉시 닫혀(Line 105~109) 실패 피드백 없이 끝날 수 있습니다.await onConfirm()성공 후에만 close/unmount 하시고, 실패 시에는 모달 유지 + 에러 안내가 필요합니다.제안 diff
interface ConfirmModalProps { title: string; description?: string; - onConfirm: () => void; + onConfirm: () => Promise<void> | void; } const submitEdit = async (values: EventFormSubmitValues) => { const formData = serializeUpdateFestivalFormData(values); await updateMutation.mutateAsync(formData); navigate(NAV_PATH.noticeList(festivalId)); }; ... <RectButton variant='primary' disabled={updateMutation.isPending} - onClick={() => { - onConfirm(); - close(); - unmount(); - }} + onClick={async () => { + try { + await onConfirm(); + close(); + unmount(); + } catch (error) { + // TODO: 토스트/에러 메시지 노출 + } + }} > 확인 </RectButton>#!/bin/bash # 1) mutateAsync 사용부와 try/catch 부재 확인 rg -n -C3 'const submitEdit = async|mutateAsync\(' apps/host/src/pages/event-edit/event-edit.tsx # 2) 확인 버튼에서 onConfirm 호출 직후 close/unmount 되는지 확인 rg -nPU '(?s)onClick=\{\(\) => \{\s*onConfirm\(\);\s*close\(\);\s*unmount\(\);' apps/host/src/pages/event-edit/event-edit.tsx # 3) mutation hook의 onError 부재 여부 확인 rg -n -C4 'useMutation|onSuccess|onError' apps/host/src/features/event-edit/use-event-edit.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/host/src/pages/event-edit/event-edit.tsx` around lines 63 - 67, The submit flow currently calls updateMutation.mutateAsync in submitEdit without a try/catch and the modal close/unmount (triggered by onConfirm → close/unmount) runs regardless of mutation failure; wrap the await updateMutation.mutateAsync(formData) in try/catch inside submitEdit, only call onConfirm() and then close()/unmount() after the await succeeds, and in the catch branch present an error to the user (e.g., set a local error state or call the existing toast/error helper) so the modal remains open on failure; reference submitEdit, updateMutation.mutateAsync, onConfirm, close, and unmount when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/host/src/pages/event-edit/event-edit.tsx`:
- Around line 63-67: The submit flow currently calls updateMutation.mutateAsync
in submitEdit without a try/catch and the modal close/unmount (triggered by
onConfirm → close/unmount) runs regardless of mutation failure; wrap the await
updateMutation.mutateAsync(formData) in try/catch inside submitEdit, only call
onConfirm() and then close()/unmount() after the await succeeds, and in the
catch branch present an error to the user (e.g., set a local error state or call
the existing toast/error helper) so the modal remains open on failure; reference
submitEdit, updateMutation.mutateAsync, onConfirm, close, and unmount when
making the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 727059b7-0889-4781-9f1b-72aa09efc624
📒 Files selected for processing (2)
apps/host/src/pages/event-edit/event-edit.tsxapps/host/src/widgets/home/festival-actions/festival-actions.tsx
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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 (2)
apps/host/src/pages/event-edit/event-edit.tsx (2)
89-111:⚠️ Potential issue | 🟠 MajorMust: 확인 버튼
onClick내부에isPending가드를 추가해 주세요.
overlay.open()으로 렌더링된 모달은 별도의 React 트리에서 동작하기 때문에,updateMutation.isPending상태가 변경되어도 모달이 리렌더링되지 않아요. 따라서disabledprop이 실시간으로 갱신되지 않고, 사용자가 확인 버튼을 여러 번 클릭하면 mutation이 중복 호출될 수 있어요.
onClick핸들러 내부에서isPending을 체크하면 실행 시점의 최신 상태를 읽기 때문에 중복 호출을 방지할 수 있어요.🛡️ 수정 제안
<RectButton variant='primary' disabled={updateMutation.isPending} onClick={() => { + if (updateMutation.isPending) { + return; + } + const formData = serializeUpdateFestivalFormData(values); updateMutation.mutate(formData, { onSuccess: () => { close(); unmount(); navigate(NAV_PATH.noticeList(festivalId)); }, onError: () => { toast.show( '공연 수정에 실패했어요.', '잠시 후 다시 시도해 주세요.', ); }, }); }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/host/src/pages/event-edit/event-edit.tsx` around lines 89 - 111, The onClick handler for the RectButton must guard against duplicate submits by reading the latest updateMutation.isPending before proceeding; inside the onClick callback (where serializeUpdateFestivalFormData is called and updateMutation.mutate is invoked) add an immediate check like `if (updateMutation.isPending) return;` (or equivalent) so the handler returns early when a mutation is in-flight, then proceed to call serializeUpdateFestivalFormData and updateMutation.mutate with the existing onSuccess/onError behavior (preserving close, unmount, navigate, and toast.show).
77-87: 🧹 Nitpick | 🔵 TrivialSuggest: 취소 버튼에도 동일한 가드를 추가하는 것을 권장해요.
확인 버튼과 동일한 stale closure 이슈가 있어요. 취소 버튼은 mutation을 호출하지 않지만, 일관성을 위해
onClick내부에서도isPending체크를 추가하면 더 안전해요.♻️ 수정 제안
<RectButton variant='secondary' disabled={updateMutation.isPending} onClick={() => { + if (updateMutation.isPending) { + return; + } close(); unmount(); }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/host/src/pages/event-edit/event-edit.tsx` around lines 77 - 87, The 취소 RectButton's onClick should guard against stale closure like the 확인 button: check updateMutation.isPending inside the onClick handler for the RectButton (the same place that calls close() and unmount()) and return early if pending; this keeps behavior consistent with the 확인 flow and prevents running close()/unmount() during an ongoing mutation while still keeping the button disabled prop tied to updateMutation.isPending.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/host/src/pages/event-edit/event-edit.tsx`:
- Around line 128-130: Rename the boolean variable hasDeletedCategory to use an
"is" prefix per guidelines (e.g., isAnyCategoryDeleted or isCategoryDeleted) in
the event-edit component: update the declaration (const hasDeletedCategory =
...) and all its subsequent references so the code continues to check
initialValues.activeCategoryIds.some(...) vs
values.activeCategoryIds.includes(...) under the new name; ensure any related
TypeScript types, props, or tests that reference hasDeletedCategory are updated
to the new identifier.
---
Outside diff comments:
In `@apps/host/src/pages/event-edit/event-edit.tsx`:
- Around line 89-111: The onClick handler for the RectButton must guard against
duplicate submits by reading the latest updateMutation.isPending before
proceeding; inside the onClick callback (where serializeUpdateFestivalFormData
is called and updateMutation.mutate is invoked) add an immediate check like `if
(updateMutation.isPending) return;` (or equivalent) so the handler returns early
when a mutation is in-flight, then proceed to call
serializeUpdateFestivalFormData and updateMutation.mutate with the existing
onSuccess/onError behavior (preserving close, unmount, navigate, and
toast.show).
- Around line 77-87: The 취소 RectButton's onClick should guard against stale
closure like the 확인 button: check updateMutation.isPending inside the onClick
handler for the RectButton (the same place that calls close() and unmount()) and
return early if pending; this keeps behavior consistent with the 확인 flow and
prevents running close()/unmount() during an ongoing mutation while still
keeping the button disabled prop tied to updateMutation.isPending.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 23ffba52-35c7-468f-8a8c-2c363a91602a
📒 Files selected for processing (1)
apps/host/src/pages/event-edit/event-edit.tsx
| const hasDeletedCategory = initialValues.activeCategoryIds.some( | ||
| (id) => !values.activeCategoryIds.includes(id), | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Suggest: 코딩 가이드라인에 따라 boolean 변수명을 is 접두사로 변경하는 것을 고려해 주세요.
현재 hasDeletedCategory는 변수인데, 코딩 가이드라인에서는 is 접두사는 boolean 변수에, has 접두사는 boolean을 반환하는 유틸리티 함수에 사용하도록 권장하고 있어요.
♻️ 수정 제안
- const hasDeletedCategory = initialValues.activeCategoryIds.some(
+ const isCategoryDeleted = initialValues.activeCategoryIds.some(
(id) => !values.activeCategoryIds.includes(id),
);
openConfirmModal({
- title: hasDeletedCategory
+ title: isCategoryDeleted
? '공지 카테고리를 수정하시겠어요?'
: '수정하시겠어요?',
- description: hasDeletedCategory
+ description: isCategoryDeleted
? '카테고리를 수정하면\n해당 카테고리로 작성된 공지가 삭제돼요.'
: undefined,
values,
});As per coding guidelines: "Use 'is' prefix for boolean variables (e.g., isActive)" and "Use 'has' prefix for utility functions that return boolean values"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/host/src/pages/event-edit/event-edit.tsx` around lines 128 - 130, Rename
the boolean variable hasDeletedCategory to use an "is" prefix per guidelines
(e.g., isAnyCategoryDeleted or isCategoryDeleted) in the event-edit component:
update the declaration (const hasDeletedCategory = ...) and all its subsequent
references so the code continues to check
initialValues.activeCategoryIds.some(...) vs
values.activeCategoryIds.includes(...) under the new name; ensure any related
TypeScript types, props, or tests that reference hasDeletedCategory are updated
to the new identifier.
Sohyunnnn
left a comment
Sohyunnnn
left a comment
There was a problem hiding this comment.
리뷰 반영사항 전부 확인했습니다! 수고하셨습니다 😽
추가 코멘트 남겨두었으니 확인 바랍니다
| mutationFn: (formData: FormData) => putFestival(festivalId, formData), | ||
| onSuccess: async () => { | ||
| await queryClient.invalidateQueries({ | ||
| queryKey: ORGANIZERS_QUERY_KEY.FESTIVAL_DETAIL(festivalId), | ||
| }); | ||
| }, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
처음 코드를 봤을 때는 PR에 적어주신 것처럼
수정 요청 성공 -> 관련 detail query invalidate -> 페이지 이동
흐름의 의도가 명확하다고 생각하여 리뷰 남기지 않았었습니다.
다만 수정해주신 현재 코드에서는 호출부 onSuccess에서 바로 후속 동작을 처리하고 있어서, invalidate 완료를 반드시 기다려야 하는 요구가 아니라면 useFestivalUpdateMutation 내부 onSuccess의 async/await는 제거해도 괜찮아 보입니다.
There was a problem hiding this comment.
나은님이 PR에 적어주신 흐름의 의도를 유지하려면, 현재 mutate로 변경했다 할지라도 async/await을 유지해야 의도한 것처럼 API 호출 -> invalidateQueries 실행 -> 끝날 때까지 대기 -> 호출부 onSuccess(페이지 라우팅) 실행 이라는 흐름이 유지될 것 같은데 소현님의 의견이 궁금합니다!
물론 이런 흐름의 의도를 유지하는 것이 불필요하다고 판단되면 말씀해주신 것처럼 제거하여, 불필요한 대기를 줄이는 것도 좋을 듯 합니다.
There was a problem hiding this comment.
리뷰 감사합니다!
말씀해 주신 것처럼 불필요한 대기를 줄여서 더 가볍게 가져가는 것도 좋다고 생각합니다. 다만 이번 PR의 시작점이 수정 이후에 관련 detail query를 다시 무효화해서 최신 데이터가 안정적으로 반영되도록 만드는 것이었기 때문에, 수정 성공 → detail query invalidate 완료까지 대기 → 그 다음 호출부 onSuccess 실행의 흐름이 유지되도록 하는 것이 처음 목적에 더 가깝다고 생각했습니다.
우선 기획 쪽에 QA 반영사항 전달 후, 성능상 불필요한 대기가 실제로 문제되는 지점이 보이면 그때 await 제거 여부를 검토해 보도록 하겠습니다! 두 분 다 좋은 의견 감사합니다 🙇♀️
3 participants
📌 Summary
📚 Tasks
🔍 Describe
문제...
1차 스프린트 QA 사항들을 반영했어요. (모달 텍스트 수정 / 공연 수정 후 상세 데이터 갱신 처리)
주요 이슈는 공연 수정 후에 다시 수정 페이지로 진입했을 때,
무대/부스 정보(AddedItem)의 수정사항이 반영되지 않는다는 것이었어요.원인...
확인 결과, 수정 요청은 성공하지만 이후 화면에서 최신 데이터를 안정적으로 GET 해오지 못해 AddedItem 목록이 최신 상태로 보이지 않는 것이 원인이었습니다.
이전 구조에서 수정 페이지는
FESTIVAL_DETAIL(festivalId)쿼리를 통해 상세 데이터를 조회하고 있는데, 공연 수정 mutation 쪽에서는 성공 이후 해당 상세 데이터 쿼리를 갱신하거나 무효화하는 로직이 없었습니다.따라서 PUT 요청이 성공하더라도 React Query 캐시에 남아 있는 이전 데이터가 다음 진입 시 그대로 사용되어, 사용자가 수정이 반영되지 않은 것처럼 보이는 화면에 진입하게 되는 것이었어요.
해결....
해당 이슈 해결을 위해
useFestivalUpdateMutation내부에queryClient.invalidateQueries를 추가했습니다. mutation 성공 시FESTIVAL_DETAIL(festivalId)쿼리를 무효화함으로써 수정 페이지로 다시 진입할 때 최신 서버 데이터를 조회하도록 수정했어요.더불어 수정 성공 후 페이지 이동 전 invalidate가 끝나도록
submitEdit부분도 정리했습니다. invalidate를 mutation 훅 내부에서 await 하고 이동하도록 정리하여,수정 요청 성공 -> 관련 detail query invalidate -> 페이지 이동이라는 흐름이 더 명확해지도록 했어요.👀 To Reviewer
최신 상태가 잘 반영되는지 확인 부탁드려요!
더 좋은 방법이 있다면 꼭!!! 리뷰 남겨주시면 감사하겠습니다... 🧎♀️