feat(canvas): right-click a canvas in the nav to delete it#2790
Merged
Conversation
Add a context menu to canvas rows in the channel navigation tree with a Delete action. Wraps DashboardRow in the same @posthog/quill ContextMenu primitives TaskRow already uses; left-click still navigates to the canvas. Delete calls the existing useDashboardMutations().deleteDashboard, which invalidates the list cache so the row disappears, disables the item while the delete is in flight, and toasts on failure. If you delete the canvas you're currently viewing, it falls back to the channel index. Generated-By: PostHog Code Task-Id: d25cc24c-41de-4106-85f3-13c90eaa3309
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/ui/src/features/canvas/components/ChannelsList.tsx:269-283
**No confirmation before permanent canvas deletion**
`deleteDashboard` fires immediately when the menu item is clicked — there is no "Are you sure?" step. Unlike removing a task from a channel (which only unfiles it), this permanently destroys the canvas and its spec. A mis-click on a long-form canvas is not recoverable. `ChannelMenu.onDelete` has the same gap, but there the channel-level deletion is at least visually more prominent; the inline list item here is easier to trigger by accident.
### Issue 2 of 2
packages/ui/src/features/canvas/components/ChannelsList.tsx:272
**Exact `pathname` match may miss sub-routes**
The redirect guard uses strict equality (`=== \`…/dashboards/${dashboard.id}\``). If any child routes exist under a dashboard (e.g. a settings or edit sub-path), deleting while on one of those routes would not trigger navigation away, leaving the user on a now-dead route. `ChannelMenu.onDelete` uses `pathname.startsWith(…)` for exactly this reason. Consider aligning to the same approach.
```suggestion
if (pathname.startsWith(`/website/${channelId}/dashboards/${dashboard.id}`)) {
```
Reviews (1): Last reviewed commit: "feat(canvas): right-click a canvas in th..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+269
to
+283
| const onDelete = async () => { | ||
| try { | ||
| await deleteDashboard(dashboard.id); | ||
| if (pathname === `/website/${channelId}/dashboards/${dashboard.id}`) { | ||
| void navigate({ | ||
| to: "/website/$channelId", | ||
| params: { channelId }, | ||
| }); | ||
| } | ||
| /> | ||
| } catch (error) { | ||
| toast.error("Couldn't delete canvas", { | ||
| description: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
deleteDashboard fires immediately when the menu item is clicked — there is no "Are you sure?" step. Unlike removing a task from a channel (which only unfiles it), this permanently destroys the canvas and its spec. A mis-click on a long-form canvas is not recoverable. ChannelMenu.onDelete has the same gap, but there the channel-level deletion is at least visually more prominent; the inline list item here is easier to trigger by accident.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/features/canvas/components/ChannelsList.tsx
Line: 269-283
Comment:
**No confirmation before permanent canvas deletion**
`deleteDashboard` fires immediately when the menu item is clicked — there is no "Are you sure?" step. Unlike removing a task from a channel (which only unfiles it), this permanently destroys the canvas and its spec. A mis-click on a long-form canvas is not recoverable. `ChannelMenu.onDelete` has the same gap, but there the channel-level deletion is at least visually more prominent; the inline list item here is easier to trigger by accident.
How can I resolve this? If you propose a fix, please make it concise.| const onDelete = async () => { | ||
| try { | ||
| await deleteDashboard(dashboard.id); | ||
| if (pathname === `/website/${channelId}/dashboards/${dashboard.id}`) { |
Contributor
There was a problem hiding this comment.
pathname match may miss sub-routes
The redirect guard uses strict equality (=== \…/dashboards/${dashboard.id}`). If any child routes exist under a dashboard (e.g. a settings or edit sub-path), deleting while on one of those routes would not trigger navigation away, leaving the user on a now-dead route. ChannelMenu.onDeleteusespathname.startsWith(…)` for exactly this reason. Consider aligning to the same approach.
Suggested change
| if (pathname === `/website/${channelId}/dashboards/${dashboard.id}`) { | |
| if (pathname.startsWith(`/website/${channelId}/dashboards/${dashboard.id}`)) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/features/canvas/components/ChannelsList.tsx
Line: 272
Comment:
**Exact `pathname` match may miss sub-routes**
The redirect guard uses strict equality (`=== \`…/dashboards/${dashboard.id}\``). If any child routes exist under a dashboard (e.g. a settings or edit sub-path), deleting while on one of those routes would not trigger navigation away, leaving the user on a now-dead route. `ChannelMenu.onDelete` uses `pathname.startsWith(…)` for exactly this reason. Consider aligning to the same approach.
```suggestion
if (pathname.startsWith(`/website/${channelId}/dashboards/${dashboard.id}`)) {
```
How can I resolve this? If you propose a fix, please make it concise.
Address review feedback on the right-click delete menu: - Add an AlertDialog confirmation before the permanent delete instead of firing immediately on click, following the ReplaceSkillDialog pattern. - Use pathname.startsWith for the post-delete redirect so deleting while on a child route of the canvas still navigates away (mirrors ChannelMenu.onDelete). Generated-By: PostHog Code Task-Id: d25cc24c-41de-4106-85f3-13c90eaa3309
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.
Summary
Right-clicking a canvas name in the channel navigation sidebar now opens a context menu with a Delete option that removes the canvas.
Changes
packages/ui/src/features/canvas/components/ChannelsList.tsx— theDashboardRowcomponent now:ContextMenu(the same@posthog/quillprimitivesTaskRowalready uses). Left-click still navigates to the canvas.useDashboardMutations().deleteDashboard(id)— firesdashboards.deleteover tRPC →DashboardsService.delete, then invalidates the list cache so the row disappears. Disabled while a delete is in flight; toasts on failure.TaskRow.onRemove/ChannelMenu.onDeletepattern).Testing
pnpm --filter @posthog/ui typecheckpasses clean.biome lintandbiome formatclean on the file.🤖 Generated with Claude Code