feat: add WidgetValueStore for centralized widget value management #8594
Conversation
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
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:
📝 WalkthroughWalkthroughAdds a Pinia-backed WidgetValueStore and migrates widget metadata/value access to it; moves advanced/hidden flags to top-level widget fields; updates BaseWidget to register with the store; introduces global ID deduplication in LGraph; many tests updated to use Pinia and store-backed fixtures. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Vue Component
participant Store as WidgetValueStore
participant Node as BaseWidget
participant DOM as DOM Widget
UI->>Store: read widgetState(nodeId, widgetName)
Store-->>UI: reactive widgetState (value,label,hidden,advanced,options)
UI->>Node: user interaction -> update handler
Node->>Store: registerWidget(nodeId, widgetName) / set value
Store-->>DOM: propagate reactive changes
DOM-->>UI: UI reflects updated store-backed state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
|
Playwright: ✅ 521 passed, 0 failed · 3 flaky 📊 Browser Reports
|
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 02/11/2026, 02:23:35 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 22.2 kB (baseline 22.2 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 854 kB (baseline 855 kB) • 🟢 -283 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 68.8 kB (baseline 68.8 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 451 kB (baseline 451 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 11 added / 11 removed User & Accounts — 16 kB (baseline 16 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 5 added / 5 removed Editors & Dialogs — 751 B (baseline 751 B) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 1 added / 1 removed UI Components — 36.6 kB (baseline 36.6 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed Data & Services — 2.13 MB (baseline 2.12 MB) • 🔴 +3.37 kBStores, services, APIs, and repositories
Status: 12 added / 12 removed Utilities & Hooks — 236 kB (baseline 236 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 12 added / 12 removed Vendor & Third-Party — 8.77 MB (baseline 8.77 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.21 MB (baseline 7.21 MB) • 🔴 +406 BBundles that do not match a named category
Status: 54 added / 54 removed |
DrJKL
changed the title
DrJKL
force-pushed
the
drjkl/widget-store
branch
from
7d3ae89 to
474b148
Compare
DrJKL
added
the
New Browser Test Expectations
|
Updating Playwright Expectations |
DrJKL
commented
Feb 5, 2026
| tooltip?: string | ||
|
|
||
| private _state: WidgetState |
Contributor
Author
There was a problem hiding this comment.
There are likely more properties that we'll want to move into the store.
DrJKL
force-pushed
the
drjkl/widget-store
branch
from
62b9bbd to
f318a7a
Compare
DrJKL
commented
Feb 5, 2026
| return String(scopedId).replace(/^(.*:)+/, '') as NodeId | ||
| } | ||
|
|
||
| export interface WidgetState< |
Contributor
Author
There was a problem hiding this comment.
I'm hoping to be able to remove this and instead compose types soon.
This should also let us eliminate some redundant interfaces floating around in various modules.
dosubot
bot
added
the
size:XL
github-actions
bot
removed
the
New Browser Test Expectations
Amp-Thread-ID: https://ampcode.com/threads/T-019c2554-9721-705e-82f9-7742306a49cc Co-authored-by: Amp <amp@ampcode.com>
- Add _nodeId field and setNodeId() method - Delegate value getter/setter to store when node ID set - Seed store with current value on setNodeId() call Amp-Thread-ID: https://ampcode.com/threads/T-019c2559-977f-77a2-9633-d8fed5b5a2ad Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c2567-2a52-7010-a85e-79eb76be24a1 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c2570-5dc2-7518-b20b-2b87f14bb2f3 Co-authored-by: Amp <amp@ampcode.com>
…tivity - BaseWidget.value getter/setter now delegates to WidgetValueStore when nodeId is set - Widget registration deferred until node is added to graph (to get valid ID) - LGraph.add() registers widgets with store after assigning node ID - Removed vueTrack pattern from useGraphNodeManager - store provides reactivity - Simplified useReactiveWidgetValue to return widget.value directly This enables reactive widget values across Vue components without custom tracking infrastructure. Extensions continue to work unchanged via the preserved widget.value API. Amp-Thread-ID: https://ampcode.com/threads/T-019c25f1-e920-70bb-afaf-59366bdab667 Co-authored-by: Amp <amp@ampcode.com>
Add WidgetState interface and widgetStates Map alongside existing values Map to support full widget lifecycle management: - WidgetState: nodeId, name, type, value, label, hidden, disabled, advanced, promoted, options, serialize - Registration: registerWidget(), unregisterWidget(), unregisterNode() - Getters: getWidget(), getNodeWidgets(), getVisibleWidgets(), getAdvancedWidgets(), getPromotedWidgets() - Setters: setHidden(), setDisabled(), setAdvanced(), setPromoted(), setLabel() Existing get/set/remove/removeNode API preserved for backward compat. set() now syncs value to WidgetState when widget is registered. Amp-Thread-ID: https://ampcode.com/threads/T-019c2626-df19-75cf-b9c9-11fe9735083d Co-authored-by: Amp <amp@ampcode.com>
- Convert label/hidden/disabled/advanced/promoted to getter/setter pairs - Getters read from store when registered, fallback to internal value - Setters update both internal field and store - Add BaseWidget.test.ts with 7 tests for store integration Amp-Thread-ID: https://ampcode.com/threads/T-019c262b-bed5-714e-bfc8-c4d6949e2fed Co-authored-by: Amp <amp@ampcode.com>
- setNodeId() now calls registerWidget() with all widget metadata - Updated BaseWidget tests with 3 new tests for automatic registration - Fixed IWidgetOptions type to use unknown instead of unknown[] Amp-Thread-ID: https://ampcode.com/threads/T-019c2631-966c-73a0-a399-7fe85bd6d495 Co-authored-by: Amp <amp@ampcode.com>
DrJKL
added
the
preview-gpu
Comment on lines
+60
to
+65
| function getNodeWidgets(nodeId: NodeId): WidgetState[] { | ||
| const prefix = `${nodeId}:` | ||
| return [...widgetStates.value] | ||
| .filter(([key]) => key.startsWith(prefix)) | ||
| .map(([, state]) => state) | ||
| } |
Contributor
There was a problem hiding this comment.
Are we afraid of derived maps or secondary views that would allow for O(widgets on node) queries rather than O(total widgets in graph). I know this is pretty fundamental to the entire ECS idea.
Contributor
There was a problem hiding this comment.
In theory we can leverage reactivity to determine if a sub-map is dirty or not.
Contributor
Author
There was a problem hiding this comment.
Not afraid, no. I would want to see that there was a real performance issue before introducing intermediate state.
Contributor
There was a problem hiding this comment.
Would you use "intermediate state" to characterize derived/computed views on the same backing data? If computed's are intermediate state, then a lot of what we do in Vue, a lot of caching/memoization, etc. are all bad. I typically think of "intermediate state" as something that has risk of becoming desynced and/or requires cleanup and manually syncing. If not, then it can't have an inherent negative connotation really.
- Move advanced flag to widget.advanced instead of widget.options.advanced - Include read_only in render-critical options extraction - Remove nodeId from error fallback widget shape Amp-Thread-ID: https://ampcode.com/threads/T-019c3b84-6254-772f-a565-7682d5af55a9 Co-authored-by: Amp <amp@ampcode.com>
![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
🤖 Fix all issues with AI agents
In `@src/services/litegraphService.ts`:
- Around line 218-222: The code assigns hidden to widget.options.hidden which
bypasses BaseWidget's dedicated hidden getter/setter (and its _state.hidden);
change the assignment to use the top-level property like widget.hidden =
inputSpec.hidden (matching widget.advanced = inputSpec.advanced) so the value is
stored via the BaseWidget hidden accessor instead of widget.options.hidden;
update the block where widget.options is assigned (and remove or avoid writing
hidden into widget.options) to ensure consumers/readers that use widget.hidden
see the correct value.
Contributor
There was a problem hiding this comment.
nit/suggestion: Consider adding logging to Sentry if node:widget-name is not unique so we can verify that invariant.
Contributor
There was a problem hiding this comment.
Do we need to add unregisterWidget(nodeId, name) and unregisterNode(nodeId), calls in LGraph.remove() or the node manager's handleNodeRemoved, or wherever appropriate?
Contributor
Author
There was a problem hiding this comment.
Once we sort out the Nodes' state similarly to this and have a real sense of their removal from the overall workflow as opposed to the currently rendered graph, yes. But right now the node's underlying info is tied to whether we intend to display it which is the cause of several of the subgraph bugs (cleanup hooks being tied to removal that get called when entering or exiting a subgraph)
Comment on lines
+82
to
+83
| private _state: Omit<WidgetState, 'nodeId'> & | ||
| Partial<Pick<WidgetState, 'nodeId'>> |
Contributor
There was a problem hiding this comment.
nit: Is this actually a mirrored state or just like a pending state? I am curious how to name/document this better? Also should we document that the _state reference swpas on setNodeId?
Contributor
Author
There was a problem hiding this comment.
It's the pending state until the widget is registered in the store, then it's swapped for the reactive proxy.
Comment on lines
95
to
104
| set hidden(value: boolean | undefined) { | ||
| this._state.hidden = value ?? false | ||
| } | ||
|
|
||
| get disabled(): boolean | undefined { | ||
| return this._state.disabled | ||
| } | ||
| set disabled(value: boolean | undefined) { | ||
| this._state.disabled = value ?? false | ||
| } |
Contributor
There was a problem hiding this comment.
question: should we add additional coercion that wasn't there before (i.e., should we be coercing to false? Worried about downstream effects)?
Contributor
Author
There was a problem hiding this comment.
We could leave it as undefined. I would hope we're not differentiating between undefined and false anywhere, but I wouldn't be shocked to find out that we were.
src/lib/litegraph/src/LGraphNode.ts
Outdated
Comment on lines
1964
to
1968
| if ( | ||
| this.id !== -1 && | ||
| 'setNodeId' in widget && | ||
| typeof widget.setNodeId === 'function' | ||
| ) { |
Contributor
There was a problem hiding this comment.
nit: this duck type guard is repeated a few times and might be worth extracting
Contributor
Author
There was a problem hiding this comment.
Looks like it was twice, but I love a type guard 😁
Contributor
christian-byrne
left a comment
christian-byrne
left a comment
There was a problem hiding this comment.
What I might recommend for this PR, but totally optional:
- Move the LGraph.ensureGloablIdUniqueness, subgraph nested tests/assets/fixtures, subgraph collision ID fix to a separate PR
- Possible move the LGraph.state get/set and subgraph sharing to above PR or in a stack
- Move parallelization of node defs to a separate PR and considering merging on next minor version given risk vs. reward
src/lib/litegraph/src/LGraph.ts
Outdated
| if (node.widgets) { | ||
| for (const widget of node.widgets) { | ||
| if ('setNodeId' in widget && typeof widget.setNodeId === 'function') { | ||
| widget.setNodeId(node.id) |
Contributor
There was a problem hiding this comment.
This is happening in add, why no cleanup in remove?
Contributor
Author
There was a problem hiding this comment.
Because the lifecycle for nodes is a little silly, especially when packing/unpacking/traversing subgraphs.
Contributor
Author
There was a problem hiding this comment.
See earlier response 🙂
| w.options?.hidden || | ||
| (w.options?.advanced && !includesAdvanced.value) | ||
| w.hidden || | ||
| (w.advanced && !includesAdvanced.value) |
Contributor
There was a problem hiding this comment.
note: I think this also will break for old/legacy widgets that are not instanceof BaseWidget
Contributor
Author
There was a problem hiding this comment.
Removed hidden/advanced, keeping those half in options for now.
Comment on lines
+22
to
+29
| 'preview_markdown', | ||
| ['MARKDOWN', {}], | ||
| app | ||
| ).widget as DOMWidget<HTMLTextAreaElement, string> | ||
|
|
||
| const showValueWidgetPlain = ComfyWidgets['STRING']( | ||
| this, | ||
| 'preview', | ||
| 'preview_text', |
Contributor
There was a problem hiding this comment.
nit (non-blocking): Technically a breaking change for a few extensions: https://cs.comfy.org/search?q=context:global+.name+%3D%3D+preview&patternType=keyword&sm=0
Contributor
Author
There was a problem hiding this comment.
They depend on the internals of the Preview as Text node?
| node.widgets?.forEach((w) => w.triggerDraw?.()) | ||
| } | ||
|
|
||
| // Extract only render-critical options (canvasOnly, advanced, read_only) |
Contributor
There was a problem hiding this comment.
nit/question: Ah, didn't see this when I made previous comment. Wonder if it's the best solution for wiring inputSpec onto the widgets.
Contributor
Author
There was a problem hiding this comment.
Almost certainly not. we'd probably want to compose this with the nodeDef store. Part of that will also be eliminating ProxyWidgets so that we can avoid having to follow the subgraph chain through to the concrete widget.
Extract the repeated setNodeId duck-type check into a NodeBindable interface in types/widgets and an isNodeBindable type guard in utils/type, replacing inline guards in LGraph and LGraphNode. Amp-Thread-ID: https://ampcode.com/threads/T-019c405e-abb7-770d-97e9-eaa59edfe0a8 Co-authored-by: Amp <amp@ampcode.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/lib/litegraph/src/LGraph.ts`:
- Around line 2539-2577: In ensureGlobalIdUniqueness, avoid choosing newId =
++state.lastNodeId that may collide with reservedNodeIds/usedNodeIds; instead,
when remapping a duplicate node (use symbols node.id, state.lastNodeId,
usedNodeIds, remappedIds, graph._nodes_by_id), advance state.lastNodeId until it
yields a value not present in usedNodeIds (e.g., increment in a loop), then
assign that newId to node.id, update graph._nodes_by_id, add the newId to
usedNodeIds and remappedIds, and ensure state.lastNodeId is set to the final
newId before calling patchLinkNodeIds on graph._links and
graph.floatingLinksInternal.
🧹 Nitpick comments (1)
src/lib/litegraph/src/widgets/BaseWidget.ts (1)
83-84: Consider extracting the complex type to a named type alias.The inline type
Omit<WidgetState, 'nodeId'> & Partial<Pick<WidgetState, 'nodeId'>>is verbose. Extracting it to a named type would improve readability.♻️ Suggested refactor
+type WidgetStateWithOptionalNodeId = Omit<WidgetState, 'nodeId'> & + Partial<Pick<WidgetState, 'nodeId'>> + export abstract class BaseWidget<TWidget extends IBaseWidget = IBaseWidget> implements IBaseWidget, NodeBindable { // ... - private _state: Omit<WidgetState, 'nodeId'> & - Partial<Pick<WidgetState, 'nodeId'>> + private _state: WidgetStateWithOptionalNodeId
Advance lastNodeId in a loop until it yields a value not present in usedNodeIds, preventing reassigned IDs from colliding with reserved or already-used IDs. Amp-Thread-ID: https://ampcode.com/threads/T-019c408a-5cee-7784-9736-4f53f9936ba4 Co-authored-by: Amp <amp@ampcode.com>
Collaborator
|
Thoroughly discussed offline. PR has my approval, but will be merged after 1.40.0 |
Amp-Thread-ID: https://ampcode.com/threads/T-019c44dd-e8b5-7610-973b-7bc6e554f6ec Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c46e0-f3de-7131-bf8d-9fb1da183f38 Co-authored-by: Amp <amp@ampcode.com>
dosubot
bot
added
size:XL
These are input-spec-derived properties set in litegraphService.ts, not widget runtime state. Restored as plain properties on BaseWidget and read from widget.options in the Vue renderer. Amp-Thread-ID: https://ampcode.com/threads/T-019c48e9-f93f-773b-b6b6-d965b479a0be Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c48e9-f93f-773b-b6b6-d965b479a0be Co-authored-by: Amp <amp@ampcode.com>
2 tasks
2 tasks
Contributor
Author
Option exercised 🫡 |
AustinMroz
approved these changes
Feb 11, 2026
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
Implements Phase 1 of the Vue-owns-truth pattern for widget values. Widget values are now canonical in a Pinia store;
widget.valuedelegates to the store while preserving full backward compatibility.Changes
src/stores/widgetValueStore.ts- centralized widget value storage withget/set/remove/removeNodeAPIwidget.valuegetter/setter now delegates to store when widget is associated with a nodeaddCustomWidget()automatically callswidget.setNodeId(this.id)to wire widgets to their nodesWhy
This foundation enables:
computed(() => store.get(...))Backward Compatibility
widget.value = xnode.widgets[i].valuewidget.callbacknode.onWidgetChangedTesting
┆Issue is synchronized with this Notion page by Unito