feat: Vue node viewport virtualization and low-zoom level of detail to reduce DOM and rendering work in large Nodes 2.0 workflows - #14119
Conversation
🎨 Storybook: 🚧 Building...🎭 Playwright: ⏳ Running... |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughVue node rendering now supports opt-in viewport virtualization, interaction-aware hydration, persistent slot layouts, zoom-based low-detail rendering, compact collapsed-node presentation, and related unit and browser tests. ChangesVue node rendering
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GraphCanvas
participant useViewportVirtualization
participant LGraphNode
participant Canvas
GraphCanvas->>useViewportVirtualization: pass graph nodes and virtualization setting
useViewportVirtualization-->>GraphCanvas: return renderedNodes
GraphCanvas->>LGraphNode: mount rendered node
LGraphNode->>useViewportVirtualization: emit node-mounted(nodeId)
Canvas->>useViewportVirtualization: report settled transform
useViewportVirtualization-->>GraphCanvas: update viewport node set
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@browser_tests/tests/vueNodes/viewportVirtualization.spec.ts`:
- Around line 120-122: Update the virtualization test around vueNodes.getNodeIds
so it waits for the 150 ms settle side-effect, such as the offset becoming
settled, before asserting the node IDs once. Remove the immediate expect.poll on
getNodeIds and preserve the expected [nodeId] assertion after the settle window
to verify the mount set remains frozen.
In `@src/renderer/extensions/vueNodes/composables/useSlotElementTracking.test.ts`:
- Around line 373-402: Add an assertion to the test case “uses a matching node
container when a foreign slot entry comes first” verifying that
layoutStore.getSlotLayout(foreignSlotKey) is undefined or otherwise absent after
syncNodeSlotLayoutsFromDOM(NODE_ID), while preserving the existing matching-slot
position assertion.
In `@src/renderer/extensions/vueNodes/composables/useSlotElementTracking.ts`:
- Around line 370-374: The virtualization-clearing flow must reconcile retained
entries whose elements were unset, rather than leaving their stopWatch and
cached layouts active after clearViewportVirtualizedNodeIds(). Update the
relevant useSlotElementTracking logic to prune entries no longer present in
virtualizedNodeIds or expand them through the normal synchronization path, while
preserving active entries and preventing stale updateNodeSlotsFromCache calls
without DOM elements.
In
`@src/renderer/extensions/vueNodes/composables/useViewportVirtualization.test.ts`:
- Around line 21-51: Update the hoisted useRafFn mock so its returned isActive
is a reactive Ref<boolean>, such as a shallowRef, rather than a plain object
with a one-time value. Keep rafWatcher.pause and rafWatcher.resume synchronized
with that ref when toggling active state, preserving the existing immediate
initialization behavior.
- Around line 111-163: Expand the virtualization tests around
useViewportVirtualization to assert the published isNodeViewportVirtualized
state after the queued refresh settles, including the focused node’s virtualized
status. Add enabled/disabled boundary coverage verifying the state is cleared
when virtualization is disabled, and assert it is also cleared after scope
disposal; exercise the existing replaceViewportVirtualizedNodeIds publication
path rather than only checking renderedNodes.
In `@src/renderer/extensions/vueNodes/composables/useViewportVirtualization.ts`:
- Around line 147-158: Ensure mutations to hydratedNodeIds and
pendingHydrationNodeIds in the watcher also invalidate renderedNodes by
incrementing hydrationVersion after the deletions. Preserve the existing void
hydrationVersion.value dependency in renderedNodes and verify every mutation
path updates the version.
- Around line 302-321: Update the disabled branch of the enabled-state watcher
to cancel any pending refreshFrame and hydrationFrame callbacks in addition to
settleTimer, resetting their handles consistently with the cleanup performed in
onScopeDispose before rebuilding the full viewport node set.
- Around line 132-134: Normalize link.node.id before adding it to result in the
activeCanvas renderLinks branch, matching the existing capturingNode.id handling
in the surrounding viewport virtualization logic. Preserve the null check and
ensure numeric IDs are converted to the same string representation used by the
protection lookup.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2e6578f6-8af4-4821-af13-f95f5d609a28
📒 Files selected for processing (17)
browser_tests/fixtures/ComfyPage.tsbrowser_tests/fixtures/VueNodeHelpers.tsbrowser_tests/tests/vueNodes/viewportVirtualization.spec.tssrc/assets/css/style.csssrc/components/graph/GraphCanvas.vuesrc/composables/graph/useGraphNodeManager.tssrc/platform/settings/constants/coreSettings.tssrc/renderer/extensions/vueNodes/components/LGraphNode.vuesrc/renderer/extensions/vueNodes/composables/useSlotElementTracking.test.tssrc/renderer/extensions/vueNodes/composables/useSlotElementTracking.tssrc/renderer/extensions/vueNodes/composables/useViewportVirtualization.test.tssrc/renderer/extensions/vueNodes/composables/useViewportVirtualization.tssrc/renderer/extensions/vueNodes/composables/useVueNodeLOD.test.tssrc/renderer/extensions/vueNodes/composables/useVueNodeLOD.tssrc/renderer/extensions/vueNodes/composables/viewportVirtualizationState.tssrc/renderer/extensions/vueNodes/stores/nodeSlotRegistryStore.tssrc/schemas/apiSchema.ts
ecbae00 to
e16ce89
Compare
|
@r-vage is attempting to deploy a commit to the comfy product design Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/assets/css/style.css`:
- Around line 19-55: Scope the `.dom-widget` selector in the
`html.vue-nodes-low-detail` rule to `
.lg-node[data-node-id]:not([data-node-id^='preview-'])`, matching the adjacent
node-scoped selectors. Preserve the existing hidden and non-interactive behavior
only for non-preview Vue node widgets, not unrelated document-wide widgets.
In `@src/platform/settings/constants/coreSettings.ts`:
- Around line 1204-1244: Update the settings entries identified by
Comfy.VueNodes.ViewportVirtualization, Comfy.VueNodes.LowZoomLOD, and
Comfy.VueNodes.FullDetailZoom to use vue-i18n keys instead of inline English
name and tooltip literals. Add the corresponding English translations to
src/locales/en/main.json and resolve them through the repository’s existing i18n
path, preserving the current wording and setting behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 212ec936-edc7-4a0b-a458-0bb878545c0c
📒 Files selected for processing (17)
browser_tests/fixtures/ComfyPage.tsbrowser_tests/fixtures/VueNodeHelpers.tsbrowser_tests/tests/vueNodes/viewportVirtualization.spec.tssrc/assets/css/style.csssrc/components/graph/GraphCanvas.vuesrc/composables/graph/useGraphNodeManager.tssrc/platform/settings/constants/coreSettings.tssrc/renderer/extensions/vueNodes/components/LGraphNode.vuesrc/renderer/extensions/vueNodes/composables/useSlotElementTracking.test.tssrc/renderer/extensions/vueNodes/composables/useSlotElementTracking.tssrc/renderer/extensions/vueNodes/composables/useViewportVirtualization.test.tssrc/renderer/extensions/vueNodes/composables/useViewportVirtualization.tssrc/renderer/extensions/vueNodes/composables/useVueNodeLOD.test.tssrc/renderer/extensions/vueNodes/composables/useVueNodeLOD.tssrc/renderer/extensions/vueNodes/composables/viewportVirtualizationState.tssrc/renderer/extensions/vueNodes/stores/nodeSlotRegistryStore.tssrc/schemas/apiSchema.ts
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/extensions/vueNodes/composables/useVueNodeLOD.test.ts`:
- Around line 5-21: Add assertions to the shouldUseVueNodeLowDetail tests
covering NaN, Infinity, and -Infinity canvas scale inputs, verifying each
returns false even when both settings are enabled. Keep the existing threshold
and settings coverage unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ea065018-ad8f-4079-b37e-c4a07f75d1f4
📒 Files selected for processing (17)
browser_tests/fixtures/ComfyPage.tsbrowser_tests/fixtures/VueNodeHelpers.tsbrowser_tests/tests/vueNodes/viewportVirtualization.spec.tssrc/assets/css/style.csssrc/components/graph/GraphCanvas.vuesrc/composables/graph/useGraphNodeManager.tssrc/platform/settings/constants/coreSettings.tssrc/renderer/extensions/vueNodes/components/LGraphNode.vuesrc/renderer/extensions/vueNodes/composables/useSlotElementTracking.test.tssrc/renderer/extensions/vueNodes/composables/useSlotElementTracking.tssrc/renderer/extensions/vueNodes/composables/useViewportVirtualization.test.tssrc/renderer/extensions/vueNodes/composables/useViewportVirtualization.tssrc/renderer/extensions/vueNodes/composables/useVueNodeLOD.test.tssrc/renderer/extensions/vueNodes/composables/useVueNodeLOD.tssrc/renderer/extensions/vueNodes/composables/viewportVirtualizationState.tssrc/renderer/extensions/vueNodes/stores/nodeSlotRegistryStore.tssrc/schemas/apiSchema.ts
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 3 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@browser_tests/tests/vueNodes/viewportVirtualization.spec.ts`:
- Around line 87-90: Update the viewport virtualization test around the offset
polling and getNodeIds assertion to wait beyond the 150 ms settle delay before
sampling node IDs. Follow the existing group-drag test’s page.clock-based timing
approach, then assert the frozen node set after the delayed unmount window.
In
`@src/renderer/extensions/vueNodes/composables/useViewportVirtualization.test.ts`:
- Around line 128-139: Clear the shared virtualized-node-id module state in the
beforeEach setup of the viewport virtualization test suite, using the same reset
approach as useSlotElementTracking.test.ts. Ensure each test starts with an
empty viewportVirtualizationState Set, independent of whether a prior test
reached scope.stop(), while preserving the existing rafWatcher and timer
cleanup.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2ea782bf-0b43-4e0c-b9b2-9b674e08fc5c
📒 Files selected for processing (17)
browser_tests/fixtures/ComfyPage.tsbrowser_tests/fixtures/VueNodeHelpers.tsbrowser_tests/tests/vueNodes/viewportVirtualization.spec.tssrc/assets/css/style.csssrc/components/graph/GraphCanvas.vuesrc/composables/graph/useGraphNodeManager.tssrc/platform/settings/constants/coreSettings.tssrc/renderer/extensions/vueNodes/components/LGraphNode.vuesrc/renderer/extensions/vueNodes/composables/useSlotElementTracking.test.tssrc/renderer/extensions/vueNodes/composables/useSlotElementTracking.tssrc/renderer/extensions/vueNodes/composables/useViewportVirtualization.test.tssrc/renderer/extensions/vueNodes/composables/useViewportVirtualization.tssrc/renderer/extensions/vueNodes/composables/useVueNodeLOD.test.tssrc/renderer/extensions/vueNodes/composables/useVueNodeLOD.tssrc/renderer/extensions/vueNodes/composables/viewportVirtualizationState.tssrc/renderer/extensions/vueNodes/stores/nodeSlotRegistryStore.tssrc/schemas/apiSchema.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@browser_tests/tests/creditHelpers.spec.ts`:
- Around line 256-258: Replace both icon-based pricing locators in
browser_tests/tests/creditHelpers.spec.ts at lines 256-258 and 328-330 with a
header-scoped exact text locator for “10.6”, using header.getByText; do not
depend on CreditBadge icon CSS classes.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b6699c79-ce5e-445e-b5a3-10d834c880b9
📒 Files selected for processing (8)
browser_tests/fixtures/ComfyPage.tsbrowser_tests/tests/creditHelpers.spec.tssrc/locales/en/settings.jsonsrc/platform/settings/constants/coreSettings.tssrc/renderer/extensions/vueNodes/components/LGraphNode.vuesrc/renderer/extensions/vueNodes/components/NodeHeader.test.tssrc/renderer/extensions/vueNodes/components/NodeHeader.vuesrc/schemas/apiSchema.ts
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/renderer/extensions/vueNodes/composables/useViewportVirtualization.test.ts (1)
128-133: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReset the shared virtualized-node-id module state in
beforeEach.
viewportVirtualizationStateholds a module-levelSetthat outlives each test; it is only cleared as a side effect ofscope.stop()on the happy path, so any test that throws earlier leaks membership into the next test'sisNodeViewportVirtualizedassertions. The sibling suite (useSlotElementTracking.test.tsLine 142) already callsclearViewportVirtualizedNodeIds()here.As per path instructions,
.agents/checks/test-quality.mdrequires ensuring strong isolation (no shared mutable state).💚 Proposed fix
+import { clearViewportVirtualizedNodeIds } from './viewportVirtualizationState' + describe('viewport virtualization behavior', () => { beforeEach(() => { + clearViewportVirtualizedNodeIds() rafWatcher.callback = undefined🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/extensions/vueNodes/composables/useViewportVirtualization.test.ts` around lines 128 - 133, Update the test setup beforeEach to reset the module-level virtualized-node ID state by calling clearViewportVirtualizedNodeIds(), alongside the existing rafWatcher resets. Ensure each test starts with an empty viewport virtualization set, including when a prior test exits before scope.stop().Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/extensions/vueNodes/composables/useViewportVirtualization.ts`:
- Around line 101-136: Move protected-node computation out of the unconditional
per-frame path in refreshProtectedNodeIds and cache the result. Refresh the
cache only from the existing relevant event listeners and watches for canvas
interaction state, canvasStore.selectedNodeIds,
titleEditorStore.titleEditorTarget, focused/capturing nodes, and linkDragState;
keep the RAF loop limited to transform comparison and applying the cached set,
while preserving protection updates whenever these inputs change.
- Around line 286-288: Update the watcher around allNodes in
useViewportVirtualization to stop using JSON.stringify as its reactive key. Use
a cheaper node-id comparison mechanism, such as a stable comparable key or an
areNodeIdSetsEqual guard inside the callback, so unrelated node updates do not
allocate and compare a serialized full id list on every re-evaluation.
---
Duplicate comments:
In
`@src/renderer/extensions/vueNodes/composables/useViewportVirtualization.test.ts`:
- Around line 128-133: Update the test setup beforeEach to reset the
module-level virtualized-node ID state by calling
clearViewportVirtualizedNodeIds(), alongside the existing rafWatcher resets.
Ensure each test starts with an empty viewport virtualization set, including
when a prior test exits before scope.stop().
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e4ea1e5f-f2f5-4c65-8552-e960e5bdadc5
📒 Files selected for processing (21)
browser_tests/fixtures/ComfyPage.tsbrowser_tests/fixtures/VueNodeHelpers.tsbrowser_tests/tests/creditHelpers.spec.tsbrowser_tests/tests/vueNodes/viewportVirtualization.spec.tssrc/assets/css/style.csssrc/components/graph/GraphCanvas.vuesrc/composables/graph/useGraphNodeManager.tssrc/locales/en/settings.jsonsrc/platform/settings/constants/coreSettings.tssrc/renderer/extensions/vueNodes/components/LGraphNode.vuesrc/renderer/extensions/vueNodes/components/NodeHeader.test.tssrc/renderer/extensions/vueNodes/components/NodeHeader.vuesrc/renderer/extensions/vueNodes/composables/useSlotElementTracking.test.tssrc/renderer/extensions/vueNodes/composables/useSlotElementTracking.tssrc/renderer/extensions/vueNodes/composables/useViewportVirtualization.test.tssrc/renderer/extensions/vueNodes/composables/useViewportVirtualization.tssrc/renderer/extensions/vueNodes/composables/useVueNodeLOD.test.tssrc/renderer/extensions/vueNodes/composables/useVueNodeLOD.tssrc/renderer/extensions/vueNodes/composables/viewportVirtualizationState.tssrc/renderer/extensions/vueNodes/stores/nodeSlotRegistryStore.tssrc/schemas/apiSchema.ts
2965d32 to
adb9bbb
Compare
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/renderer/extensions/vueNodes/composables/useViewportVirtualization.test.ts`:
- Around line 338-368: Replace the animationFrames.size assertions in the “does
not reconcile node ids when only node data changes” test with an observable
virtualization outcome after updating title. Avoid asserting RAF scheduling or
mock state; verify the public node virtualization behavior remains unchanged
while preserving the existing setup and title update flow.
In `@src/renderer/extensions/vueNodes/composables/useVueNodeLOD.test.ts`:
- Around line 52-74: Update the test around useVueNodeLOD to assert the
observable low-detail class or rendered LOD state remains correct across
consecutive RAF callbacks with an unchanged NaN scale, rather than asserting
fullDetailZoom call counts. If no stable public behavior can be asserted, remove
this implementation-detail test.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f5845113-b428-4069-a420-90143ea69b67
📒 Files selected for processing (7)
browser_tests/tests/vueNodes/viewportVirtualization.spec.tssrc/locales/en/settings.jsonsrc/platform/settings/constants/coreSettings.tssrc/renderer/extensions/vueNodes/composables/useViewportVirtualization.test.tssrc/renderer/extensions/vueNodes/composables/useViewportVirtualization.tssrc/renderer/extensions/vueNodes/composables/useVueNodeLOD.test.tssrc/renderer/extensions/vueNodes/composables/useVueNodeLOD.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/extensions/vueNodes/composables/useVueNodeLOD.test.ts`:
- Around line 56-75: Update the test’s effectScope lifecycle around
useVueNodeLOD by wrapping the callback assertions in try/finally and calling
scope.stop() in the finally block. Preserve the existing assertions while
ensuring watcher cleanup and DOM state reset even when the test fails.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6a513889-5945-4bf6-af43-ac85e446472d
📒 Files selected for processing (2)
src/renderer/extensions/vueNodes/composables/useViewportVirtualization.test.tssrc/renderer/extensions/vueNodes/composables/useVueNodeLOD.test.ts
Summary
Adds experimental Vue node viewport virtualization and low-zoom level of detail to reduce DOM and rendering work in large Nodes 2.0 workflows while preserving layout and active interactions.
Changes
Review Focus
CodeRabbit follow-up review completed with no remaining findings.
Testing
pnpm typecheckpnpm typecheck:browserbrowser_tests/tests/vueNodes/viewportVirtualization.spec.ts