perf: avoid per-frame reactive mutations in DomWidgets positioning - #15684
Open
christian-byrne wants to merge 5 commits into
Open
perf: avoid per-frame reactive mutations in DomWidgets positioning#15684christian-byrne wants to merge 5 commits into
christian-byrne wants to merge 5 commits into
Conversation
Eliminates unconditional 60fps reactive writes that triggered Vue's
dependency tracking on every draw frame, causing cascading re-renders.
DomWidgets.vue:
- Track viewport (ds.offset/scale) and selected node bounds between
frames. Only write widgetState.pos when the value actually changed,
but force reassignment when viewport pans or selected node moves
(needed because ds.offset is non-reactive — downstream watchers
won't fire unless pos gets a new array identity).
- Guard all per-frame writes (pos, size, zIndex, readonly,
computedDisabled) with equality checks.
- Snapshot widget.computedDisabled into widgetState each frame so
DomWidget.vue can watch a reactive property instead of reading the
non-reactive litegraph field directly.
DomWidget.vue:
- Replace the { deep: true } watcher over the entire widgetState object
with two focused watchers: one for pos/size/visible (calls
updatePosition) and one for zIndex/readonly/computedDisabled/
enableDomClipping (calls composeStyle only).
- Read widgetState.computedDisabled instead of widget.computedDisabled
in composeStyle() so the watcher can fire reactively.
domWidgetStore.ts:
- Add computedDisabled: boolean to DomWidgetState, initialized false.
…riant test Address @AustinMroz's style note: replace 7 bare module-level `let` variables with two plain objects (`lastViewport`, `lastSelected`), making the per-frame snapshot state easier to read and extend. Add a reactive-write budget test that directly validates the perf claim: instrument the `pos` setter on widgetState and assert zero writes across 20 idle frames (canvas and nodes both stationary).
Object.defineProperty setter interception on a Pinia reactive proxy does not reliably intercept Vue's internal reactive writes. The same invariant (no pos writes on idle frames) is correctly captured by checking that the pos array reference is unchanged after N idle frames — identical to the existing passing test for the same property. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dosubot
Bot
added
the
size:L
Contributor
|
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 (5)
Included review availability: 8 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 10 reviews per hour. 📝 WalkthroughWalkthroughChangesThe update synchronizes DOM widgets with viewport transforms, selected-node geometry, and computed-disabled state. It also replaces the deep widget-state watcher with targeted watchers and adds coverage for reassignment and idle-frame behavior. DOM widget synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change reduces redundant per-frame widget updates and makes disabled-state styling reactive; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
🎨 Storybook: ✅ Built — View Storybook🎭 Playwright: 🕵🏻 0 passed, 0 failed📊 Browser Reports
📦 Bundle Size
⚡ Performance
|
github-actions
Bot
added
the
risk:R2
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
4 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
Problem:
DomWidgets.vuewas mutatingwidgetState.pos,size,zIndex,readonly, andcomputedDisabledon everycanvas.onDrawForegroundcall (~60fps), even when the values hadn't changed. Each write triggered{ deep: true }watchers inDomWidget.vue, cascading into style recalculations and DOM mutations across all visible widgets every frame.Root cause (pos/size): Equality wasn't checked before assignment, so the array was replaced every frame regardless of whether node position changed.
Root cause (watcher): A single
{ deep: true }watcher on all ofwidgetStatefired on any property mutation, even ones that don't affect layout.Root cause (computedDisabled):
DomWidget.vuewas readingwidget.computedDisableddirectly — a non-reactive litegraph property — so Vue watchers never observed changes.Changes
DomWidgets.vueposarray reassignment only when these change (needed becauseds.offset/ds.scaleandnode.posare non-reactive — a new array identity is the only signal Vue watchers can observe)lettracking variables into two plain objects (lastViewport,lastSelected)widget.computedDisabled→widgetState.computedDisabledeach frame so the reactive store reflects the non-reactive litegraph propertyDomWidget.vue{ deep: true }watcher into two focused watchers: one for layout (pos/size/visible) and one for appearance (zIndex/readonly/computedDisabled)widgetState.computedDisabled(reactive) instead ofwidget.computedDisabled(non-reactive)domWidgetStore.tscomputedDisabled: booleantoDomWidgetStatewith JSDoc explaining the snapshot patternTests
DomWidgets.test.ts: 6 behavioral tests covering positioning, visibility, viewport pan, idle-frame identity preservation, selected-node movement, and computedDisabled mirroringDomWidget.test.ts: 2 tests covering disabled style and pointer-events when not visible; updatedcreateWidgetStateto set the snapshot field directly (no draw loop in unit tests)Test plan
pnpm vitest run src/components/graph/DomWidgets.test.ts— 8 tests passpnpm vitest run src/components/graph/widgets/DomWidget.test.ts— 2 tests passcomputedDisabledstyling: connect an input to a widget input; widget should go 50% opacity with pointer-events disabled🤖 Generated with Claude Code