🎨 Palette: Enhance destructive actions with AlertDialog UX#114
Conversation
… component Co-authored-by: Krosebrook <214532761+Krosebrook@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience for destructive actions across the application by replacing the jarring, unstylable native browser confirmation prompts with a custom, themed Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughThis PR replaces browser-based confirm prompts with AlertDialog-based confirmation flows across three pages. The actual deletion/removal logic remains unchanged; only the confirmation UI mechanism transitions from inline prompts to custom modal dialogs. Changes
Possibly related PRs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Replaces native blocking confirm() prompts with the shadcn/Radix AlertDialog for destructive actions in key pages, aligning confirmations with the app’s theme and modal UX patterns.
Changes:
- Swapped song deletion confirmation in
SongDetailsto anAlertDialog. - Swapped playlist deletion confirmation in
Playliststo anAlertDialogper playlist card. - Swapped “remove song from playlist” confirmation in
PlaylistDetailsto anAlertDialogper row.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| client/src/pages/SongDetails.tsx | Replaces delete-song confirm() with themed AlertDialog confirmation flow. |
| client/src/pages/Playlists.tsx | Replaces delete-playlist confirm() with per-card AlertDialog confirmation flow. |
| client/src/pages/PlaylistDetails.tsx | Replaces remove-song confirm() with AlertDialog confirmation flow in song rows. |
| className="opacity-0 group-hover:opacity-100 transition-opacity" | ||
| data-testid={`button-remove-${song.id}`} | ||
| aria-label={`Remove ${song.title} from playlist`} | ||
| title={`Remove ${song.title} from playlist`} | ||
| > |
There was a problem hiding this comment.
The remove-song icon button is opacity-0 by default and only becomes visible on hover. Without also revealing it on keyboard focus (e.g., focus:/focus-visible:), keyboard users can tab to an effectively invisible control. Please make the button visible when focused (similar to the focus:opacity-100 pattern used elsewhere, e.g. SongCard).
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively replaces the native window.confirm() with a much-improved AlertDialog from Shadcn/Radix UI, leading to a better and more consistent user experience for destructive actions. The implementation is solid across all modified files, and the accessibility improvements are a great touch. I have one minor suggestion in Playlists.tsx to remove a redundant piece of code for better clarity.
|
|
||
| const handleDelete = useCallback((e: React.MouseEvent, id: number) => { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); |
There was a problem hiding this comment.
This e.stopPropagation() call is redundant and can be removed. The AlertDialogContent is rendered in a portal, so events within it won't bubble up to the card's Link component. Furthermore, you've added an onClick handler with e.stopPropagation() to AlertDialogContent itself, which already prevents event propagation from its children.
![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
🧹 Nitpick comments (2)
client/src/pages/Playlists.tsx (1)
89-91: Guard delete mutation against rapid re-entry.Line 90 can fire multiple times quickly; add a pending guard in
handleDeleteto avoid duplicate delete requests.Suggested tweak
- const handleDelete = useCallback((e: React.MouseEvent, id: number) => { + const handleDelete = useCallback((e: React.MouseEvent, id: number) => { e.stopPropagation(); + if (isDeleting) return; deletePlaylist(id); - }, [deletePlaylist]); + }, [deletePlaylist, isDeleting]);Also applies to: 143-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/pages/Playlists.tsx` around lines 89 - 91, The delete action can be triggered multiple times rapidly from the AlertDialogAction click handler; modify the deletion handler (handleDelete / onDelete) in Playlists.tsx to add a "pending" guard boolean or state flag (e.g., isDeleting) that returns early if already true, set the flag before calling the delete mutation and clear it on success/error, and ensure the AlertDialogAction onClick uses that guarded handler so duplicate delete requests cannot be sent (also apply the same guard to the other delete invocation around lines ~143-146).client/src/pages/SongDetails.tsx (1)
55-59: Add failure feedback and a pending guard inhandleDelete.If deletion fails, this flow currently has no page-level feedback. Also guard re-entry while the mutation is pending.
Suggested tweak
const handleDelete = () => { + if (isDeleting) return; deleteSong(id, { onSuccess: () => { setLocation("/dashboard"); - } + }, + onError: () => { + toast({ + title: "Delete failed", + description: "Could not delete the song. Please try again.", + variant: "destructive", + }); + }, }); };As per coding guidelines, "Use toast notifications for user feedback in React components".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/pages/SongDetails.tsx` around lines 55 - 59, In handleDelete, prevent re-entry while the deleteSong mutation is pending and add toast feedback on failure (and success if desired): check the mutation status (e.g., deleteSong.isLoading or a local isDeleting flag) at the start of handleDelete and return early if pending; call deleteSong(id, { onSuccess: () => { setLocation("/dashboard"); toast.success("Song deleted"); }, onError: (err) => { toast.error("Failed to delete song: " + (err?.message || "unknown error")); } }) so the UI shows toast notifications and navigation only on success and provides error feedback on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/pages/PlaylistDetails.tsx`:
- Around line 57-63: The remove-button trigger (AlertDialogTrigger -> Button
with data-testid `button-remove-${song.id}`) is hidden via "opacity-0
group-hover:opacity-100" which makes it inaccessible to keyboard and touch
users; update the Button's className to also reveal on keyboard focus and on
small screens—for example add focus-visible:opacity-100 (or focus:opacity-100)
and a responsive prefix like sm:group-hover:opacity-100 so it remains visible to
touch/keyboard users while preserving hover behavior on larger screens; ensure
you update the same className string where AlertDialogTrigger and Button are
defined.
---
Nitpick comments:
In `@client/src/pages/Playlists.tsx`:
- Around line 89-91: The delete action can be triggered multiple times rapidly
from the AlertDialogAction click handler; modify the deletion handler
(handleDelete / onDelete) in Playlists.tsx to add a "pending" guard boolean or
state flag (e.g., isDeleting) that returns early if already true, set the flag
before calling the delete mutation and clear it on success/error, and ensure the
AlertDialogAction onClick uses that guarded handler so duplicate delete requests
cannot be sent (also apply the same guard to the other delete invocation around
lines ~143-146).
In `@client/src/pages/SongDetails.tsx`:
- Around line 55-59: In handleDelete, prevent re-entry while the deleteSong
mutation is pending and add toast feedback on failure (and success if desired):
check the mutation status (e.g., deleteSong.isLoading or a local isDeleting
flag) at the start of handleDelete and return early if pending; call
deleteSong(id, { onSuccess: () => { setLocation("/dashboard");
toast.success("Song deleted"); }, onError: (err) => { toast.error("Failed to
delete song: " + (err?.message || "unknown error")); } }) so the UI shows toast
notifications and navigation only on success and provides error feedback on
failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4de2f405-5397-46d0-8424-a838fc091935
📒 Files selected for processing (3)
client/src/pages/PlaylistDetails.tsxclient/src/pages/Playlists.tsxclient/src/pages/SongDetails.tsx
| <AlertDialog> | ||
| <AlertDialogTrigger asChild> | ||
| <Button | ||
| size="icon" | ||
| variant="ghost" | ||
| className="opacity-0 group-hover:opacity-100 transition-opacity" | ||
| data-testid={`button-remove-${song.id}`} |
There was a problem hiding this comment.
Make the remove trigger visible on focus/mobile, not hover-only.
Line 62 currently hides the destructive control unless hovered, which hurts keyboard and touch discoverability.
Suggested tweak
- className="opacity-0 group-hover:opacity-100 transition-opacity"
+ className="opacity-100 md:opacity-0 md:group-hover:opacity-100 focus-visible:opacity-100 transition-opacity"As per coding guidelines, "Use responsive Tailwind prefixes (sm:, md:, lg:) in React components".
📝 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.
| <AlertDialog> | |
| <AlertDialogTrigger asChild> | |
| <Button | |
| size="icon" | |
| variant="ghost" | |
| className="opacity-0 group-hover:opacity-100 transition-opacity" | |
| data-testid={`button-remove-${song.id}`} | |
| <AlertDialog> | |
| <AlertDialogTrigger asChild> | |
| <Button | |
| size="icon" | |
| variant="ghost" | |
| className="opacity-100 md:opacity-0 md:group-hover:opacity-100 focus-visible:opacity-100 transition-opacity" | |
| data-testid={`button-remove-${song.id}`} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/src/pages/PlaylistDetails.tsx` around lines 57 - 63, The remove-button
trigger (AlertDialogTrigger -> Button with data-testid
`button-remove-${song.id}`) is hidden via "opacity-0 group-hover:opacity-100"
which makes it inaccessible to keyboard and touch users; update the Button's
className to also reveal on keyboard focus and on small screens—for example add
focus-visible:opacity-100 (or focus:opacity-100) and a responsive prefix like
sm:group-hover:opacity-100 so it remains visible to touch/keyboard users while
preserving hover behavior on larger screens; ensure you update the same
className string where AlertDialogTrigger and Button are defined.
Krosebrook
deleted the
palette-replace-native-confirm-with-alert-dialog-8220985259139824591
branch
💡 What: Replaced native
window.confirm()calls with theAlertDialogcomponent from Shadcn/Radix UI across the application.🎯 Why: Native confirm dialogs are jarring, block the main thread, cannot be styled to match the application theme, and offer a poor user experience. The
AlertDialogprovides a seamless, thematic, and non-blocking way to confirm destructive actions.📸 Before/After: Visual update from standard browser alert prompts to fully themed modals with clear titles, descriptions, and accessible actions.
♿ Accessibility: Improved keyboard navigation, trap focus support within the modal dialog, and added an
aria-labelto the destructive button inSongDetails.tsx.PR created automatically by Jules for task 8220985259139824591 started by @Krosebrook
Summary by cubic
Replaced native confirm dialogs with Shadcn/Radix AlertDialog for destructive actions in playlists and songs. This adds a themed, non-blocking confirmation experience and improves accessibility.
Written for commit 0918485. Summary will update on new commits.
Summary by CodeRabbit