Feat: Alt+Drag to clone - Vue Nodes #6789
Conversation
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 11/21/2025, 09:33:18 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Test Results❌ Some tests failed ⏰ Completed at: 11/21/2025, 09:43:22 PM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.13 MB (baseline 3.13 MB) • 🔴 +209 BMain entry bundles and manifests
Status: 3 added / 3 removed Graph Workspace — 940 kB (baseline 945 kB) • 🟢 -4.35 kBGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 7.97 kB (baseline 7.97 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 1 added / 1 removed Panels & Settings — 306 kB (baseline 306 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 6 added / 6 removed UI Components — 141 kB (baseline 141 kB) • ⚪ 0 BReusable component library chunks
Status: 6 added / 6 removed Data & Services — 12.5 kB (baseline 12.5 kB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 2 added / 2 removed Utilities & Hooks — 2.94 kB (baseline 2.94 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 1 added / 1 removed Vendor & Third-Party — 5.32 MB (baseline 5.32 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 3.87 MB (baseline 3.87 MB) • ⚪ 0 BBundles that do not match a named category
Status: 18 added / 18 removed |
DrJKL
changed the title
dosubot
bot
added
the
size:XXL
…hrough the Component
… has a low z-index...
DrJKL
force-pushed
the
drjkl/the-prestige
branch
from
ecf499c to
6f17802
Compare
christian-byrne
left a comment
christian-byrne
left a comment
There was a problem hiding this comment.
LGTM although didn't test and also saw a console log commented out
I tracked down the log and sent it to the cornfield. Please forgive me 🙇🏻 |
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 WalkthroughWalkthroughTyped node IDs and node position tuples; moved node drag/snap into a new shared useNodeDrag composable; converted useTransformState into a shared composable and removed the TransformState injection key; refactored event/pointer APIs to use NodeId; added LGraphCanvas.cloneNodes and Alt-drag-to-clone; updated tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NodeComp as LGraphNode
participant Canvas as LGraphCanvas
participant Pointer as useNodePointerInteractions
participant Drag as useNodeDrag
participant Layout as LayoutStore
User->>NodeComp: Alt + PointerDown
NodeComp->>Canvas: cloneNodes([node])
Canvas-->>NodeComp: cloned node (offset)
NodeComp->>Pointer: pointerdown(event, nodeId)
Pointer->>Drag: startDrag(event, nodeId)
Drag->>Layout: mark dragging & record starts
User->>NodeComp: PointerMove
NodeComp->>Pointer: pointermove(event)
Pointer->>Drag: handleDrag(event, nodeId)
Drag->>Layout: RAF-updates positions
User->>NodeComp: PointerUp
NodeComp->>Pointer: pointerup(event)
Pointer->>Drag: endDrag(nodeId)
Drag->>Layout: apply snap, batch final updates, clear dragging
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests-ui/tests/renderer/extensions/vueNodes/composables/useNodeEventHandlers.test.ts (1)
52-64:vi.resetAllMockswill break your manual module mocksAll your module mocks (
useCanvasStore,useLayoutMutations,useGraphNodeManager,useCanvasInteractions) are implemented withvi.fn(() => ...)insidevi.mockfactories. Callingvi.resetAllMocks()inbeforeEach(line 84) resets each mock's implementation, causinguseCanvasStore()and friends to returnundefinedinstead of the configured instances once this suite is un-skipped.To preserve the factory implementations while clearing call history, use
vi.clearAllMocks()instead. This keeps the mocks usable across tests while still resetting call counts.Also applies to: 17-44 (all module mocks), 75 (describe.skip), 83-86 (beforeEach)
🧹 Nitpick comments (11)
src/lib/litegraph/src/LGraphCanvas.ts (1)
1774-1803: SharedcloneNodeshelper looks correct; consider minimal hardening for external callersCentralizing cloning into
cloneNodesand havingonMenuNodeClonerespectselectedItems(falling back to the clicked node) is a solid improvement and keeps behavior consistent with the existing serialize/deserialize flow. The +5/+5 offset via_deserializeItemswill yield the expected “nudged” clones for single and multi-selection.Two small, non-blocking suggestions:
- Add a defensive guard for
LGraphCanvas.active_canvasinsidecloneNodes(e.g. early return or a clear error) so that accidental calls when no canvas is active fail more predictably than with a genericTypeError.- Optionally treat an empty
nodesarray as a no-op insidecloneNodes(you already guard inonMenuNodeClone, but this would make the helper safer for other static call sites, such as Vue alt‑drag integration).These don’t change semantics but would make the new static helper more robust if it’s reused elsewhere.
src/composables/graph/useGraphNodeManager.ts (1)
16-69: Align GraphNodeManager API withNodeIdfor consistencyUpdating
VueNodeData.idtoNodeIdis good for type clarity, butvueNodeDataandgetNodestill exposestringin their signatures. To keep things consistent and future‑proof ifNodeIdever stops being a bare string, consider:-export interface GraphNodeManager { - // Reactive state - safe data extracted from LiteGraph nodes - vueNodeData: ReadonlyMap<string, VueNodeData> - - // Access to original LiteGraph nodes (non-reactive) - getNode(id: string): LGraphNode | undefined +export interface GraphNodeManager { + // Reactive state - safe data extracted from LiteGraph nodes + vueNodeData: ReadonlyMap<NodeId, VueNodeData> + + // Access to original LiteGraph nodes (non-reactive) + getNode(id: NodeId): LGraphNode | undefinedand updating the internal Maps/signatures accordingly. This is purely a typing/ergonomics improvement; behavior stays the same.
tests-ui/tests/renderer/extensions/vueNodes/components/LGraphNode.test.ts (1)
17-26: Transform state mock is sufficient but slightly under-specifiedThe mock only exposes
screenToCanvas,canvasToScreen,camera, andisNodeInViewport. That’s fine for current LGraphNode usage, but if the component later destructurestransformStyleorsyncWithCanvasfromuseTransformState, tests will start failing due to missing properties. Consider expanding the mock to match the real composable surface to make these tests more future‑proof.tests-ui/tests/renderer/extensions/vueNodes/composables/useNodeEventHandlers.test.ts (1)
252-303:toggleNodeSelectionAfterPointerUptests appear inconsistentTwo tests call
toggleNodeSelectionAfterPointerUp('node-1', true)with effectively the same local setup (mockNode!.selected = true), but they assert opposite behaviors (one expects deselect + update, the other expects no updates) while comments describe different scenarios (“selected at pointer down” vs “not previously selected”).If the real behavior depends on additional state (e.g. something set during pointer‑down), consider:
- Explicitly driving that state via
handleNodeSelectin each test, or- Adjusting the comments and expectations so the scenarios and assertions actually differ.
Right now these tests are skipped, but un‑skipping them in this state will likely cause confusion even if they pass by accident.
src/renderer/extensions/vueNodes/layout/useNodeDrag.ts (1)
18-215: Drag implementation is correct and nicely factoredThe shared
useNodeDragcomposable cleanly handles:
- Capturing initial node + multi‑selection positions and mouse origin in canvas space.
- Using
screenToCanvasto compute deltas, then applying them viamoveNodefor the primary and other selected nodes.- Throttling updates with
requestAnimationFrameand deferring pointer capture to drag time (which should play well with Alt+drag cloning).- Applying snap‑to‑grid only on drag end via
batchUpdateNodeBounds, with proper change checks and shift‑key–driven behavior.- Cleaning up drag state, shift tracking, and any pending RAF in
endDrag.If you ever need concurrent drags (e.g. multi‑pointer scenarios), the shared, single‑instance state will need revisiting, but for standard canvas interactions this design is appropriate.
src/renderer/extensions/vueNodes/components/LGraphNode.vue (1)
22-23: Global dragging cursor may affect all nodes, not just the active one
cursorClassderives fromlayoutStore.isDraggingVueNodes.value, so while any Vue node is being dragged, all non‑pinned nodes render withcursor-grabbing. If the intent is to visually indicate a “global dragging” mode, this is fine; if you only want the actively dragged nodes to show the grabbing cursor, you’ll need a per‑node drag state instead of a single global flag.Also applies to: 443-451
src/renderer/extensions/vueNodes/layout/useNodeLayout.ts (1)
1-2: Minimal layout API looks solid; assume nodeId is stableThis refactor to
useNodeLayoutas a thin wrapper overlayoutStore.getNodeLayoutRefplusmoveNodeTois clean and keeps layout concerns in one place. ThetoValue(nodeIdMaybe)call at setup time assumes the node id won’t change; that’s appropriate for graph node ids but is worth keeping in mind if you ever pass a truly dynamic ref here.Also applies to: 7-8, 30-31, 35-38, 47-48
src/renderer/core/layout/transform/useTransformState.ts (1)
55-56: Shared transform composable matches the single‑canvas designWrapping the previous implementation in
useTransformStateIndividual()and exporting it viacreateSharedComposablegives you a single shared camera/transform state across all consumers, which aligns with the “one transform container per canvas” architecture. The helper refactors to named functions keep behavior identical and improve clarity/hoisting.One thing to keep in mind: if you ever support multiple independent canvases in the same app, this singleton transform state will need to be revisited or parameterized.
Also applies to: 68-69, 95-103, 116-121, 142-146, 155-158, 161-164, 167-170, 183-189, 199-205, 216-219, 249-251
src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.ts (3)
33-36: Stale comment vs implementation and earlystartDragsemanticsIn
onPointerdownthe comment says “Don’t start drag yet – wait for pointer move to exceed threshold”, but you already callstartDrag(event, nodeId)here. That’s consistent with using pointermove to flipisDraggingVueNodesonly after the threshold while still capturing the initial drag state, but the comment is now misleading.I’d either:
- Update the comment to reflect the actual intent (“initialize drag context now, but don’t mark as dragging until threshold”), or
- Move
startDraginto the threshold/multi‑select branches if you want the code to match the original comment.Also applies to: 45-52, 60-65
67-79: DoublestartDragcall in multi‑select pathIn
onPointermove, the multi‑select branch (lmbDown && multiSelect && !isDragging) callsstartDrag(event, nodeId)again even thoughstartDragwas already called on pointerdown. IfuseNodeDrag.startDraghas any non‑idempotent side effects, this could be surprising.Given the current design, it’s probably safe but you might want to:
- Rely solely on the pointerdown
startDragfor single‑select and multi‑select, or- Guard the multi‑select branch to avoid re‑calling
startDragif an active drag context for this pointer already exists.Also applies to: 81-94
97-110: Global dragging flag and cleanup behaviorUsing
layoutStore.isDraggingVueNodesas the single source of truth for “any Vue node is being dragged” keeps things simple and matches the rest of the layout system, and wrappingendDraginsafeDragEndwith afinally‑style cleanup is a good defensive pattern.Just be aware that:
onContextmenuintentionally skipsendDrag, so any internal state inuseNodeDragrelies on its own guards not to treat subsequent moves as part of the same drag.onScopeDisposeonly resets the globalisDraggingVueNodesflag; if a component unmounts mid‑drag,useNodeDrag’s internal context must be robust against that scenario.Given the current global “single drag at a time” assumption, this is acceptable, but it’s worth revisiting if you ever allow overlapping drags.
Also applies to: 112-132, 134-152
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/composables/graph/useGraphNodeManager.ts(2 hunks)src/lib/litegraph/src/LGraphCanvas.ts(2 hunks)src/renderer/core/layout/injectionKeys.ts(0 hunks)src/renderer/core/layout/transform/TransformPane.vue(2 hunks)src/renderer/core/layout/transform/useTransformState.ts(10 hunks)src/renderer/core/spatial/boundsCalculator.ts(1 hunks)src/renderer/extensions/minimap/data/AbstractMinimapDataSource.ts(2 hunks)src/renderer/extensions/vueNodes/components/LGraphNode.vue(6 hunks)src/renderer/extensions/vueNodes/composables/useNodeEventHandlers.ts(7 hunks)src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.test.ts(13 hunks)src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.ts(2 hunks)src/renderer/extensions/vueNodes/composables/useSlotLinkInteraction.ts(2 hunks)src/renderer/extensions/vueNodes/interactions/resize/useNodeResize.ts(1 hunks)src/renderer/extensions/vueNodes/layout/useNodeDrag.ts(1 hunks)src/renderer/extensions/vueNodes/layout/useNodeLayout.ts(3 hunks)tests-ui/tests/composables/element/useTransformState.test.ts(6 hunks)tests-ui/tests/performance/transformPerformance.test.ts(0 hunks)tests-ui/tests/renderer/extensions/vueNodes/components/LGraphNode.test.ts(2 hunks)tests-ui/tests/renderer/extensions/vueNodes/composables/useNodeEventHandlers.test.ts(14 hunks)
💤 Files with no reviewable changes (2)
- src/renderer/core/layout/injectionKeys.ts
- tests-ui/tests/performance/transformPerformance.test.ts
🧰 Additional context used
🧬 Code graph analysis (10)
src/renderer/extensions/vueNodes/interactions/resize/useNodeResize.ts (1)
src/renderer/core/layout/transform/useTransformState.ts (1)
useTransformState(249-251)
src/composables/graph/useGraphNodeManager.ts (3)
src/lib/litegraph/src/LGraphNode.ts (1)
NodeId(92-92)src/renderer/core/layout/types.ts (1)
NodeId(39-39)src/lib/litegraph/src/litegraph.ts (1)
NodeId(114-114)
src/renderer/extensions/vueNodes/layout/useNodeDrag.ts (7)
src/renderer/core/layout/operations/layoutMutations.ts (1)
useLayoutMutations(67-340)src/renderer/core/canvas/canvasStore.ts (1)
useCanvasStore(24-152)src/renderer/core/layout/transform/useTransformState.ts (1)
useTransformState(249-251)src/renderer/extensions/vueNodes/composables/useNodeSnap.ts (1)
useNodeSnap(12-73)src/renderer/extensions/vueNodes/composables/useShiftKeySync.ts (1)
useShiftKeySync(25-107)src/renderer/core/layout/store/layoutStore.ts (1)
layoutStore(1458-1458)src/renderer/core/layout/types.ts (1)
NodeBoundsUpdate(34-37)
src/renderer/extensions/vueNodes/composables/useSlotLinkInteraction.ts (1)
src/lib/litegraph/src/LGraphCanvas.ts (1)
event(3678-3683)
src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.ts (3)
src/renderer/extensions/vueNodes/layout/useNodeDrag.ts (1)
useNodeDrag(18-18)src/renderer/core/canvas/useCanvasInteractions.ts (1)
useCanvasInteractions(12-135)src/renderer/extensions/vueNodes/utils/selectionUtils.ts (1)
isMultiSelectKey(8-10)
tests-ui/tests/renderer/extensions/vueNodes/composables/useNodeEventHandlers.test.ts (1)
src/composables/graph/useVueNodeLifecycle.ts (1)
useVueNodeLifecycle(182-184)
src/renderer/extensions/minimap/data/AbstractMinimapDataSource.ts (1)
src/renderer/core/spatial/boundsCalculator.ts (1)
PositionedNode(14-17)
src/renderer/extensions/vueNodes/composables/useNodeEventHandlers.ts (1)
src/lib/litegraph/src/LGraphNode.ts (1)
collapsed(3420-3422)
src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.test.ts (3)
src/renderer/core/layout/types.ts (1)
NodeLayout(44-52)src/renderer/core/layout/store/layoutStore.ts (3)
getNodeLayoutRef(204-305)setSource(930-932)layoutStore(1458-1458)src/renderer/extensions/vueNodes/composables/useNodeEventHandlers.ts (1)
useNodeEventHandlers(164-166)
src/lib/litegraph/src/LGraphCanvas.ts (1)
src/lib/litegraph/src/interfaces.ts (3)
IContextMenuValue(405-426)IContextMenuOptions(382-403)Positionable(77-119)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: collect
- GitHub Check: test
- GitHub Check: setup
- GitHub Check: lint-and-format
🔇 Additional comments (12)
src/renderer/extensions/vueNodes/composables/useSlotLinkInteraction.ts (1)
385-395: Stoppingpointermovepropagation for the active pointer session is a good isolation stepThe new
event.stopPropagation()insidehandlePointerMove, guarded bypointerSession.matches(event), cleanly isolates link-drag pointer moves from other interaction layers (e.g., node drag / Alt‑clone) without affecting unrelated pointers. This looks correct and consistent with the refactored interaction model.src/renderer/extensions/minimap/data/AbstractMinimapDataSource.ts (2)
3-3: LGTM: Type import aligns with explicit typing below.The import of
PositionedNodesupports the explicit type annotation in thegetBoundsmethod, improving type safety.
33-38: LGTM: Explicit typing improves type safety.The explicit
PositionedNodetype annotation correctly mapsMinimapNodeDatato the format expected bycalculateNodeBounds, with proper tuple construction forposandsize.src/renderer/core/spatial/boundsCalculator.ts (1)
14-17: <!-- [scratchpad_start]
[task review_file_2/2 review_segment_1/1 verify_breaking_change resolved]
Confirmed compatibility - no breaking change[observations]
- LiteGraphDataSource.ts shows: node.pos[0], node.pos[1], node.size[0], node.size[1]
- LGraphNode objects from litegraph ALREADY have pos and size as tuples/arrays
- graphThumbnailRenderer passes graph._nodes (which are LGraphNode objects)
- PositionedNode interface expects pos: [number, number] and size: [number, number]
- LGraphNode already matches this structure!
[analysis]
NO BREAKING CHANGE:
- LGraphNode objects from litegraph already have pos and size as tuple-like structures
- AbstractMinimapDataSource converts from MinimapNodeData to PositionedNode
- graphThumbnailRenderer passes LGraphNode directly which already satisfies PositionedNode interface
- Type narrowing from ArrayLike to strict [number, number] doesn't break because LGraphNode.pos and .size are already exactly that
[pending]
- None - verification complete
[done]
Confirmed all usages are compatible:
- AbstractMinimapDataSource: converts MinimapNodeData to PositionedNode tuples ✓
- graphThumbnailRenderer: passes LGraphNode[] which has pos/size as tuples ✓
- useMinimapViewport: imports other functions, not PositionedNode ✓
- Tests mock the module, so no compatibility issues ✓
All consumers are compatible with the stricter tuple types. No breaking change detected.
After verifying all usages of
PositionedNodeacross the codebase, the type narrowing fromArrayLike<number>to[number, number]is safe:
- AbstractMinimapDataSource: Correctly converts
MinimapNodeDatatoPositionedNodetuples ([node.x, node.y])- graphThumbnailRenderer: Passes
LGraphNode[]directly, which already hasposandsizeas tuple structures (verified via LiteGraphDataSource.ts showingnode.pos[0],node.pos[1], etc.)- useMinimapViewport: Only imports helper functions, not
PositionedNodetype- Test mocks: Don't create incompatibilities
The stricter type accurately reflects the existing implementation without introducing breaking changes.
src/renderer/extensions/vueNodes/interactions/resize/useNodeResize.ts (1)
10-15: TypingtransformStatefromuseTransformStateis a good moveDeriving the option type via
ReturnType<typeof useTransformState>keeps this composable aligned with the actual transform hook API and still satisfies the existing usage (transformState.camera.z). No issues spotted.tests-ui/tests/composables/element/useTransformState.test.ts (1)
31-37: Shared transform state and tuple typing look correctUsing a single
transformStateinstance with per‑testsyncWithCanvaskeeps state predictable and aligns with the shared composable pattern. The explicit[number, number]tuple types fornodePos/nodeSizematch the underlying API expectations and improve type checking without altering behavior.Also applies to: 185-196, 212-215, 239-252
src/renderer/core/layout/transform/TransformPane.vue (1)
20-41: Refactor to shareduseTransformStatein TransformPane looks solidSwitching to
const { camera, transformStyle, syncWithCanvas } = useTransformState()and dropping provide/inject keeps this component focused on its own needs while leveraging the shared transform state. The RAF loop guards against missingprops.canvasand continues to emittransformUpdateas before, so behavior and responsibilities stay clear.src/renderer/extensions/vueNodes/components/LGraphNode.vue (3)
140-151: Shared transform + litegraph imports look consistentUsing
useTransformState()as a shared composable and importingLGraphCanvas,LGraphEventMode, andLiteGraphdirectly here cleanly aligns node rendering/transform state with the underlying canvas and enum usage. I don’t see issues with this wiring.Also applies to: 156-158, 204-205
198-201: NodeId‑based right‑click handling is coherentSwitching to
handleNodeRightClick(event as PointerEvent, nodeData.id)and de‑structuring it alongside other node event handlers keeps the context‑menu path consistent with the new NodeId‑centric API surface. Looks good.Also applies to: 304-305
278-279: useNodeLayout + style bindings are straightforwardUsing
useNodeLayout(() => nodeData.id)forposition,size, andzIndexand feeding those directly into the inlinetransform/zIndexstyle keeps DOM positioning in sync with layout state with minimal surface area (moveNodeToused for resize). No issues spotted here.Also applies to: 42-42
src/renderer/extensions/vueNodes/composables/useNodeEventHandlers.ts (2)
30-61: LGTM! Clean refactoring to NodeId-based API.The refactoring from VueNodeData to NodeId is well-structured. The function correctly retrieves the node from nodeManager, includes proper null checks, and preserves the multi-select and selection logic.
102-120: LGTM! Proper delegation to handleNodeSelect.The right-click handler correctly ensures the node is selected before showing the context menu by delegating to
handleNodeSelect, maintaining consistent selection behavior.
src/renderer/extensions/vueNodes/composables/useNodeEventHandlers.ts
Outdated
Show resolved
Hide resolved
src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.test.ts
Show resolved
Hide resolved
|
I’ve been able to test the shift/select behaviours here ✅ |
… asserting calls not behavior. useNodeEventHandlers
…his style of test, but it'll take a lot more effort to do real behavior checks.
DrJKL
added
the
New Browser Test Expectations
|
Updating Playwright Expectations |
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/renderer/extensions/vueNodes/composables/useNodePointerInteractions.test.ts (1)
158-181: Remove or fix skipped test that expects incorrect behavior.This test expects
handleNodeSelectto be called onpointerdown, but the actual implementation only callshandleNodeSelectduringpointermovewhen the drag threshold is exceeded. The test should either be removed (if this behavior change is intentional) or updated to match the current implementation.Do you want me to suggest an updated test that correctly verifies the selection behavior during drag threshold detection?
🧹 Nitpick comments (2)
tests-ui/tests/renderer/extensions/vueNodes/composables/useNodeEventHandlers.test.ts (2)
81-86: Confirmvi.resetAllMocksbehavior with your Vitest versionYou switched to
vi.resetAllMocks()inbeforeEach. In recent Vitest versions this resets mocks back to their original implementations, but older semantics (and some docs) describemockResetas replacing implementations with an empty function. If your environment followed the latter, module mocks likeuseCanvasStoreoruseGraphNodeManager(created viavi.mock(...)) could lose their factory behavior between tests, breaking destructuring likeconst { canvas } = useCanvasStore().Given tests are passing this is probably fine, but it’s worth double‑checking your Vitest version and its
resetAllMockssemantics. If you ever hit issues with mocks losing their base implementations, considervi.clearAllMocks()here instead so that only call history is cleared while keeping implementations intact.
251-301:toggleNodeSelectionAfterPointerUptests describe different scenarios but use identical setupThe two multi‑select tests:
- Line 252:
"deselects node that was selected at pointer down"- Line 264:
"node not previously selected: no-op"both currently set
mockNode!.selected = trueand then calltoggleNodeSelectionAfterPointerUp('node-1', true). As written, the setup for “previously selected” vs “not previously selected” is identical, so the second test doesn’t clearly exercise a distinct state and can’t catch regressions that differ between those scenarios.Consider either:
- Adjusting the second test’s setup (e.g., start from a different
mockNode.selected/ selection list state, or callhandleNodeSelectto simulate the pointer‑down path), or- Renaming the test description if both are intentionally covering the same behavior.
While you’re here, you might also use
testNodeIdinstead of the'node-1'literal in these calls and expectations to keep the test ID DRY.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.test.ts(11 hunks)tests-ui/tests/renderer/extensions/vueNodes/composables/useNodeEventHandlers.test.ts(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.test.ts (4)
src/renderer/core/layout/types.ts (1)
NodeLayout(44-52)src/renderer/core/layout/store/layoutStore.ts (3)
getNodeLayoutRef(204-305)setSource(930-932)layoutStore(1458-1458)src/renderer/extensions/vueNodes/composables/useNodeEventHandlers.ts (1)
useNodeEventHandlers(161-163)src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.ts (1)
useNodePointerInteractions(12-168)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: setup
- GitHub Check: setup
- GitHub Check: lint-and-format
- GitHub Check: test
- GitHub Check: collect
🔇 Additional comments (3)
src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.test.ts (2)
1-11: Good progress unskipping and updating to new API.The test suite has been successfully updated to work with the new nodeId-based API surface, addressing the major concerns from the previous review. The imports now reflect the new dependencies (Pinia testing setup,
layoutStore,useNodeDrag, etc.), and the suite is no longer fully skipped. However, one test remains skipped on line 158 that needs attention.
1-324: Test suite successfully updated to new API surface.This test file has been significantly improved to address the concerns from the previous review:
- ✅ Test suite is no longer fully skipped (only one test remains skipped)
- ✅ Mocks updated to reflect new API:
useNodeDrag,useNodeEventHandlers,layoutStore- ✅ Tests now use nodeId strings (
'test-node-123') instead ofVueNodeDataobjects- ✅ Tests verify interactions with
layoutStore.isDraggingVueNodesfor drag state management- ✅ Event handlers now receive node IDs as parameters matching the new implementation
The remaining issues (explicit state reset, one skipped test, misleading comments) are minor and can be addressed as follow-ups.
Based on learnings
tests-ui/tests/renderer/extensions/vueNodes/composables/useNodeEventHandlers.test.ts (1)
52-64: Type-onlyGraphNodeManagerimport in mock is correct and keeps runtime cleanUsing
import type { GraphNodeManager }and restricting it to type assertions inside theuseGraphNodeManagermock avoids adding runtime dependencies while still giving good typing fornodeManager. The mock structure (returning ashallowRefwithgetNode→mockNode) looks consistent with howuseNodeEventHandlersis exercised in the tests.
src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.test.ts
Show resolved
Hide resolved
src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.test.ts
Show resolved
Hide resolved
github-actions
bot
removed
the
New Browser Test Expectations
Summary
Replicate the alt+drag to clone behavior present in litegraph.
Changes
Screenshots (if applicable)
Screen.Recording.2025-11-20.215637.mp4
┆Issue is synchronized with this Notion page by Unito