feat: migrate artist delete to dedicated api#1665
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 16 minutes and 39 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2657aa6cdd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
components/Artists/DropDown.tsx
Outdated
| await getArtists(); | ||
| } catch (error) { | ||
| setArtists(previousArtists); |
There was a problem hiding this comment.
If deleteArtist succeeds but getArtists() fails (e.g., transient API/network error), this catch path restores previousArtists, which re-adds an artist that was already deleted server-side. That leaves the UI in an incorrect state and can prompt duplicate delete attempts. Handle post-delete refresh failures separately so rollback only happens when the DELETE request itself fails.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b21109b. Rollback now only happens when the DELETE call fails; refresh failures keep the optimistic removal and surface a toast instead.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files
Confidence score: 3/5
- There is a concrete user-impact risk: in both
components/Artists/DropDown.tsxandcomponents/ArtistSetting/DeleteModal.tsx, agetArtists()refresh failure can trigger rollback logic and re-add an artist that was actually deleted. - The issue is moderate severity (5/10) with reasonable confidence, so this is likely not merge-blocking, but it can cause confusing stale/incorrect UI state after delete operations.
- This PR likely becomes safer once delete failure handling is separated from refresh failure handling, so successful deletes are not undone by fetch errors.
- Pay close attention to
components/Artists/DropDown.tsxandcomponents/ArtistSetting/DeleteModal.tsx- rollback is currently coupled to refresh errors, which can resurrect deleted artists.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="components/Artists/DropDown.tsx">
<violation number="1" location="components/Artists/DropDown.tsx:23">
P2: `getArtists()` errors will trigger the rollback, which can re-add an artist even after the delete succeeded. Only roll back when the delete fails and handle refresh failures separately to avoid showing stale data.</violation>
</file>
<file name="components/ArtistSetting/DeleteModal.tsx">
<violation number="1" location="components/ArtistSetting/DeleteModal.tsx:38">
P2: `getArtists()` shares the same catch as the delete call, so a refresh failure rolls back to `previousArtists` even if the delete already succeeded. Handle refresh errors separately so you don’t resurrect a deleted artist in the UI.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="hooks/useDeleteArtist.ts">
<violation number="1" location="hooks/useDeleteArtist.ts:33">
P2: If `onSuccess` throws, the catch block rolls back the optimistic removal even though the server-side delete already succeeded. Move `options?.onSuccess?.()` after the first try-catch to match the stated intent of only rolling back on delete API failure.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| options?.onSuccess?.(); | ||
| } catch (error) { | ||
| setArtists(previousArtists); | ||
| toast.error(error instanceof Error ? error.message : "Failed to delete artist"); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
P2: If onSuccess throws, the catch block rolls back the optimistic removal even though the server-side delete already succeeded. Move options?.onSuccess?.() after the first try-catch to match the stated intent of only rolling back on delete API failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At hooks/useDeleteArtist.ts, line 33:
<comment>If `onSuccess` throws, the catch block rolls back the optimistic removal even though the server-side delete already succeeded. Move `options?.onSuccess?.()` after the first try-catch to match the stated intent of only rolling back on delete API failure.</comment>
<file context>
@@ -0,0 +1,50 @@
+ }
+
+ await deleteArtist(accessToken, artistId);
+ options?.onSuccess?.();
+ } catch (error) {
+ setArtists(previousArtists);
</file context>
| options?.onSuccess?.(); | |
| } catch (error) { | |
| setArtists(previousArtists); | |
| toast.error(error instanceof Error ? error.message : "Failed to delete artist"); | |
| return; | |
| } | |
| } catch (error) { | |
| setArtists(previousArtists); | |
| toast.error(error instanceof Error ? error.message : "Failed to delete artist"); | |
| return; | |
| } | |
| options?.onSuccess?.(); |
2 participants
Summary
DELETE /api/artists/{id}endpointapp/api/artist/removerouteStack
Testing
pnpm exec eslint components/ArtistSetting/DeleteModal.tsx components/Artists/DropDown.tsx lib/artists/deleteArtist.tsSummary by cubic
Migrated artist deletion to the dedicated
DELETE /api/artists/{id}endpoint and centralized the flow in auseDeleteArtisthook with optimistic UI and clearer errors. Roll back only if the delete API fails; keep state if refresh fails and show a toast.app/api/artist/removeand addedlib/artists/deleteArtistusinggetClientApiBaseUrl()and Bearer auth.hooks/useDeleteArtistto handle optimistic updates,@privy-io/react-authtoken, refresh viagetArtists(), andsonnertoasts.DeleteModalandArtists/DropDownto use the hook and simplify delete UI.Written for commit 4f5e58a. Summary will update on new commits.