feat: synthetic widgets getter for SubgraphNode (proxy-widget-v2)#8856
Merged
feat: synthetic widgets getter for SubgraphNode (proxy-widget-v2)#8856
Conversation
🎭 Playwright: ✅ 539 passed, 0 failed · 9 flaky📊 Browser Reports
|
🎨 Storybook: ✅ Built — View Storybook |
Contributor
📝 WalkthroughWalkthroughReplaces legacy proxy-widget system with a promotion store and PromotedWidgetView; adds promotion-aware DOM positioning, transition-grace, promoted previews (image/video/audio), promotion-driven outline coloring, refactors SubgraphNode/widget plumbing, and includes broad UI, store, test, and fixture updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SubgraphNode
participant PromotionStore
participant PromotedView
participant DomWidgetStore
participant VueComp
User->>SubgraphNode: Convert node → subgraph
SubgraphNode->>PromotionStore: promoteRecommendedWidgets(subgraph)
PromotionStore->>PromotionStore: promote(subgraphId, interiorNodeId, widgetName)
SubgraphNode->>PromotedView: createPromotedWidgetView(...)
PromotedView->>DomWidgetStore: syncDomOverride(node, widget)
DomWidgetStore-->>PromotedView: positionOverride registered
User->>VueComp: Interact with UI
VueComp->>PromotionStore: isPromoted(subgraphId, interiorNodeId, widgetName)?
PromotionStore-->>VueComp: true/false
VueComp->>PromotedView: render / style selection
User->>VueComp: Demote widget
VueComp->>PromotionStore: demote(...)
PromotionStore->>PromotedView: onRemoved()
PromotedView->>DomWidgetStore: clearPositionOverride(widgetId)
sequenceDiagram
participant DomWidgetsComp
participant DomWidgetStore
participant Canvas
DomWidgetsComp->>DomWidgetStore: read widgets + positionOverride
alt positionOverride active (same graph)
DomWidgetsComp->>DomWidgetsComp: useOverride = true
DomWidgetsComp->>DomWidgetsComp: compute position from override.node/widget
else override exists but not active
DomWidgetsComp->>DomWidgetsComp: add widgetId to transitionGrace
DomWidgetsComp->>DomWidgetsComp: render using override position for 1 frame
end
DomWidgetsComp->>Canvas: drawForeground (apply z-index via getDomWidgetZIndex)
Canvas->>DomWidgetsComp: next frame tick
DomWidgetsComp->>DomWidgetsComp: remove expired transitionGrace entries
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 |
📦 Bundle: 4.39 MB gzip 🔴 +7.49 kBDetailsSummary
Category Glance App Entry Points — 17.9 kB (baseline 17.9 kB) • 🟢 -4 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 968 kB (baseline 962 kB) • 🔴 +5.46 kBGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 68.8 kB (baseline 68.8 kB) • 🟢 -11 BTop-level views, pages, and routed surfaces
Status: 11 added / 11 removed Panels & Settings — 436 kB (baseline 436 kB) • 🟢 -19 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed User & Accounts — 16 kB (baseline 16 kB) • 🟢 -4 BAuthentication, profile, and account management bundles
Status: 6 added / 6 removed Editors & Dialogs — 736 B (baseline 738 B) • 🟢 -2 BModals, dialogs, drawers, and in-app editors
Status: 1 added / 1 removed UI Components — 46.9 kB (baseline 47 kB) • 🟢 -7 BReusable component library chunks
Status: 12 added / 12 removed Data & Services — 2.54 MB (baseline 2.52 MB) • 🔴 +19.1 kBStores, services, APIs, and repositories
Status: 13 added / 13 removed Utilities & Hooks — 58.3 kB (baseline 58.3 kB) • 🟢 -7 BHelpers, composables, and utility bundles
Status: 17 added / 17 removed Vendor & Third-Party — 8.84 MB (baseline 8.84 MB) • 🔴 +389 BExternal libraries and shared vendor chunks
Status: 3 added / 3 removed Other — 7.62 MB (baseline 7.62 MB) • 🔴 +420 BBundles that do not match a named category
Status: 75 added / 75 removed |
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/composables/graph/useGraphNodeManager.ts (1)
252-271:⚠️ Potential issue | 🟠 MajorSafeWidgetData.name can desync slot metadata for promoted widgets.
Line 252-271 now sets
nametosourceWidgetName, but slot metadata is keyed byinput.widget.name(outer input name).refreshNodeSlots()later looks up bySafeWidgetData.name, so promoted widgets may stop receiving slotMetadata updates (link status, index). Consider keepingnameaswidget.nameand, if needed, add a separate field for the interior name.🛠️ Suggested fix (keep `name` aligned with slot metadata)
- const name = - 'sourceWidgetName' in widget - ? (widget as { sourceWidgetName: string }).sourceWidgetName - : widget.name + const name = widget.name
🤖 Fix all issues with AI agents
In `@src/components/graph/widgets/DomWidget.vue`:
- Around line 71-75: The selection logic uses a potentially stale reference:
when positionOverride exists you check overrideInGraph but still set ownerNode =
override.node; change ownerNode to use the graph-resolved instance
(overrideInGraph) when present so comparisons against selectedNode use the exact
node instance returned by lgCanvas.graph?.getNodeById; update the isSelected
calculation to compare selectedNode against that resolved ownerNode (keep using
widgetState.widget.node when no overrideInGraph).
In `@src/core/graph/subgraph/promotedWidgetView.test.ts`:
- Around line 51-52: The describe block is using a string title instead of a
function reference which violates vitest/prefer-describe-function-title; update
the test suite header to pass the function reference (createPromotedWidgetView)
instead of the string literal, e.g., replace
describe('createPromotedWidgetView', ...) with
describe(createPromotedWidgetView, ...) and apply the same pattern for other
suites in this file that reference functions or classes.
- Around line 19-29: The test file defines a module-level mutable mock object
(mockDomWidgetStore) which can leak state across tests; replace it with a
hoisted mock via vi.hoisted() and ensure you reset or reinitialize it in each
test's beforeEach. Specifically, change the module mock for useDomWidgetStore to
return a hoisted store (created with vi.hoisted()) and in your beforeEach
reassign or clear its properties (widgetStates map and the spy functions like
setPositionOverride/clearPositionOverride) so each test gets a fresh mock;
update references to mockDomWidgetStore, useDomWidgetStore, and the beforeEach
to perform the reset.
In `@src/core/graph/subgraph/promotedWidgetView.ts`:
- Around line 1-12: Replace the hardcoded "disconnected" / canvas placeholder
label strings in promotedWidgetView with vue-i18n calls: import/use the i18n
composable (useI18n or existing t(...) helper) in the component and replace
occurrences of the literal canvas label and the other user-facing strings around
the canvas (the block currently around lines ~224-238) with t('your.key.path')
calls; add corresponding keys to the English locale JSON (main.json) using
descriptive keys like "promotedWidget.disconnected" and
"promotedWidget.canvasLabel" and use those keys in the t(...) calls so all
user-facing text goes through i18n.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/graph/DomWidgets.vue`:
- Around line 54-55: The membership check uses
currentGraph.getNodeById(posNode.id) which can return a different node with the
same id from another graph; replace it with a graph-identity check so we only
treat nodes that actually belong to currentGraph as in-scope. Concretely, change
the isInCorrectGraph computation to compare the node's graph identity to
currentGraph (for example use posNode.graph === currentGraph or
posNode.getGraph() === currentGraph, or use a dedicated API like
currentGraph.containsNode(posNode) if available) instead of calling
currentGraph.getNodeById(posNode.id); keep nodeVisible =
lgCanvas.isNodeVisible(posNode) as-is.
In `@src/stores/domWidgetStore.ts`:
- Around line 13-16: PositionOverride's widget property is currently typed as
IBaseWidget which becomes a reactive proxy under Pinia and breaks
strict-equality checks; change the type to Raw<IBaseWidget> for
PositionOverride.widget so the stored widget stays the original object (not a
proxy). Update any other declarations/usages of positionOverride.widget in this
module (the same pattern around the other positionOverride handling) to use
Raw<IBaseWidget> as well so the strict-equality guard continues to work.
🧹 Nitpick comments (1)
src/stores/domWidgetStore.ts (1)
77-99: Prefer function declarations for new store actions.Consider using function declarations for
setPositionOverrideandclearPositionOverrideto align with the repository rule for pure functions.Example refactor
-const setPositionOverride = ( - widgetId: string, - override: PositionOverride -) => { +function setPositionOverride(widgetId: string, override: PositionOverride) { const state = widgetStates.value.get(widgetId) if (!state) return const current = state.positionOverride if ( current && current.node === override.node && current.widget === override.widget ) return state.positionOverride = { node: markRaw(override.node), widget: markRaw(override.widget) } -} +} -const clearPositionOverride = (widgetId: string) => { +function clearPositionOverride(widgetId: string) { const state = widgetStates.value.get(widgetId) if (state) state.positionOverride = undefined -} +}As per coding guidelines "Do not use function expressions if it's possible to use function declarations instead".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/litegraph/src/subgraph/SubgraphNode.ts (1)
770-782:⚠️ Potential issue | 🟠 MajorSelf-assignment in
clone()appears ineffective.Line 774 performs
this.properties.proxyWidgets = this.properties.proxyWidgetson the original node, not the clone. Sinceproperties.proxyWidgetsis a plain property (no setter), this assignment has no effect. The comment suggests intent to "reset DOM widget ownership," but this won't happen.If the intent is to give the clone its own copy of the promotion list to avoid shared state:
🐛 Proposed fix
override clone() { const clone = super.clone() - // force reasign so domWidgets reset ownership - - this.properties.proxyWidgets = this.properties.proxyWidgets + // Give the clone its own copy of the promotion list + clone.properties.proxyWidgets = [...(this.properties.proxyWidgets ?? [])] //TODO: Consider deep cloning subgraphs here.
🤖 Fix all issues with AI agents
In `@src/lib/litegraph/src/subgraph/SubgraphNode.ts`:
- Around line 673-693: The filter in SubgraphNode.ensureWidgetRemoved uses loose
equality (n == view.sourceNodeId); change it to strict equality so the predicate
becomes !(n === view.sourceNodeId && w === view.sourceWidgetName). Update the
comparison in the filter inside ensureWidgetRemoved (referencing
ensureWidgetRemoved, PromotedWidgetView, _getPromotionList, and
properties.proxyWidgets) to use === for both n vs view.sourceNodeId and w vs
view.sourceWidgetName to match the rest of the class.
🧹 Nitpick comments (1)
src/lib/litegraph/src/subgraph/SubgraphNode.ts (1)
185-191: Consider adding a debug warning in the empty setter.The empty setter silently discards assignments to
this.widgets. While intentional (SubgraphNodes have no native widgets), this could mask bugs in calling code that expects assignment to work. A development-time warning could aid debugging.💡 Optional: Add debug warning
Object.defineProperty(this, 'widgets', { get: () => this._getPromotedViews(), - set: () => {}, + set: () => { + if (import.meta.env.DEV) { + console.warn('[SubgraphNode] Direct widget assignment is ignored; use promotion list instead.') + } + }, configurable: true, enumerable: true })
DrJKL
added
the
New Browser Test Expectations
|
Updating Playwright Expectations |
github-actions
bot
removed
the
New Browser Test Expectations
…es 1-2) Replace SubgraphNode.widgets with a synthetic getter backed by lightweight PromotedWidgetView objects resolved from properties.proxyWidgets. Phase 1: Create promotedWidgetView.ts factory - Factory function creates plain objects with property descriptors - Delegates type/options/tooltip to interior widget via getters - Store-backed value/label/hidden via WidgetValueStore - Drawing delegates to toConcreteWidget with disconnected placeholder fallback Phase 2: Synthetic widgets getter on SubgraphNode - Replace `override widgets = []` with declare + Object.defineProperty getter - Memoized _getPromotionList() invalidates on reference change - _getPromotedViews() with view cache for positional stability - _resolveLegacyEntry() handles -1 migration from old format - Remove onConfigure patching and dead proxy code from proxyWidget.ts - Update _internalConfigureAfterSlots to clear view cache Amp-Thread-ID: https://ampcode.com/threads/T-019c58a0-5dcd-7670-8b26-b166cef85eee Co-authored-by: Amp <amp@ampcode.com>
…list + view cache Phase 3 of proxy-widget-v2: _setWidget no longer creates widget copies via Object.assign with getter overrides. Instead it adds [nodeId, widgetName] to properties.proxyWidgets and creates/retrieves a PromotedWidgetView from _viewCache. Added interiorNode param to _setWidget and node field to SubgraphInputEventMap input-connected event. Removed BaseWidget/AssetWidget imports from SubgraphNode.ts. Amp-Thread-ID: https://ampcode.com/threads/T-019c58c1-f590-731f-bace-df9771a83125 Co-authored-by: Amp <amp@ampcode.com>
- Remove isProxyWidget, isDisconnectedWidget, DisconnectedWidget.ts - Simplify subgraph-opened handler to use synthetic widgets getter - Override ensureWidgetRemoved to remove from promotion list + view cache - Override removeWidgetByName to delegate to ensureWidgetRemoved - Simplify onRemoved: clear _viewCache, dispatch widget-demoted for all - Update all consumers to use sourceNodeId/sourceWidgetName pattern - Update pruneDisconnected to validate against actual subgraph nodes - Net: -55 lines, 11 files changed Amp-Thread-ID: https://ampcode.com/threads/T-019c58cd-90c7-75c0-917f-74b0b626e4de Co-authored-by: Amp <amp@ampcode.com>
- Override removeWidget on SubgraphNode to delegate to ensureWidgetRemoved - Verified serialize() skips promoted views (serialize: false) - Verified serialize() value sync for System 1 views via slot name matching - Added tests: removeWidget, removeWidgetByName, input cleanup, serialize behavior Amp-Thread-ID: https://ampcode.com/threads/T-019c58df-099f-753a-82d7-ca25c1aebc76 Co-authored-by: Amp <amp@ampcode.com>
- 31 new tests for createPromotedWidgetView, SubgraphNode.widgets getter caching/memoization, promote/demote cycle, disconnected state - Verified old infrastructure already deleted (Phases 2-4) - Confirmed zero references to PromotedWidgetSlot, PromotedDomWidgetAdapter - useGraphNodeManager.ts needs no changes (uses sourceNodeId check) Amp-Thread-ID: https://ampcode.com/threads/T-019c58e4-c39a-704b-895e-d6d4b1ac2ebc Co-authored-by: Amp <amp@ampcode.com>
extractVueNodeData was overwriting SubgraphNode's synthetic widgets getter with a static shallowReactive snapshot, preventing Vue from seeing promote/demote changes. Now detects existing getter via Object.getOwnPropertyDescriptor, preserves it, and syncs results into the reactive array on each access. All Phase 7 validation criteria verified: - DOM widget promotion renders placeholder - Canvas/Vue rendering of promoted widgets - RSP promote/demote/reorder - Context menu promotion - Disconnect/reconnect interior widget - Nested subgraphs - 0 fix commits across all phases Amp-Thread-ID: https://ampcode.com/threads/T-019c58ee-31eb-76aa-beec-ccddc42484bb Co-authored-by: Amp <amp@ampcode.com>
Promoted DOM widgets (textarea, image upload, component widgets) now render their actual element on the SubgraphNode surface instead of a text placeholder. - Add positionOverride to DomWidgetState for redirecting widget positioning to SubgraphNode - Register override in PromotedWidgetView via y setter and draw() - Fall back to interior node when inside the subgraph (override node not in current graph) - Fix clipping: DomWidget.vue uses getNodeById fallback for ownerNode - Fix active flag: bypass active check when override is valid - Add PromotedWidgetView.node to interface, extract isPromotedWidgetView type guard - Clean up overrides on demote/removal in SubgraphNode - Add 6 new tests for override behavior Amp-Thread-ID: https://ampcode.com/threads/T-019c595f-ee25-775a-a71f-cbac34aa6167 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c597a-0409-73d7-83aa-61e6bf3d009a Co-authored-by: Amp <amp@ampcode.com>
Use graph identity check instead of getNodeById for override visibility (IDs not unique across graphs). Add equality guard in setPositionOverride to skip redundant reactive updates on every frame. Amp-Thread-ID: https://ampcode.com/threads/T-019c597d-ab3f-74cc-86e2-7d03c14b63df Co-authored-by: Amp <amp@ampcode.com>
- When a promoted widget is connected to a SubgraphInput node, unpromote it first so it transitions to being linked via the input - Fix serialize() to use input._widget directly instead of ambiguous name-based matching that could sync wrong values across widgets with the same name Amp-Thread-ID: https://ampcode.com/threads/T-019c5984-fadf-759e-909e-d298c740484a Co-authored-by: Amp <amp@ampcode.com>
…ous name lookup When multiple widgets share the same name (e.g. two CLIP 'text' widgets), removeWidgetByName picked the first match, removing the wrong widget. Use the specific PromotedWidgetView bound to the input slot instead. Amp-Thread-ID: https://ampcode.com/threads/T-019c59c8-2b8c-7297-bc0f-5c7070323219 Co-authored-by: Amp <amp@ampcode.com>
…body Promoted DOM widgets overlay the node body and intercept pointer events. Amp-Thread-ID: https://ampcode.com/threads/T-019c55f5-bf2b-7087-b0b8-0ac3be03c7c9 Co-authored-by: Amp <amp@ampcode.com>
…test
- Cache _getPromotedViews result for reference stability (widgets getter)
- Replace Object.create(null) with {} for standard Object prototype
- Revert navigateIntoSubgraph to dblclick approach (title button click fails in CI)
Amp-Thread-ID: https://ampcode.com/threads/T-019c59d8-632b-750c-a1a2-93920c89f63e
Co-authored-by: Amp <amp@ampcode.com>
…omotion
Replace 4 memoization fields in SubgraphNode and multiple independent
parseProxyWidgets() calls with a single Pinia store (reactive Map).
- Add usePromotionStore with getPromotions, isPromoted, setPromotions,
promote, demote actions
- Hydrate store from properties.proxyWidgets on SubgraphNode.onConfigure
- Sync store back to properties.proxyWidgets on SubgraphNode.serialize
- Remove _proxyWidgetsRaw, _promotionList, _viewsCached, _viewsCacheDirty
fields and _getPromotionList() from SubgraphNode
- Remove customRef patterns from SubgraphEditor.vue and TabSubgraphInputs.vue
- Remove matchesPropertyItem, matchesWidgetItem, getProxyWidgets helpers
- Remove emit('update:proxyWidgets') + parent handler in RightSidePanel.vue
- Remove unused exports: widgetItemToProperty, PromotionEntry,
ProxyWidgetsProperty
Amp-Thread-ID: https://ampcode.com/threads/T-019c5afe-a7fd-725d-9efc-557b672e55a8
Co-authored-by: Amp <amp@ampcode.com>
- Export getWidgetName from proxyWidgetUtils, remove duplicate in SubgraphEditor - Add movePromotion action to promotionStore, use in TabSubgraphInputs - Use parents.some() instead of parents[0] in SectionWidgets Amp-Thread-ID: https://ampcode.com/threads/T-019c5afe-a7fd-725d-9efc-557b672e55a8 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c5b0d-e795-70c4-a9ba-8c97d5d44854 Co-authored-by: Amp <amp@ampcode.com>
SubgraphNode now calls usePromotionStore() during configuration, requiring an active Pinia instance in tests. Amp-Thread-ID: https://ampcode.com/threads/T-019c5b49-c288-7247-8976-cb5a6451be02 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c8748-cff0-72b9-9688-0f4569277403 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c8748-cff0-72b9-9688-0f4569277403 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c8748-cff0-72b9-9688-0f4569277403 Co-authored-by: Amp <amp@ampcode.com>
getNodes() serializes the entire LGraphNode object graph across CDP, which fails in CI due to reactive proxies and circular references. All callers only needed .length, so add a getNodeCount() helper that returns a primitive. Amp-Thread-ID: https://ampcode.com/threads/T-019c8779-b3d2-719f-a3cf-cc7b1d8fe0aa Co-authored-by: Amp <amp@ampcode.com>
- proxyWidgetUtils.ts -> promotionUtils.ts - proxyWidget.ts (schema) -> promotionSchema.ts - PromotedWidgetViewImpl -> PromotedWidgetView (drop Impl suffix) Amp-Thread-ID: https://ampcode.com/threads/T-019c87b7-2cba-736a-9d48-adb1505464b8 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c87b7-2cba-736a-9d48-adb1505464b8 Co-authored-by: Amp <amp@ampcode.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
browser_tests/tests/dialog.spec.ts (1)
57-64: Consider simplifying the nested assertion pattern.The nested
expect(async () => { await expect(...).not.toBeVisible() }).toPass()adds an extra retry layer over Playwright's built-in auto-waiting. While this defensive approach works for flaky undo/redo timing, it's verbose.A simpler alternative would be to increase the assertion timeout directly:
await expect(missingNodesWarning).not.toBeVisible({ timeout: 5000 })That said, if the double-retry is intentional to handle edge cases where undo/redo state propagation is unpredictable, the current approach is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@browser_tests/tests/dialog.spec.ts` around lines 57 - 64, The nested retry pattern wraps an assertion inside expect(...).toPass() which duplicates Playwright's auto-waiting and is verbose; replace the two occurrences that wrap await expect(missingNodesWarning).not.toBeVisible() (the blocks after the initial check and after comfyPage.keyboard.redo()) with a single direct assertion that increases the timeout on the Playwright matcher (use expect(missingNodesWarning).not.toBeVisible with a timeout of 5000) so the code relies on Playwright's built-in waiting instead of the extra toPass wrapper.browser_tests/fixtures/helpers/NodeOperationsHelper.ts (1)
20-24: Consider unifying or differentiating the two node-count methods.
getNodeCount()andgetGraphNodesCount()both return the node count but differ in null-safety:getGraphNodesCount()uses optional chaining with a0fallback, whilegetNodeCount()uses non-null assertions that will throw ifapporgraphis undefined.If both are needed, consider documenting when to use each. If only one is needed long-term, consider deprecating or removing the other to avoid confusion.
Also applies to: 36-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@browser_tests/fixtures/helpers/NodeOperationsHelper.ts` around lines 20 - 24, There are two methods, getNodeCount() and getGraphNodesCount(), that return the same value but differ in null-safety; pick one approach and make them consistent: either (A) unify to the safe implementation used by getGraphNodesCount() (use optional chaining and a 0 fallback) and remove or deprecate the other, or (B) keep both but add clear JSDoc above getNodeCount() and getGraphNodesCount() in NodeOperationsHelper.ts explaining their distinct contracts (e.g., getNodeCount() throws on missing app/graph, getGraphNodesCount() returns 0 if missing) so callers know which to use; prefer option A (convert getNodeCount() to use optional chaining and fallback to 0, or remove it) to avoid surprising exceptions.lint-staged.config.ts (1)
9-21: Prefer a named function and immutable command list construction.This keeps the rule compliant with the “no function expressions” and “avoid mutable state” guidelines while preserving behavior.
🔧 Suggested refactor
export default { 'tests-ui/**': () => 'echo "Files in tests-ui/ are deprecated. Colocate tests with source files." && exit 1', './**/*.js': (stagedFiles: string[]) => formatAndEslint(stagedFiles), - './**/*.{ts,tsx,vue,mts}': (stagedFiles: string[]) => { - const commands = [...formatAndEslint(stagedFiles), 'pnpm typecheck'] - - const hasBrowserTestsChanges = stagedFiles - .map((f) => path.relative(process.cwd(), f).replace(/\\/g, '/')) - .some((f) => f.startsWith('browser_tests/')) - - if (hasBrowserTestsChanges) { - commands.push('pnpm typecheck:browser') - } - - return commands - } + './**/*.{ts,tsx,vue,mts}': lintStagedTypes } + +function lintStagedTypes(stagedFiles: string[]) { + const hasBrowserTestsChanges = stagedFiles + .map((f) => path.relative(process.cwd(), f).replace(/\\/g, '/')) + .some((f) => f.startsWith('browser_tests/')) + + return [ + ...formatAndEslint(stagedFiles), + 'pnpm typecheck', + ...(hasBrowserTestsChanges ? ['pnpm typecheck:browser'] : []) + ] +}As per coding guidelines, “Avoid mutable state; prefer immutability and assignment at point of declaration” and “Do not use function expressions if it's possible to use function declarations instead.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lint-staged.config.ts` around lines 9 - 21, Replace the inline arrow function with a named function (e.g., function buildSourceCommands(stagedFiles: string[])) and remove the mutable push by building the commands immutably: create a const baseCommands = [...formatAndEslint(stagedFiles), 'pnpm typecheck'] and then set const commands = hasBrowserTestsChanges ? [...baseCommands, 'pnpm typecheck:browser'] : baseCommands; keep the same hasBrowserTestsChanges calculation, return commands, and use the new function name where the rule value currently references the arrow function.src/composables/node/useNodeCanvasImagePreview.test.ts (1)
43-46: Consider importing the constant instead of hardcoding the string.The test hardcodes
'$$canvas-image-preview'rather than using the exportedCANVAS_IMAGE_PREVIEW_WIDGETconstant. This creates a maintenance risk if the constant value changes.♻️ Suggested fix
-import { useNodeCanvasImagePreview } from './useNodeCanvasImagePreview' +import { + CANVAS_IMAGE_PREVIEW_WIDGET, + useNodeCanvasImagePreview +} from './useNodeCanvasImagePreview'Then update the assertion:
expect(imagePreviewWidget).toHaveBeenCalledWith(node, { type: 'IMAGE_PREVIEW', - name: '$$canvas-image-preview' + name: CANVAS_IMAGE_PREVIEW_WIDGET })Also update line 52:
- node.addWidget('text', '$$canvas-image-preview', '', () => undefined, {}) + node.addWidget('text', CANVAS_IMAGE_PREVIEW_WIDGET, '', () => undefined, {})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/composables/node/useNodeCanvasImagePreview.test.ts` around lines 43 - 46, Replace the hardcoded widget name in the test assertion with the exported constant CANVAS_IMAGE_PREVIEW_WIDGET: in useNodeCanvasImagePreview.test.ts update the expect call that invokes imagePreviewWidget(node, { type: 'IMAGE_PREVIEW', name: '$$canvas-image-preview' }) to use name: CANVAS_IMAGE_PREVIEW_WIDGET (importing it at the top of the test), and similarly replace any other occurrences of the literal '$$canvas-image-preview' in this test (e.g., the later assertion at line 52) with the same imported constant to keep the test in sync with the source constant.browser_tests/fixtures/utils/litegraphUtils.ts (1)
480-485: Prefer a function declaration for the local helper.You can keep the same behavior while following the project rule by capturing
comfyPageand using a declaration.As per coding guidelines, "Do not use function expressions if it's possible to use function declarations instead".♻️ Suggested refactor
- const checkIsInSubgraph = async () => { - return this.comfyPage.page.evaluate(() => { + const { comfyPage } = this + async function checkIsInSubgraph() { + return comfyPage.page.evaluate(() => { const graph = window.app!.canvas.graph return !!graph && 'inputNode' in graph }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@browser_tests/fixtures/utils/litegraphUtils.ts` around lines 480 - 485, The local helper is defined as an arrow function expression; convert it to an async function declaration named checkIsInSubgraph that closes over comfyPage (i.e., keep using comfyPage.page.evaluate) so behavior is identical: replace "const checkIsInSubgraph = async () => { return this.comfyPage.page.evaluate(... ) }" with "async function checkIsInSubgraph() { return comfyPage.page.evaluate(() => { const graph = window.app!.canvas.graph; return !!graph && 'inputNode' in graph }) }" (ensure references to comfyPage, comfyPage.page.evaluate, window.app!.canvas.graph and the 'inputNode' check remain unchanged).src/composables/graph/useGraphNodeManager.test.ts (1)
85-115: Avoid hard-coded interior node id in promotion testUsing the actual node id keeps the test resilient if ID assignment ever changes.
♻️ Suggested tweak
const interiorNode = new LGraphNode('interior') interiorNode.id = 10 subgraph.add(interiorNode) + const interiorNodeId = String(interiorNode.id) usePromotionStore().promote( subgraphNode.rootGraph.id, subgraphNode.id, - '10', + interiorNodeId, '$$canvas-image-preview' )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/composables/graph/useGraphNodeManager.test.ts` around lines 85 - 115, The test hard-codes the interior node id ('10') when calling usePromotionStore().promote; replace that literal with the actual interiorNode.id (converted to string if promote expects a string) to keep the test resilient—i.e., use String(interiorNode.id) (or interiorNode.id) in the call to usePromotionStore().promote so the test uses the real id from the LGraphNode instance created above.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@browser_tests/tests/colorPalette.spec.ts`:
- Around line 248-259: The test silently passes if app.graph.serialize is
missing because the inline getter returns { nodes: [] }; instead, detect the
absence of serialize and fail the test or return an explicit flag so the
assertion can't be bypassed: check for the presence of (globalThis as { app?: {
graph?: { serialize: () => unknown } } }).app?.graph?.serialize and if missing
either throw an Error or return { missingSerialize: true } and then add an
assertion on parsed.missingSerialize === undefined (or assert serialize exists)
before using parsed.nodes; update references to app.graph.serialize and
parsed.nodes accordingly so the test fails loudly when serialization is
unavailable.
In `@src/components/rightSidePanel/parameters/SectionWidgets.vue`:
- Around line 78-92: The call to promotionStore.isPromoted in SectionWidgets.vue
can miss matches because widget.sourceNodeId may be a number while other call
sites pass string IDs; update the promoted-lookup branch in the anonymous
function returned by parents.some to normalize the source ID by calling
String(widget.sourceNodeId) (just like the fallback uses String(widgetNode.id)),
so both branches call promotionStore.isPromoted with string IDs and the same
parameters used elsewhere (isPromotedWidgetView, promotionStore.isPromoted,
sourceWidgetName).
In `@src/components/rightSidePanel/subgraph/SubgraphEditor.vue`:
- Around line 170-195: The bulk promotion functions showAll, hideAll, and
showRecommended update promotionStore but don’t trigger a canvas redraw; after
their loops you should mark the canvas as dirty or emit the app’s invalidate
event (for example call your canvas invalidation helper such as
canvasStore.markDirty() or emit('invalidate-canvas')) so the UI repaints; update
showAll, hideAll and showRecommended to call that invalidation method after
performing all promotionStore.promote()/demote() calls.
---
Nitpick comments:
In `@browser_tests/fixtures/helpers/NodeOperationsHelper.ts`:
- Around line 20-24: There are two methods, getNodeCount() and
getGraphNodesCount(), that return the same value but differ in null-safety; pick
one approach and make them consistent: either (A) unify to the safe
implementation used by getGraphNodesCount() (use optional chaining and a 0
fallback) and remove or deprecate the other, or (B) keep both but add clear
JSDoc above getNodeCount() and getGraphNodesCount() in NodeOperationsHelper.ts
explaining their distinct contracts (e.g., getNodeCount() throws on missing
app/graph, getGraphNodesCount() returns 0 if missing) so callers know which to
use; prefer option A (convert getNodeCount() to use optional chaining and
fallback to 0, or remove it) to avoid surprising exceptions.
In `@browser_tests/fixtures/utils/litegraphUtils.ts`:
- Around line 480-485: The local helper is defined as an arrow function
expression; convert it to an async function declaration named checkIsInSubgraph
that closes over comfyPage (i.e., keep using comfyPage.page.evaluate) so
behavior is identical: replace "const checkIsInSubgraph = async () => { return
this.comfyPage.page.evaluate(... ) }" with "async function checkIsInSubgraph() {
return comfyPage.page.evaluate(() => { const graph = window.app!.canvas.graph;
return !!graph && 'inputNode' in graph }) }" (ensure references to comfyPage,
comfyPage.page.evaluate, window.app!.canvas.graph and the 'inputNode' check
remain unchanged).
In `@browser_tests/tests/dialog.spec.ts`:
- Around line 57-64: The nested retry pattern wraps an assertion inside
expect(...).toPass() which duplicates Playwright's auto-waiting and is verbose;
replace the two occurrences that wrap await
expect(missingNodesWarning).not.toBeVisible() (the blocks after the initial
check and after comfyPage.keyboard.redo()) with a single direct assertion that
increases the timeout on the Playwright matcher (use
expect(missingNodesWarning).not.toBeVisible with a timeout of 5000) so the code
relies on Playwright's built-in waiting instead of the extra toPass wrapper.
In `@lint-staged.config.ts`:
- Around line 9-21: Replace the inline arrow function with a named function
(e.g., function buildSourceCommands(stagedFiles: string[])) and remove the
mutable push by building the commands immutably: create a const baseCommands =
[...formatAndEslint(stagedFiles), 'pnpm typecheck'] and then set const commands
= hasBrowserTestsChanges ? [...baseCommands, 'pnpm typecheck:browser'] :
baseCommands; keep the same hasBrowserTestsChanges calculation, return commands,
and use the new function name where the rule value currently references the
arrow function.
In `@src/composables/graph/useGraphNodeManager.test.ts`:
- Around line 85-115: The test hard-codes the interior node id ('10') when
calling usePromotionStore().promote; replace that literal with the actual
interiorNode.id (converted to string if promote expects a string) to keep the
test resilient—i.e., use String(interiorNode.id) (or interiorNode.id) in the
call to usePromotionStore().promote so the test uses the real id from the
LGraphNode instance created above.
In `@src/composables/node/useNodeCanvasImagePreview.test.ts`:
- Around line 43-46: Replace the hardcoded widget name in the test assertion
with the exported constant CANVAS_IMAGE_PREVIEW_WIDGET: in
useNodeCanvasImagePreview.test.ts update the expect call that invokes
imagePreviewWidget(node, { type: 'IMAGE_PREVIEW', name: '$$canvas-image-preview'
}) to use name: CANVAS_IMAGE_PREVIEW_WIDGET (importing it at the top of the
test), and similarly replace any other occurrences of the literal
'$$canvas-image-preview' in this test (e.g., the later assertion at line 52)
with the same imported constant to keep the test in sync with the source
constant.
Contributor
christian-byrne
left a comment
christian-byrne
left a comment
There was a problem hiding this comment.
Nice refactoring in the latest commits. The PromotedWidgetViewManager cache reconciliation pattern is clean, and centralizing resolvePromotedWidgetSource reduces duplication.
One minor observation on getProjectedWidget in promotedWidgetView.ts (lines 257-280): the cache invalidation check compares projectedWidget.type !== resolved.widget.type after already checking node/widget identity. If the source widget type can change while the widget reference stays the same, this is correct. Otherwise this comparison is redundant since a changed type would imply a different widget reference. Not blocking, just a clarity question.
The getNodeCount() fix for CDP serialization is a good catch - avoids serializing the entire reactive node graph across the protocol boundary.
- Remove redundant widget type comparison in promotedWidgetView cache invalidation (type is immutable on widget instances) - Make colorPalette E2E test fail loudly when app.graph.serialize is unavailable instead of silently passing with empty nodes Amp-Thread-ID: https://ampcode.com/threads/T-019c880b-42ef-7156-adad-0bccd20e5d79 Co-authored-by: Amp <amp@ampcode.com>
Contributor
|
Nice refactoring in the latest commits. The One minor observation on The |
PromotedWidgetView defines options/type/name/value as prototype getters, which object spread silently drops. Build SimplifiedWidget by reading properties directly and querying widgetValueStore, matching the NodeWidgets.vue canvas pattern. - Replace enhancedWidget spread with explicit simplifiedWidget computed - Resolve source widget via resolvePromotedWidgetSource - Query widgetValueStore for value/label/options - Export getControlWidget from useGraphNodeManager - Add WidgetItem tests for promoted widget property passthrough Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019c88f3-8080-70c9-a9fc-df268c580fd9
…to drjkl/subgraphs-are-the-best
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/components/rightSidePanel/subgraph/SubgraphEditor.vue (1)
170-195:⚠️ Potential issue | 🟡 MinorInvalidate the canvas after bulk promotion updates.
Bulk promote/demote operations update the store but don’t force a redraw, so the UI can stay stale until the next repaint.
🔧 Proposed fix
function showAll() { const node = activeNode.value if (!node) return for (const [n, w] of filteredCandidates.value) { promotionStore.promote(node.rootGraph.id, node.id, String(n.id), w.name) } + canvasStore.canvas?.setDirty(true, true) } function hideAll() { const node = activeNode.value if (!node) return for (const [n, w] of filteredActive.value) { if (String(n.id) === '-1') continue promotionStore.demote( node.rootGraph.id, node.id, String(n.id), getWidgetName(w) ) } + canvasStore.canvas?.setDirty(true, true) } function showRecommended() { const node = activeNode.value if (!node) return for (const [n, w] of recommendedWidgets.value) { promotionStore.promote(node.rootGraph.id, node.id, String(n.id), w.name) } + canvasStore.canvas?.setDirty(true, true) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/rightSidePanel/subgraph/SubgraphEditor.vue` around lines 170 - 195, The bulk promote/demote functions (showAll, hideAll, showRecommended) update promotionStore but don't trigger a canvas redraw, leaving the UI stale; after calling promotionStore.promote or promotionStore.demote in those functions, call the canvas invalidation/redraw helper (e.g., invoke the existing invalidateCanvas/refreshCanvas/forceRerender function used elsewhere in this component) once after the loop so the graph re-renders; ensure you reference showAll, hideAll, showRecommended and promotionStore.promote/demote and batch the redraw call outside the loops to avoid excessive repaints.src/components/rightSidePanel/parameters/SectionWidgets.vue (1)
78-85:⚠️ Potential issue | 🟡 MinorNormalize promoted source node IDs before promotion lookup.
widget.sourceNodeIdcan be numeric, which can cause promotionStore lookups to miss. Normalize to string for consistency with other call sites.🔧 Proposed fix
- widget.sourceNodeId, + String(widget.sourceNodeId),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/rightSidePanel/parameters/SectionWidgets.vue` around lines 78 - 85, The promotion lookup can miss because widget.sourceNodeId may be a number; update the call site inside the parents.some block (where isPromotedWidgetView(widget) leads to promotionStore.isPromoted(...)) to normalize the source node id to a string (e.g., pass String(widget.sourceNodeId) or widget.sourceNodeId?.toString()) when calling promotionStore.isPromoted so the lookup uses the same string-formatted ID as other call sites.browser_tests/tests/subgraphPromotion.spec.ts (1)
158-160:⚠️ Potential issue | 🟡 MinorUse the centralized TestIds for the subgraph enter button.
Hard-coding the test id string bypasses the selector registry. Use the shared TestIds constant instead.
🔧 Proposed fix
- const enterButton = subgraphVueNode.getByTestId('subgraph-enter-button') + const enterButton = subgraphVueNode.getByTestId( + TestIds.widgets.subgraphEnterButton + )As per coding guidelines, "Use the centralized TestIds from
browser_tests/fixtures/selectors.tsfor test element selection".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@browser_tests/tests/subgraphPromotion.spec.ts` around lines 158 - 160, Replace the hard-coded test id string in the subgraph test with the centralized TestIds constant: update the selector in the subgraphPromotion.spec.ts where you call subgraphVueNode.getByTestId('subgraph-enter-button') to use the shared TestIds identifier (e.g., TestIds.SUBGRAPH_ENTER_BUTTON) from browser_tests/fixtures/selectors.ts and import TestIds at the top if not already present; keep the variable name enterButton and the getByTestId call otherwise unchanged.
🧹 Nitpick comments (7)
src/components/rightSidePanel/parameters/WidgetItem.test.ts (2)
156-203: Tests validate propagation but could benefit from additional edge cases.The current tests verify that widget properties are correctly passed through to the stub component. This is valuable coverage for the promoted widget refactor.
Consider adding tests for:
- Edge case when
optionsisundefinedor empty- Behavior when
resolvePromotedWidgetSourcereturns a defined value (currently mocked to returnundefined)Would you like me to draft additional test cases for these edge scenarios?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/rightSidePanel/parameters/WidgetItem.test.ts` around lines 156 - 203, Add tests covering edge cases for promoted widget propagation: add one test using createMockPromotedWidgetView with options undefined or {} and assert stub.props('widget').options is undefined or an empty object as expected (via mountWidgetItem and StubWidgetComponent), and add another test that stubs resolvePromotedWidgetSource to return a concrete promoted source value then mountWidgetItem(createMockPromotedWidgetView(...)) and assert the stub receives the resolved properties (options/type/name/value) to verify behavior when resolvePromotedWidgetSource returns a defined value.
74-95: Consider usingsatisfiesfor mock object type validation.Per repository learnings, when creating test helper functions that construct mock objects implementing an interface, prefer using
satisfies InterfaceTypefor shape validation instead ofas unknown astype assertions.♻️ Suggested improvement
function createMockNode(overrides: Partial<LGraphNode> = {}): LGraphNode { - return { + const base = { id: 1, type: 'TestNode', isSubgraphNode: () => false, graph: { rootGraph: { id: 'test-graph-id' } }, ...overrides - } as unknown as LGraphNode + } + return base as unknown as LGraphNode } function createMockWidget(overrides: Partial<IBaseWidget> = {}): IBaseWidget { - return { + const base = { name: 'test_widget', type: 'combo', value: 'option_a', y: 0, options: { values: ['option_a', 'option_b', 'option_c'] }, ...overrides - } as IBaseWidget + } + return base as IBaseWidget }Note: Full
satisfiesusage would require defining the complete interface shape, which may be impractical for partial mocks. The current approach is acceptable given the complexity ofLGraphNodeandIBaseWidgetinterfaces.Based on learnings: "prefer using
satisfies InterfaceTypefor shape validation instead of type assertions likeas Partial<InterfaceType> as InterfaceTypeoras any."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/rightSidePanel/parameters/WidgetItem.test.ts` around lines 74 - 95, Replace the unsafe "as unknown as" assertions in the test helpers with TypeScript's "satisfies" to validate the mock shapes: update createMockNode to return an object expression that uses "satisfies LGraphNode" (keeping the overrides: Partial<LGraphNode> and spread) and update createMockWidget similarly to use "satisfies IBaseWidget" for the widget shape; this preserves flexible Partial overrides while enabling compile-time shape checks for createMockNode and createMockWidget.browser_tests/tests/vueNodes/widgets/load/uploadWidgets.spec.ts (1)
21-26: Consider using.first().toBeVisible()for more idiomatic Playwright assertions.The
expect.poll()with.count()works, but Playwright's built-in auto-retrying assertions are generally preferred. Using.first().toBeVisible()is more idiomatic when asserting "at least one element exists."♻️ Suggested refactor
- await expect - .poll(() => comfyPage.page.getByText('Error loading image').count()) - .toBeGreaterThan(0) - await expect - .poll(() => comfyPage.page.getByText('Error loading video').count()) - .toBeGreaterThan(0) + await expect( + comfyPage.page.getByText('Error loading image').first() + ).toBeVisible() + await expect( + comfyPage.page.getByText('Error loading video').first() + ).toBeVisible()As per coding guidelines: Follow Playwright Best Practices — built-in locator assertions auto-retry and provide better error messages on failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@browser_tests/tests/vueNodes/widgets/load/uploadWidgets.spec.ts` around lines 21 - 26, Replace the manual expect.poll(... .count()) checks with Playwright's built-in locator assertions: use the getByText(...) locator's .first() and call toBeVisible() so the assertions auto-retry and give better errors; specifically update the two assertions that use comfyPage.page.getByText('Error loading image') and comfyPage.page.getByText('Error loading video') to assert .first().toBeVisible() instead of polling counts.browser_tests/tests/menu.spec.ts (2)
206-210: Prefer non-retrying assertions insidetoPass()blocks.Using auto-retrying Playwright assertions like
expect(menu).toBeVisible()insidetoPass()can cause inefficient retry behavior—each inner assertion waits its full timeout before failing, then the outertoPassretries the entire block.Consider using synchronous checks with
.isVisible()for the locator assertions:♻️ Suggested refactor
await expect(async () => { - await expect(menu).toBeVisible() - await expect(themeSubmenu).toBeVisible() + expect(await menu.isVisible()).toBe(true) + expect(await themeSubmenu.isVisible()).toBe(true) expect(await topbar.isMenuItemActive(lightThemeItem)).toBe(true) }).toPass({ timeout: 5000 })Based on learnings: Playwright best practices recommend that assertions inside
toPasscallbacks should not be auto-retrying.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@browser_tests/tests/menu.spec.ts` around lines 206 - 210, The toPass block currently contains auto-retrying Playwright assertions (expect(menu).toBeVisible(), expect(themeSubmenu).toBeVisible()) which causes double retrying; change those to synchronous visibility checks by calling await menu.isVisible() and await themeSubmenu.isVisible() and assert the booleans (e.g. expect(await menu.isVisible()).toBe(true)) inside the async callback passed to toPass while keeping the existing call to await topbar.isMenuItemActive(lightThemeItem) (or assert its boolean similarly); in short, replace auto-retrying toBeVisible() calls with .isVisible() checks and simple boolean expectations inside the toPass block so only the outer retry is exercised.
231-238: Same refactor applies here—prefer non-retrying assertions insidetoPass().♻️ Suggested refactor
await expect(async () => { - await expect(menu).toBeVisible() - await expect(themeItems2.submenu).toBeVisible() + expect(await menu.isVisible()).toBe(true) + expect(await themeItems2.submenu.isVisible()).toBe(true) expect(await topbar.isMenuItemActive(themeItems2.darkTheme)).toBe(true) expect(await topbar.isMenuItemActive(themeItems2.lightTheme)).toBe( false ) }).toPass({ timeout: 5000 })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@browser_tests/tests/menu.spec.ts` around lines 231 - 238, Inside the toPass wrapper you currently call Playwright's retrying assertions (await expect(menu).toBeVisible() and await expect(themeItems2.submenu).toBeVisible()); replace those with non-retrying checks or move them outside toPass: either await the visibility first (await expect(menu).toBeVisible(); await expect(themeItems2.submenu).toBeVisible();) before entering the toPass block, or convert the calls inside toPass to synchronous checks using isVisible() (e.g., expect(await menu.isVisible()).toBe(true) and expect(await themeItems2.submenu.isVisible()).toBe(true)), keeping the other checks that call topbar.isMenuItemActive(...) inside toPass; update references to menu, themeItems2.submenu, topbar.isMenuItemActive, themeItems2.darkTheme, and themeItems2.lightTheme accordingly.lint-staged.config.ts (2)
9-20: Prefer a named function declaration for the lint-staged handler.
The inline arrow function can be replaced with a declared function and referenced from the config to align with the repo’s function-style guideline.♻️ Suggested refactor
export default { 'tests-ui/**': () => 'echo "Files in tests-ui/ are deprecated. Colocate tests with source files." && exit 1', './**/*.js': (stagedFiles: string[]) => formatAndEslint(stagedFiles), - './**/*.{ts,tsx,vue,mts}': (stagedFiles: string[]) => { + './**/*.{ts,tsx,vue,mts}': typeScriptCommands, +} + +function typeScriptCommands(stagedFiles: string[]) { const commands = [...formatAndEslint(stagedFiles), 'pnpm typecheck'] const hasBrowserTestsChanges = stagedFiles .map((f) => path.relative(process.cwd(), f).replace(/\\/g, '/')) .some((f) => f.startsWith('browser_tests/')) if (hasBrowserTestsChanges) { commands.push('pnpm typecheck:browser') } return commands - } -} +}As per coding guidelines, “Do not use function expressions if it's possible to use function declarations instead.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lint-staged.config.ts` around lines 9 - 20, The inline arrow handler for the lint-staged entry should be replaced with a named function declaration: extract the block currently assigned to './**/*.{ts,tsx,vue,mts}': (stagedFiles: string[]) => { ... } into a declared function (e.g., function lintStagedHandler(stagedFiles: string[])) containing the same logic (commands creation, hasBrowserTestsChanges detection, push of 'pnpm typecheck:browser' when appropriate) and then reference that function in the config object instead of the inline arrow; update any type annotations to match and ensure formatAndEslint, path, and process.cwd() usages remain unchanged.
10-18: Avoid mutating the commands array.
Build the list immutably to match the repo’s immutability guideline.♻️ Suggested refactor
- const commands = [...formatAndEslint(stagedFiles), 'pnpm typecheck'] - const hasBrowserTestsChanges = stagedFiles .map((f) => path.relative(process.cwd(), f).replace(/\\/g, '/')) .some((f) => f.startsWith('browser_tests/')) - - if (hasBrowserTestsChanges) { - commands.push('pnpm typecheck:browser') - } - - return commands + return [ + ...formatAndEslint(stagedFiles), + 'pnpm typecheck', + ...(hasBrowserTestsChanges ? ['pnpm typecheck:browser'] : []) + ]As per coding guidelines, “Avoid mutable state; prefer immutability and assignment at point of declaration.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lint-staged.config.ts` around lines 10 - 18, The code mutates the commands array via commands.push; instead, build it immutably at declaration: compute hasBrowserTestsChanges (from stagedFiles.map(...).some(...)) and then create a const commands using spreads and a conditional spread to include 'pnpm typecheck:browser' when needed (e.g., const commands = [...formatAndEslint(stagedFiles), 'pnpm typecheck', ...(hasBrowserTestsChanges ? ['pnpm typecheck:browser'] : [])]); this avoids mutation of commands and keeps logic around formatAndEslint, stagedFiles and the conditional inclusion of typecheck:browser immutable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@browser_tests/fixtures/helpers/NodeOperationsHelper.ts`:
- Around line 36-38: getNodeCount currently calls page.evaluate(() =>
window.app!.graph.nodes.length) which throws if window.app or window.app.graph
is uninitialized; change the evaluate callback to a null-safe access (e.g. check
window.app and window.app.graph and return 0 when missing) mirroring the pattern
used by getGraphNodesCount so the helper becomes resilient during
startup/workflow switches; locate getNodeCount and replace the non‑null
assertion usage inside page.evaluate with a guarded expression that safely
returns a number.
In `@browser_tests/helpers/promotedWidgets.ts`:
- Around line 43-51: The function getPromotedWidgetCount currently returns the
total number of widgets on a node (node?.widgets?.length) which conflicts with
its name; either rename it to getNodeWidgetCount or change its implementation to
count promoted widgets by using getPromotedWidgets(comfyPage, nodeId) and
returning its length. Update callers accordingly and keep references to
getPromotedWidgetCount and getPromotedWidgets (and the proxyWidgets semantics)
so the behavior matches the function name.
In `@browser_tests/tests/dialog.spec.ts`:
- Around line 41-46: Replace the CSS class locator with the centralized test-id
selector: change the declaration using
comfyPage.page.locator('.comfy-missing-nodes') to use the page.getByTestId(...)
helper and the shared selectors map (e.g., selectors.MISSING_NODES or
TEST_IDS.MISSING_NODES exported from fixtures/selectors), adding that test id to
the selectors map if it doesn't exist; update the variable name
(missingNodesWarning) usage unchanged so the rest of the steps (loadWorkflow,
Escape key, visibility assertions) keep working with the new getByTestId
locator.
---
Duplicate comments:
In `@browser_tests/tests/subgraphPromotion.spec.ts`:
- Around line 158-160: Replace the hard-coded test id string in the subgraph
test with the centralized TestIds constant: update the selector in the
subgraphPromotion.spec.ts where you call
subgraphVueNode.getByTestId('subgraph-enter-button') to use the shared TestIds
identifier (e.g., TestIds.SUBGRAPH_ENTER_BUTTON) from
browser_tests/fixtures/selectors.ts and import TestIds at the top if not already
present; keep the variable name enterButton and the getByTestId call otherwise
unchanged.
In `@src/components/rightSidePanel/parameters/SectionWidgets.vue`:
- Around line 78-85: The promotion lookup can miss because widget.sourceNodeId
may be a number; update the call site inside the parents.some block (where
isPromotedWidgetView(widget) leads to promotionStore.isPromoted(...)) to
normalize the source node id to a string (e.g., pass String(widget.sourceNodeId)
or widget.sourceNodeId?.toString()) when calling promotionStore.isPromoted so
the lookup uses the same string-formatted ID as other call sites.
In `@src/components/rightSidePanel/subgraph/SubgraphEditor.vue`:
- Around line 170-195: The bulk promote/demote functions (showAll, hideAll,
showRecommended) update promotionStore but don't trigger a canvas redraw,
leaving the UI stale; after calling promotionStore.promote or
promotionStore.demote in those functions, call the canvas invalidation/redraw
helper (e.g., invoke the existing invalidateCanvas/refreshCanvas/forceRerender
function used elsewhere in this component) once after the loop so the graph
re-renders; ensure you reference showAll, hideAll, showRecommended and
promotionStore.promote/demote and batch the redraw call outside the loops to
avoid excessive repaints.
---
Nitpick comments:
In `@browser_tests/tests/menu.spec.ts`:
- Around line 206-210: The toPass block currently contains auto-retrying
Playwright assertions (expect(menu).toBeVisible(),
expect(themeSubmenu).toBeVisible()) which causes double retrying; change those
to synchronous visibility checks by calling await menu.isVisible() and await
themeSubmenu.isVisible() and assert the booleans (e.g. expect(await
menu.isVisible()).toBe(true)) inside the async callback passed to toPass while
keeping the existing call to await topbar.isMenuItemActive(lightThemeItem) (or
assert its boolean similarly); in short, replace auto-retrying toBeVisible()
calls with .isVisible() checks and simple boolean expectations inside the toPass
block so only the outer retry is exercised.
- Around line 231-238: Inside the toPass wrapper you currently call Playwright's
retrying assertions (await expect(menu).toBeVisible() and await
expect(themeItems2.submenu).toBeVisible()); replace those with non-retrying
checks or move them outside toPass: either await the visibility first (await
expect(menu).toBeVisible(); await expect(themeItems2.submenu).toBeVisible();)
before entering the toPass block, or convert the calls inside toPass to
synchronous checks using isVisible() (e.g., expect(await
menu.isVisible()).toBe(true) and expect(await
themeItems2.submenu.isVisible()).toBe(true)), keeping the other checks that call
topbar.isMenuItemActive(...) inside toPass; update references to menu,
themeItems2.submenu, topbar.isMenuItemActive, themeItems2.darkTheme, and
themeItems2.lightTheme accordingly.
In `@browser_tests/tests/vueNodes/widgets/load/uploadWidgets.spec.ts`:
- Around line 21-26: Replace the manual expect.poll(... .count()) checks with
Playwright's built-in locator assertions: use the getByText(...) locator's
.first() and call toBeVisible() so the assertions auto-retry and give better
errors; specifically update the two assertions that use
comfyPage.page.getByText('Error loading image') and
comfyPage.page.getByText('Error loading video') to assert .first().toBeVisible()
instead of polling counts.
In `@lint-staged.config.ts`:
- Around line 9-20: The inline arrow handler for the lint-staged entry should be
replaced with a named function declaration: extract the block currently assigned
to './**/*.{ts,tsx,vue,mts}': (stagedFiles: string[]) => { ... } into a declared
function (e.g., function lintStagedHandler(stagedFiles: string[])) containing
the same logic (commands creation, hasBrowserTestsChanges detection, push of
'pnpm typecheck:browser' when appropriate) and then reference that function in
the config object instead of the inline arrow; update any type annotations to
match and ensure formatAndEslint, path, and process.cwd() usages remain
unchanged.
- Around line 10-18: The code mutates the commands array via commands.push;
instead, build it immutably at declaration: compute hasBrowserTestsChanges (from
stagedFiles.map(...).some(...)) and then create a const commands using spreads
and a conditional spread to include 'pnpm typecheck:browser' when needed (e.g.,
const commands = [...formatAndEslint(stagedFiles), 'pnpm typecheck',
...(hasBrowserTestsChanges ? ['pnpm typecheck:browser'] : [])]); this avoids
mutation of commands and keeps logic around formatAndEslint, stagedFiles and the
conditional inclusion of typecheck:browser immutable.
In `@src/components/rightSidePanel/parameters/WidgetItem.test.ts`:
- Around line 156-203: Add tests covering edge cases for promoted widget
propagation: add one test using createMockPromotedWidgetView with options
undefined or {} and assert stub.props('widget').options is undefined or an empty
object as expected (via mountWidgetItem and StubWidgetComponent), and add
another test that stubs resolvePromotedWidgetSource to return a concrete
promoted source value then mountWidgetItem(createMockPromotedWidgetView(...))
and assert the stub receives the resolved properties (options/type/name/value)
to verify behavior when resolvePromotedWidgetSource returns a defined value.
- Around line 74-95: Replace the unsafe "as unknown as" assertions in the test
helpers with TypeScript's "satisfies" to validate the mock shapes: update
createMockNode to return an object expression that uses "satisfies LGraphNode"
(keeping the overrides: Partial<LGraphNode> and spread) and update
createMockWidget similarly to use "satisfies IBaseWidget" for the widget shape;
this preserves flexible Partial overrides while enabling compile-time shape
checks for createMockNode and createMockWidget.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
browser_tests/tests/nodeSearchBox.spec.ts-snapshots/auto-linked-node-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/nodeSearchBox.spec.ts-snapshots/link-context-menu-search-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/vueNodes/interactions/node/imagePreview.spec.ts-snapshots/vue-node-multiple-promoted-previews-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/vueNodes/widgets/load/uploadWidgets.spec.ts-snapshots/vue-nodes-upload-widgets-chromium-linux.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (33)
.github/workflows/ci-lint-format.yamlbrowser_tests/assets/subgraphs/subgraph-with-multiple-promoted-previews.jsonbrowser_tests/fixtures/helpers/CommandHelper.tsbrowser_tests/fixtures/helpers/DragDropHelper.tsbrowser_tests/fixtures/helpers/NodeOperationsHelper.tsbrowser_tests/fixtures/helpers/SubgraphHelper.tsbrowser_tests/fixtures/selectors.tsbrowser_tests/fixtures/utils/litegraphUtils.tsbrowser_tests/fixtures/ws.tsbrowser_tests/helpers/promotedWidgets.tsbrowser_tests/tests/browserTabTitle.spec.tsbrowser_tests/tests/changeTracker.spec.tsbrowser_tests/tests/colorPalette.spec.tsbrowser_tests/tests/dialog.spec.tsbrowser_tests/tests/menu.spec.tsbrowser_tests/tests/sidebar/workflows.spec.tsbrowser_tests/tests/subgraph.spec.tsbrowser_tests/tests/subgraphPromotion.spec.tsbrowser_tests/tests/vueNodes/interactions/node/imagePreview.spec.tsbrowser_tests/tests/vueNodes/widgets/load/uploadWidgets.spec.tslint-staged.config.tssrc/components/breadcrumb/SubgraphBreadcrumb.vuesrc/components/graph/DomWidgets.test.tssrc/components/graph/DomWidgets.vuesrc/components/graph/widgets/domWidgetZIndex.test.tssrc/components/graph/widgets/domWidgetZIndex.tssrc/components/rightSidePanel/RightSidePanel.vuesrc/components/rightSidePanel/parameters/SectionWidgets.vuesrc/components/rightSidePanel/parameters/TabSubgraphInputs.vuesrc/components/rightSidePanel/parameters/WidgetActions.vuesrc/components/rightSidePanel/parameters/WidgetItem.test.tssrc/components/rightSidePanel/parameters/WidgetItem.vuesrc/components/rightSidePanel/subgraph/SubgraphEditor.vue
💤 Files with no reviewable changes (2)
- src/components/rightSidePanel/RightSidePanel.vue
- browser_tests/fixtures/ws.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/rightSidePanel/parameters/WidgetActions.vue
DrJKL
added
the
New Browser Test Expectations
github-actions
bot
removed
the
New Browser Test Expectations
…tors - getPromotedWidgetCount now delegates to getPromotedWidgets (proxyWidgets) instead of counting all node widgets - Replace .comfy-missing-nodes CSS locator with data-testid in dialog.spec.ts - Add missingNodes to TestIds.dialogs in selectors.ts Amp-Thread-ID: https://ampcode.com/threads/T-019c8bcf-5b2b-7338-83f4-5c5a23b90e85 Co-authored-by: Amp <amp@ampcode.com>
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/locales/en/main.json`:
- Around line 1078-1082: Update the two locale strings for consistency: change
"enterSearchAliases" to use a hyphen (“Enter search aliases (comma-separated)”)
and change "unpromoteWidget" to match the promote phrasing by removing the
hyphen and capitalizing consistently (“Unpromote Widget: {name}”), leaving
"promoteWidget" unchanged; update these values in the JSON for the keys
enterSearchAliases and unpromoteWidget.
Comment on lines
+1078
to
+1082
| "enterSearchAliases": "Enter search aliases (comma separated)", | ||
| "disconnected": "Disconnected", | ||
| "linked": "(Linked)", | ||
| "promoteWidget": "Promote Widget: {name}", | ||
| "unpromoteWidget": "Un-Promote Widget: {name}" |
Contributor
There was a problem hiding this comment.
Fix wording consistency in new subgraphStore labels
Line 1078 should be hyphenated, and Line 1082’s “Un-Promote” is inconsistent with “Promote Widget.” Consider “Unpromote” for UI consistency.
💬 Proposed wording tweaks
- "enterSearchAliases": "Enter search aliases (comma separated)",
+ "enterSearchAliases": "Enter search aliases (comma-separated)",
@@
- "unpromoteWidget": "Un-Promote Widget: {name}"
+ "unpromoteWidget": "Unpromote Widget: {name}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/locales/en/main.json` around lines 1078 - 1082, Update the two locale
strings for consistency: change "enterSearchAliases" to use a hyphen (“Enter
search aliases (comma-separated)”) and change "unpromoteWidget" to match the
promote phrasing by removing the hyphen and capitalizing consistently
(“Unpromote Widget: {name}”), leaving "promoteWidget" unchanged; update these
values in the JSON for the keys enterSearchAliases and unpromoteWidget.
AustinMroz
approved these changes
Feb 23, 2026
Collaborator
AustinMroz
left a comment
AustinMroz
left a comment
There was a problem hiding this comment.
Lots of big and scary changes here, but it needs to be merged early into the release cycle. It has my approval.
Contributor
Author
YOLO |
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
Replace the Proxy-based proxy widget system with a store-driven architecture where
promotionStoreandwidgetValueStoreare the single sources of truth for subgraph widget promotion and widget values, andSubgraphNode.widgetsis a synthetic getter composing lightweightPromotedWidgetViewobjects from store state.Motivation
The subgraph widget promotion system previously scattered state across multiple unsynchronized layers:
node.properties.proxyWidgets(tuples on the LiteGraph node)proxyWidget.tswithOverlayobjects,DisconnectedWidgetsingleton, andisProxyWidgettype guardsparseProxyWidgets()viacustomRefhackswidget.promoted = true/falseset onsubgraph-openedeventsThis led to 4+ independent parsings of the same data, complex cache invalidation, and no reactive contract between the promotion state and the rendering layer. Widget values were similarly owned by LiteGraph with no Vue-reactive backing.
The core principle driving these changes: Vue owns truth. Pinia stores are the canonical source; LiteGraph objects delegate to stores via getters/setters; Vue components react to store state directly.
Changes
New stores (single sources of truth)
promotionStore— ReactiveMap<NodeId, PromotionEntry[]>tracking which interior widgets are promoted on which SubgraphNode instances. Graph-scoped by root graph ID to prevent cross-workflow state collision. Replacesproperties.proxyWidgetsparsing,customRefhacks,widget.promotedmutation, and thesubgraph-openedevent listener.widgetValueStore— Graph-scopedMap<WidgetKey, WidgetState>that is the canonical owner of widget values.BaseWidget.valuedelegates to this store via getter/setter when a node ID is assigned. Eliminates the need for Proxy-based value forwarding.Synthetic widgets getter (SubgraphNode)
SubgraphNode.widgetsis now a getter that readspromotionStore.getPromotions(rootGraphId, nodeId)and returns cachedPromotedWidgetViewobjects. No stubs, no Proxies, no fake widgets persisted in the array. The setter is a no-op — mutations go throughpromotionStore.PromotedWidgetView
A class behind a
createPromotedWidgetViewfactory, implementing thePromotedWidgetViewinterface. Delegates value/type/options/drawing to the resolved interior widget and stores. Owns positional state (y,computedHeight) for canvas layout. Cached byPromotedWidgetViewManagerfor object-identity stability across frames.DOM widget promotion
Promoted DOM widgets (textarea, image upload, etc.) render on the SubgraphNode surface via
positionOverrideindomWidgetStore.DomWidgets.vuechecks for overrides and uses the SubgraphNode's coordinates instead of the interior node's.Promoted previews
New
usePromotedPreviewscomposable resolves image/audio/video preview widgets from promoted entries, enabling SubgraphNodes to display previews of interior preview nodes.Deleted
proxyWidget.ts(257 lines) — Proxy handler,Overlay,newProxyWidget,isProxyWidgetDisconnectedWidget.ts(39 lines) — Singleton Proxy targetuseValueTransform.ts(32 lines) — Replaced by store delegationKey architectural changes
BaseWidget.valuegetter/setter delegates towidgetValueStorewhen node ID is setLGraph.add()reordered:node.graphassigned before widgetsetNodeId(enables store registration)LGraph.clear()cleans up graph-scoped stores to prevent stale entries across workflow switchespromotionStoreandwidgetValueStorestate nested under root graph UUID for multi-workflow isolationSubgraphNode.serialize()writes promotions back toproperties.proxyWidgetsfor persistence compatibility-1promotion entries resolved and migrated on first load with dev warningTest coverage
promotionStore.test.ts,widgetValueStore.test.ts,promotedWidgetView.test.ts(921 lines),subgraphNodePromotion.test.ts,proxyWidgetUtils.test.ts,DomWidgets.test.ts,PromotedWidgetViewManager.test.ts,usePromotedPreviews.test.ts,resolvePromotedWidget.test.ts,subgraphPseudoWidgetCache.test.tssubgraphPromotion.spec.ts(622 lines) — promote/demote, manual/auto promotion, paste preservation, seed control augmentation, image preview promotion;imagePreview.spec.tsextended with multi-promoted-preview coverageReview focus
rootGraphId) — verify isolation across workflows/tabs and cleanup onLGraph.clear()PromotedWidgetViewpositional stability —_arrangeWidgetswrites toy/computedHeighton cached objects; getter returns fresh array but stable object references-1entry migration — resolved and written back on first load; unresolvable entries dropped with dev warningpromotionStorestate →properties.proxyWidgetson serialize, hydrated back on configureDiff breakdown (excluding lockfile)
┆Issue is synchronized with this Notion page by Unito