feat: centralize node image rendering state in NodeImageStore#9435
Open
christian-byrne wants to merge 5 commits intomainfrom
Open
feat: centralize node image rendering state in NodeImageStore#9435christian-byrne wants to merge 5 commits intomainfrom
christian-byrne wants to merge 5 commits intomainfrom
Conversation
dosubot
bot
added
the
size:L
🎨 Storybook: ✅ Built — View Storybook |
🎭 Playwright: ✅ 549 passed, 0 failed · 6 flaky📊 Browser Reports
|
Contributor
📝 WalkthroughWalkthroughThis PR introduces a centralized Pinia store ( Changes
Sequence DiagramsequenceDiagram
participant App as GraphCanvas
participant Store as nodeImageStore
participant Graph as LGraph
participant Node as LGraphNode
App->>Store: setNodeLocatorResolver(workflowStore.nodeToNodeLocatorId)
App->>Graph: comfyApp.setup()
Graph->>Graph: add(node)
alt Pinia is Active
Graph->>Store: useNodeImageStore()
Graph->>Store: installPropertyProjection(node)
Store->>Node: Object.defineProperty(node, 'imgs', ...)
Store->>Node: Object.defineProperty(node, 'imageIndex', ...)
Store->>Node: Object.defineProperty(node, 'imageRects', ...)
Store->>Node: Object.defineProperty(node, 'pointerDown', ...)
Store->>Node: Object.defineProperty(node, 'overIndex', ...)
end
Node->>Store: node.imgs (getter → store lookup)
Node->>Store: node.imgs = value (setter → store write)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
Comment |
📦 Bundle: 4.59 MB gzip 🔴 +766 BDetailsSummary
Category Glance App Entry Points — 17.7 kB (baseline 17.7 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.01 MB (baseline 1.01 MB) • 🔴 +94 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 72.5 kB (baseline 72.5 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 11 added / 11 removed Panels & Settings — 440 kB (baseline 440 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed User & Accounts — 16.1 kB (baseline 16.1 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 7 added / 7 removed Editors & Dialogs — 78.3 kB (baseline 78.3 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 56.7 kB (baseline 56.7 kB) • ⚪ 0 BReusable component library chunks
Status: 13 added / 13 removed Data & Services — 2.8 MB (baseline 2.79 MB) • 🔴 +2.58 kBStores, services, APIs, and repositories
Status: 16 added / 16 removed Utilities & Hooks — 57.2 kB (baseline 57.2 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 14 added / 14 removed Vendor & Third-Party — 8.9 MB (baseline 8.9 MB) • 🔴 +112 BExternal libraries and shared vendor chunks
Status: 6 added / 6 removed Other — 8.04 MB (baseline 8.04 MB) • ⚪ 0 BBundles that do not match a named category
Status: 84 added / 84 removed |
⚡ Performance Report
Raw data{
"timestamp": "2026-03-12T16:06:04.330Z",
"gitSha": "aba7e9fa5bb049113aa106c5f7fbb10d0949ab9a",
"branch": "refactor/node-output-store-centralization",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2048.778000000084,
"styleRecalcs": 10,
"styleRecalcDurationMs": 7.905,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 410.607,
"heapDeltaBytes": -5495720
},
{
"name": "canvas-idle",
"durationMs": 2028.5930000000008,
"styleRecalcs": 11,
"styleRecalcDurationMs": 8.787999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 357.379,
"heapDeltaBytes": 338700
},
{
"name": "canvas-idle",
"durationMs": 2034.7590000000082,
"styleRecalcs": 11,
"styleRecalcDurationMs": 8.751999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 349.754,
"heapDeltaBytes": 1069568
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1928.162000000043,
"styleRecalcs": 81,
"styleRecalcDurationMs": 39.937,
"layouts": 12,
"layoutDurationMs": 3.509,
"taskDurationMs": 894.931,
"heapDeltaBytes": 1248588
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1763.9580000000024,
"styleRecalcs": 74,
"styleRecalcDurationMs": 34.53399999999999,
"layouts": 12,
"layoutDurationMs": 3.3400000000000003,
"taskDurationMs": 729.9780000000001,
"heapDeltaBytes": -1047360
},
{
"name": "canvas-mouse-sweep",
"durationMs": 2013.2160000000567,
"styleRecalcs": 81,
"styleRecalcDurationMs": 39.737,
"layouts": 12,
"layoutDurationMs": 3.08,
"taskDurationMs": 954.5199999999999,
"heapDeltaBytes": 1294904
},
{
"name": "dom-widget-clipping",
"durationMs": 574.3039999999837,
"styleRecalcs": 13,
"styleRecalcDurationMs": 7.592999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 343.812,
"heapDeltaBytes": 12003336
},
{
"name": "dom-widget-clipping",
"durationMs": 586.0989999999902,
"styleRecalcs": 14,
"styleRecalcDurationMs": 8.341999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 338.77599999999995,
"heapDeltaBytes": 12882616
},
{
"name": "dom-widget-clipping",
"durationMs": 581.0229999999592,
"styleRecalcs": 13,
"styleRecalcDurationMs": 10.819,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 334.08000000000004,
"heapDeltaBytes": 12322204
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 592.7369999999428,
"styleRecalcs": 49,
"styleRecalcDurationMs": 12.126999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 387.564,
"heapDeltaBytes": -3284960
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 619.2350000000033,
"styleRecalcs": 52,
"styleRecalcDurationMs": 16.381,
"layouts": 1,
"layoutDurationMs": 0.289,
"taskDurationMs": 421.09200000000004,
"heapDeltaBytes": -2226556
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 627.3579999999583,
"styleRecalcs": 49,
"styleRecalcDurationMs": 11.497999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 390.161,
"heapDeltaBytes": -2199172
},
{
"name": "subgraph-idle",
"durationMs": 2025.3760000000511,
"styleRecalcs": 14,
"styleRecalcDurationMs": 13.746,
"layouts": 1,
"layoutDurationMs": 0.23899999999999996,
"taskDurationMs": 359.056,
"heapDeltaBytes": 832656
},
{
"name": "subgraph-idle",
"durationMs": 1990.7210000000077,
"styleRecalcs": 12,
"styleRecalcDurationMs": 9.848,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 344.202,
"heapDeltaBytes": 763108
},
{
"name": "subgraph-idle",
"durationMs": 2016.1930000000439,
"styleRecalcs": 12,
"styleRecalcDurationMs": 13.022999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 352.33799999999997,
"heapDeltaBytes": 172264
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1721.3339999999562,
"styleRecalcs": 79,
"styleRecalcDurationMs": 41.47,
"layouts": 17,
"layoutDurationMs": 4.489,
"taskDurationMs": 666.034,
"heapDeltaBytes": 278944
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1698.1549999999288,
"styleRecalcs": 76,
"styleRecalcDurationMs": 34.704,
"layouts": 16,
"layoutDurationMs": 4.529,
"taskDurationMs": 645.178,
"heapDeltaBytes": -316948
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1690.1819999999361,
"styleRecalcs": 77,
"styleRecalcDurationMs": 36.038000000000004,
"layouts": 16,
"layoutDurationMs": 4.465999999999999,
"taskDurationMs": 658.418,
"heapDeltaBytes": -548840
}
]
} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/stores/nodeImageStore.ts`:
- Around line 20-30: The bug is that DEFAULT_STATE is a shallow freeze so nested
arrays (imgs, imageRects) are shared and mutable; update the code so getters and
any default returns use a fresh instance from createDefaultState() (or
deep-freeze the nested arrays) instead of returning the single DEFAULT_STATE,
and change DEFAULT_STATE usage to either be removed or replaced with a const
DEFAULT_STATE_FACTORY = createDefaultState and call it where defaults are
returned (affecting DEFAULT_STATE, createDefaultState, and any getter that
currently returns DEFAULT_STATE to avoid node.imageRects.push(...) mutating
shared state).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a7bb672-7418-417f-9725-031be62a1c1b
📒 Files selected for processing (5)
src/components/graph/GraphCanvas.vuesrc/lib/litegraph/src/LGraph.tssrc/stores/imagePreviewStore.tssrc/stores/nodeImageStore.test.tssrc/stores/nodeImageStore.ts
dosubot
bot
added
size:XL
Contributor
Author
|
Addressed CodeRabbit review: DEFAULT_STATE now deep-freezes nested arrays (imgs, imageRects) to prevent shared mutation. Added immutability test. |
christian-byrne
force-pushed
the
refactor/node-output-store-centralization
branch
from
4260c22 to
2552946
Compare
AustinMroz
previously approved these changes
Mar 5, 2026
Collaborator
AustinMroz
left a comment
AustinMroz
left a comment
There was a problem hiding this comment.
Changes look safe, have my approval, and are a step in the right direction.
- None of the store state is reactive.
node.imagesis tightly coupled, but not included with the store.- Off the top of my head, this data is already duplicated across a ~3 different places (node, nodeoutputstore, app.ts?) so it probably requires a lot more work with compatibility layers
- My primary hope with these changes is that the logic for transforming the 'path to an image' to an
image elementwould be handled by the store.- This is the super ugly part where node state is dependent upon receiving draw calls instead of being generated on state access.
- Definitely work for a followup PR
- I think the other store PRs are keying off the now-globally-unique nodeIds. There's some moderate concern for future store access if stores are keyed by different properties.
- Given there's not a performant way to do node lookup without locatorIds, I think this PR is in the right. Any reconciling will occur with the release of the proposed "Node Store"
Introduce useNodeImageStore — a Pinia store keyed by NodeLocatorId that owns imgs, imageIndex, imageRects, pointerDown, and overIndex state. LGraphNode properties delegate to the store via Object.defineProperty getters/setters installed in LGraph.add(), so all existing consumer code (~18 files) continues to read/write node.imgs, node.imageIndex, etc. unchanged. Key design decisions: - Plain Map (not reactive) avoids Vue proxy overhead in the canvas render hot path. - peekState() + frozen DEFAULT_STATE for read-only access prevents unbounded Map growth from getter-only calls. - Module-scoped setNodeLocatorResolver() breaks circular dependency (LGraph → store → workflowStore → app → litegraph). - imgs getter returns undefined when empty to preserve node.imgs?.length optional chaining semantics. Cleanup is wired into removeNodeOutputs (per-node) and resetAllOutputsAndPreviews (bulk clear). Fixes #9242 Amp-Thread-ID: https://ampcode.com/threads/T-019cbd88-ca30-76ec-abfa-38949748ba3d
The call to setNodeLocatorResolver was lost when resolving merge conflicts during rebase onto main. Re-adds it in the bootstrap sequence before comfyApp.setup().
christian-byrne
force-pushed
the
refactor/node-output-store-centralization
branch
from
dc4ee72 to
c57d4f5
Compare
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
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
Centralize per-node image rendering state (imgs, imageIndex, imageRects, pointerDown, overIndex) in a Pinia store keyed by NodeLocatorId, following the established ECS pattern from useWidgetValueStore (#8594) and usePromotionStore (#8856).
Changes
useNodeImageStorewith property projection viaObject.defineProperty— LGraphNode properties delegate to store getters/setters, so all ~18 consumer files work unchanged.Map(not reactive) avoids Vue proxy overhead in the canvas render hot pathpeekState()+ frozenDEFAULT_STATEprevents unbounded Map growth from read-only accesssetNodeLocatorResolver()breaks circular dependency (LGraph → store → workflowStore → app → litegraph)imgsgetter returnsundefinedwhen empty to preservenode.imgs?.lengthoptional chaining semanticsremoveNodeOutputs(per-node) andresetAllOutputsAndPreviews(bulk clear)Review Focus
setNodeLocatorResolver) is called inGraphCanvas.vuebeforecomfyApp.setup(), andLGraph.add()is guarded bygetActivePinia()for unit test safety.node.imgs,node.imageIndex, etc. continues to work via the property projection.Fixes #9242
┆Issue is synchronized with this Notion page by Unito