fix(web): allow deleting nodes from Workflow Builder (#971)#1113
Merged
Wirasm merged 4 commits intocoleam00:devfrom Apr 22, 2026
Merged
fix(web): allow deleting nodes from Workflow Builder (#971)#1113Wirasm merged 4 commits intocoleam00:devfrom
Wirasm merged 4 commits intocoleam00:devfrom
Conversation
Three independent gaps prevented users from deleting nodes added to the Workflow Builder canvas: dropped nodes were never auto-selected so keyboard shortcuts silently no-oped, no right-click context menu existed, and the Delete Node button was buried in the Advanced tab (hidden below the viewport for Prompt/Command, completely absent for Bash since bash nodes have no Advanced tab). Fixes coleam00#971.
📝 WalkthroughWalkthroughMoved deletion UI out of the Advanced tab into a persistent inspector header, added a canvas right-click context menu with Delete action, auto-select newly created nodes on drop/quick-add, and made Backspace and Delete both trigger node deletion. Changes
Sequence DiagramsequenceDiagram
participant User
participant Canvas as WorkflowCanvas
participant Builder as WorkflowBuilder
participant Inspector as NodeInspector
participant Keyboard as useBuilderKeyboard
rect rgba(200, 150, 255, 0.5)
Note over User,Canvas: Drag-and-Drop Node
User->>Canvas: Drag node onto canvas
Canvas->>Canvas: onDrop() creates node
Canvas->>Canvas: onNodeSelect(id) — auto-select new node
Canvas->>Inspector: open/refresh inspector for selected node
end
rect rgba(100, 200, 255, 0.5)
Note over User,Keyboard: Delete via Keyboard
User->>Keyboard: Press Delete or Backspace
Keyboard->>Keyboard: preventDefault()
Keyboard->>Builder: actions.onDeleteSelected()
Builder->>Builder: handleNodeDeleteById(selectedId)
Builder->>Canvas: remove node & edges, clear selection
end
rect rgba(150, 200, 150, 0.5)
Note over User,Canvas: Delete via Context Menu
User->>Canvas: Right-click node
Canvas->>Canvas: onNodeContextMenu() opens menu, auto-select node
User->>Canvas: Click "Delete node"
Canvas->>Builder: onNodeDelete(nodeId)
Builder->>Builder: handleNodeDeleteById(nodeId)
Builder->>Canvas: remove node & edges, clear selection
end
rect rgba(200, 200, 100, 0.5)
Note over User,Inspector: Delete via Header Button
User->>Inspector: Click "Delete" in DagInspector header
Inspector->>Builder: onDelete()
Builder->>Builder: handleNodeDelete() → handleNodeDeleteById()
Builder->>Canvas: remove node & edges, clear selection
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/components/workflows/WorkflowCanvas.tsx`:
- Around line 175-181: The canvas-created node flows call setNodes(...) and then
onNodeSelect/onDirty but never call onPushSnapshot, so undo/redo won't capture
this addition; update both places that append a node (the handler using setNodes
and the other occurrence around lines 292-299) to invoke onPushSnapshot?.()
immediately before mutating state (i.e., before calling setNodes) so the new
node addition is pushed as a snapshot; keep the existing onNodeSelect(id) and
onDirty() calls after the setNodes call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b781b628-ca2b-4875-80e9-cc3581c0876f
📒 Files selected for processing (4)
packages/web/src/components/workflows/NodeInspector.tsxpackages/web/src/components/workflows/WorkflowBuilder.tsxpackages/web/src/components/workflows/WorkflowCanvas.tsxpackages/web/src/hooks/useBuilderKeyboard.ts
Call onPushSnapshot() before setNodes() in both onDrop and quick-add handlers so that node additions are captured by undo/redo history. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
PR Review Summary — #1113 (
|
| ID | Agent | Issue | Location |
|---|---|---|---|
| C1 | code-reviewer | handleNodeDeleteById captures nodes/edges in its dep array, so pushSnapshot({nodes, edges}) can snapshot pre-drop state if the user presses Delete/Backspace in the same tick as auto-select after a drop. Undo stack can record a snapshot that does not include the just-dropped node. Fix: hold nodes/edges in refs and drop them from the dep array. |
WorkflowBuilder.tsx:236-245 |
| C2 | code-reviewer | Context menu uses position: fixed with raw e.clientX/e.clientY; no clamping. A right-click near the viewport edge renders the menu partially off-screen. Fix: clamp with Math.min(x, innerWidth - MENU_W) / Math.min(y, innerHeight - MENU_H). |
WorkflowCanvas.tsx:389-409 |
Important Issues
| ID | Agent | Issue | Location |
|---|---|---|---|
| I1 | code-reviewer | role="menu" + role="menuitem" without autoFocus, tabIndex, or arrow-key handling violates the ARIA menu contract. For a single-item menu the cleanest fix is to drop the roles entirely and rely on the already-accessible <button>. |
WorkflowCanvas.tsx:390-408 |
| I2 | code-reviewer | onPushSnapshot={() => pushSnapshot({ nodes, edges })} is an inline closure (not memoized). Causes onDrop to reconstruct every render and introduces a narrow stale-closure window. Fix: useCallback or the ref pattern from C1. |
WorkflowBuilder.tsx:491-493 |
| I3 | code-reviewer | isInputTarget() covers INPUT/TEXTAREA/SELECT/contentEditable but not role="combobox" / role="textbox" / role="searchbox". A future shadcn/Radix widget with focus could cause Backspace to delete a node while the user is editing. Extend the guard to check ARIA roles. |
useBuilderKeyboard.ts:104-109 |
| I4 | comment-analyzer | CLAUDE.md explicitly says not to reference issue numbers in comments ("they rot"). Seven 971-references were added across the four files. | NodeInspector.tsx:701, WorkflowBuilder.tsx:235, WorkflowCanvas.tsx:178/295/308/388, useBuilderKeyboard.ts:107 |
| I5 | comment-analyzer | The handleNodeDeleteById block comment describes WHAT + lists callers; CLAUDE.md says to avoid WHAT comments. Callers are IDE-findable. |
WorkflowBuilder.tsx:233 |
| I6 | pr-test-analyzer | useBuilderKeyboard.ts is a pure event handler already testable with bun test — no RTL needed. The Backspace guard has high regression potential (silent data loss if a future edit breaks the isInputTarget check). Rating: 8/10. One focused test is ~30 min of work. |
useBuilderKeyboard.ts |
Suggestions
| ID | Agent | Suggestion | Location |
|---|---|---|---|
| S1 | code-simplifier | Collapse handleNodeDeleteById + handleNodeDelete into one callback with an id param. Callers all have access to selectedNodeId. Removes one useCallback, one dep array, and the block comment explaining the pair. |
WorkflowBuilder.tsx:232-246 |
| S2 | code-simplifier | closeContextMenu = useCallback(() => setContextMenu(null), []) wraps a stable setter — no value, and it bloats the useEffect dep array. Call setContextMenu(null) directly. |
WorkflowCanvas.tsx (new code) |
| S3 | code-simplifier | Rename onPointer → onClickOutside. "Pointer" in web API terms means PointerEvent; the handler takes MouseEvent. |
WorkflowCanvas.tsx |
| S4 | docs-impact | packages/docs-web/src/content/docs/adapters/web.md:170-177 — the Workflow Builder section doesn't mention the new delete affordances. The "keyboard shortcuts" bullet (line 174) currently implies only undo/redo. Add a "Delete node" bullet covering Delete/Backspace, the header button, and the right-click menu. |
docs-web |
Strengths
{ capture: true }on the dismissal listeners correctly beats ReactFlow's own bubble-phase handlers and anystopPropagation()calls (verified by comment-analyzer).isInputTarget()guard for the existingDeletecase already covered the common cases; extending to Backspace is the right call.- The
onPushSnapshot?.()calls added inonDropandhandleQuickAddNodeare a real correctness win — undo before add was missing before this PR. - Moving Delete to the inspector header (visible for
bashwhich has no Advanced tab) is the correct UX resolution for the original bug. - No type leaks, no
any, no new dependencies, no backend changes — scope matches the PR description exactly.
Recommended Next Steps
- Fix C1 (stale-closure snapshot) and C2 (viewport clamping) before merge.
- Remove the 7 issue-number references in comments per CLAUDE.md (I4).
- Consider dropping
role="menu"/role="menuitem"(I1) — the simplest conformant path. - Add a short
useBuilderKeyboardtest for the Delete/Backspace +isInputTargetinvariant (I6). - One-line docs addition in
web.md(S4). - I2, I3, I5, and the simplifications (S1–S3) are nice-to-have and can land in a follow-up.
- Hold nodes/edges in refs so handleNodeDeleteById and onPushSnapshot can't capture stale pre-drop state (fixes undo-stack correctness). - Clamp context-menu x/y to viewport so right-click near edges stays fully on-screen. - Drop non-conformant role=menu/menuitem from the single-item context menu; rely on the native button for accessibility. - Extend isInputTarget() to cover ARIA combobox/textbox/searchbox so Backspace in Radix/shadcn widgets never nukes a node. - Extract handleBuilderKeydown as a pure function and add tests covering the Delete/Backspace + isInputTarget invariant. - Remove issue-number references from code comments per CLAUDE.md. - Document the new delete affordances in the Workflow Builder docs. - Inline context-menu dismissal, rename pointer handler, drop unused deps in keyboardActions useMemo. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (5)
packages/web/src/components/workflows/WorkflowBuilder.tsx (2)
400-404: Redundant double-guard onselectedNodeId.
onDeleteSelectedguards onselectedNodeIdand then callshandleNodeDelete, which performs the same guard. You can simplify by callinghandleNodeDeletedirectly (it already no-ops when nothing is selected) — this was also flagged as suggestion S1 in the review summary.Proposed simplification
- onDeleteSelected: (): void => { - if (selectedNodeId) { - handleNodeDelete(); - } - }, + onDeleteSelected: handleNodeDelete,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/workflows/WorkflowBuilder.tsx` around lines 400 - 404, The onDeleteSelected handler redundantly checks selectedNodeId before calling handleNodeDelete which already no-ops when nothing is selected; remove the outer guard so onDeleteSelected simply calls handleNodeDelete directly (update the onDeleteSelected function to call handleNodeDelete unconditionally and remove references to selectedNodeId inside that handler).
175-186: Ref-sync pattern: consider syncing during render for concurrent-safety.Syncing refs in a
useEffectis generally fine here becausepushSnapshotLatestis invoked from user-driven event handlers that run after commit. However, under React 19 concurrent rendering, a render can be started and discarded before the effect runs, which means any callback that readsnodesRef.currentbetween a committed state update and the effect could momentarily see a stale value.For strictly latest-state reads in callbacks, a common pattern is to assign during render:
Alternative ref-sync pattern
- const nodesRef = useRef(nodes); - const edgesRef = useRef(edges); - useEffect(() => { - nodesRef.current = nodes; - edgesRef.current = edges; - }, [nodes, edges]); + const nodesRef = useRef(nodes); + const edgesRef = useRef(edges); + // Sync during render so event handlers fired between render and effect + // still observe the latest committed state. + nodesRef.current = nodes; + edgesRef.current = edges;Functionally equivalent for this PR's use case — raising as an optional refinement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/workflows/WorkflowBuilder.tsx` around lines 175 - 186, The current pattern updates nodesRef/edgesRef inside a useEffect which can be stale under concurrent rendering; instead assign nodesRef.current = nodes and edgesRef.current = edges during render (before defining pushSnapshotLatest) so pushSnapshotLatest (which uses nodesRef/edgesRef) always reads the freshest values; locate the refs named nodesRef and edgesRef and move the sync into render (or at least perform the assignment immediately before the useCallback for pushSnapshotLatest), and remove or keep the effect only if you still need it for legacy timing.packages/web/src/hooks/useBuilderKeyboard.test.ts (1)
86-136: LGTM — focused tests addressing review feedback I6.The delete invariant coverage is comprehensive: Delete/Backspace on canvas fires
onDeleteSelected; INPUT/TEXTAREA/contentEditable/ARIA combobox/textbox all suppress;enabled=falsesuppresses shortcuts. Tests are deterministic and don't rely onmock.module.Optional: consider also asserting
e.preventDefaultwas called on the Delete/Backspace canvas cases to pin down that invariant, since downstream ReactFlow behavior depends on it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/hooks/useBuilderKeyboard.test.ts` around lines 86 - 136, Add assertions that the synthetic event's preventDefault was called in the canvas Delete/Backspace tests to ensure the handler suppresses native behavior: in the tests that call handleBuilderKeydown(makeEvent('Delete'..., actions) and makeEvent('Backspace'..., actions) where tagName is 'DIV', assert that the event object's preventDefault was invoked (alongside the existing expect on actions.calls.onDeleteSelected) by checking the mock event created by makeEvent; reference the handleBuilderKeydown and makeEvent helpers and the actions.calls.onDeleteSelected expectation when adding these preventsDefault assertions.packages/web/src/components/workflows/WorkflowCanvas.tsx (2)
304-317: Viewport clamping addresses C2; consider also clamping the lower bound.The right/bottom clamping with approximate menu dimensions correctly prevents overflow in the common case. One optional hardening: if the viewport is narrower than
CONTEXT_MENU_WIDTH(rare, but possible on very small windows/iframes),innerWidth - CONTEXT_MENU_WIDTHis negative and the menu is pushed off-screen to the left. Clamping the lower bound to 0 avoids that:Proposed refinement
- const x = Math.min(e.clientX, window.innerWidth - CONTEXT_MENU_WIDTH); - const y = Math.min(e.clientY, window.innerHeight - CONTEXT_MENU_HEIGHT); + const x = Math.max(0, Math.min(e.clientX, window.innerWidth - CONTEXT_MENU_WIDTH)); + const y = Math.max(0, Math.min(e.clientY, window.innerHeight - CONTEXT_MENU_HEIGHT));Also,
CONTEXT_MENU_WIDTH/CONTEXT_MENU_HEIGHTare static values — hoisting them to module scope (alongside the existingresolveNodeLabel) avoids re-binding on each render.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/workflows/WorkflowCanvas.tsx` around lines 304 - 317, The context menu position logic in handleNodeContextMenu can produce negative x/y when the viewport is smaller than CONTEXT_MENU_WIDTH/HEIGHT; change the clamping to Math.max(0, Math.min(...)) so x = Math.max(0, Math.min(e.clientX, window.innerWidth - CONTEXT_MENU_WIDTH)) and similarly for y before calling setContextMenu({ x, y, nodeId: node.id }). Also hoist CONTEXT_MENU_WIDTH and CONTEXT_MENU_HEIGHT to module scope (next to resolveNodeLabel) so they are not re-bound on each render.
385-402: Droppingrole="menu"/role="menuitem"resolves I1.For a single-action popover, relying on native
<button>semantics is more correct than asserting menu roles without full arrow-key navigation and focus management. If/when this grows to multiple items, either implement the full ARIA menu pattern (roving tabindex, arrow keys,aria-activedescendant) or reach for a primitive like@radix-ui/react-dropdown-menu.One small accessibility gap remains: the menu doesn't auto-focus when opened, so keyboard-only users who right-click via
Shift+F10can't tab directly to Delete without leaving the canvas. Non-blocking for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/workflows/WorkflowCanvas.tsx` around lines 385 - 402, The context menu currently relies on a native button (good) but it doesn't auto-focus when opened; add a ref (e.g., deleteButtonRef) to the Delete button and, inside a useEffect that watches contextMenu, call deleteButtonRef.current?.focus() when contextMenu is non-null so keyboard users can immediately tab/activate the action; keep the existing contextMenuRef and setContextMenu/onNodeDelete logic unchanged and do not introduce ARIA menu/menuitem roles unless you implement full menu keyboard handling later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/web/src/components/workflows/WorkflowBuilder.tsx`:
- Around line 400-404: The onDeleteSelected handler redundantly checks
selectedNodeId before calling handleNodeDelete which already no-ops when nothing
is selected; remove the outer guard so onDeleteSelected simply calls
handleNodeDelete directly (update the onDeleteSelected function to call
handleNodeDelete unconditionally and remove references to selectedNodeId inside
that handler).
- Around line 175-186: The current pattern updates nodesRef/edgesRef inside a
useEffect which can be stale under concurrent rendering; instead assign
nodesRef.current = nodes and edgesRef.current = edges during render (before
defining pushSnapshotLatest) so pushSnapshotLatest (which uses
nodesRef/edgesRef) always reads the freshest values; locate the refs named
nodesRef and edgesRef and move the sync into render (or at least perform the
assignment immediately before the useCallback for pushSnapshotLatest), and
remove or keep the effect only if you still need it for legacy timing.
In `@packages/web/src/components/workflows/WorkflowCanvas.tsx`:
- Around line 304-317: The context menu position logic in handleNodeContextMenu
can produce negative x/y when the viewport is smaller than
CONTEXT_MENU_WIDTH/HEIGHT; change the clamping to Math.max(0, Math.min(...)) so
x = Math.max(0, Math.min(e.clientX, window.innerWidth - CONTEXT_MENU_WIDTH)) and
similarly for y before calling setContextMenu({ x, y, nodeId: node.id }). Also
hoist CONTEXT_MENU_WIDTH and CONTEXT_MENU_HEIGHT to module scope (next to
resolveNodeLabel) so they are not re-bound on each render.
- Around line 385-402: The context menu currently relies on a native button
(good) but it doesn't auto-focus when opened; add a ref (e.g., deleteButtonRef)
to the Delete button and, inside a useEffect that watches contextMenu, call
deleteButtonRef.current?.focus() when contextMenu is non-null so keyboard users
can immediately tab/activate the action; keep the existing contextMenuRef and
setContextMenu/onNodeDelete logic unchanged and do not introduce ARIA
menu/menuitem roles unless you implement full menu keyboard handling later.
In `@packages/web/src/hooks/useBuilderKeyboard.test.ts`:
- Around line 86-136: Add assertions that the synthetic event's preventDefault
was called in the canvas Delete/Backspace tests to ensure the handler suppresses
native behavior: in the tests that call
handleBuilderKeydown(makeEvent('Delete'..., actions) and
makeEvent('Backspace'..., actions) where tagName is 'DIV', assert that the event
object's preventDefault was invoked (alongside the existing expect on
actions.calls.onDeleteSelected) by checking the mock event created by makeEvent;
reference the handleBuilderKeydown and makeEvent helpers and the
actions.calls.onDeleteSelected expectation when adding these preventsDefault
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: efb2655b-869a-4c6e-8bb6-ecf53072717c
📒 Files selected for processing (7)
packages/docs-web/src/content/docs/adapters/web.mdpackages/web/package.jsonpackages/web/src/components/workflows/NodeInspector.tsxpackages/web/src/components/workflows/WorkflowBuilder.tsxpackages/web/src/components/workflows/WorkflowCanvas.tsxpackages/web/src/hooks/useBuilderKeyboard.test.tspackages/web/src/hooks/useBuilderKeyboard.ts
✅ Files skipped from review due to trivial changes (2)
- packages/web/package.json
- packages/docs-web/src/content/docs/adapters/web.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/src/hooks/useBuilderKeyboard.ts
6 tasks
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
2 participants
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
@archon/web.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
onNodeDeleteproponNodeContextMenuhandleronDeleteprop unchangedonDeleteprop from AdvancedTabLabel Snapshot
risk: lowsize: Swebweb:WorkflowCanvas,web:NodeInspector,web:WorkflowBuilder,web:useBuilderKeyboardChange Metadata
bugwebLinked Issue
Validation Evidence (required)
bun run validateoutput and manual browser testingSecurity Impact (required)
NoNoNoNoCompatibility / Migration
YesNoNoHuman Verification (required)
What was personally validated beyond CI:
Side Effects / Blast Radius (required)
Rollback Plan (required)
git revert <commit-sha>— single atomic commitRisks and Mitigations
e.preventDefault()on Backspace could cause unexpected behavior if focus escapes a text inputisInputTarget()guard inuseBuilderKeyboardchecks forinput,textarea, and[contenteditable]before the keydown handler firesbg-surface-elevated,border-border,text-error) consistent with the rest of the UISummary by CodeRabbit
New Features
Bug Fixes / Improvements
Documentation