feat(workflow): improve undo/redo plus group duplicate/delete behavior#84
Open
Wildandpeaceful wants to merge 2 commits intoshrimbly:masterfrom
Open
feat(workflow): improve undo/redo plus group duplicate/delete behavior#84Wildandpeaceful wants to merge 2 commits intoshrimbly:masterfrom
Wildandpeaceful wants to merge 2 commits intoshrimbly:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds a history snapshot system with undo/redo, group-aware deletion, group/node duplication with non-overlapping placement, and selected-group tracking; integrates these into canvas keyboard handlers and store APIs. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Canvas as WorkflowCanvas
participant Store as WorkflowStore
participant History as History Stack
User->>Canvas: Press Delete / trigger delete
Canvas->>Store: handleBeforeDelete()
Store->>History: _pushHistorySnapshot()
History->>History: capture structural snapshot
Store->>Store: deleteSelection(nodeIds, groupIds)
Store->>Canvas: emit state change
Canvas->>Canvas: render updated workflow
User->>Canvas: Press Undo (Cmd/Ctrl+Z)
Canvas->>Store: undo()
Store->>History: pop undoHistory
History->>Store: restore snapshot
Store->>Canvas: emit state change
Canvas->>Canvas: render restored workflow
sequenceDiagram
actor User
participant Canvas as WorkflowCanvas
participant Store as WorkflowStore
participant Placement as Placement Logic
participant History as History Stack
User->>Canvas: Press Duplicate (Cmd/Ctrl+D)
Canvas->>Store: duplicateSelectedNodes()
Store->>History: _pushHistorySnapshot()
History->>History: capture snapshot
Store->>Placement: compute non-overlapping offset
Placement->>Store: return offset
Store->>Store: clone nodes/groups, remap IDs/edges/groups
Store->>Canvas: emit state change
Canvas->>Canvas: render duplicated elements
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/store/workflowStore.ts (1)
1344-1388:⚠️ Potential issue | 🔴 CriticalGroup drag/resize floods undo history — undo becomes unusable after dragging a group
updateGroup(line 1346) andmoveGroupNodes(line 1373) each call_pushHistorySnapshot()unconditionally.GroupsOverlay.tsx's mousemove handler calls both on every pixel-threshold event during drag and resize. At 60 fps, the 60-entry ring buffer fills in roughly 30 events (~0.5 s of movement), wiping all pre-drag undo history. After releasing the mouse the user can only undo through the last 30 intermediate drag positions, not back to the state before the drag started.
onNodesChangecorrectly throttles this by gating its history push ondragging === false(line 895–898). Group drag needs the same treatment.The recommended pattern: capture one snapshot at drag/resize start (in
handleHeaderMouseDown/handleResizeMouseDown), and skip_pushHistorySnapshotinsideupdateGroup/moveGroupNodeswhen called from drag continuations.🔧 Suggested fix – add `skipHistory` parameter
// WorkflowStore interface (lines ~371-373) - updateGroup: (groupId: string, updates: Partial<NodeGroup>) => void; + updateGroup: (groupId: string, updates: Partial<NodeGroup>, skipHistory?: boolean) => void; - moveGroupNodes: (groupId: string, delta: { x: number; y: number }) => void; + moveGroupNodes: (groupId: string, delta: { x: number; y: number }, skipHistory?: boolean) => void; // updateGroup implementation - updateGroup: (groupId: string, updates: Partial<NodeGroup>) => { + updateGroup: (groupId: string, updates: Partial<NodeGroup>, skipHistory = false) => { if (!get().groups[groupId]) return; - get()._pushHistorySnapshot(); + if (!skipHistory) get()._pushHistorySnapshot(); set((state) => ({ groups: { ...state.groups, [groupId]: { ...state.groups[groupId], ...updates } }, hasUnsavedChanges: true, })); }, // moveGroupNodes implementation - moveGroupNodes: (groupId: string, delta: { x: number; y: number }) => { + moveGroupNodes: (groupId: string, delta: { x: number; y: number }, skipHistory = false) => { if (delta.x === 0 && delta.y === 0) return; - get()._pushHistorySnapshot(); + if (!skipHistory) get()._pushHistorySnapshot(); // ... },In
GroupsOverlay.tsx, capture history once at drag/resize start and passskipHistory: trueduring movement:// handleHeaderMouseDown + _pushHistorySnapshot(); // capture state before drag begins setIsDragging(true); // handleResizeMouseDown + _pushHistorySnapshot(); // capture state before resize begins setIsResizing(true); // drag mousemove - updateGroup(groupId, { position: { ... } }); + updateGroup(groupId, { position: { ... } }, true); - moveGroupNodes(groupId, { x: deltaX, y: deltaY }); + moveGroupNodes(groupId, { x: deltaX, y: deltaY }, true); // resize mousemove - updateGroup(groupId, { size: ..., position: ... }); + updateGroup(groupId, { size: ..., position: ... }, true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/workflowStore.ts` around lines 1344 - 1388, updateGroup and moveGroupNodes call get()._pushHistorySnapshot() on every invocation, which floods undo history during group drag/resize; change both signatures to accept an optional skipHistory boolean (e.g., updateGroup(groupId: string, updates: Partial<NodeGroup>, skipHistory = false) and moveGroupNodes(groupId: string, delta: {x:number;y:number}, skipHistory = false)), only call get()._pushHistorySnapshot() when skipHistory is false, and update GroupsOverlay.tsx to call get()._pushHistorySnapshot() once at drag/resize start (handleHeaderMouseDown / handleResizeMouseDown) and pass skipHistory = true for the subsequent movement calls so intermediate moves don’t create history snapshots.
🧹 Nitpick comments (3)
src/store/__tests__/workflowStore.undoRedo.test.ts (2)
40-47:clearUndoRedoHistory()inbeforeEachis redundant
clearWorkflow()already resetsundoHistory,redoHistory,canUndo, andcanRedoto their initial values (seeclearWorkflowinworkflowStore.ts). The second call is a no-op.♻️ Suggested cleanup
beforeEach(() => { const store = useWorkflowStore.getState(); act(() => { store.clearWorkflow(); - store.clearUndoRedoHistory(); }); vi.clearAllMocks(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/__tests__/workflowStore.undoRedo.test.ts` around lines 40 - 47, The beforeEach in the test calls both clearWorkflow() and clearUndoRedoHistory(), but clearWorkflow() already resets undoHistory, redoHistory, canUndo and canRedo, so remove the redundant call to clearUndoRedoHistory() in the beforeEach; keep the act wrapper and clearWorkflow() call (from useWorkflowStore.getState()) and vi.clearAllMocks() unchanged to maintain test setup.
328-342:as nevercast should use a typed partial insteadCasting to
neversilences the type checker entirely."images"is a validOutputGalleryNodeDatafield — using a typed partial expresses the intent more precisely and would catch if the field name changes.♻️ Suggested fix
- store.updateNodeData(galleryId, { - images: ["img-1", "img-2"], - } as never); + store.updateNodeData(galleryId, { + images: ["img-1", "img-2"], + } as Partial<OutputGalleryNodeData>);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/__tests__/workflowStore.undoRedo.test.ts` around lines 328 - 342, The test uses an unsafe "as never" cast when calling updateNodeData; replace it with a typed partial so the compiler can validate the "images" field. Change the cast on the payload passed to updateNodeData in the test to "as Partial<OutputGalleryNodeData>" (or the appropriate Partial type for your node data) and keep the rest of the test logic (addNode, clearUndoRedoHistory, updateNodeData, and the undo assertions) unchanged.src/components/WorkflowCanvas.tsx (1)
1167-1237: Inconsistent indentation after the delete block.Lines 1167–1237 (the
getViewportCenterhelper through the Shift+key node-creation hotkeys) are indented with 6 spaces as if still inside a nested block, while the surrounding code uses 4 spaces. This was likely left over from a removed enclosing block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/WorkflowCanvas.tsx` around lines 1167 - 1237, The block starting at the getViewportCenter helper and the Shift+key hotkey handling (functions/variables: getViewportCenter, defaultDimensions, nodeType, addNode) is over-indented by two spaces; reformat those lines to match the surrounding 4-space indentation level so the helper and hotkey switch live at the same scope as neighboring code, keeping all internal indentation for the switch, defaultDimensions object, and addNode call intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/store/workflowStore.ts`:
- Around line 755-758: In deleteSelection, filter uniqueNodeIds to only those
that exist (e.g., via get().nodes or the store's node map) just like
uniqueGroupIds is filtered, so that phantom node IDs don't cause a history
snapshot; update uniqueNodeIds = Array.from(new Set(nodeIds)).filter(id =>
!!get().nodes[id]) (or the equivalent store accessor), then keep the existing
early-return check and subsequent logic so history is only pushed when there are
actually existing nodes or groups to delete.
- Around line 653-740: The undo/redo handlers currently set isRunning: false and
clear currentNodeIds without aborting an in-flight execution, which can let
pending async pipeline results apply to the restored state; before applying the
restored snapshot in both undo and redo, if get(). _abortController exists call
its abort() (and clear/reset the controller reference) to terminate any running
workflow, then proceed to set isRunning: false and currentNodeIds: []; update
references to _abortController in the undo and redo functions to guard the abort
call (e.g., check get(). _abortController and call get().
_abortController.abort()).
---
Outside diff comments:
In `@src/store/workflowStore.ts`:
- Around line 1344-1388: updateGroup and moveGroupNodes call
get()._pushHistorySnapshot() on every invocation, which floods undo history
during group drag/resize; change both signatures to accept an optional
skipHistory boolean (e.g., updateGroup(groupId: string, updates:
Partial<NodeGroup>, skipHistory = false) and moveGroupNodes(groupId: string,
delta: {x:number;y:number}, skipHistory = false)), only call
get()._pushHistorySnapshot() when skipHistory is false, and update
GroupsOverlay.tsx to call get()._pushHistorySnapshot() once at drag/resize start
(handleHeaderMouseDown / handleResizeMouseDown) and pass skipHistory = true for
the subsequent movement calls so intermediate moves don’t create history
snapshots.
---
Nitpick comments:
In `@src/components/WorkflowCanvas.tsx`:
- Around line 1167-1237: The block starting at the getViewportCenter helper and
the Shift+key hotkey handling (functions/variables: getViewportCenter,
defaultDimensions, nodeType, addNode) is over-indented by two spaces; reformat
those lines to match the surrounding 4-space indentation level so the helper and
hotkey switch live at the same scope as neighboring code, keeping all internal
indentation for the switch, defaultDimensions object, and addNode call intact.
In `@src/store/__tests__/workflowStore.undoRedo.test.ts`:
- Around line 40-47: The beforeEach in the test calls both clearWorkflow() and
clearUndoRedoHistory(), but clearWorkflow() already resets undoHistory,
redoHistory, canUndo and canRedo, so remove the redundant call to
clearUndoRedoHistory() in the beforeEach; keep the act wrapper and
clearWorkflow() call (from useWorkflowStore.getState()) and vi.clearAllMocks()
unchanged to maintain test setup.
- Around line 328-342: The test uses an unsafe "as never" cast when calling
updateNodeData; replace it with a typed partial so the compiler can validate the
"images" field. Change the cast on the payload passed to updateNodeData in the
test to "as Partial<OutputGalleryNodeData>" (or the appropriate Partial type for
your node data) and keep the rest of the test logic (addNode,
clearUndoRedoHistory, updateNodeData, and the undo assertions) unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/GroupsOverlay.tsxsrc/components/KeyboardShortcutsDialog.tsxsrc/components/WorkflowCanvas.tsxsrc/components/__tests__/GroupsOverlay.test.tsxsrc/store/__tests__/workflowStore.undoRedo.test.tssrc/store/workflowStore.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/store/workflowStore.ts (1)
1356-1366:⚠️ Potential issue | 🟠 Major
updateGroupandmoveGroupNodespush history on every call, including during drag and resizeBoth drag and resize operations in
GroupsOverlay.tsxattach mousemove listeners that callupdateGroup()andmoveGroupNodes()unconditionally. Since each method calls_pushHistorySnapshot()without checking for drag-end, a typical 1-second drag creates ~120 history entries (2 per frame × 60fps), saturating the 60-entry undo history in under 500ms and evicting meaningful checkpoints.🛡️ Proposed fix — only push history at drag-end
Add an
isDraggingparameter to both methods and gate the history push:- moveGroupNodes: (groupId: string, delta: { x: number; y: number }) => { - if (delta.x === 0 && delta.y === 0) return; - get()._pushHistorySnapshot(); + moveGroupNodes: (groupId: string, delta: { x: number; y: number }, isDragging = true) => { + if (delta.x === 0 && delta.y === 0) return; + if (!isDragging) { + get()._pushHistorySnapshot(); + } set((state) => ({- updateGroup: (groupId: string, updates: Partial<NodeGroup>) => { - if (!get().groups[groupId]) return; - get()._pushHistorySnapshot(); + updateGroup: (groupId: string, updates: Partial<NodeGroup>, isDragging = true) => { + if (!get().groups[groupId]) return; + if (!isDragging) { + get()._pushHistorySnapshot(); + } set((state) => ({Then update callers (GroupsOverlay.tsx and GroupNode.tsx) to pass
isDraggingduring drag/resize and manually call_pushHistorySnapshot()only on drag-end in thehandleMouseUphandlers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/workflowStore.ts` around lines 1356 - 1366, updateGroup and moveGroupNodes are calling _pushHistorySnapshot() on every call (including during mousemove), which floods the undo stack during drags; modify both function signatures (updateGroup and moveGroupNodes) to accept an optional isDragging boolean (default false) and only call _pushHistorySnapshot() when isDragging is false (i.e., on non-drag updates), then update the callers in GroupsOverlay.tsx and GroupNode.tsx so mousemove/resize handlers pass isDragging=true and do NOT trigger history; instead call get()._pushHistorySnapshot() once from the mouseup/handleMouseUp handlers to create the snapshot at drag-end.
♻️ Duplicate comments (1)
src/store/workflowStore.ts (1)
656-703: Unresolved:undo/redodo not guard against in-flight async execution resultsThe code now aborts the controller before restoring the snapshot, but in-flight network calls that haven't yet checked
signal.abortedcan still callupdateNodeData()after the snapshot is restored — mixing the pre-undo topology with new execution output. The previously proposed guard (if (state.isRunning) return;) or callingstopWorkflow()first would close this window.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/workflowStore.ts` around lines 656 - 703, The undo path can still receive in-flight async execution results that call updateNodeData() after the snapshot is restored; to fix this, ensure the workflow is fully stopped before applying the restored snapshot by invoking stopWorkflow() (or the centralized stop routine) at the start of undo (or returning early if state.isRunning) so the controller abort and isRunning state are cleared before any state writes; reference undo, _abortController, isRunning, stopWorkflow, and updateNodeData to locate and sequence the calls so no async callbacks can mix pre-undo topology with post-undo outputs.
🧹 Nitpick comments (2)
src/store/workflowStore.ts (2)
1095-1109: Consider unifying the options type to a single object shapeThe 5-condition
isPointOptionsdiscriminator exists only for backward compatibility with the oldXYPositioncall signature. If all callers can be updated, collapsing to a single{ offset?: XYPosition; selectedGroupId?: string | null }object eliminates the runtime type-check boilerplate and keeps the public API clean.♻️ Proposed simplification
- duplicateSelectedNodes: (options: XYPosition | { offset?: XYPosition; selectedGroupId?: string | null } = { x: 50, y: 50 }) => { - const isPointOptions = - typeof options === "object" && - options !== null && - "x" in options && - "y" in options && - typeof options.x === "number" && - typeof options.y === "number"; - const opts = isPointOptions - ? { offset: options as XYPosition, selectedGroupId: null as string | null } - : { - offset: (options as { offset?: XYPosition; selectedGroupId?: string | null }).offset || { x: 50, y: 50 }, - selectedGroupId: (options as { offset?: XYPosition; selectedGroupId?: string | null }).selectedGroupId ?? null, - }; + duplicateSelectedNodes: (options: { offset?: XYPosition; selectedGroupId?: string | null } = {}) => { + const opts = { + offset: options.offset ?? { x: 50, y: 50 }, + selectedGroupId: options.selectedGroupId ?? null, + };Update callers in
WorkflowCanvas.tsxaccordingly (e.g.duplicateSelectedNodes({ offset: { x: 50, y: 50 } })).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/workflowStore.ts` around lines 1095 - 1109, The duplicateSelectedNodes function currently discriminates between an old XYPosition and a new options object using a complex isPointOptions check; simplify by changing its signature to accept a single options object type { offset?: XYPosition; selectedGroupId?: string | null } (provide a default offset { x:50, y:50 } inside), remove the isPointOptions discriminator and related casts, and adapt any callers (e.g., in WorkflowCanvas.tsx) to call duplicateSelectedNodes({ offset: { x:50, y:50 } }) or pass selectedGroupId as needed so the runtime type-check boilerplate is eliminated.
229-231:JSON.stringifyequality is order-sensitive — low-risk but worth notingIf
nodesoredgesarrays are ever reordered without structural changes (unlikely given Zustand's immutable append/filter pattern, but possible viaapplyNodeChanges), two structurally identical snapshots would not compare equal, causing an unnecessary undo step to be silently skipped over. A deep-equality library or normalized comparison would be more robust, though the current implementation is adequate for the typical mutation patterns here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/workflowStore.ts` around lines 229 - 231, The equality check in isStructurallyEqualSnapshot relies on JSON.stringify(toStructuralSnapshot(...)) which is order-sensitive and can misreport structurally identical snapshots if nodes/edges are reordered; update isStructurallyEqualSnapshot to perform an order-insensitive comparison by either normalizing the structural snapshot before stringifying (e.g., sort nodes and edges deterministically by id/keys) or use a deep equality utility (e.g., lodash/isEqual or a stable-deep-equal) on the results of toStructuralSnapshot(a) and toStructuralSnapshot(b), preserving the existing return type and using the same function names (isStructurallyEqualSnapshot, toStructuralSnapshot) so callers remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/store/workflowStore.ts`:
- Around line 634-641: The dedup guard in _pushHistorySnapshot is ineffective
because cloneHistorySnapshot produces deep-cloned objects so comparisons like
last.nodes === snapshot.nodes always fail; update _pushHistorySnapshot to
compare the new snapshot to the last entry using
isStructurallyEqualSnapshot(last, snapshot) (checking nodes, edges, groups,
edgeStyle structurally) and return early when they match so undoHistory does not
get flooded with identical-state frames.
- Around line 74-76: The new node kinds objectIsolate, falIsolate, and
sceneCopilot are wired into workflowStore (executeObjectIsolate,
executeFalIsolate, executeSceneCopilot) but missing required registrations; add
each to the NodeType union in src/types/nodes.ts, add entries for their
dimensions in defaultNodeDimensions (src/store/utils/nodeDefaults.ts), return
sensible defaults for their data in createDefaultNodeData(), register their
React node components in the nodeTypes map (src/components/WorkflowCanvas.tsx),
and ensure the execution functions are exported from src/store/execution/
(export executeObjectIsolate, executeFalIsolate, executeSceneCopilot) so the
switch cases and addNode flow can actually create, render and run these node
types.
---
Outside diff comments:
In `@src/store/workflowStore.ts`:
- Around line 1356-1366: updateGroup and moveGroupNodes are calling
_pushHistorySnapshot() on every call (including during mousemove), which floods
the undo stack during drags; modify both function signatures (updateGroup and
moveGroupNodes) to accept an optional isDragging boolean (default false) and
only call _pushHistorySnapshot() when isDragging is false (i.e., on non-drag
updates), then update the callers in GroupsOverlay.tsx and GroupNode.tsx so
mousemove/resize handlers pass isDragging=true and do NOT trigger history;
instead call get()._pushHistorySnapshot() once from the mouseup/handleMouseUp
handlers to create the snapshot at drag-end.
---
Duplicate comments:
In `@src/store/workflowStore.ts`:
- Around line 656-703: The undo path can still receive in-flight async execution
results that call updateNodeData() after the snapshot is restored; to fix this,
ensure the workflow is fully stopped before applying the restored snapshot by
invoking stopWorkflow() (or the centralized stop routine) at the start of undo
(or returning early if state.isRunning) so the controller abort and isRunning
state are cleared before any state writes; reference undo, _abortController,
isRunning, stopWorkflow, and updateNodeData to locate and sequence the calls so
no async callbacks can mix pre-undo topology with post-undo outputs.
---
Nitpick comments:
In `@src/store/workflowStore.ts`:
- Around line 1095-1109: The duplicateSelectedNodes function currently
discriminates between an old XYPosition and a new options object using a complex
isPointOptions check; simplify by changing its signature to accept a single
options object type { offset?: XYPosition; selectedGroupId?: string | null }
(provide a default offset { x:50, y:50 } inside), remove the isPointOptions
discriminator and related casts, and adapt any callers (e.g., in
WorkflowCanvas.tsx) to call duplicateSelectedNodes({ offset: { x:50, y:50 } })
or pass selectedGroupId as needed so the runtime type-check boilerplate is
eliminated.
- Around line 229-231: The equality check in isStructurallyEqualSnapshot relies
on JSON.stringify(toStructuralSnapshot(...)) which is order-sensitive and can
misreport structurally identical snapshots if nodes/edges are reordered; update
isStructurallyEqualSnapshot to perform an order-insensitive comparison by either
normalizing the structural snapshot before stringifying (e.g., sort nodes and
edges deterministically by id/keys) or use a deep equality utility (e.g.,
lodash/isEqual or a stable-deep-equal) on the results of toStructuralSnapshot(a)
and toStructuralSnapshot(b), preserving the existing return type and using the
same function names (isStructurallyEqualSnapshot, toStructuralSnapshot) so
callers remain unchanged.
Comment on lines
+74
to
+76
| executeObjectIsolate, | ||
| executeFalIsolate, | ||
| executeSceneCopilot, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the three new node types in critical registration locations
echo "=== Checking NodeType union in src/types/index.ts ==="
rg -n "type NodeType" -A 30 src/types/index.ts | head -50
echo ""
echo "=== Checking for objectIsolate/falIsolate/sceneCopilot in types files ==="
rg -n "objectIsolate|falIsolate|sceneCopilot" src/types/ --type ts
echo ""
echo "=== Checking defaultNodeDimensions in workflowStore ==="
rg -n "defaultNodeDimensions\s*=" -A 25 src/store/workflowStore.ts | head -50
echo ""
echo "=== Checking for objectIsolate/falIsolate/sceneCopilot in store/workflowStore ==="
rg -n "objectIsolate|falIsolate|sceneCopilot" src/store/workflowStore.ts | head -20
echo ""
echo "=== Checking createDefaultNodeData function ==="
rg -n "createDefaultNodeData" -A 40 src/store/workflowStore.ts | head -60
echo ""
echo "=== Checking node components directory ==="
ls -la src/components/nodes/ | grep -E "objectIsolate|falIsolate|sceneCopilot|index"Repository: shrimbly/node-banana
Length of output: 2510
🏁 Script executed:
#!/bin/bash
# Search for NodeType definition more broadly
echo "=== Find NodeType definition ==="
rg -n "type NodeType\s*=" src/ --type ts -A 30
echo ""
echo "=== Check nodeDefaults.ts file ==="
cat -n src/store/utils/nodeDefaults.ts | head -100
echo ""
echo "=== List actual node component files ==="
find src/components/nodes -type f -name "*.tsx" -o -name "*.ts" | sort
echo ""
echo "=== Check components/nodes/index.ts exports ==="
cat -n src/components/nodes/index.tsRepository: shrimbly/node-banana
Length of output: 7106
🏁 Script executed:
#!/bin/bash
# Check createDefaultNodeData for the missing node types and look for any other references
echo "=== Check createDefaultNodeData full implementation ==="
rg -n "createDefaultNodeData" -A 150 src/store/utils/nodeDefaults.ts | tail -100
echo ""
echo "=== Search for any ObjectIsolate/FalIsolate/SceneCopilot component files ==="
find src -name "*bject*" -o -name "*al*" -o -name "*cene*" | grep -i "isolate\|falIsolate\|sceneCopilot"
echo ""
echo "=== Check WorkflowCanvas for node type registration ==="
rg -n "nodeTypes|minimap" src/components/WorkflowCanvas.tsx -B 2 -A 5 | head -80Repository: shrimbly/node-banana
Length of output: 4404
🏁 Script executed:
#!/bin/bash
# Get the full createDefaultNodeData function to see all cases
echo "=== Full createDefaultNodeData switch statement ==="
rg -n "case \"" src/store/utils/nodeDefaults.ts | grep -E "objectIsolate|falIsolate|sceneCopilot|splitGrid|output|videoStitch"
echo ""
echo "=== Count total cases in createDefaultNodeData ==="
rg -n "case \"" src/store/utils/nodeDefaults.ts | wc -l
echo ""
echo "=== Show end of createDefaultNodeData ==="
rg -n "createDefaultNodeData" -A 200 src/store/utils/nodeDefaults.ts | tail -50Repository: shrimbly/node-banana
Length of output: 1745
🏁 Script executed:
#!/bin/bash
# Get the very end of createDefaultNodeData to see if it ends after videoTrim
echo "=== Rest of createDefaultNodeData after videoTrim ==="
rg -n "createDefaultNodeData" -A 250 src/store/utils/nodeDefaults.ts | tail -30
echo ""
echo "=== Check if objectIsolate/falIsolate/sceneCopilot cases exist anywhere ==="
rg -n "case \"objectIsolate\"|case \"falIsolate\"|case \"sceneCopilot\"" src/store/utils/nodeDefaults.ts
echo ""
echo "=== Full nodeDefaults.ts line count ==="
wc -l src/store/utils/nodeDefaults.ts
echo ""
echo "=== Check end of file for any additional switch statements ==="
tail -50 src/store/utils/nodeDefaults.tsRepository: shrimbly/node-banana
Length of output: 2368
🏁 Script executed:
#!/bin/bash
# Check full nodeTypes registration in WorkflowCanvas
echo "=== Full nodeTypes registration in WorkflowCanvas ==="
rg -n "const nodeTypes" -A 25 src/components/WorkflowCanvas.tsx
echo ""
echo "=== Verify no other files define ObjectIsolate/FalIsolate/SceneCopilot ==="
rg -n "ObjectIsolate|FalIsolate|SceneCopilot" src/ --type ts --type tsx
echo ""
echo "=== Check if there are data interfaces for these types in types/nodes.ts ==="
rg -n "ObjectIsolateNodeData|FalIsolateNodeData|SceneCopilotNodeData" src/types/Repository: shrimbly/node-banana
Length of output: 1131
🏁 Script executed:
#!/bin/bash
# Double-check: look at the imports in workflowStore where these execute functions are imported
echo "=== Show the imports in workflowStore ==="
head -100 src/store/workflowStore.ts | grep -A 20 "from \"./execution\""
echo ""
echo "=== Check if these execute functions exist in execution module ==="
rg -n "export.*executeObjectIsolate|export.*executeFalIsolate|export.*executeSceneCopilot" src/store/execution/
echo ""
echo "=== List execution module files ==="
ls -la src/store/execution/Repository: shrimbly/node-banana
Length of output: 1744
🏁 Script executed:
#!/bin/bash
# Check the execution module index.ts
echo "=== Check execution/index.ts ==="
cat -n src/store/execution/index.ts
echo ""
echo "=== Search for the three execute functions in all execution files ==="
rg -n "executeObjectIsolate|executeFalIsolate|executeSceneCopilot" src/store/execution/Repository: shrimbly/node-banana
Length of output: 1761
Complete missing node type registrations for objectIsolate, falIsolate, and sceneCopilot
These node types are wired into execution logic in workflowStore.ts but lack required registrations across the codebase:
- Not in
NodeTypeunion (src/types/nodes.ts): The switch cases at lines 1579, 1582, 1585, 1904, 1907, 1910 are dead code—node.typecan never match these values through normaladdNode()flow - Not in
defaultNodeDimensions(src/store/utils/nodeDefaults.ts): CallingaddNode()with these types will fail at runtime when attempting to destructureundefined - Not in
createDefaultNodeData()(src/store/utils/nodeDefaults.ts): No default node data defined - Not in
nodeTypesmap (src/components/WorkflowCanvas.tsx): UI cannot render these nodes - Execution functions do not exist:
executeObjectIsolate,executeFalIsolate,executeSceneCopilotare imported in workflowStore.ts but are not exported from src/store/execution/
Per coding guidelines, adding a new node type requires updates to the NodeType union, defaultNodeDimensions, createDefaultNodeData(), node component definitions, and nodeTypes registration. This PR has incomplete implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/store/workflowStore.ts` around lines 74 - 76, The new node kinds
objectIsolate, falIsolate, and sceneCopilot are wired into workflowStore
(executeObjectIsolate, executeFalIsolate, executeSceneCopilot) but missing
required registrations; add each to the NodeType union in src/types/nodes.ts,
add entries for their dimensions in defaultNodeDimensions
(src/store/utils/nodeDefaults.ts), return sensible defaults for their data in
createDefaultNodeData(), register their React node components in the nodeTypes
map (src/components/WorkflowCanvas.tsx), and ensure the execution functions are
exported from src/store/execution/ (export executeObjectIsolate,
executeFalIsolate, executeSceneCopilot) so the switch cases and addNode flow can
actually create, render and run these node types.
Comment on lines
+634
to
+641
| const last = state.undoHistory[state.undoHistory.length - 1]; | ||
| if ( | ||
| last && | ||
| last.nodes === snapshot.nodes && | ||
| last.edges === snapshot.edges && | ||
| last.groups === snapshot.groups && | ||
| last.edgeStyle === snapshot.edgeStyle | ||
| ) return; |
There was a problem hiding this comment.
_pushHistorySnapshot dedup guard is dead code — every push is unconditional
last in undoHistory was stored via cloneHistorySnapshot, so its .nodes/.edges/.groups are deep-cloned references that can never be === to state.nodes/state.edges/state.groups. The guard always evaluates to false, meaning every call appends a new entry regardless of whether the state actually changed.
This has a concrete UX impact: operations that fire many times per drag frame — like updateGroup (group resize) and moveGroupNodes (group drag) — flood the 60-entry history with intermediate states. The 60-entry cap will evict meaningful undo checkpoints, and each position-change frame is structurally distinct, so the isStructurallyEqualSnapshot skip in undo/redo doesn't help here.
🔧 Proposed fix
Replace reference equality with isStructurallyEqualSnapshot:
const last = state.undoHistory[state.undoHistory.length - 1];
- if (
- last &&
- last.nodes === snapshot.nodes &&
- last.edges === snapshot.edges &&
- last.groups === snapshot.groups &&
- last.edgeStyle === snapshot.edgeStyle
- ) return;
+ if (last && isStructurallyEqualSnapshot(last, snapshot)) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/store/workflowStore.ts` around lines 634 - 641, The dedup guard in
_pushHistorySnapshot is ineffective because cloneHistorySnapshot produces
deep-cloned objects so comparisons like last.nodes === snapshot.nodes always
fail; update _pushHistorySnapshot to compare the new snapshot to the last entry
using isStructurallyEqualSnapshot(last, snapshot) (checking nodes, edges,
groups, edgeStyle structurally) and return early when they match so undoHistory
does not get flooded with identical-state frames.
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
Scope
This PR is intentionally scoped to workflow deletion/duplication/undo-redo only.
Validation
Summary by CodeRabbit
New Features
Bug Fixes / UX
Tests