DO NOT SUBMIT - perf: virtualize Vue nodes outside the viewport - #15030
Draft
benjcooley wants to merge 33 commits into
Draft
DO NOT SUBMIT - perf: virtualize Vue nodes outside the viewport#15030benjcooley wants to merge 33 commits into
benjcooley wants to merge 33 commits into
Conversation
dosubot
Bot
added
the
size:L
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded viewport culling for Vue graph nodes with QuadTree indexing, reactive mounting, live-state retention, delayed unmounting, link culling, and a settings bypass. Layout versions now ignore unchanged bounds. Drag cleanup now respects drag ownership. ChangesViewport culling
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟡 Moderate · up to The change virtualizes Vue graph nodes by mounting only nodes near the viewport, but delayed pruning can still synchronously admit many nodes and defeat the per-refresh mount cap, causing frame-time spikes on large graphs; the culling root also uses a production test-ID lookup. These bounded runtime risks should be fixed or explicitly accepted before merge. Sequence Diagram(s)sequenceDiagram
participant GraphCanvas
participant useViewportCulling
participant nodeCullingIndex
participant VueNodeRenderer
GraphCanvas->>useViewportCulling: provide node bounds and viewport state
useViewportCulling->>nodeCullingIndex: query expanded viewport bounds
nodeCullingIndex-->>useViewportCulling: return intersecting node IDs
useViewportCulling-->>GraphCanvas: update mounted node IDs
GraphCanvas->>VueNodeRenderer: render mounted Vue nodes
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (6 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
|
✅ All contributors have signed the CLA. Thank you! This PR is ready to be merged. |
🎨 Storybook: ✅ Built — View Storybook🎭 Playwright: ✅ 1831 passed, 0 failed · 2 flaky📊 Browser Reports
📦 Bundle: 8.86 MB gzip 🔴 +7.13 kBDetailsSummary
Category Glance App Entry Points — 3.71 kB (baseline 3.71 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.39 MB (baseline 1.37 MB) • 🔴 +17.4 kBGraph editor runtime, canvas, workflow orchestration
Status: 2 added / 2 removed / 1 unchanged Views & Navigation — 124 kB (baseline 124 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 16 added / 16 removed / 1 unchanged Panels & Settings — 566 kB (baseline 566 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 11 added / 11 removed / 15 unchanged User & Accounts — 27.7 kB (baseline 27.7 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 8 added / 8 removed / 3 unchanged Editors & Dialogs — 125 kB (baseline 125 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 8 added / 8 removed UI Components — 67.1 kB (baseline 67.1 kB) • 🔴 +2 BReusable component library chunks
Status: 14 added / 14 removed Data & Services — 3.53 MB (baseline 3.52 MB) • 🔴 +9.65 kBStores, services, APIs, and repositories
Status: 17 added / 17 removed Utilities & Hooks — 550 kB (baseline 550 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 28 added / 28 removed / 10 unchanged Vendor & Third-Party — 16.8 MB (baseline 16.8 MB) • 🔴 +19 BExternal libraries and shared vendor chunks
Status: 6 added / 6 removed / 12 unchanged Other — 14.2 MB (baseline 14.2 MB) • 🔴 +14 BBundles that do not match a named category
Status: 113 added / 113 removed / 172 unchanged ⚡ Performance
|
This was referenced Aug 11, 2026
Contributor
Author
|
I have read and agree to the Contributor License Agreement |
Contributor
Author
|
recheck |
benjcooley
force-pushed
the
benjcooley/vue-nodes-viewport-culling
branch
3 times, most recently
from
August 11, 2026 05:37
755e64e to
dc16632
Compare
christian-byrne
added
preview-cpu
christian-byrne
left a comment
christian-byrne
left a comment
Contributor
There was a problem hiding this comment.
Reviewed with a multi-agent pass (12 reviewer profiles, mutation-based red/green verification in an isolated worktree, and a cross-model adversarial pass). Findings were deduped and then re-derived against the code at head by an independent verification stage; 17 dropped as unverifiable, wrong, or pre-existing. 17 survive.
Submitting as COMMENT rather than REQUEST_CHANGES because you own the merge call, but flagging explicitly so it does not get lost in the list: C1 is blocking severity. Unmounting any node clears the process-global layoutStore.isDraggingVueNodes, and useNodeDrag is a shared singleton that nulls the drag origin when that flag drops. Before this PR a node only unmounted on deletion or graph swap, so the hook could not run mid-gesture; culling makes unmount camera-driven, and the camera moves during a drag because auto-pan runs. Pinning does not protect against it, because the flag is cleared by the unmount of a different node.
Credit where the review confirmed things rather than found them, so you know what was checked and cleared: the getCullingBounds sign convention is correct against TransformPane, NodeId key types are consistent throughout, the empty-graph and single-node root sizing is fine, the camera is correctly kept out of the node list's reactive graph, and the useThrottleFn(() => fn, ms) inertness trap that made the #6209 culling a no-op is not repeated here. Mutation testing killed 6 of 7 mutants, and both red/green claims in your description hold: deleting the eager-prune branch fails the pan test with expected 60 to be less than 20, and swapping the graph-extent root for QUADTREE_CONFIG.DEFAULT_BOUNDS fails the out-of-bounds test.
Two process notes rather than code findings:
- The 0.02% pixel-parity claim is the entire justification for the change being safe, and nothing in the repo enforces it. There is no
@perftest and no visual-parity check, so both the correctness claim and the speed claim can regress silently..claude/skills/perf-fix-with-proof/SKILL.mddescribes the repo's convention here (baseline test lands first, in its own PR). - Worth knowing before you invest more in this branch: #14259 is an open draft implementing viewport virtualization, low-zoom LOD, and the FE-1557 slot-geometry-across-unmount work in one PR, and it conflicts with this on the same
v-forline. It is setting-gated where this is always-on. Someone should pick before either lands.
benjcooley
force-pushed
the
benjcooley/minimap-large-graph-perf
branch
from
August 14, 2026 00:02
fda0bb1 to
37e86ce
Compare
benjcooley
force-pushed
the
benjcooley/vue-nodes-viewport-culling
branch
from
August 14, 2026 00:02
dc16632 to
6be9d69
Compare
github-actions
Bot
added
the
risk:R3
benjcooley
force-pushed
the
benjcooley/minimap-large-graph-perf
branch
from
August 14, 2026 00:33
37e86ce to
34433df
Compare
Contributor
Author
|
Split the repository-wide |
benjcooley
added
the
New Browser Test Expectations
github-actions
Bot
removed
the
New Browser Test Expectations
…ulling-keepalive-fallback
benjcooley
enabled auto-merge
August 18, 2026 01:04
benjcooley
added
the
New Browser Test Expectations
github-actions
Bot
removed
the
New Browser Test Expectations
benjcooley
changed the title
benjcooley
disabled auto-merge
August 18, 2026 17:47
Contributor
Author
|
I am going to replace this PR with a vastly simpler KEEPALIVE only version which basically does almost the same thing. We can discuss wether to keep this version, but after line by line reviewing by human eyeballs, this is a lot of code. |
benjcooley
marked this pull request as draft
August 18, 2026 17:59
claude Bot
pushed a commit
that referenced
this pull request
Aug 21, 2026
…cture #15030 (viewport culling) was retired and replaced by #15405, which ships KeepAlive-only viewport retention (Comfy.VueNodes.ViewportKeepAlive, default on) with no destructive-unmount mode. Rebase this benchmark onto #15405's branch and drive it off that setting directly instead of the deleted useViewportCulling.ts and the now-nonexistent Comfy.VueNodes.ViewportCullingKeepAlive setting this PR had added. - Drop the Comfy.VueNodes.ViewportCullingKeepAlive setting and the GraphCanvas.vue KeepAlive branch that pointed at it - #15405's GraphCanvas already renders through KeepAlive unconditionally, gated by Comfy.VueNodes.ViewportKeepAlive. - Rewrite viewportCullingBenchmark.spec.ts as a two-way KeepAlive on/off comparison (no third "destroy" variant, since one no longer exists). - Drop the mount-admission/departure debounce-settle helpers; KeepAlive activation is one throttled refresh (REFRESH_THROTTLE_MS = 100ms in useViewportKeepAlive.ts), so settling is a single idleFrames wait.
christian-byrne
left a comment
christian-byrne
left a comment
Contributor
There was a problem hiding this comment.
@DrJKL — this PR is superseded by #15405 but still carries 7 unresolved threads, all yours. Since closing it would dispose of them silently, here is where each one actually ended up. I verified all of these at the replacement heads rather than reading the replies.
| Your blocker | Where it lives now |
|---|---|
| Extension compatibility — no opt-out for extension-owned DOM | Restored in #15405 as extensionManager.vueNodes.registerCullingOptOut(nodeType), ref-counted, same store and extensionTypes surface. One deliberate semantic change: it is now admission, not retention — a registered type stays attached wherever it is, rather than only being retained if already attached. That is the right call under KeepAlive (the state it protects is exactly what detachment destroys) but it is your blocker, so worth your confirmation. |
Second QuadTree duplicating layoutStore |
nodeCullingIndex.ts stays deleted; consumers go through layoutStore.queryNodesInBounds(). The SpatialIndexManager hardening was dropped by the rebase onto #15405 and is now restored on #15031 — unindexableEntries, isIndexable, if (!indexed) rebuild(), batched rebuild, and a fail-open query. Regression test inserts at x: 50_000, outside the ±10000 root. |
useViewportCulling implicit state machine |
Moot — the composable and its four-file split are gone. #15405's KeepAlive path is ~165 lines with one throttle and no admission scheduler, so the state machine you objected to no longer exists rather than having been reorganised. |
| Pin-set hash collisions | Carried over. Pin membership is an explicit reactive Set; the JSON.stringify membership key that replaced the fingerprint is also gone now. |
| Touch-pan golden | Fixed on main in #15356; neither successor changes the golden. |
| LGraphCanvas link fast-rejection | Not present on #15405 or #15031 — that optimisation did not come across. Nothing to confirm, but flagging it since your thread was the only record of it. |
| Slot-sync readiness ownership | Reworked on #15031; the clear is gated on isLowQuality and the re-arm on wasLow && !low, so the release-before-measure case and the cold-start self-undo are both closed. |
The general point, since it cost real coverage here: a fix that closed a thread on a retired PR is not a fix that shipped. Two of the above were silently reverted by the restructure — the opt-out API and the SpatialIndex hardening — and neither would have been caught by reviewing the replacements against main. Both are now back with tests.
I have approved #15405 and #15031 with one condition: they land together, or #15031 immediately behind, because #15405 alone has no bound on the activation set at low zoom. That is the answer to your "would it be better to get both it and the successor in together?" — yes.
Suggest closing this PR once you have looked at the opt-out semantics on #15405. You are assigned there.
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
3 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
Large Nodes 2.0 workflows kept every node mounted in the live DOM. This virtualizes nodes against an expanded viewport while preserving graph/layout state independently of renderer attachment.
Changes
layoutStorespatial index; remote and local geometry updates feed the same derived index.Comfy.VueNodes.ViewportCullingkey, but make it choose teardown strategy rather than disable virtualization:KeepAlive.There is no public all-nodes-live mode. For graphs below the 150-node threshold, virtualization remains dormant because its overhead is not useful.
Performance
On the 245-node production fixture, viewport virtualization reduced DOM-range pan/zoom interaction time across Chromium, Firefox, and WebKit. Median speedups ranged from 2.3× to 14.3× versus keeping every node live. The same benchmark stayed above #15031's LOD threshold; stacking #15031 introduced no measurable regression.
KeepAliveand destructive unmounting were in the same performance tier. This indicates the main gain comes from removing offscreen trees from active DOM layout/rendering, while the setting provides a legitimate memory-versus-lifecycle tradeoff.Validation
KeepAlivenode leaves the live DOM and returns as the same element.Split from #15016 (part 2 of 3). #15031 adds LOD rendering below its zoom threshold.