fix(desktop): use Alerter for automation detail delete confirm#3695
Conversation
Closes SUPER-436. Replaces native window.confirm() on the automation detail page with the shared alert() helper so the confirm matches the rest of the app's UI.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe deletion confirmation on the automation detail page was changed from a synchronous browser Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR replaces the native Confidence Score: 5/5Safe to merge — a focused, correct swap of native confirm() for the shared Alerter dialog with no logic regressions. The only finding is a P2 UX suggestion about silent error feedback on delete failure, which is a pre-existing limitation of the shared Alerter component rather than a defect introduced by this PR. No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/automations/$automationId/page.tsx | Replaces native window.confirm() with the shared Alerter dialog; uses mutateAsync() so the dialog tracks loading state and remains open on failure. No logic or correctness issues introduced. |
Sequence Diagram
sequenceDiagram
participant User
participant AutomationDetailHeader
participant Alerter
participant deleteMutation
participant Router
User->>AutomationDetailHeader: Click Delete
AutomationDetailHeader->>Alerter: alert({ title, description, actions })
Alerter-->>User: Show shadcn Dialog
alt User clicks Cancel
User->>Alerter: onClick() no-op
Alerter-->>User: Close dialog
else User clicks Delete
User->>Alerter: onClick() mutateAsync()
Alerter->>deleteMutation: mutateAsync()
Note over Alerter: loadingIndex set, spinner shown
alt Delete succeeds
deleteMutation-->>Alerter: resolves
deleteMutation->>Router: onSuccess navigate /automations
Alerter-->>User: setIsOpen(false)
else Delete fails
deleteMutation-->>Alerter: throws error
Alerter->>Alerter: console.error no user feedback
Alerter-->>User: Dialog stays open, buttons re-enabled
end
end
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/automations/$automationId/page.tsx
Line: 92-94
Comment:
**Silent failure on delete error**
If `deleteMutation.mutateAsync()` rejects, `Alerter.handleAction` catches the error and logs it to the console, but the dialog simply re-enables its buttons with no user-visible feedback. The user won't know the deletion failed — they'll just see the spinner stop with the dialog still open. Consider passing an `onError` option to the mutation (or surfacing a toast/message) so failures don't disappear silently.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(desktop): use Alerter for automation..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| onClick: async () => { | ||
| await deleteMutation.mutateAsync(); | ||
| }, |
There was a problem hiding this comment.
If deleteMutation.mutateAsync() rejects, Alerter.handleAction catches the error and logs it to the console, but the dialog simply re-enables its buttons with no user-visible feedback. The user won't know the deletion failed — they'll just see the spinner stop with the dialog still open. Consider passing an onError option to the mutation (or surfacing a toast/message) so failures don't disappear silently.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/automations/$automationId/page.tsx
Line: 92-94
Comment:
**Silent failure on delete error**
If `deleteMutation.mutateAsync()` rejects, `Alerter.handleAction` catches the error and logs it to the console, but the dialog simply re-enables its buttons with no user-visible feedback. The user won't know the deletion failed — they'll just see the spinner stop with the dialog still open. Consider passing an `onError` option to the mutation (or surfacing a toast/message) so failures don't disappear silently.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/automations/$automationId/page.tsx (1)
83-98: LGTM — async flow is correct.The switch to
alert()correctly leverages the Alerter's async action handling:mutateAsync()is awaited so the dialog stays open (and the Delete button shows its loading state) until the mutation resolves, and navigation continues to occur viadeleteMutation.onSuccess. SinceAlerter.handleActionalready catches and logs rejectedonClickpromises (seepackages/ui/src/atoms/Alert/Alert.tsxlines 44-54), theawaithere won't produce an unhandled rejection on delete failure.One small cleanup:
AlertAction.onClickis optional (onClick?: () => void | Promise<void>), so the Cancel action'sonClick: () => {}can be dropped.♻️ Optional simplification
actions: [ - { label: "Cancel", variant: "outline", onClick: () => {} }, + { label: "Cancel", variant: "outline" }, { label: "Delete", variant: "destructive", onClick: async () => { await deleteMutation.mutateAsync(); }, }, ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/automations/`$automationId/page.tsx around lines 83 - 98, The Cancel action in the alert payload is providing an empty noop handler even though AlertAction.onClick is optional; remove the unnecessary onClick: () => {} from the Cancel action inside the onDelete handler so the actions array only includes the Cancel action object with label and variant and the Delete action that calls await deleteMutation.mutateAsync(); this simplifies the alert payload in the onDelete closure that calls alert(...) and avoids an unused noop function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/automations/`$automationId/page.tsx:
- Around line 83-98: The Cancel action in the alert payload is providing an
empty noop handler even though AlertAction.onClick is optional; remove the
unnecessary onClick: () => {} from the Cancel action inside the onDelete handler
so the actions array only includes the Cancel action object with label and
variant and the Delete action that calls await deleteMutation.mutateAsync();
this simplifies the alert payload in the onDelete closure that calls alert(...)
and avoids an unused noop function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a11c6b7-b523-4992-877a-27c77221808f
📒 Files selected for processing (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/automations/$automationId/page.tsx
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
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="apps/desktop/src/renderer/routes/_authenticated/_dashboard/automations/$automationId/page.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/automations/$automationId/page.tsx:93">
P2: Wrap the `deleteMutation.mutateAsync()` call in a try/catch (or supply an `onError` callback) and surface a toast or inline message on failure. Currently, if the delete request rejects, the dialog re-enables its buttons with no user-visible feedback, silently swallowing the error.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Addresses review feedback on #3695: the Alerter would silently swallow a failed delete. Wrap mutateAsync in toast.promise so the user sees loading, success, and error states.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Kitenite
deleted the
satya/super-436-schedule-automation-delete-button-uses-native-menu-instead
branch
1 participant
Summary
window.confirm()on the automation detail page with the sharedalert()helper from@superset/ui/atoms/Alert, matching the rest of the app's delete-confirm UX.Closes SUPER-436.
Test plan
/automationsSummary by cubic
Replaces native
window.confirm()on the automation detail page with the sharedalert()from@superset/ui/atoms/Alertso delete confirmations use the app’s shadcn dialog on desktop. WrapsdeleteMutation.mutateAsync()intoast.promisefrom@superset/ui/sonnerto show loading, success, and error states; closes SUPER-436.Written for commit f7f6388. Summary will update on new commits.
Summary by CodeRabbit