feat : to-sing 곡 순서 변경 및 삭제 낙관적 업데이트 적용 (#230)#231
Open
GulSam00 wants to merge 1 commit into
Open
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Owner
Author
|
/describe |
Owner
Author
|
/review |
Owner
Author
|
/improve |
Review Summary by QodoFix optimistic updates for to-sing song mutations
WalkthroughsDescription• Fix useDeleteToSingSongMutation missing return statement in onMutate • Remove redundant invalidateQueries calls from both mutations • Prevent optimistic updates from being overwritten by server refetch • Maintain rollback functionality on mutation failure Diagramflowchart LR
A["Delete/Patch Mutation"] -->|onMutate| B["Cancel & Cache Previous State"]
B -->|setQueryData| C["Apply Optimistic Update"]
C -->|onError| D["Rollback to Previous State"]
C -->|onSuccess| E["Keep Optimistic Data"]
E -->|onSettled| F["Invalidate searchSong Only"]
File Changes1. apps/web/src/queries/tosingSongQuery.ts
|
Code Review by Qodo
Context used✅ Tickets:
🎫 to-sing 페이지 곡 순서 변경 및 삭제 낙관적 업데이트 적용 1. old.filter without null-guard
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
60
to
62
| queryClient.setQueryData(['toSingSong'], (old: ToSingSong[]) => { | ||
| old.filter(song => song.songs.id !== songId); | ||
| return old.filter(song => song.songs.id !== songId); | ||
| }); |
There was a problem hiding this comment.
1. old.filter without null-guard 📎 Requirement gap ☼ Reliability
The optimistic delete updater assumes old is always an array, but React Query can pass undefined when the cache is empty/unset, causing a runtime exception. This can prevent the song from being removed immediately and break the expected optimistic-update behavior for deletion.
Agent Prompt
## Issue description
The optimistic delete cache updater calls `old.filter(...)` without guarding for `old` possibly being `undefined`.
## Issue Context
React Query's `setQueryData` updater may receive `undefined` if the query cache is not yet set. A runtime error here breaks the optimistic delete behavior required for the to-sing page.
## Fix Focus Areas
- apps/web/src/queries/tosingSongQuery.ts[60-62]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| alert(error.message ?? 'PATCH 실패'); | ||
| queryClient.setQueryData(['toSingSong'], context?.prev); | ||
| }, | ||
| onSettled: () => { |
There was a problem hiding this comment.
2. Reorder uses stale weights 🐞 Bug ≡ Correctness
usePatchToSingSongMutation replaces the cached list with newItems but never updates the moved item’s order_weight, and this PR removed the post-mutation invalidate of ['toSingSong'], so cached order_weight stays stale for up to 5 minutes. Subsequent drag/drop operations compute newWeight from these stale order_weight values and can persist incorrect weights to the server, causing ordering to “jump” after a later refetch/reload.
Agent Prompt
### Issue description
After a successful reorder, the cache keeps the previous `order_weight` values because the optimistic update only reorders `newItems` and `patchToSingSong` returns `void`. Since this PR removed `invalidateQueries(['toSingSong'])`, subsequent reorder operations compute `newWeight` from stale `order_weight` values and can send incorrect weights to the server.
### Issue Context
- `useToSingSong` computes `newWeight` using neighbor items’ `order_weight`.
- `usePatchToSingSongMutation` sets `['toSingSong']` cache to `newItems` as-is.
- `QueryClient` defaults prevent fast refetches (`refetchOnWindowFocus: false`, `staleTime: 5m`).
### Fix Focus Areas
- apps/web/src/queries/tosingSongQuery.ts[80-100]
- apps/web/src/hooks/useToSingSong.ts[43-64]
### Suggested fix
1. In `usePatchToSingSongMutation.onMutate`, destructure `{ songId, newWeight, newItems }` (not just `newItems`).
2. Before calling `setQueryData`, create `optimisticItems` that updates the moved item’s `order_weight` to `newWeight` (e.g., `map` by `songs.id`), then set the cache to `optimisticItems`.
3. Optionally, add a debounced `invalidateQueries(['toSingSong'])` in `onSuccess` (not `onSettled`) if you still want periodic server reconciliation without overriding in-flight optimistic interactions.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
|
Persistent review updated to latest commit 518938c |
|
PR Description updated to latest commit (518938c)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
📌 변경 사항
useDeleteToSingSongMutationonMutate버그 수정:old.filter()결과를return하지 않아undefined가 캐시에 설정되던 문제 수정useDeleteToSingSongMutationonSettled에서invalidateQueries(['toSingSong'])제거 — 낙관적 업데이트가 서버 재요청으로 덮어씌워지던 문제 수정usePatchToSingSongMutationonSettled에서invalidateQueries(['toSingSong'])제거 — 동일 이유onError에서 이전 상태 롤백은 그대로 유지💬 추가 참고 사항
PR Type
Bug fix, Enhancement
Description
Fix
useDeleteToSingSongMutationmissing return statement inonMutateRemove unnecessary
invalidateQueriescalls from both mutationsPrevent optimistic updates from being overwritten by server refetches
Maintain rollback functionality on mutation failures
Diagram Walkthrough
File Walkthrough
tosingSongQuery.ts
Fix optimistic updates and remove query invalidationapps/web/src/queries/tosingSongQuery.ts
returnstatement inuseDeleteToSingSongMutationonMutatecallback to properly filter songs
invalidateQueries(['toSingSong'])fromonSettledinuseDeleteToSingSongMutationto prevent optimistic update overrideinvalidateQueries(['toSingSong'])fromonSettledinusePatchToSingSongMutationfor consistencyonSettledcallback