Sketch Lab: fix group delete bug and add backspace option for delete#73881
Sketch Lab: fix group delete bug and add backspace option for delete#73881molly-moen wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes Sketch Lab deletion semantics in the React Flow canvas so deleting/cutting a group reliably deletes its grouped children, and adds Backspace as an additional delete hotkey.
Changes:
- Add
expandGroupDeletionto expand group deletions to include children (and associated edges), wired via React Flow’sonBeforeDelete. - Update cut behavior to rely on the centralized pre-delete expansion rather than manually deleting group children.
- Allow both
DeleteandBackspacekeys to trigger deletion.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/src/sketchlab/reactFlow/utils/grouping.ts | Adds expandGroupDeletion to expand group deletions to include children and related edges. |
| apps/src/sketchlab/reactFlow/components/ReactFlowCanvas.tsx | Wires onBeforeDelete to expandGroupDeletion; adds Backspace as a delete key. |
| apps/src/sketchlab/reactFlow/hooks/useCopyPaste.ts | Simplifies cut for groups to delete only the group node and rely on onBeforeDelete. |
| apps/test/unit/sketchlab/reactFlow/utils/groupingTest.ts | Adds unit tests for expandGroupDeletion. |
bencodeorg
left a comment
bencodeorg
left a comment
There was a problem hiding this comment.
Thanks for doing this!
| onNodeClick: isGrabMode ? undefined : handleNodeClick, | ||
| onEdgeClick: isGrabMode ? undefined : handleEdgeClick, | ||
| deleteKeyCode: !readOnly && !isGrabMode ? 'Delete' : null, | ||
| deleteKeyCode: !readOnly && !isGrabMode ? ['Delete', 'Backspace'] : null, |
| for (const edge of allEdges) { | ||
| if ( | ||
| !finalEdgeIds.has(edge.id) && | ||
| (finalNodeIds.has(edge.source) || finalNodeIds.has(edge.target)) |
There was a problem hiding this comment.
This would delete an edge between a group and a non-grouped node, yeah? I feel like we'd keep those but 🤷♂️ . But ideally still delete an edge that is only attached on one side (and it's to a grouped node)
There was a problem hiding this comment.
hmm good question. It is consistent with our current deletion behavior, if you delete a node, any edges attached to it are deleted, even if they are attached to another node.
|
Tangentially, the docs for this really make it seem like this should just work? Are we doing something special? |
We must be given the bug we've seen, or their logic is flawed 🤷♀️ |
This PR fixes a bug when deleting a group. Previously, if you deleted a group it would move to the top right of the canvas and the elements would become immovable. This was because we weren't deleting the children of a group, only the group itself, and the orphaned nodes became buggy.
React Flow has the option to supply a onBeforeDelete handler, which modifies which nodes/edges are being deleted. I added that here, which also works for our 'cut' action, which was doing some custom logic for groups before.
While I was here, I also added the option to use
backspacefor delete as well as thedeletekey, since mac keyboards generally don't have adeletekey, and we ran into some confusion here. You can still backspace in a text element to delete text without deleting an element. Depending on which goes in first, I'll make sure to update the shortcuts menu added in #73869 to include the new option.Before
Screen.Recording.2026-07-15.at.3.08.07.PM.mov
After
Screen.Recording.2026-07-15.at.3.09.22.PM.mov
Links
Testing story
Tested locally.