Still Draft Feature: ECS Migration - #14246
Open
DrJKL wants to merge 25 commits into
Open
Conversation
🎭 Playwright: ✅ 1993 passed, 0 failed📊 Browser Reports
🎨 Storybook: ✅ Built — View Storybook📦 Bundle: 9.11 MB gzip 🟢 -1.88 kBDetailsSummary
Category Glance App Entry Points — 3.71 kB (baseline 3.71 kB) • 🟢 -2 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.39 MB (baseline 1.37 MB) • 🔴 +20.1 kBGraph editor runtime, canvas, workflow orchestration
Status: 2 added / 2 removed / 1 unchanged Views & Navigation — 124 kB (baseline 124 kB) • 🔴 +2 BTop-level views, pages, and routed surfaces
Status: 15 added / 15 removed / 2 unchanged Panels & Settings — 591 kB (baseline 591 kB) • 🟢 -5 BConfiguration panels, inspectors, and settings screens
Status: 12 added / 12 removed / 15 unchanged User & Accounts — 27.5 kB (baseline 27.5 kB) • 🔴 +1 BAuthentication, profile, and account management bundles
Status: 7 added / 7 removed / 4 unchanged Editors & Dialogs — 125 kB (baseline 125 kB) • 🟢 -2 BModals, dialogs, drawers, and in-app editors
Status: 8 added / 8 removed UI Components — 112 kB (baseline 67.1 kB) • 🔴 +44.9 kBReusable component library chunks
Status: 15 added / 14 removed Data & Services — 3.49 MB (baseline 3.53 MB) • 🟢 -34.1 kBStores, services, APIs, and repositories
Status: 15 added / 15 removed / 2 unchanged Utilities & Hooks — 549 kB (baseline 549 kB) • 🟢 -20 BHelpers, composables, and utility bundles
Status: 28 added / 28 removed / 9 unchanged Vendor & Third-Party — 18 MB (baseline 18.1 MB) • 🟢 -55.4 kBExternal libraries and shared vendor chunks
Status: 4 added / 4 removed / 14 unchanged Other — 14.1 MB (baseline 14.1 MB) • 🔴 +231 BBundles that do not match a named category
Status: 123 added / 123 removed / 162 unchanged ⚡ Performance
|
Contributor
|
Important Review skippedToo many files! This PR contains 400 files, which is 100 over the limit of 300. To get a review, reduce the PR to 300 files or fewer by splitting it into smaller PRs or changing its base branch. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (14)
📒 Files selected for processing (400)
You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR migrates LiteGraph's link, reroute, and node topology from mutable slot mirrors ( Estimated code review effort: 5 (Critical) | ~180+ minutes ChangesCore ECS store migration (link/reroute/node-data/badge stores)
Sequence Diagram(s)sequenceDiagram
participant LGraphNode as LGraphNode (connect)
participant LGraph as LGraph
participant LinkStore as linkStore (Pinia)
participant RerouteStore as rerouteStore (Pinia)
participant LayoutStore as layoutStore (Pinia)
LGraphNode->>LGraph: _addLink(link)
LGraph->>LinkStore: registerLink(graphId, topology)
LGraph->>RerouteStore: anchorRerouteChain(link)
RerouteStore->>LinkStore: derive membership from parentId chain
Note over LGraphNode,LinkStore: input.link / output.links no longer written directly
LGraphNode->>LGraph: disconnectInput/disconnectOutput
LGraph->>LinkStore: deleteLink / unregisterLinkTopology
LGraph->>LayoutStore: deleteLinkLayout(linkId)
sequenceDiagram
participant Node as LGraphNode
participant Events as graph.events
participant Hooks as useErrorClearingHooks
participant Telemetry as installNodeAddedTelemetry
Node->>Events: dispatch('node:added', { node })
Events->>Hooks: node:added listener
Hooks->>Hooks: install per-node error hooks
Events->>Telemetry: node:added listener
Telemetry->>Telemetry: trackNodeAdded(node)
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
DrJKL
added
the
New Browser Test Expectations
|
Updating Playwright Expectations |
github-actions
Bot
removed
the
New Browser Test Expectations
![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: 49
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/extensions/vueNodes/components/NodeSlots.vue (1)
10-19: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCollapse the per-slot lookups into one computed row list.
getActualInputIndexnow runs three times per input during render (:key,:index, and inside:connected), each a linearfindIndexovernodeData.inputs— O(N²) per node render on the canvas hot path. Building the rows once also removes the duplicatedrootGraphIdguards inisInputConnected/isOutputConnected.♻️ Sketch
const inputRows = computed(() => { const graphId = canvasStore.rootGraphId return filteredInputs.value.map((input, filteredIndex) => { const actualIndex = nodeData.inputs.indexOf(input) const index = actualIndex !== -1 ? actualIndex : filteredIndex return { input, index, connected: graphId ? linkStore.isInputSlotConnected(graphId, nodeData.id, index) : false } }) })- <InputSlot - v-for="(input, index) in filteredInputs" - :key="`input-${input.name}-${getActualInputIndex(input, index)}`" - :slot-data="input" - :node-type="nodeData?.type || ''" - :node-id="nodeData.id" - :has-error="inputHasError(input)" - :index="getActualInputIndex(input, index)" - :connected="isInputConnected(getActualInputIndex(input, index))" - /> + <InputSlot + v-for="row in inputRows" + :key="`input-${row.input.name}-${row.index}`" + :slot-data="row.input" + :node-type="nodeData.type" + :node-id="nodeData.id" + :has-error="inputHasError(row.input)" + :index="row.index" + :connected="row.connected" + />Also applies to: 119-125
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/extensions/vueNodes/components/NodeSlots.vue` around lines 10 - 19, In NodeSlots.vue, add computed inputRows and outputRows that resolve each slot’s actual index once and calculate connection state using the rootGraphId within the computed mapping. Update the InputSlot and OutputSlot v-for blocks to consume these row objects for keys, slot data, indices, and connected state, removing repeated getActualInputIndex calls and redundant rootGraphId guards from isInputConnected/isOutputConnected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@browser_tests/tests/vueNodes/interactions/links/linkInteraction.spec.ts`:
- Around line 1360-1365: Update the link-resolution logic near the Preview Image
node to stop reading the removed inputs[0].link mirror. Resolve the node’s
connected link through the graph topology API or graph.links, then use that
link’s identifier with getLink to obtain parentId while preserving the existing
failure messages.
In `@docs/architecture/entity-interactions.md`:
- Around line 106-108: Update the Legend in entity-interactions.md to define the
-.- edge style used by the derived linkIds and floatingLinkIds relationships,
including its meaning as a dotted, non-directional derived relationship. Keep
the existing owns, references, and extends legend entries unchanged.
In `@docs/architecture/link-topology-store.md`:
- Around line 18-21: Add language annotations to both new fenced code blocks:
use an appropriate tag such as ts or text for the LinkTopology shape in
docs/architecture/link-topology-store.md lines 18-21, and ts for the BadgeData
union in docs/architecture/node-badge-store.md lines 40-44, resolving
markdownlint MD040 without changing the documented content.
In `@docs/architecture/node-data-store.md`:
- Around line 28-43: Add the TypeScript language identifier to the NodeState
fenced code block in the architecture documentation, changing the opening fence
to use ts while leaving the schema content unchanged.
In `@docs/architecture/proto-ecs-stores.md`:
- Around line 388-413: Update the migration matrix and priority entries to
reflect the implemented state: add NodeDataStore-extracted data to the Node row,
remove stale input.link/output.links mirror work from the Link and Slot gaps,
and adjust the related priority descriptions accordingly. Use the existing
NodeDataStore and output-slot-connectivity documentation as the source of truth
while preserving the remaining unimplemented extraction work.
In `@src/components/builder/AppModeWidgetList.vue`:
- Around line 89-110: Make mappedSelections a pure computed by removing
ensureSelectedWidgetState from its getter and register resolved widgets in a
watch/watchEffect driven by resolvedInputs instead. In the computed, check
isWidgetInputLinked before calling nodeToNodeData so filtered linked inputs do
not build unused data; preserve the existing selection mapping for unlinked
ALWAYS-mode entries.
In `@src/composables/graph/useNodeErrorFlagSync.test.ts`:
- Around line 32-34: Restore Vitest mocks before each test in the setup for this
suite, alongside setActivePinia, so the rootGraph and isGraphReady getter spies
created by setupGraphWithStore and the subgraph tests do not accumulate across
it blocks. Use the project’s required reset/restore pattern consistently for all
tests.
In `@src/composables/graph/useVueNodeLifecycle.ts`:
- Around line 94-99: Update disposeVueNodeLayout to clear all node entries from
layoutStore after stopping synchronization and listeners, undoing the data
seeded by initializeVueNodeLayout. Ensure the shouldRenderVueNodes transition
handled by the whenever block leaves layoutStore empty when Vue rendering is
disabled, preserving the existing cleanup state resets.
In `@src/composables/node/useNodePricing.ts`:
- Around line 589-593: The cache-miss fallback in the node pricing evaluation
flow must use the previous label for the current signature rather than the
newest label from any cached signature. Update the state associated with
scheduleEvaluation or the surrounding caller to track and return the last label
per signature/read context, while preserving the empty-string fallback when no
matching prior label exists.
In `@src/core/graph/subgraph/promotionUtils.ts`:
- Around line 298-308: Replace the direct promotedState.label mutation in the
hostInput promotion block with the widget store’s dedicated label-update action,
adding that action if none exists. Pass hostInput.widgetId and sourceSlot.label
through the store API so state invariants, telemetry, and persistence hooks are
preserved.
In `@src/core/graph/widgets/dynamicWidgets.test.ts`:
- Around line 10-11: Remove the redundant module-level
setActivePinia(createTestingPinia({ stubActions: false })) call in
dynamicWidgets.test.ts, keeping the beforeEach setup as the sole initialization
so each test receives a fresh testing Pinia.
In `@src/core/graph/widgets/dynamicWidgets.ts`:
- Around line 269-276: Replace the deprecated indexed link lookup in the
topology loop with the graph’s store-consistent getLink() method, using
topology.id as the lookup key and preserving the existing missing-link continue
behavior.
- Around line 555-563: Move the syncNodeWidgetOrder(node) call out of the
toRemove iteration and invoke it once after the loop completes. Preserve the
existing widget removal and cleanup behavior, including skipping inputs without
a widget name.
In `@src/extensions/core/widgetInputs.test.ts`:
- Around line 19-37: Remove the explicit 30_000 ms timeout argument from the
synchronous test defined by “resets itself when the store reports a link the
graph cannot resolve,” leaving the test body and its assertions unchanged so
Vitest uses its default timeout.
In `@src/extensions/core/widgetValuePropagation.ts`:
- Around line 25-28: Update the early-return guard in the widget value
propagation function to account for both store links and extraLinks, allowing
propagation when extraLinks is non-empty even if linked is empty. Keep returning
early only when neither source contains endpoints, and preserve the existing
endpoint-merging behavior.
In `@src/lib/litegraph/src/__fixtures__/nodeHelpers.ts`:
- Around line 38-55: Update createTestWidgetNode to register an onTestFinished
cleanup alongside its WIDGET_NODE_TYPE registration, restoring the previous
LiteGraph.registered_node_types entry or removing the test type after the test
completes, matching createTestNode’s cleanup pattern. Keep node creation and
graph.add behavior unchanged.
In `@src/lib/litegraph/src/LGraph.inputSlotRealign.test.ts`:
- Around line 319-322: Update the parameter list for the parameterized test
“rekeys a serialized %s” to destructure each it.for tuple into its name and
usePurgedAlias values, ensuring both registered-link and purged-alias cases
execute with their intended boolean.
In `@src/lib/litegraph/src/LGraphCanvas.ts`:
- Line 5706: Update the clip-path shape selection near drawNodeShape to use
node.renderingShape instead of node.shape or a direct BOX fallback, keeping
clipping consistent with the shape resolved by the node body and its
class/default fallbacks.
In `@src/lib/litegraph/src/LGraphNode.test.ts`:
- Around line 878-899: Remove the redundant “defaults to a normal title” test
because it only checks constructor initialization. In the remaining “titleMode
in node state” test, avoid the private _state shape by asserting the NO_TITLE
value through the store-held state returned by getGraphNodesFor(...) or the
public title_mode accessor, while preserving coverage that TitlelessNode’s
static title_mode is propagated.
In `@src/lib/litegraph/src/LGraphNode.ts`:
- Around line 347-349: Make the _state accessors consistent by routing the id
setter, flags setter, and resizable setter through setTrackedState, ensuring
each emits node:property:changed like title, mode, color, bgcolor, and
showAdvanced. Update the affected setters and preserve their existing values and
behavior, including resizable changes triggered by pin() and configure().
- Around line 4023-4031: Optimize updateComputedDisabled to avoid repeated
inputs scans during rendering: update getSlotFromWidget or the surrounding
widget loop to return/reuse the matching input index and determine connectivity
directly from that result, rather than calling this.inputs.indexOf(slot) after
the lookup. Preserve the existing computedDisabled behavior for widgets without
slots, disabled widgets, and connected inputs.
- Around line 1149-1157: The output serialization mapping in LGraphNode
serialization currently masks a type mismatch with `@ts-expect-error`. Remove that
directive and reconcile the return type of outputAsSerialisable with the
ISserialisedNode outputs element type so the mapping type-checks without
suppression.
- Around line 1062-1070: Update the input restoration loop in configure() to
invoke onConnectionsChange only when the serialized input contains a link that
was successfully restored; skip the callback for bare inputs, matching the
output restoration behavior. Continue invoking onInputAdded for every input.
- Around line 1884-1911: Update removeInput so the replaceNodeInputs failure
path, identified when this.inputs still includes slotInfo, logs a warning before
returning. Keep the existing removal callback, floating-link adjustment, and
canvas-dirty behavior unchanged for successful removals.
In `@src/lib/litegraph/src/linkDeduplication.ts`:
- Around line 54-72: Remove the unused link lookup and its guard from
purgeOrphanedLinks; iterate over non-survivor ids and call graph._removeLink(id)
directly, preserving the survivor re-registration logic afterward.
In `@src/lib/litegraph/src/LLink.ts`:
- Around line 98-112: Update applyEndpointPatch to propagate failures from
useLinkStore().updateEndpoint instead of only logging result.error and returning
normally. Throw or otherwise surface the rejected patch through the existing
endpoint setter/configure call chain, while preserving direct Object.assign
behavior for unregistered links.
In `@src/lib/litegraph/src/node/slotLinks.ts`:
- Around line 156-158: Collapse the short single-statement if block in the
finalInputs validation by placing the condition and throw on one line,
preserving the existing duplicate-slot check and error message.
In `@src/lib/litegraph/src/node/slotUtils.test.ts`:
- Around line 32-43: Update the ordering assertion in the “serialises the links
leaving the slot, ascending by id” test to sort LinkId values with an explicit
numeric comparator. Keep the existing length assertion and verify
serialised.links against its numerically ascending copy.
In `@src/lib/litegraph/src/nodeBadgeDraw.ts`:
- Around line 76-86: The registerBadgeRowsProvider function permanently pins the
first provider and offers no cleanup path. Add an unregister/disposer mechanism
tied to the registered provider, or expose a reset API for HMR and tests, while
preserving rejection of conflicting active registrations.
- Around line 52-62: Update the badge construction loop to create the credits
icon only when row.kind is exactly 'credits', rather than treating every
non-core row as credits. Preserve core-row skipping and ensure other BadgeData
kinds are handled explicitly or rejected by the type system instead of silently
receiving the credits icon.
In `@src/lib/litegraph/src/subgraph/__fixtures__/subgraphHelpers.ts`:
- Around line 68-95: Update enableSubgraphNodeCreation to keep the event
listener and type-to-constructor registrations local to each helper instance.
Make the returned disposer remove its own listener and delete only registrations
whose current constructor still matches the locally stored constructor,
preventing helpers from affecting each other; add coverage that creates another
subgraph after disposal and verifies no registration occurs.
In `@src/lib/litegraph/src/subgraph/subgraphDeduplication.ts`:
- Around line 114-129: Parameterize findNextAvailableId with an identifier-space
label and use it in the exhaustion error instead of the hardcoded node-specific
message. Update each caller, including node and reroute ID allocation, to
provide the appropriate label while preserving the existing allocation behavior.
In `@src/lib/litegraph/src/subgraph/subgraphUtils.test.ts`:
- Line 3: Update the tests in subgraphUtils.test.ts to restore mocked globals
after every test, using an afterEach hook with vi.restoreAllMocks(). Ensure
console.error is restored even when assertions fail, and remove any reliance on
cleanup that only runs after successful assertions.
In `@src/platform/cloud/subscription/composables/useFreeTierQuota.ts`:
- Around line 28-31: Update hasInvalidNodes in useFreeTierQuota to derive the
root graph from useCanvasStore().currentGraph instead of the non-reactive
app.graph, preserving the existing graphCreditsBadges check and false fallback.
Ensure the composable accesses the reactive currentGraph source so
freeTierExecutionPermitted recalculates after workflow switches.
In `@src/renderer/extensions/minimap/data/MinimapDataSource.test.ts`:
- Around line 48-59: Update registerNodeState so useNodeDataStore().registerNode
receives the graphId parameter as its store partition key instead of the
hardcoded GRAPH_ID, while continuing to set the same graphId on the created node
state.
In `@src/renderer/extensions/vueNodes/components/LGraphNode.vue`:
- Around line 702-711: Update the widgetIds computed in LGraphNode.vue to derive
graph identity from the reactive canvasStore.rootGraphId instead of
app.rootGraph?.id. Preserve the existing empty-result guard and getNodeWidgetIds
lookup, ensuring hasRenderableWidgets and downstream computeds refresh when the
root graph is replaced.
In `@src/renderer/extensions/vueNodes/components/NodeHeader.test.ts`:
- Around line 22-32: Replace the local makeNodeData fixture with the shared
createNodeState fixture from litegraphTestUtils, adding the required import and
passing the existing overrides through it. Remove the hand-rolled NodeState
defaults so shared fields, including the ALWAYS mode, remain consistent.
In `@src/renderer/extensions/vueNodes/components/NodeWidgets.test.ts`:
- Around line 265-267: Replace the Tailwind class assertion in the NodeWidgets
test with an assertion on a semantic error signal exposed by the widget row,
such as data-has-error or aria-invalid. Update the relevant widget-row rendering
symbol to provide that signal if needed, while preserving the existing error
behavior.
In `@src/renderer/extensions/vueNodes/composables/useProcessedWidgets.test.ts`:
- Around line 342-351: Update the test around processWidgets to avoid asserting
the literal Tailwind value ring ring-component-node-widget-advanced. Verify the
advanced/non-advanced borderStyle behavior by comparing results for showAdvanced
or reuse a shared source constant, and apply the same change to the duplicate
assertion near the other advanced-widget test.
In `@src/renderer/extensions/vueNodes/composables/useProcessedWidgets.ts`:
- Around line 411-414: Update the valueTooltip logic in useProcessedWidgets so
object-valued asset/combo widgets do not produce a tooltip from String(value),
particularly avoiding “[object Object]”. Only attach the tooltip when the value
is a meaningful primitive string representation longer than 10 characters, while
preserving the existing type and length checks.
In `@src/renderer/glsl/useGLSLUniforms.test.ts`:
- Around line 21-32: Update makeGlslNode to use a real Subgraph and LiteGraph
links from the shared test factories instead of overriding getInputLink with a
hand-rolled inputs-to-subgraph mapping. Remove the getInputLink mock so
extractUniformSources exercises the actual LGraphNode.getInputLink behavior,
while preserving the test’s intended input-link setup.
In `@src/services/litegraphService.ts`:
- Around line 467-485: Extract the duplicated serialized-output merge into a
module-level helper, such as mergeSerialisedOutputs, preserving the existing
zip, RESERVED_KEYS merge, extra-output handling, and outputAsSerialisable
fallback. Replace the inline blocks in src/services/litegraphService.ts:467-485
and src/services/litegraphService.ts:579-597 with calls to the shared helper.
In `@src/stores/widgetValueStore.graphReactivity.test.ts`:
- Around line 279-293: Strengthen the test “registers plain render metadata for
non-promoted widgets” by replacing the definedness and absent sourceExecutionId
checks with assertions for the expected non-promoted render metadata, including
hasLayoutSize: false and isDOMWidget: false. Ensure the assertions distinguish
plain registration from promoted widget registration rather than only checking
defaults.
In `@src/stores/widgetValueStore.ts`:
- Around line 146-151: Update getWidgetRenderState to validate widgetId with the
existing isWidgetId guard before calling parseWidgetId; return undefined for
non-conforming IDs, matching getWidget’s behavior, while preserving the current
graph-state lookup for valid IDs.
- Around line 61-69: Split lazy creation from reads in the widget-order store:
add an ensure helper for mutation paths and a non-mutating peek helper for
reads. Update appendNodeWidgetOrder and setNodeWidgetOrder to use
ensureNodeWidgetOrder, and getNodeWidgetIds and reconcileNodeWidgetOrder to use
peekNodeWidgetOrder so computed reads do not insert entries. Apply the same
ensure/peek separation to getWidgetRenderState, ensuring read access does not
create per-graph maps.
In `@src/stores/workspace/favoritedWidgetsStore.ts`:
- Around line 76-79: Track the deferred migration from (nodeLocatorId,
widgetName) keys to host-scoped WidgetId keys by opening an issue that documents
the favoritedWidgets persistence-format change, required one-time migration of
workflow.extra.favoritedWidgets, and removal of the
isShownOnParents/favoriteNode indirection for promoted widgets.
In `@src/systems/badgeSystem.pricing.test.ts`:
- Around line 13-22: The getNodeDisplayPrice mock currently reads
useLinkStore().isInputSlotConnected, allowing the badge computed to react
without touchPricingSources registering the dependency. Make getNodeDisplayPrice
pure by deriving its result from node data or a plain test-controlled
counter/override, then update the test setup to flip that value and assert
recomputation specifically through touchPricingSources.
In `@src/systems/badgeSystem.ts`:
- Around line 212-220: Update the Settings map entries for
Comfy.NodeBadge.NodeIdBadgeMode, Comfy.NodeBadge.NodeLifeCycleBadgeMode, and
Comfy.NodeBadge.NodeSourceBadgeMode to use NodeBadgeMode, then remove the
corresponding assertions from the badgeModes initialization. Ensure
settingStore.get infers and returns NodeBadgeMode directly for these keys.
- Around line 167-187: Update gatherSubgraphCredits to touch all dynamic pricing
dependencies for the single inner API leaf, not only
pricing.getNodeRevisionRef(leaf.id). Reuse the dependency-registration behavior
from touchPricingSources, passing the appropriate inner graph-id keys from
wrapper.subgraph for priced widget values and input connections, while
preserving the existing multi-leaf and display-price behavior.
---
Outside diff comments:
In `@src/renderer/extensions/vueNodes/components/NodeSlots.vue`:
- Around line 10-19: In NodeSlots.vue, add computed inputRows and outputRows
that resolve each slot’s actual index once and calculate connection state using
the rootGraphId within the computed mapping. Update the InputSlot and OutputSlot
v-for blocks to consume these row objects for keys, slot data, indices, and
connected state, removing repeated getActualInputIndex calls and redundant
rootGraphId guards from isInputConnected/isOutputConnected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 77516a24-b905-4d0c-b70f-f93cbf262d19
⛔ Files ignored due to path filters (14)
browser_tests/tests/domWidget.spec.ts-snapshots/focus-mode-on-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/execution.spec.ts-snapshots/execution-error-unconnected-slot-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/graphCanvasMenu.spec.ts-snapshots/canvas-with-visible-links-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/mobileBaseline.spec.ts-snapshots/mobile-default-workflow-mobile-chrome-linux.pngis excluded by!**/*.pngbrowser_tests/tests/mobileBaseline.spec.ts-snapshots/mobile-graph-canvas-toolbar-mobile-chrome-linux.pngis excluded by!**/*.pngbrowser_tests/tests/nodeBadge.spec.ts-snapshots/node-badge-left-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/saveImageAndWebp.spec.ts-snapshots/save-image-and-webm-preview-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/selectionToolbox.spec.ts-snapshots/selection-toolbox-multiple-nodes-border-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/selectionToolbox.spec.ts-snapshots/selection-toolbox-multiple-selections-border-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/selectionToolbox.spec.ts-snapshots/selection-toolbox-single-node-no-border-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/viewport.spec.ts-snapshots/viewport-fits-when-saved-offscreen-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/vueNodes/interactions/canvas/pan.spec.ts-snapshots/vue-nodes-paned-with-touch-mobile-chrome-linux.pngis excluded by!**/*.pngbrowser_tests/tests/vueNodes/nodeStates/colors.spec.ts-snapshots/vue-node-custom-colors-dark-all-colors-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/vueNodes/nodeStates/colors.spec.ts-snapshots/vue-node-custom-colors-light-all-colors-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (272)
browser_tests/assets/missing/missing_model_nested_promoted_widget.jsonbrowser_tests/assets/missing/missing_models_in_subgraph.jsonbrowser_tests/assets/nodes/duplicate_node_ids.jsonbrowser_tests/fixtures/helpers/NodeOperationsHelper.tsbrowser_tests/tests/appModeBuilder.spec.tsbrowser_tests/tests/nodeBadge.spec.tsbrowser_tests/tests/nodeDisplay.spec.tsbrowser_tests/tests/propertiesPanel/errorsTabCloudMissingModels.spec.tsbrowser_tests/tests/propertiesPanel/errorsTabModeAware.spec.tsbrowser_tests/tests/vueNodes/interactions/links/linkInteraction.spec.tsbrowser_tests/tests/vueNodes/nodeStates/registration.spec.tsbrowser_tests/tests/vueNodes/widgets/legacy.spec.tsbrowser_tests/tests/vueNodes/widgets/widgetReactivity.spec.tsbrowser_tests/tests/workflowPersistence.spec.tsdocs/adr/0008-entity-component-system.mddocs/architecture/domain-glossary.mddocs/architecture/ecs-lifecycle-scenarios.mddocs/architecture/ecs-migration-plan.mddocs/architecture/ecs-target-architecture.mddocs/architecture/entity-interactions.mddocs/architecture/entity-problems.mddocs/architecture/link-topology-store.mddocs/architecture/node-badge-store.mddocs/architecture/node-data-store.mddocs/architecture/output-slot-connectivity.mddocs/architecture/proto-ecs-stores.mddocs/architecture/reroute-chain-store.mdsrc/components/builder/AppModeWidgetList.vuesrc/components/graph/GraphCanvas.vuesrc/components/graph/widgets/domWidgetZIndex.test.tssrc/components/rightSidePanel/errors/useErrorGroups.test.tssrc/components/rightSidePanel/errors/useErrorGroups.tssrc/components/rightSidePanel/parameters/SectionWidgets.vuesrc/components/rightSidePanel/parameters/TabSubgraphInputs.test.tssrc/components/rightSidePanel/parameters/TabSubgraphInputs.vuesrc/components/rightSidePanel/parameters/WidgetActions.test.tssrc/components/rightSidePanel/parameters/WidgetActions.vuesrc/components/rightSidePanel/parameters/WidgetItem.test.tssrc/components/rightSidePanel/parameters/WidgetItem.vuesrc/components/rightSidePanel/subgraph/SubgraphEditor.test.tssrc/components/rightSidePanel/subgraph/SubgraphEditor.vuesrc/composables/graph/useErrorClearingHooks.test.tssrc/composables/graph/useErrorClearingHooks.tssrc/composables/graph/useGraphNodeManager.test.tssrc/composables/graph/useGraphNodeManager.tssrc/composables/graph/useNodeErrorFlagSync.test.tssrc/composables/graph/useVueNodeLifecycle.test.tssrc/composables/graph/useVueNodeLifecycle.tssrc/composables/node/useNodeBadge.tssrc/composables/node/useNodePricing.test.tssrc/composables/node/useNodePricing.tssrc/composables/node/usePriceBadge.test.tssrc/composables/node/usePriceBadge.tssrc/composables/useUpstreamValue.test.tssrc/composables/useUpstreamValue.tssrc/core/graph/subgraph/migration/proxyWidgetMigration.test.tssrc/core/graph/subgraph/migration/proxyWidgetMigration.tssrc/core/graph/subgraph/promotedInputWidget.tssrc/core/graph/subgraph/promotionUtils.test.tssrc/core/graph/subgraph/promotionUtils.tssrc/core/graph/subgraph/resolveConcretePromotedWidget.test.tssrc/core/graph/subgraph/resolveSubgraphInputLink.tssrc/core/graph/widgets/dynamicWidgets.test.tssrc/core/graph/widgets/dynamicWidgets.tssrc/core/graph/widgets/matchTypeConfiguring.test.tssrc/extensions/core/groupNode.tssrc/extensions/core/load3d.tssrc/extensions/core/rerouteNode.tssrc/extensions/core/saveImageExtraOutput.test.tssrc/extensions/core/widgetInputs.test.tssrc/extensions/core/widgetInputs.tssrc/extensions/core/widgetValuePropagation.test.tssrc/extensions/core/widgetValuePropagation.tssrc/lib/litegraph/src/LGraph.inputSlotRealign.test.tssrc/lib/litegraph/src/LGraph.serialise.test.tssrc/lib/litegraph/src/LGraph.test.tssrc/lib/litegraph/src/LGraph.tssrc/lib/litegraph/src/LGraphBadge.tssrc/lib/litegraph/src/LGraphCanvas.clipboard.test.tssrc/lib/litegraph/src/LGraphCanvas.cloneZIndex.test.tssrc/lib/litegraph/src/LGraphCanvas.drawConnections.test.tssrc/lib/litegraph/src/LGraphCanvas.ghost.test.tssrc/lib/litegraph/src/LGraphCanvas.groupSelection.test.tssrc/lib/litegraph/src/LGraphCanvas.slotHitDetection.test.tssrc/lib/litegraph/src/LGraphCanvas.tssrc/lib/litegraph/src/LGraphNode.nodeState.test.tssrc/lib/litegraph/src/LGraphNode.test.tssrc/lib/litegraph/src/LGraphNode.tssrc/lib/litegraph/src/LGraphNodeProperties.test.tssrc/lib/litegraph/src/LGraphNodeProperties.tssrc/lib/litegraph/src/LLink.store.test.tssrc/lib/litegraph/src/LLink.test.tssrc/lib/litegraph/src/LLink.tssrc/lib/litegraph/src/LiteGraphGlobal.tssrc/lib/litegraph/src/Reroute.store.test.tssrc/lib/litegraph/src/Reroute.tssrc/lib/litegraph/src/__fixtures__/nodeHelpers.tssrc/lib/litegraph/src/canvas/FloatingRenderLink.test.tssrc/lib/litegraph/src/canvas/FloatingRenderLink.tssrc/lib/litegraph/src/canvas/LinkConnector.core.test.tssrc/lib/litegraph/src/canvas/LinkConnector.integration.test.tssrc/lib/litegraph/src/canvas/LinkConnector.test.tssrc/lib/litegraph/src/canvas/LinkConnector.tssrc/lib/litegraph/src/canvas/LinkConnectorSubgraphInputValidation.test.tssrc/lib/litegraph/src/canvas/ToInputFromIoNodeLink.tssrc/lib/litegraph/src/canvas/ToInputRenderLink.tssrc/lib/litegraph/src/infrastructure/LGraphEventMap.tssrc/lib/litegraph/src/interfaces.tssrc/lib/litegraph/src/linkDeduplication.tssrc/lib/litegraph/src/litegraph.tssrc/lib/litegraph/src/node/NodeInputSlot.test.tssrc/lib/litegraph/src/node/NodeInputSlot.tssrc/lib/litegraph/src/node/NodeOutputSlot.test.tssrc/lib/litegraph/src/node/NodeOutputSlot.tssrc/lib/litegraph/src/node/NodeSlot.test.tssrc/lib/litegraph/src/node/NodeSlot.tssrc/lib/litegraph/src/node/SlotBase.tssrc/lib/litegraph/src/node/slotEcosystemPatterns.test.tssrc/lib/litegraph/src/node/slotLinks.test.tssrc/lib/litegraph/src/node/slotLinks.tssrc/lib/litegraph/src/node/slotUtils.test.tssrc/lib/litegraph/src/node/slotUtils.tssrc/lib/litegraph/src/nodeBadgeDraw.test.tssrc/lib/litegraph/src/nodeBadgeDraw.tssrc/lib/litegraph/src/serialization.test.tssrc/lib/litegraph/src/subgraph/ExecutableNodeDTO.test.tssrc/lib/litegraph/src/subgraph/ExecutableNodeDTO.tssrc/lib/litegraph/src/subgraph/SubgraphConversion.test.tssrc/lib/litegraph/src/subgraph/SubgraphIO.test.tssrc/lib/litegraph/src/subgraph/SubgraphInput.tssrc/lib/litegraph/src/subgraph/SubgraphInputNode.tssrc/lib/litegraph/src/subgraph/SubgraphNode.tssrc/lib/litegraph/src/subgraph/SubgraphOutput.tssrc/lib/litegraph/src/subgraph/SubgraphSerialization.test.tssrc/lib/litegraph/src/subgraph/SubgraphSlotConnections.test.tssrc/lib/litegraph/src/subgraph/SubgraphWidgetPromotion.test.tssrc/lib/litegraph/src/subgraph/__fixtures__/README.mdsrc/lib/litegraph/src/subgraph/__fixtures__/subgraphHelpers.test.tssrc/lib/litegraph/src/subgraph/__fixtures__/subgraphHelpers.tssrc/lib/litegraph/src/subgraph/subgraphDeduplication.test.tssrc/lib/litegraph/src/subgraph/subgraphDeduplication.tssrc/lib/litegraph/src/subgraph/subgraphUtils.test.tssrc/lib/litegraph/src/subgraph/subgraphUtils.tssrc/lib/litegraph/src/types/graphTriggers.tssrc/lib/litegraph/src/types/serialisation.tssrc/lib/litegraph/src/types/widgets.tssrc/lib/litegraph/src/utils/collections.test.tssrc/lib/litegraph/src/utils/collections.tssrc/lib/litegraph/src/utils/widget.tssrc/lib/litegraph/src/widgets/BaseWidget.test.tssrc/lib/litegraph/src/widgets/BaseWidget.tssrc/locales/ar/main.jsonsrc/locales/en/main.jsonsrc/locales/es/main.jsonsrc/locales/fa/main.jsonsrc/locales/fr/main.jsonsrc/locales/he/main.jsonsrc/locales/ja/main.jsonsrc/locales/ko/main.jsonsrc/locales/pt-BR/main.jsonsrc/locales/ru/main.jsonsrc/locales/tr/main.jsonsrc/locales/zh-TW/main.jsonsrc/locales/zh/main.jsonsrc/platform/cloud/subscription/composables/useFreeTierQuota.tssrc/platform/missingMedia/missingMediaStore.tssrc/platform/missingModel/missingModelScan.test.tssrc/platform/missingModel/missingModelScan.tssrc/platform/missingModel/missingModelStore.tssrc/platform/nodeReplacement/useNodeReplacement.test.tssrc/platform/nodeReplacement/useNodeReplacement.tssrc/platform/telemetry/nodeAdded/installNodeAddedTelemetry.test.tssrc/platform/telemetry/nodeAdded/installNodeAddedTelemetry.tssrc/platform/workflow/core/services/workflowService.insertWorkflow.test.tssrc/platform/workflow/core/services/workflowService.tssrc/renderer/core/canvas/canvasStore.test.tssrc/renderer/core/canvas/canvasStore.tssrc/renderer/core/canvas/links/linkConnectorAdapter.tssrc/renderer/core/layout/operations/layoutMutations.test.tssrc/renderer/core/layout/operations/layoutMutations.tssrc/renderer/core/layout/store/layoutStore.test.tssrc/renderer/core/layout/store/layoutStore.tssrc/renderer/core/layout/types.tssrc/renderer/extensions/linearMode/PartnerNodesList.vuesrc/renderer/extensions/minimap/composables/useMinimap.test.tssrc/renderer/extensions/minimap/composables/useMinimapGraph.test.tssrc/renderer/extensions/minimap/composables/useMinimapGraph.tssrc/renderer/extensions/minimap/data/AbstractMinimapDataSource.tssrc/renderer/extensions/minimap/data/LayoutStoreDataSource.tssrc/renderer/extensions/minimap/data/MinimapDataSource.test.tssrc/renderer/extensions/minimap/data/MinimapDataSourceFactory.tssrc/renderer/extensions/minimap/minimapCanvasRenderer.test.tssrc/renderer/extensions/vueNodes/components/LGraphNode.subgraph.test.tssrc/renderer/extensions/vueNodes/components/LGraphNode.test.tssrc/renderer/extensions/vueNodes/components/LGraphNode.vuesrc/renderer/extensions/vueNodes/components/LGraphNodePreview.test.tssrc/renderer/extensions/vueNodes/components/LGraphNodePreview.vuesrc/renderer/extensions/vueNodes/components/NodeContent.vuesrc/renderer/extensions/vueNodes/components/NodeHeader.test.tssrc/renderer/extensions/vueNodes/components/NodeHeader.vuesrc/renderer/extensions/vueNodes/components/NodeSlots.test.tssrc/renderer/extensions/vueNodes/components/NodeSlots.vuesrc/renderer/extensions/vueNodes/components/NodeWidgets.test.tssrc/renderer/extensions/vueNodes/components/NodeWidgets.vuesrc/renderer/extensions/vueNodes/components/WidgetGrid.vuesrc/renderer/extensions/vueNodes/composables/useNodeEventHandlers.test.tssrc/renderer/extensions/vueNodes/composables/useNodeEventHandlers.tssrc/renderer/extensions/vueNodes/composables/useNodePointerInteractions.test.tssrc/renderer/extensions/vueNodes/composables/useNodePointerInteractions.tssrc/renderer/extensions/vueNodes/composables/useNodeTooltips.test.tssrc/renderer/extensions/vueNodes/composables/useNodeTooltips.tssrc/renderer/extensions/vueNodes/composables/usePartitionedBadges.test.tssrc/renderer/extensions/vueNodes/composables/usePartitionedBadges.tssrc/renderer/extensions/vueNodes/composables/useProcessedWidgets.test.tssrc/renderer/extensions/vueNodes/composables/useProcessedWidgets.tssrc/renderer/extensions/vueNodes/composables/useSlotLinkInteraction.autoPan.test.tssrc/renderer/extensions/vueNodes/composables/useSlotLinkInteraction.tssrc/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.test.tssrc/renderer/extensions/vueNodes/layout/nodeSizeReflow.test.tssrc/renderer/extensions/vueNodes/types/widgetGrid.tssrc/renderer/extensions/vueNodes/utils/__tests__/nodeDataUtils.test.tssrc/renderer/extensions/vueNodes/utils/nodeDataUtils.tssrc/renderer/extensions/vueNodes/utils/nodeErrorState.tssrc/renderer/extensions/vueNodes/widgets/components/WidgetButton.test.tssrc/renderer/extensions/vueNodes/widgets/components/WidgetButton.vuesrc/renderer/extensions/vueNodes/widgets/components/WidgetDOM.test.tssrc/renderer/extensions/vueNodes/widgets/components/WidgetDOM.vuesrc/renderer/extensions/vueNodes/widgets/components/WidgetLegacy.vuesrc/renderer/extensions/vueNodes/widgets/composables/useImageUploadWidget.test.tssrc/renderer/extensions/vueNodes/widgets/composables/useProgressTextWidget.tssrc/renderer/extensions/vueNodes/widgets/composables/useWidgetRenderer.test.tssrc/renderer/extensions/vueNodes/widgets/registry/widgetRegistry.tssrc/renderer/glsl/glslPreviewUtils.tssrc/renderer/glsl/useGLSLUniforms.test.tssrc/renderer/glsl/useGLSLUniforms.tssrc/scripts/app.test.tssrc/scripts/app.tssrc/scripts/promotedWidgetControl.test.tssrc/scripts/promotedWidgetControl.tssrc/scripts/widgets.tssrc/services/litegraphService.tssrc/stores/appModeStore.test.tssrc/stores/linkStore.test.tssrc/stores/linkStore.tssrc/stores/nodeDataStore.test.tssrc/stores/nodeDataStore.tssrc/stores/rerouteStore.test.tssrc/stores/rerouteStore.tssrc/stores/widgetValueStore.graphReactivity.test.tssrc/stores/widgetValueStore.test.tssrc/stores/widgetValueStore.tssrc/stores/workspace/favoritedWidgetsStore.tssrc/systems/badgeSystem.pricing.test.tssrc/systems/badgeSystem.subgraph.test.tssrc/systems/badgeSystem.test.tssrc/systems/badgeSystem.tssrc/types/badgeData.tssrc/types/linkTopology.tssrc/types/nodeState.tssrc/types/rerouteChain.tssrc/types/simplifiedWidget.tssrc/types/widgetState.tssrc/utils/__tests__/litegraphTestUtils.tssrc/utils/graphTraversalUtil.test.tssrc/utils/graphTraversalUtil.tssrc/utils/linkFixer.test.tssrc/utils/linkFixer.tssrc/utils/litegraphUtil.test.tssrc/utils/litegraphUtil.tssrc/utils/searchAndReplace.test.tssrc/utils/widgetUtil.tstools/devtools/web/legacyWidget.js
💤 Files with no reviewable changes (27)
- src/lib/litegraph/src/LGraphNodeProperties.test.ts
- src/composables/node/usePriceBadge.test.ts
- src/composables/node/usePriceBadge.ts
- src/lib/litegraph/src/LGraphNodeProperties.ts
- src/lib/litegraph/src/LGraphBadge.ts
- src/composables/graph/useGraphNodeManager.ts
- src/composables/graph/useGraphNodeManager.test.ts
- browser_tests/tests/nodeBadge.spec.ts
- src/locales/ru/main.json
- src/lib/litegraph/src/canvas/ToInputRenderLink.ts
- src/locales/ar/main.json
- src/locales/ja/main.json
- src/locales/zh/main.json
- src/lib/litegraph/src/types/graphTriggers.ts
- src/lib/litegraph/src/node/SlotBase.ts
- src/locales/zh-TW/main.json
- src/locales/tr/main.json
- src/lib/litegraph/src/canvas/ToInputFromIoNodeLink.ts
- src/locales/es/main.json
- src/locales/fr/main.json
- src/locales/fa/main.json
- src/scripts/app.ts
- browser_tests/fixtures/helpers/NodeOperationsHelper.ts
- src/renderer/core/layout/types.ts
- src/locales/ko/main.json
- src/locales/pt-BR/main.json
- src/locales/he/main.json
Comment on lines
+1360
to
+1365
| const linkId = graph.nodes.find((n) => n.title === 'Preview Image') | ||
| ?.inputs[0].link | ||
| if (!linkId) return 'failed to resolve link id' | ||
|
|
||
| const rerouteId = graph.getLink(linkId)?.parentId | ||
| if (!rerouteId) return 'failed to resolve reroute id' |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stop reading the removed input-link mirror.
input.link is a legacy mirror removed by this migration, so this poll cannot resolve the link. Find the Preview Image link through the graph topology API/store (or graph.links) before checking its parentId.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@browser_tests/tests/vueNodes/interactions/links/linkInteraction.spec.ts`
around lines 1360 - 1365, Update the link-resolution logic near the Preview
Image node to stop reading the removed inputs[0].link mirror. Resolve the node’s
connected link through the graph topology API or graph.links, then use that
link’s identifier with getLink to obtain parentId while preserving the existing
failure messages.
Comment on lines
+89
to
110
| const mappedSelections = computed((): WidgetEntry[] => { | ||
| return resolvedInputs.value.flatMap((entry) => { | ||
| if (entry.status !== 'resolved') return [] | ||
| const { widgetId, node, widget, config } = entry | ||
| if (node.mode !== LGraphEventMode.ALWAYS) return [] | ||
|
|
||
| if (!nodeDataByNode.has(node)) { | ||
| nodeDataByNode.set(node, nodeToNodeData(node)) | ||
| } | ||
| const fullNodeData = nodeDataByNode.get(node)! | ||
|
|
||
| const matchingWidget = fullNodeData.widgets?.find((vueWidget) => { | ||
| if (vueWidget.slotMetadata?.linked) return false | ||
| return vueWidget.widgetId === widgetId | ||
| }) | ||
| if (!matchingWidget) return [] | ||
|
|
||
| matchingWidget.slotMetadata = undefined | ||
| matchingWidget.nodeId = node.id | ||
| ensureSelectedWidgetState(widgetId, widget) | ||
| const fullNodeData = nodeToNodeData(node, widgetId) | ||
| if (isWidgetInputLinked(node, widget.name)) return [] | ||
|
|
||
| return [ | ||
| { | ||
| key: widgetId, | ||
| persistedHeight: config?.height, | ||
| description: config?.description, | ||
| nodeData: { | ||
| ...fullNodeData, | ||
| widgets: [matchingWidget] | ||
| }, | ||
| nodeData: fullNodeData, | ||
| widgetIds: [widgetId], | ||
| action: { widget, node } | ||
| } | ||
| ] | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
mappedSelections mutates store state while deriving.
ensureSelectedWidgetState writes into widgetValueStore from inside a computed getter that also reads widgetValueStore.getWidget(...). Registering a widget invalidates the map this getter just tracked, so the computed re-runs on its own write (it only converges because the second pass short-circuits). Registration belongs in a watchEffect/watch on resolvedInputs, leaving the computed pure.
Also, nodeToNodeData(node, widgetId) (which builds a drop indicator and an image URL) is evaluated before the isWidgetInputLinked early return, so it is wasted for every filtered-out row.
♻️ Proposed restructure
-const mappedSelections = computed((): WidgetEntry[] => {
+watchEffect(() => {
+ for (const entry of resolvedInputs.value) {
+ if (entry.status !== 'resolved') continue
+ ensureSelectedWidgetState(entry.widgetId, entry.widget)
+ }
+})
+
+const mappedSelections = computed((): WidgetEntry[] => {
return resolvedInputs.value.flatMap((entry) => {
if (entry.status !== 'resolved') return []
const { widgetId, node, widget, config } = entry
if (node.mode !== LGraphEventMode.ALWAYS) return []
-
- ensureSelectedWidgetState(widgetId, widget)
- const fullNodeData = nodeToNodeData(node, widgetId)
if (isWidgetInputLinked(node, widget.name)) return []
return [
{
key: widgetId,
persistedHeight: config?.height,
description: config?.description,
- nodeData: fullNodeData,
+ nodeData: nodeToNodeData(node, widgetId),
widgetIds: [widgetId],
action: { widget, node }
}
]
})
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const mappedSelections = computed((): WidgetEntry[] => { | |
| return resolvedInputs.value.flatMap((entry) => { | |
| if (entry.status !== 'resolved') return [] | |
| const { widgetId, node, widget, config } = entry | |
| if (node.mode !== LGraphEventMode.ALWAYS) return [] | |
| if (!nodeDataByNode.has(node)) { | |
| nodeDataByNode.set(node, nodeToNodeData(node)) | |
| } | |
| const fullNodeData = nodeDataByNode.get(node)! | |
| const matchingWidget = fullNodeData.widgets?.find((vueWidget) => { | |
| if (vueWidget.slotMetadata?.linked) return false | |
| return vueWidget.widgetId === widgetId | |
| }) | |
| if (!matchingWidget) return [] | |
| matchingWidget.slotMetadata = undefined | |
| matchingWidget.nodeId = node.id | |
| ensureSelectedWidgetState(widgetId, widget) | |
| const fullNodeData = nodeToNodeData(node, widgetId) | |
| if (isWidgetInputLinked(node, widget.name)) return [] | |
| return [ | |
| { | |
| key: widgetId, | |
| persistedHeight: config?.height, | |
| description: config?.description, | |
| nodeData: { | |
| ...fullNodeData, | |
| widgets: [matchingWidget] | |
| }, | |
| nodeData: fullNodeData, | |
| widgetIds: [widgetId], | |
| action: { widget, node } | |
| } | |
| ] | |
| }) | |
| }) | |
| watchEffect(() => { | |
| for (const entry of resolvedInputs.value) { | |
| if (entry.status !== 'resolved') continue | |
| ensureSelectedWidgetState(entry.widgetId, entry.widget) | |
| } | |
| }) | |
| const mappedSelections = computed((): WidgetEntry[] => { | |
| return resolvedInputs.value.flatMap((entry) => { | |
| if (entry.status !== 'resolved') return [] | |
| const { widgetId, node, widget, config } = entry | |
| if (node.mode !== LGraphEventMode.ALWAYS) return [] | |
| if (isWidgetInputLinked(node, widget.name)) return [] | |
| return [ | |
| { | |
| key: widgetId, | |
| persistedHeight: config?.height, | |
| description: config?.description, | |
| nodeData: nodeToNodeData(node, widgetId), | |
| widgetIds: [widgetId], | |
| action: { widget, node } | |
| } | |
| ] | |
| }) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/builder/AppModeWidgetList.vue` around lines 89 - 110, Make
mappedSelections a pure computed by removing ensureSelectedWidgetState from its
getter and register resolved widgets in a watch/watchEffect driven by
resolvedInputs instead. In the computed, check isWidgetInputLinked before
calling nodeToNodeData so filtered linked inputs do not build unused data;
preserve the existing selection mapping for unlinked ALWAYS-mode entries.
Comment on lines
+32
to
+34
| vi.spyOn(app, 'rootGraph', 'get').mockReturnValue(graph) | ||
| vi.spyOn(app, 'isGraphReady', 'get').mockReturnValue(true) | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Spies on app.rootGraph/app.isGraphReady are never restored between tests.
setupGraphWithStore() (and the two subgraph tests) call vi.spyOn(app, 'rootGraph', 'get')/vi.spyOn(app, 'isGraphReady', 'get') repeatedly across 7 it() blocks, but beforeEach only calls setActivePinia(...) — there's no vi.restoreAllMocks()/vi.resetAllMocks(). Each new vi.spyOn wraps the previous spy instead of replacing it cleanly, which is harmless today but is exactly the pattern the testing guidance calls out.
♻️ Suggested fix
beforeEach(() => {
setActivePinia(createTestingPinia({ stubActions: false }))
})
+
+ afterEach(() => {
+ vi.restoreAllMocks()
+ })Based on learnings, "Mocking: keep vi.mock() module mocks contained; reset with vi.resetAllMocks() in beforeEach" (docs/testing/vitest-patterns.md, required context for this test file).
Also applies to: 127-129, 208-210
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/composables/graph/useNodeErrorFlagSync.test.ts` around lines 32 - 34,
Restore Vitest mocks before each test in the setup for this suite, alongside
setActivePinia, so the rootGraph and isGraphReady getter spies created by
setupGraphWithStore and the subgraph tests do not accumulate across it blocks.
Use the project’s required reset/restore pattern consistently for all tests.
Source: Path instructions
Comment on lines
+146
to
+151
| function getWidgetRenderState( | ||
| widgetId: WidgetId | ||
| ): WidgetRenderState | undefined { | ||
| const { graphId } = parseWidgetId(widgetId) | ||
| return getGraphWidgetRenderStates(graphId).get(widgetId) | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
getWidgetRenderState lacks the isWidgetId guard its sibling has, so a bad id throws.
getWidget returns undefined for a non-conforming id, but getWidgetRenderState goes straight to parseWidgetId, which throws Invalid widget id. It's called per-widget from useProcessedWidgets.processWidget inside a computed, so one malformed id from an externally supplied widgetIds list takes down the whole node's widget rendering rather than skipping a row.
🛡️ Proposed fix
function getWidgetRenderState(
widgetId: WidgetId
): WidgetRenderState | undefined {
+ if (!isWidgetId(widgetId)) return undefined
+
const { graphId } = parseWidgetId(widgetId)
return getGraphWidgetRenderStates(graphId).get(widgetId)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function getWidgetRenderState( | |
| widgetId: WidgetId | |
| ): WidgetRenderState | undefined { | |
| const { graphId } = parseWidgetId(widgetId) | |
| return getGraphWidgetRenderStates(graphId).get(widgetId) | |
| } | |
| function getWidgetRenderState( | |
| widgetId: WidgetId | |
| ): WidgetRenderState | undefined { | |
| if (!isWidgetId(widgetId)) return undefined | |
| const { graphId } = parseWidgetId(widgetId) | |
| return getGraphWidgetRenderStates(graphId).get(widgetId) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/stores/widgetValueStore.ts` around lines 146 - 151, Update
getWidgetRenderState to validate widgetId with the existing isWidgetId guard
before calling parseWidgetId; return undefined for non-conforming IDs, matching
getWidget’s behavior, while preserving the current graph-state lookup for valid
IDs.
Comment on lines
+167
to
+187
| function gatherSubgraphCredits(wrapper: SubgraphNode): PricingBadgeSources { | ||
| const pricing = useNodePricing() | ||
| const apiLeaves = mapUniqueNodes(wrapper.subgraph, (node) => | ||
| !node.isSubgraphNode() && node.constructor?.nodeData?.api_node | ||
| ? node | ||
| : undefined | ||
| ) | ||
| for (const leaf of apiLeaves) { | ||
| void pricing.getNodeRevisionRef(leaf.id).value | ||
| } | ||
|
|
||
| if (apiLeaves.length !== 1) { | ||
| return { kind: 'subgraph', apiNodeCount: apiLeaves.length, singleLabel: '' } | ||
| } | ||
| const leaf = apiLeaves[0] | ||
| const singleLabel = pricing.getNodeDisplayPrice( | ||
| leaf, | ||
| collectPromotedOverrides(wrapper, leaf) | ||
| ) | ||
| return { kind: 'subgraph', apiNodeCount: 1, singleLabel } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Wrapper credits don't track the inner node's dynamic pricing sources.
The non-subgraph path calls touchPricingSources(graphId, node) (Line 193), which registers dependencies on priced widget values and priced input connections. gatherSubgraphCredits only touches getNodeRevisionRef(leaf.id), and collectPromotedOverrides only reads promoted widget values. An inner api node priced on a non-promoted widget or on an inner input connection will therefore keep a stale wrapper badge, since the revision ref only bumps after an evaluation that this computed is itself gating.
🐛 Proposed fix for the single-leaf case
const leaf = apiLeaves[0]
+ const graphId = wrapper.graph?.rootGraph.id
+ if (graphId !== undefined) touchPricingSources(graphId, leaf)
const singleLabel = pricing.getNodeDisplayPrice(
leaf,
collectPromotedOverrides(wrapper, leaf)
)Note the inner node lives in wrapper.subgraph; use whichever graph id keys the link/widget stores for inner nodes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function gatherSubgraphCredits(wrapper: SubgraphNode): PricingBadgeSources { | |
| const pricing = useNodePricing() | |
| const apiLeaves = mapUniqueNodes(wrapper.subgraph, (node) => | |
| !node.isSubgraphNode() && node.constructor?.nodeData?.api_node | |
| ? node | |
| : undefined | |
| ) | |
| for (const leaf of apiLeaves) { | |
| void pricing.getNodeRevisionRef(leaf.id).value | |
| } | |
| if (apiLeaves.length !== 1) { | |
| return { kind: 'subgraph', apiNodeCount: apiLeaves.length, singleLabel: '' } | |
| } | |
| const leaf = apiLeaves[0] | |
| const singleLabel = pricing.getNodeDisplayPrice( | |
| leaf, | |
| collectPromotedOverrides(wrapper, leaf) | |
| ) | |
| return { kind: 'subgraph', apiNodeCount: 1, singleLabel } | |
| } | |
| function gatherSubgraphCredits(wrapper: SubgraphNode): PricingBadgeSources { | |
| const pricing = useNodePricing() | |
| const apiLeaves = mapUniqueNodes(wrapper.subgraph, (node) => | |
| !node.isSubgraphNode() && node.constructor?.nodeData?.api_node | |
| ? node | |
| : undefined | |
| ) | |
| for (const leaf of apiLeaves) { | |
| void pricing.getNodeRevisionRef(leaf.id).value | |
| } | |
| if (apiLeaves.length !== 1) { | |
| return { kind: 'subgraph', apiNodeCount: apiLeaves.length, singleLabel: '' } | |
| } | |
| const leaf = apiLeaves[0] | |
| const graphId = wrapper.graph?.rootGraph.id | |
| if (graphId !== undefined) touchPricingSources(graphId, leaf) | |
| const singleLabel = pricing.getNodeDisplayPrice( | |
| leaf, | |
| collectPromotedOverrides(wrapper, leaf) | |
| ) | |
| return { kind: 'subgraph', apiNodeCount: 1, singleLabel } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/systems/badgeSystem.ts` around lines 167 - 187, Update
gatherSubgraphCredits to touch all dynamic pricing dependencies for the single
inner API leaf, not only pricing.getNodeRevisionRef(leaf.id). Reuse the
dependency-registration behavior from touchPricingSources, passing the
appropriate inner graph-id keys from wrapper.subgraph for priced widget values
and input connections, while preserving the existing multi-leaf and
display-price behavior.
Comment on lines
+28
to
+43
| ``` | ||
| NodeState { | ||
| id: NodeId | ||
| graphId: UUID // owning (sub)graph — partitioning + locator ids | ||
| type: string // identity | ||
| title: string | ||
| titleMode?: TitleMode | ||
| mode: LGraphEventMode | ||
| flags: { collapsed?, pinned?, ghost? } | ||
| color?: string | ||
| bgcolor?: string | ||
| shape?: RenderShape | ||
| resizable?: boolean | ||
| showAdvanced?: boolean | ||
| } | ||
| ``` |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language to the NodeState code fence.
Use ts so Markdown linting passes and the schema is highlighted correctly.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 28-28: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/architecture/node-data-store.md` around lines 28 - 43, Add the
TypeScript language identifier to the NodeState fenced code block in the
architecture documentation, changing the opening fence to use ts while leaving
the schema content unchanged.
Source: Linters/SAST tools
| import { createTestingPinia } from '@pinia/testing' | ||
| import { setActivePinia } from 'pinia' | ||
| import { beforeEach, describe, expect, it } from 'vitest' | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest' |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Restore the console spy unconditionally.
An assertion failure skips mockRestore(), leaving console.error mocked for
later tests. Use afterEach(() => vi.restoreAllMocks()) or a try/finally.
Also applies to: 35-50
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/litegraph/src/subgraph/subgraphUtils.test.ts` at line 3, Update the
tests in subgraphUtils.test.ts to restore mocked globals after every test, using
an afterEach hook with vi.restoreAllMocks(). Ensure console.error is restored
even when assertions fail, and remove any reliance on cleanup that only runs
after successful assertions.
Source: Path instructions
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 49
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/extensions/vueNodes/components/NodeSlots.vue (1)
10-19: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCollapse the per-slot lookups into one computed row list.
getActualInputIndexnow runs three times per input during render (:key,:index, and inside:connected), each a linearfindIndexovernodeData.inputs— O(N²) per node render on the canvas hot path. Building the rows once also removes the duplicatedrootGraphIdguards inisInputConnected/isOutputConnected.♻️ Sketch
const inputRows = computed(() => { const graphId = canvasStore.rootGraphId return filteredInputs.value.map((input, filteredIndex) => { const actualIndex = nodeData.inputs.indexOf(input) const index = actualIndex !== -1 ? actualIndex : filteredIndex return { input, index, connected: graphId ? linkStore.isInputSlotConnected(graphId, nodeData.id, index) : false } }) })- <InputSlot - v-for="(input, index) in filteredInputs" - :key="`input-${input.name}-${getActualInputIndex(input, index)}`" - :slot-data="input" - :node-type="nodeData?.type || ''" - :node-id="nodeData.id" - :has-error="inputHasError(input)" - :index="getActualInputIndex(input, index)" - :connected="isInputConnected(getActualInputIndex(input, index))" - /> + <InputSlot + v-for="row in inputRows" + :key="`input-${row.input.name}-${row.index}`" + :slot-data="row.input" + :node-type="nodeData.type" + :node-id="nodeData.id" + :has-error="inputHasError(row.input)" + :index="row.index" + :connected="row.connected" + />Also applies to: 119-125
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/extensions/vueNodes/components/NodeSlots.vue` around lines 10 - 19, In NodeSlots.vue, add computed inputRows and outputRows that resolve each slot’s actual index once and calculate connection state using the rootGraphId within the computed mapping. Update the InputSlot and OutputSlot v-for blocks to consume these row objects for keys, slot data, indices, and connected state, removing repeated getActualInputIndex calls and redundant rootGraphId guards from isInputConnected/isOutputConnected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@browser_tests/tests/vueNodes/interactions/links/linkInteraction.spec.ts`:
- Around line 1360-1365: Update the link-resolution logic near the Preview Image
node to stop reading the removed inputs[0].link mirror. Resolve the node’s
connected link through the graph topology API or graph.links, then use that
link’s identifier with getLink to obtain parentId while preserving the existing
failure messages.
In `@docs/architecture/entity-interactions.md`:
- Around line 106-108: Update the Legend in entity-interactions.md to define the
-.- edge style used by the derived linkIds and floatingLinkIds relationships,
including its meaning as a dotted, non-directional derived relationship. Keep
the existing owns, references, and extends legend entries unchanged.
In `@docs/architecture/link-topology-store.md`:
- Around line 18-21: Add language annotations to both new fenced code blocks:
use an appropriate tag such as ts or text for the LinkTopology shape in
docs/architecture/link-topology-store.md lines 18-21, and ts for the BadgeData
union in docs/architecture/node-badge-store.md lines 40-44, resolving
markdownlint MD040 without changing the documented content.
In `@docs/architecture/node-data-store.md`:
- Around line 28-43: Add the TypeScript language identifier to the NodeState
fenced code block in the architecture documentation, changing the opening fence
to use ts while leaving the schema content unchanged.
In `@docs/architecture/proto-ecs-stores.md`:
- Around line 388-413: Update the migration matrix and priority entries to
reflect the implemented state: add NodeDataStore-extracted data to the Node row,
remove stale input.link/output.links mirror work from the Link and Slot gaps,
and adjust the related priority descriptions accordingly. Use the existing
NodeDataStore and output-slot-connectivity documentation as the source of truth
while preserving the remaining unimplemented extraction work.
In `@src/components/builder/AppModeWidgetList.vue`:
- Around line 89-110: Make mappedSelections a pure computed by removing
ensureSelectedWidgetState from its getter and register resolved widgets in a
watch/watchEffect driven by resolvedInputs instead. In the computed, check
isWidgetInputLinked before calling nodeToNodeData so filtered linked inputs do
not build unused data; preserve the existing selection mapping for unlinked
ALWAYS-mode entries.
In `@src/composables/graph/useNodeErrorFlagSync.test.ts`:
- Around line 32-34: Restore Vitest mocks before each test in the setup for this
suite, alongside setActivePinia, so the rootGraph and isGraphReady getter spies
created by setupGraphWithStore and the subgraph tests do not accumulate across
it blocks. Use the project’s required reset/restore pattern consistently for all
tests.
In `@src/composables/graph/useVueNodeLifecycle.ts`:
- Around line 94-99: Update disposeVueNodeLayout to clear all node entries from
layoutStore after stopping synchronization and listeners, undoing the data
seeded by initializeVueNodeLayout. Ensure the shouldRenderVueNodes transition
handled by the whenever block leaves layoutStore empty when Vue rendering is
disabled, preserving the existing cleanup state resets.
In `@src/composables/node/useNodePricing.ts`:
- Around line 589-593: The cache-miss fallback in the node pricing evaluation
flow must use the previous label for the current signature rather than the
newest label from any cached signature. Update the state associated with
scheduleEvaluation or the surrounding caller to track and return the last label
per signature/read context, while preserving the empty-string fallback when no
matching prior label exists.
In `@src/core/graph/subgraph/promotionUtils.ts`:
- Around line 298-308: Replace the direct promotedState.label mutation in the
hostInput promotion block with the widget store’s dedicated label-update action,
adding that action if none exists. Pass hostInput.widgetId and sourceSlot.label
through the store API so state invariants, telemetry, and persistence hooks are
preserved.
In `@src/core/graph/widgets/dynamicWidgets.test.ts`:
- Around line 10-11: Remove the redundant module-level
setActivePinia(createTestingPinia({ stubActions: false })) call in
dynamicWidgets.test.ts, keeping the beforeEach setup as the sole initialization
so each test receives a fresh testing Pinia.
In `@src/core/graph/widgets/dynamicWidgets.ts`:
- Around line 269-276: Replace the deprecated indexed link lookup in the
topology loop with the graph’s store-consistent getLink() method, using
topology.id as the lookup key and preserving the existing missing-link continue
behavior.
- Around line 555-563: Move the syncNodeWidgetOrder(node) call out of the
toRemove iteration and invoke it once after the loop completes. Preserve the
existing widget removal and cleanup behavior, including skipping inputs without
a widget name.
In `@src/extensions/core/widgetInputs.test.ts`:
- Around line 19-37: Remove the explicit 30_000 ms timeout argument from the
synchronous test defined by “resets itself when the store reports a link the
graph cannot resolve,” leaving the test body and its assertions unchanged so
Vitest uses its default timeout.
In `@src/extensions/core/widgetValuePropagation.ts`:
- Around line 25-28: Update the early-return guard in the widget value
propagation function to account for both store links and extraLinks, allowing
propagation when extraLinks is non-empty even if linked is empty. Keep returning
early only when neither source contains endpoints, and preserve the existing
endpoint-merging behavior.
In `@src/lib/litegraph/src/__fixtures__/nodeHelpers.ts`:
- Around line 38-55: Update createTestWidgetNode to register an onTestFinished
cleanup alongside its WIDGET_NODE_TYPE registration, restoring the previous
LiteGraph.registered_node_types entry or removing the test type after the test
completes, matching createTestNode’s cleanup pattern. Keep node creation and
graph.add behavior unchanged.
In `@src/lib/litegraph/src/LGraph.inputSlotRealign.test.ts`:
- Around line 319-322: Update the parameter list for the parameterized test
“rekeys a serialized %s” to destructure each it.for tuple into its name and
usePurgedAlias values, ensuring both registered-link and purged-alias cases
execute with their intended boolean.
In `@src/lib/litegraph/src/LGraphCanvas.ts`:
- Line 5706: Update the clip-path shape selection near drawNodeShape to use
node.renderingShape instead of node.shape or a direct BOX fallback, keeping
clipping consistent with the shape resolved by the node body and its
class/default fallbacks.
In `@src/lib/litegraph/src/LGraphNode.test.ts`:
- Around line 878-899: Remove the redundant “defaults to a normal title” test
because it only checks constructor initialization. In the remaining “titleMode
in node state” test, avoid the private _state shape by asserting the NO_TITLE
value through the store-held state returned by getGraphNodesFor(...) or the
public title_mode accessor, while preserving coverage that TitlelessNode’s
static title_mode is propagated.
In `@src/lib/litegraph/src/LGraphNode.ts`:
- Around line 347-349: Make the _state accessors consistent by routing the id
setter, flags setter, and resizable setter through setTrackedState, ensuring
each emits node:property:changed like title, mode, color, bgcolor, and
showAdvanced. Update the affected setters and preserve their existing values and
behavior, including resizable changes triggered by pin() and configure().
- Around line 4023-4031: Optimize updateComputedDisabled to avoid repeated
inputs scans during rendering: update getSlotFromWidget or the surrounding
widget loop to return/reuse the matching input index and determine connectivity
directly from that result, rather than calling this.inputs.indexOf(slot) after
the lookup. Preserve the existing computedDisabled behavior for widgets without
slots, disabled widgets, and connected inputs.
- Around line 1149-1157: The output serialization mapping in LGraphNode
serialization currently masks a type mismatch with `@ts-expect-error`. Remove that
directive and reconcile the return type of outputAsSerialisable with the
ISserialisedNode outputs element type so the mapping type-checks without
suppression.
- Around line 1062-1070: Update the input restoration loop in configure() to
invoke onConnectionsChange only when the serialized input contains a link that
was successfully restored; skip the callback for bare inputs, matching the
output restoration behavior. Continue invoking onInputAdded for every input.
- Around line 1884-1911: Update removeInput so the replaceNodeInputs failure
path, identified when this.inputs still includes slotInfo, logs a warning before
returning. Keep the existing removal callback, floating-link adjustment, and
canvas-dirty behavior unchanged for successful removals.
In `@src/lib/litegraph/src/linkDeduplication.ts`:
- Around line 54-72: Remove the unused link lookup and its guard from
purgeOrphanedLinks; iterate over non-survivor ids and call graph._removeLink(id)
directly, preserving the survivor re-registration logic afterward.
In `@src/lib/litegraph/src/LLink.ts`:
- Around line 98-112: Update applyEndpointPatch to propagate failures from
useLinkStore().updateEndpoint instead of only logging result.error and returning
normally. Throw or otherwise surface the rejected patch through the existing
endpoint setter/configure call chain, while preserving direct Object.assign
behavior for unregistered links.
In `@src/lib/litegraph/src/node/slotLinks.ts`:
- Around line 156-158: Collapse the short single-statement if block in the
finalInputs validation by placing the condition and throw on one line,
preserving the existing duplicate-slot check and error message.
In `@src/lib/litegraph/src/node/slotUtils.test.ts`:
- Around line 32-43: Update the ordering assertion in the “serialises the links
leaving the slot, ascending by id” test to sort LinkId values with an explicit
numeric comparator. Keep the existing length assertion and verify
serialised.links against its numerically ascending copy.
In `@src/lib/litegraph/src/nodeBadgeDraw.ts`:
- Around line 76-86: The registerBadgeRowsProvider function permanently pins the
first provider and offers no cleanup path. Add an unregister/disposer mechanism
tied to the registered provider, or expose a reset API for HMR and tests, while
preserving rejection of conflicting active registrations.
- Around line 52-62: Update the badge construction loop to create the credits
icon only when row.kind is exactly 'credits', rather than treating every
non-core row as credits. Preserve core-row skipping and ensure other BadgeData
kinds are handled explicitly or rejected by the type system instead of silently
receiving the credits icon.
In `@src/lib/litegraph/src/subgraph/__fixtures__/subgraphHelpers.ts`:
- Around line 68-95: Update enableSubgraphNodeCreation to keep the event
listener and type-to-constructor registrations local to each helper instance.
Make the returned disposer remove its own listener and delete only registrations
whose current constructor still matches the locally stored constructor,
preventing helpers from affecting each other; add coverage that creates another
subgraph after disposal and verifies no registration occurs.
In `@src/lib/litegraph/src/subgraph/subgraphDeduplication.ts`:
- Around line 114-129: Parameterize findNextAvailableId with an identifier-space
label and use it in the exhaustion error instead of the hardcoded node-specific
message. Update each caller, including node and reroute ID allocation, to
provide the appropriate label while preserving the existing allocation behavior.
In `@src/lib/litegraph/src/subgraph/subgraphUtils.test.ts`:
- Line 3: Update the tests in subgraphUtils.test.ts to restore mocked globals
after every test, using an afterEach hook with vi.restoreAllMocks(). Ensure
console.error is restored even when assertions fail, and remove any reliance on
cleanup that only runs after successful assertions.
In `@src/platform/cloud/subscription/composables/useFreeTierQuota.ts`:
- Around line 28-31: Update hasInvalidNodes in useFreeTierQuota to derive the
root graph from useCanvasStore().currentGraph instead of the non-reactive
app.graph, preserving the existing graphCreditsBadges check and false fallback.
Ensure the composable accesses the reactive currentGraph source so
freeTierExecutionPermitted recalculates after workflow switches.
In `@src/renderer/extensions/minimap/data/MinimapDataSource.test.ts`:
- Around line 48-59: Update registerNodeState so useNodeDataStore().registerNode
receives the graphId parameter as its store partition key instead of the
hardcoded GRAPH_ID, while continuing to set the same graphId on the created node
state.
In `@src/renderer/extensions/vueNodes/components/LGraphNode.vue`:
- Around line 702-711: Update the widgetIds computed in LGraphNode.vue to derive
graph identity from the reactive canvasStore.rootGraphId instead of
app.rootGraph?.id. Preserve the existing empty-result guard and getNodeWidgetIds
lookup, ensuring hasRenderableWidgets and downstream computeds refresh when the
root graph is replaced.
In `@src/renderer/extensions/vueNodes/components/NodeHeader.test.ts`:
- Around line 22-32: Replace the local makeNodeData fixture with the shared
createNodeState fixture from litegraphTestUtils, adding the required import and
passing the existing overrides through it. Remove the hand-rolled NodeState
defaults so shared fields, including the ALWAYS mode, remain consistent.
In `@src/renderer/extensions/vueNodes/components/NodeWidgets.test.ts`:
- Around line 265-267: Replace the Tailwind class assertion in the NodeWidgets
test with an assertion on a semantic error signal exposed by the widget row,
such as data-has-error or aria-invalid. Update the relevant widget-row rendering
symbol to provide that signal if needed, while preserving the existing error
behavior.
In `@src/renderer/extensions/vueNodes/composables/useProcessedWidgets.test.ts`:
- Around line 342-351: Update the test around processWidgets to avoid asserting
the literal Tailwind value ring ring-component-node-widget-advanced. Verify the
advanced/non-advanced borderStyle behavior by comparing results for showAdvanced
or reuse a shared source constant, and apply the same change to the duplicate
assertion near the other advanced-widget test.
In `@src/renderer/extensions/vueNodes/composables/useProcessedWidgets.ts`:
- Around line 411-414: Update the valueTooltip logic in useProcessedWidgets so
object-valued asset/combo widgets do not produce a tooltip from String(value),
particularly avoiding “[object Object]”. Only attach the tooltip when the value
is a meaningful primitive string representation longer than 10 characters, while
preserving the existing type and length checks.
In `@src/renderer/glsl/useGLSLUniforms.test.ts`:
- Around line 21-32: Update makeGlslNode to use a real Subgraph and LiteGraph
links from the shared test factories instead of overriding getInputLink with a
hand-rolled inputs-to-subgraph mapping. Remove the getInputLink mock so
extractUniformSources exercises the actual LGraphNode.getInputLink behavior,
while preserving the test’s intended input-link setup.
In `@src/services/litegraphService.ts`:
- Around line 467-485: Extract the duplicated serialized-output merge into a
module-level helper, such as mergeSerialisedOutputs, preserving the existing
zip, RESERVED_KEYS merge, extra-output handling, and outputAsSerialisable
fallback. Replace the inline blocks in src/services/litegraphService.ts:467-485
and src/services/litegraphService.ts:579-597 with calls to the shared helper.
In `@src/stores/widgetValueStore.graphReactivity.test.ts`:
- Around line 279-293: Strengthen the test “registers plain render metadata for
non-promoted widgets” by replacing the definedness and absent sourceExecutionId
checks with assertions for the expected non-promoted render metadata, including
hasLayoutSize: false and isDOMWidget: false. Ensure the assertions distinguish
plain registration from promoted widget registration rather than only checking
defaults.
In `@src/stores/widgetValueStore.ts`:
- Around line 146-151: Update getWidgetRenderState to validate widgetId with the
existing isWidgetId guard before calling parseWidgetId; return undefined for
non-conforming IDs, matching getWidget’s behavior, while preserving the current
graph-state lookup for valid IDs.
- Around line 61-69: Split lazy creation from reads in the widget-order store:
add an ensure helper for mutation paths and a non-mutating peek helper for
reads. Update appendNodeWidgetOrder and setNodeWidgetOrder to use
ensureNodeWidgetOrder, and getNodeWidgetIds and reconcileNodeWidgetOrder to use
peekNodeWidgetOrder so computed reads do not insert entries. Apply the same
ensure/peek separation to getWidgetRenderState, ensuring read access does not
create per-graph maps.
In `@src/stores/workspace/favoritedWidgetsStore.ts`:
- Around line 76-79: Track the deferred migration from (nodeLocatorId,
widgetName) keys to host-scoped WidgetId keys by opening an issue that documents
the favoritedWidgets persistence-format change, required one-time migration of
workflow.extra.favoritedWidgets, and removal of the
isShownOnParents/favoriteNode indirection for promoted widgets.
In `@src/systems/badgeSystem.pricing.test.ts`:
- Around line 13-22: The getNodeDisplayPrice mock currently reads
useLinkStore().isInputSlotConnected, allowing the badge computed to react
without touchPricingSources registering the dependency. Make getNodeDisplayPrice
pure by deriving its result from node data or a plain test-controlled
counter/override, then update the test setup to flip that value and assert
recomputation specifically through touchPricingSources.
In `@src/systems/badgeSystem.ts`:
- Around line 212-220: Update the Settings map entries for
Comfy.NodeBadge.NodeIdBadgeMode, Comfy.NodeBadge.NodeLifeCycleBadgeMode, and
Comfy.NodeBadge.NodeSourceBadgeMode to use NodeBadgeMode, then remove the
corresponding assertions from the badgeModes initialization. Ensure
settingStore.get infers and returns NodeBadgeMode directly for these keys.
- Around line 167-187: Update gatherSubgraphCredits to touch all dynamic pricing
dependencies for the single inner API leaf, not only
pricing.getNodeRevisionRef(leaf.id). Reuse the dependency-registration behavior
from touchPricingSources, passing the appropriate inner graph-id keys from
wrapper.subgraph for priced widget values and input connections, while
preserving the existing multi-leaf and display-price behavior.
---
Outside diff comments:
In `@src/renderer/extensions/vueNodes/components/NodeSlots.vue`:
- Around line 10-19: In NodeSlots.vue, add computed inputRows and outputRows
that resolve each slot’s actual index once and calculate connection state using
the rootGraphId within the computed mapping. Update the InputSlot and OutputSlot
v-for blocks to consume these row objects for keys, slot data, indices, and
connected state, removing repeated getActualInputIndex calls and redundant
rootGraphId guards from isInputConnected/isOutputConnected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 77516a24-b905-4d0c-b70f-f93cbf262d19
⛔ Files ignored due to path filters (14)
browser_tests/tests/domWidget.spec.ts-snapshots/focus-mode-on-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/execution.spec.ts-snapshots/execution-error-unconnected-slot-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/graphCanvasMenu.spec.ts-snapshots/canvas-with-visible-links-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/mobileBaseline.spec.ts-snapshots/mobile-default-workflow-mobile-chrome-linux.pngis excluded by!**/*.pngbrowser_tests/tests/mobileBaseline.spec.ts-snapshots/mobile-graph-canvas-toolbar-mobile-chrome-linux.pngis excluded by!**/*.pngbrowser_tests/tests/nodeBadge.spec.ts-snapshots/node-badge-left-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/saveImageAndWebp.spec.ts-snapshots/save-image-and-webm-preview-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/selectionToolbox.spec.ts-snapshots/selection-toolbox-multiple-nodes-border-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/selectionToolbox.spec.ts-snapshots/selection-toolbox-multiple-selections-border-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/selectionToolbox.spec.ts-snapshots/selection-toolbox-single-node-no-border-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/viewport.spec.ts-snapshots/viewport-fits-when-saved-offscreen-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/vueNodes/interactions/canvas/pan.spec.ts-snapshots/vue-nodes-paned-with-touch-mobile-chrome-linux.pngis excluded by!**/*.pngbrowser_tests/tests/vueNodes/nodeStates/colors.spec.ts-snapshots/vue-node-custom-colors-dark-all-colors-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/vueNodes/nodeStates/colors.spec.ts-snapshots/vue-node-custom-colors-light-all-colors-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (272)
browser_tests/assets/missing/missing_model_nested_promoted_widget.jsonbrowser_tests/assets/missing/missing_models_in_subgraph.jsonbrowser_tests/assets/nodes/duplicate_node_ids.jsonbrowser_tests/fixtures/helpers/NodeOperationsHelper.tsbrowser_tests/tests/appModeBuilder.spec.tsbrowser_tests/tests/nodeBadge.spec.tsbrowser_tests/tests/nodeDisplay.spec.tsbrowser_tests/tests/propertiesPanel/errorsTabCloudMissingModels.spec.tsbrowser_tests/tests/propertiesPanel/errorsTabModeAware.spec.tsbrowser_tests/tests/vueNodes/interactions/links/linkInteraction.spec.tsbrowser_tests/tests/vueNodes/nodeStates/registration.spec.tsbrowser_tests/tests/vueNodes/widgets/legacy.spec.tsbrowser_tests/tests/vueNodes/widgets/widgetReactivity.spec.tsbrowser_tests/tests/workflowPersistence.spec.tsdocs/adr/0008-entity-component-system.mddocs/architecture/domain-glossary.mddocs/architecture/ecs-lifecycle-scenarios.mddocs/architecture/ecs-migration-plan.mddocs/architecture/ecs-target-architecture.mddocs/architecture/entity-interactions.mddocs/architecture/entity-problems.mddocs/architecture/link-topology-store.mddocs/architecture/node-badge-store.mddocs/architecture/node-data-store.mddocs/architecture/output-slot-connectivity.mddocs/architecture/proto-ecs-stores.mddocs/architecture/reroute-chain-store.mdsrc/components/builder/AppModeWidgetList.vuesrc/components/graph/GraphCanvas.vuesrc/components/graph/widgets/domWidgetZIndex.test.tssrc/components/rightSidePanel/errors/useErrorGroups.test.tssrc/components/rightSidePanel/errors/useErrorGroups.tssrc/components/rightSidePanel/parameters/SectionWidgets.vuesrc/components/rightSidePanel/parameters/TabSubgraphInputs.test.tssrc/components/rightSidePanel/parameters/TabSubgraphInputs.vuesrc/components/rightSidePanel/parameters/WidgetActions.test.tssrc/components/rightSidePanel/parameters/WidgetActions.vuesrc/components/rightSidePanel/parameters/WidgetItem.test.tssrc/components/rightSidePanel/parameters/WidgetItem.vuesrc/components/rightSidePanel/subgraph/SubgraphEditor.test.tssrc/components/rightSidePanel/subgraph/SubgraphEditor.vuesrc/composables/graph/useErrorClearingHooks.test.tssrc/composables/graph/useErrorClearingHooks.tssrc/composables/graph/useGraphNodeManager.test.tssrc/composables/graph/useGraphNodeManager.tssrc/composables/graph/useNodeErrorFlagSync.test.tssrc/composables/graph/useVueNodeLifecycle.test.tssrc/composables/graph/useVueNodeLifecycle.tssrc/composables/node/useNodeBadge.tssrc/composables/node/useNodePricing.test.tssrc/composables/node/useNodePricing.tssrc/composables/node/usePriceBadge.test.tssrc/composables/node/usePriceBadge.tssrc/composables/useUpstreamValue.test.tssrc/composables/useUpstreamValue.tssrc/core/graph/subgraph/migration/proxyWidgetMigration.test.tssrc/core/graph/subgraph/migration/proxyWidgetMigration.tssrc/core/graph/subgraph/promotedInputWidget.tssrc/core/graph/subgraph/promotionUtils.test.tssrc/core/graph/subgraph/promotionUtils.tssrc/core/graph/subgraph/resolveConcretePromotedWidget.test.tssrc/core/graph/subgraph/resolveSubgraphInputLink.tssrc/core/graph/widgets/dynamicWidgets.test.tssrc/core/graph/widgets/dynamicWidgets.tssrc/core/graph/widgets/matchTypeConfiguring.test.tssrc/extensions/core/groupNode.tssrc/extensions/core/load3d.tssrc/extensions/core/rerouteNode.tssrc/extensions/core/saveImageExtraOutput.test.tssrc/extensions/core/widgetInputs.test.tssrc/extensions/core/widgetInputs.tssrc/extensions/core/widgetValuePropagation.test.tssrc/extensions/core/widgetValuePropagation.tssrc/lib/litegraph/src/LGraph.inputSlotRealign.test.tssrc/lib/litegraph/src/LGraph.serialise.test.tssrc/lib/litegraph/src/LGraph.test.tssrc/lib/litegraph/src/LGraph.tssrc/lib/litegraph/src/LGraphBadge.tssrc/lib/litegraph/src/LGraphCanvas.clipboard.test.tssrc/lib/litegraph/src/LGraphCanvas.cloneZIndex.test.tssrc/lib/litegraph/src/LGraphCanvas.drawConnections.test.tssrc/lib/litegraph/src/LGraphCanvas.ghost.test.tssrc/lib/litegraph/src/LGraphCanvas.groupSelection.test.tssrc/lib/litegraph/src/LGraphCanvas.slotHitDetection.test.tssrc/lib/litegraph/src/LGraphCanvas.tssrc/lib/litegraph/src/LGraphNode.nodeState.test.tssrc/lib/litegraph/src/LGraphNode.test.tssrc/lib/litegraph/src/LGraphNode.tssrc/lib/litegraph/src/LGraphNodeProperties.test.tssrc/lib/litegraph/src/LGraphNodeProperties.tssrc/lib/litegraph/src/LLink.store.test.tssrc/lib/litegraph/src/LLink.test.tssrc/lib/litegraph/src/LLink.tssrc/lib/litegraph/src/LiteGraphGlobal.tssrc/lib/litegraph/src/Reroute.store.test.tssrc/lib/litegraph/src/Reroute.tssrc/lib/litegraph/src/__fixtures__/nodeHelpers.tssrc/lib/litegraph/src/canvas/FloatingRenderLink.test.tssrc/lib/litegraph/src/canvas/FloatingRenderLink.tssrc/lib/litegraph/src/canvas/LinkConnector.core.test.tssrc/lib/litegraph/src/canvas/LinkConnector.integration.test.tssrc/lib/litegraph/src/canvas/LinkConnector.test.tssrc/lib/litegraph/src/canvas/LinkConnector.tssrc/lib/litegraph/src/canvas/LinkConnectorSubgraphInputValidation.test.tssrc/lib/litegraph/src/canvas/ToInputFromIoNodeLink.tssrc/lib/litegraph/src/canvas/ToInputRenderLink.tssrc/lib/litegraph/src/infrastructure/LGraphEventMap.tssrc/lib/litegraph/src/interfaces.tssrc/lib/litegraph/src/linkDeduplication.tssrc/lib/litegraph/src/litegraph.tssrc/lib/litegraph/src/node/NodeInputSlot.test.tssrc/lib/litegraph/src/node/NodeInputSlot.tssrc/lib/litegraph/src/node/NodeOutputSlot.test.tssrc/lib/litegraph/src/node/NodeOutputSlot.tssrc/lib/litegraph/src/node/NodeSlot.test.tssrc/lib/litegraph/src/node/NodeSlot.tssrc/lib/litegraph/src/node/SlotBase.tssrc/lib/litegraph/src/node/slotEcosystemPatterns.test.tssrc/lib/litegraph/src/node/slotLinks.test.tssrc/lib/litegraph/src/node/slotLinks.tssrc/lib/litegraph/src/node/slotUtils.test.tssrc/lib/litegraph/src/node/slotUtils.tssrc/lib/litegraph/src/nodeBadgeDraw.test.tssrc/lib/litegraph/src/nodeBadgeDraw.tssrc/lib/litegraph/src/serialization.test.tssrc/lib/litegraph/src/subgraph/ExecutableNodeDTO.test.tssrc/lib/litegraph/src/subgraph/ExecutableNodeDTO.tssrc/lib/litegraph/src/subgraph/SubgraphConversion.test.tssrc/lib/litegraph/src/subgraph/SubgraphIO.test.tssrc/lib/litegraph/src/subgraph/SubgraphInput.tssrc/lib/litegraph/src/subgraph/SubgraphInputNode.tssrc/lib/litegraph/src/subgraph/SubgraphNode.tssrc/lib/litegraph/src/subgraph/SubgraphOutput.tssrc/lib/litegraph/src/subgraph/SubgraphSerialization.test.tssrc/lib/litegraph/src/subgraph/SubgraphSlotConnections.test.tssrc/lib/litegraph/src/subgraph/SubgraphWidgetPromotion.test.tssrc/lib/litegraph/src/subgraph/__fixtures__/README.mdsrc/lib/litegraph/src/subgraph/__fixtures__/subgraphHelpers.test.tssrc/lib/litegraph/src/subgraph/__fixtures__/subgraphHelpers.tssrc/lib/litegraph/src/subgraph/subgraphDeduplication.test.tssrc/lib/litegraph/src/subgraph/subgraphDeduplication.tssrc/lib/litegraph/src/subgraph/subgraphUtils.test.tssrc/lib/litegraph/src/subgraph/subgraphUtils.tssrc/lib/litegraph/src/types/graphTriggers.tssrc/lib/litegraph/src/types/serialisation.tssrc/lib/litegraph/src/types/widgets.tssrc/lib/litegraph/src/utils/collections.test.tssrc/lib/litegraph/src/utils/collections.tssrc/lib/litegraph/src/utils/widget.tssrc/lib/litegraph/src/widgets/BaseWidget.test.tssrc/lib/litegraph/src/widgets/BaseWidget.tssrc/locales/ar/main.jsonsrc/locales/en/main.jsonsrc/locales/es/main.jsonsrc/locales/fa/main.jsonsrc/locales/fr/main.jsonsrc/locales/he/main.jsonsrc/locales/ja/main.jsonsrc/locales/ko/main.jsonsrc/locales/pt-BR/main.jsonsrc/locales/ru/main.jsonsrc/locales/tr/main.jsonsrc/locales/zh-TW/main.jsonsrc/locales/zh/main.jsonsrc/platform/cloud/subscription/composables/useFreeTierQuota.tssrc/platform/missingMedia/missingMediaStore.tssrc/platform/missingModel/missingModelScan.test.tssrc/platform/missingModel/missingModelScan.tssrc/platform/missingModel/missingModelStore.tssrc/platform/nodeReplacement/useNodeReplacement.test.tssrc/platform/nodeReplacement/useNodeReplacement.tssrc/platform/telemetry/nodeAdded/installNodeAddedTelemetry.test.tssrc/platform/telemetry/nodeAdded/installNodeAddedTelemetry.tssrc/platform/workflow/core/services/workflowService.insertWorkflow.test.tssrc/platform/workflow/core/services/workflowService.tssrc/renderer/core/canvas/canvasStore.test.tssrc/renderer/core/canvas/canvasStore.tssrc/renderer/core/canvas/links/linkConnectorAdapter.tssrc/renderer/core/layout/operations/layoutMutations.test.tssrc/renderer/core/layout/operations/layoutMutations.tssrc/renderer/core/layout/store/layoutStore.test.tssrc/renderer/core/layout/store/layoutStore.tssrc/renderer/core/layout/types.tssrc/renderer/extensions/linearMode/PartnerNodesList.vuesrc/renderer/extensions/minimap/composables/useMinimap.test.tssrc/renderer/extensions/minimap/composables/useMinimapGraph.test.tssrc/renderer/extensions/minimap/composables/useMinimapGraph.tssrc/renderer/extensions/minimap/data/AbstractMinimapDataSource.tssrc/renderer/extensions/minimap/data/LayoutStoreDataSource.tssrc/renderer/extensions/minimap/data/MinimapDataSource.test.tssrc/renderer/extensions/minimap/data/MinimapDataSourceFactory.tssrc/renderer/extensions/minimap/minimapCanvasRenderer.test.tssrc/renderer/extensions/vueNodes/components/LGraphNode.subgraph.test.tssrc/renderer/extensions/vueNodes/components/LGraphNode.test.tssrc/renderer/extensions/vueNodes/components/LGraphNode.vuesrc/renderer/extensions/vueNodes/components/LGraphNodePreview.test.tssrc/renderer/extensions/vueNodes/components/LGraphNodePreview.vuesrc/renderer/extensions/vueNodes/components/NodeContent.vuesrc/renderer/extensions/vueNodes/components/NodeHeader.test.tssrc/renderer/extensions/vueNodes/components/NodeHeader.vuesrc/renderer/extensions/vueNodes/components/NodeSlots.test.tssrc/renderer/extensions/vueNodes/components/NodeSlots.vuesrc/renderer/extensions/vueNodes/components/NodeWidgets.test.tssrc/renderer/extensions/vueNodes/components/NodeWidgets.vuesrc/renderer/extensions/vueNodes/components/WidgetGrid.vuesrc/renderer/extensions/vueNodes/composables/useNodeEventHandlers.test.tssrc/renderer/extensions/vueNodes/composables/useNodeEventHandlers.tssrc/renderer/extensions/vueNodes/composables/useNodePointerInteractions.test.tssrc/renderer/extensions/vueNodes/composables/useNodePointerInteractions.tssrc/renderer/extensions/vueNodes/composables/useNodeTooltips.test.tssrc/renderer/extensions/vueNodes/composables/useNodeTooltips.tssrc/renderer/extensions/vueNodes/composables/usePartitionedBadges.test.tssrc/renderer/extensions/vueNodes/composables/usePartitionedBadges.tssrc/renderer/extensions/vueNodes/composables/useProcessedWidgets.test.tssrc/renderer/extensions/vueNodes/composables/useProcessedWidgets.tssrc/renderer/extensions/vueNodes/composables/useSlotLinkInteraction.autoPan.test.tssrc/renderer/extensions/vueNodes/composables/useSlotLinkInteraction.tssrc/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.test.tssrc/renderer/extensions/vueNodes/layout/nodeSizeReflow.test.tssrc/renderer/extensions/vueNodes/types/widgetGrid.tssrc/renderer/extensions/vueNodes/utils/__tests__/nodeDataUtils.test.tssrc/renderer/extensions/vueNodes/utils/nodeDataUtils.tssrc/renderer/extensions/vueNodes/utils/nodeErrorState.tssrc/renderer/extensions/vueNodes/widgets/components/WidgetButton.test.tssrc/renderer/extensions/vueNodes/widgets/components/WidgetButton.vuesrc/renderer/extensions/vueNodes/widgets/components/WidgetDOM.test.tssrc/renderer/extensions/vueNodes/widgets/components/WidgetDOM.vuesrc/renderer/extensions/vueNodes/widgets/components/WidgetLegacy.vuesrc/renderer/extensions/vueNodes/widgets/composables/useImageUploadWidget.test.tssrc/renderer/extensions/vueNodes/widgets/composables/useProgressTextWidget.tssrc/renderer/extensions/vueNodes/widgets/composables/useWidgetRenderer.test.tssrc/renderer/extensions/vueNodes/widgets/registry/widgetRegistry.tssrc/renderer/glsl/glslPreviewUtils.tssrc/renderer/glsl/useGLSLUniforms.test.tssrc/renderer/glsl/useGLSLUniforms.tssrc/scripts/app.test.tssrc/scripts/app.tssrc/scripts/promotedWidgetControl.test.tssrc/scripts/promotedWidgetControl.tssrc/scripts/widgets.tssrc/services/litegraphService.tssrc/stores/appModeStore.test.tssrc/stores/linkStore.test.tssrc/stores/linkStore.tssrc/stores/nodeDataStore.test.tssrc/stores/nodeDataStore.tssrc/stores/rerouteStore.test.tssrc/stores/rerouteStore.tssrc/stores/widgetValueStore.graphReactivity.test.tssrc/stores/widgetValueStore.test.tssrc/stores/widgetValueStore.tssrc/stores/workspace/favoritedWidgetsStore.tssrc/systems/badgeSystem.pricing.test.tssrc/systems/badgeSystem.subgraph.test.tssrc/systems/badgeSystem.test.tssrc/systems/badgeSystem.tssrc/types/badgeData.tssrc/types/linkTopology.tssrc/types/nodeState.tssrc/types/rerouteChain.tssrc/types/simplifiedWidget.tssrc/types/widgetState.tssrc/utils/__tests__/litegraphTestUtils.tssrc/utils/graphTraversalUtil.test.tssrc/utils/graphTraversalUtil.tssrc/utils/linkFixer.test.tssrc/utils/linkFixer.tssrc/utils/litegraphUtil.test.tssrc/utils/litegraphUtil.tssrc/utils/searchAndReplace.test.tssrc/utils/widgetUtil.tstools/devtools/web/legacyWidget.js
💤 Files with no reviewable changes (27)
- src/lib/litegraph/src/LGraphNodeProperties.test.ts
- src/composables/node/usePriceBadge.test.ts
- src/composables/node/usePriceBadge.ts
- src/lib/litegraph/src/LGraphNodeProperties.ts
- src/lib/litegraph/src/LGraphBadge.ts
- src/composables/graph/useGraphNodeManager.ts
- src/composables/graph/useGraphNodeManager.test.ts
- browser_tests/tests/nodeBadge.spec.ts
- src/locales/ru/main.json
- src/lib/litegraph/src/canvas/ToInputRenderLink.ts
- src/locales/ar/main.json
- src/locales/ja/main.json
- src/locales/zh/main.json
- src/lib/litegraph/src/types/graphTriggers.ts
- src/lib/litegraph/src/node/SlotBase.ts
- src/locales/zh-TW/main.json
- src/locales/tr/main.json
- src/lib/litegraph/src/canvas/ToInputFromIoNodeLink.ts
- src/locales/es/main.json
- src/locales/fr/main.json
- src/locales/fa/main.json
- src/scripts/app.ts
- browser_tests/fixtures/helpers/NodeOperationsHelper.ts
- src/renderer/core/layout/types.ts
- src/locales/ko/main.json
- src/locales/pt-BR/main.json
- src/locales/he/main.json
🛑 Comments failed to post (1)
src/renderer/extensions/vueNodes/composables/useProcessedWidgets.test.ts (1)
342-351: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Avoid asserting raw Tailwind class strings.
'ring ring-component-node-widget-advanced'is hard-coded here and again at Line 468, so a purely cosmetic class rename breaks both tests without any behavior change. Prefer asserting the advanced/non-advanced distinction (e.g. compareborderStyleforshowAdvancedvs. a non-advanced widget, or expose the class via a shared constant that both source and test import).As per coding guidelines, "Do not write change-detector tests, style-dependent tests, redundant tests".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/extensions/vueNodes/composables/useProcessedWidgets.test.ts` around lines 342 - 351, Update the test around processWidgets to avoid asserting the literal Tailwind value ring ring-component-node-widget-advanced. Verify the advanced/non-advanced borderStyle behavior by comparing results for showAdvanced or reuse a shared source constant, and apply the same change to the duplicate assertion near the other advanced-widget test.Source: Coding guidelines
DrJKL
added
the
New Browser Test Expectations
github-actions
Bot
removed
the
New Browser Test Expectations
This was referenced Jul 30, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/extensions/core/groupNode.ts (1)
926-931: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftResolve links from the graph that owns the group node.
convertToNodes()documents nested-subgraph support, butreconnectInputs()resolves the origin throughapp.rootGraph, andreconnectOutputs()queriesoutputLinks(app.rootGraph, ...). Link and node collections are graph-local. For a group inside aSubgraph, these lookups cannot resolve the subgraph topology, so unpacking can drop connections. Resolve both endpoints from the owning graph and explicitly map them into the root graph before reconnecting.Also applies to: 943-953
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/extensions/core/groupNode.ts` around lines 926 - 931, Update reconnectInputs() and reconnectOutputs() to use the graph owning the group node, rather than app.rootGraph, when resolving links, origins, and output links. Before reconnecting, explicitly map the resolved endpoints from that owning graph into the root graph so nested Subgraph topology is preserved during convertToNodes().src/extensions/core/widgetInputs.ts (1)
348-354: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSkip links whose target slot no longer exists.
theirNodecan resolve whiletheirNode.inputs[link.target_slot]isundefinedafter input layout changes or legacy workflow restoration._isValidConnection()readsinput.widget, so this path can throw during_mergeWidgetConfig(). Add a guard before calling it.Proposed fix
const theirInput = theirNode.inputs[link.target_slot] + if (!theirInput) continue // Call is valid connection so it can merge the configs when validating this._isValidConnection(theirInput, hasConfig)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/extensions/core/widgetInputs.ts` around lines 348 - 354, In the link-processing loop within _mergeWidgetConfig, guard the theirNode.inputs[link.target_slot] lookup and skip the link when the target slot is missing, before calling _isValidConnection. Preserve processing for links with an existing target input.src/renderer/core/layout/store/layoutStore.ts (2)
239-242: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilent no-op on
nullassignment can hide a real caller.The setter ignores
nullinstead of emittingdeleteNode. The comment asserts that no caller assignsnull. The ref type still allowsnull, so a future caller gets a silent no-op with no signal. Narrow the ref type toRef<NodeLayout | null>for reads only, or log at debug level when anullwrite arrives.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/core/layout/store/layoutStore.ts` around lines 239 - 242, Update the layout ref setter in the layout store to avoid silently ignoring null assignments: either expose a read-only Ref<NodeLayout | null> type so callers cannot write null, or retain the setter and emit a debug log when newLayout is null. Preserve layoutMutations.deleteNode as the deletion path for valid node deletions.
1136-1158: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winClean up floating-link geometry when removing a floating link.
removeFloatingLinkremoves topology but does not removelinkSegmentLayoutsorlinkSegmentSpatialIndexentries. Add this cleanup toremoveFloatingLink. Make the cleanup unconditional when no whole-link layout exists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/core/layout/store/layoutStore.ts` around lines 1136 - 1158, The node deletion path leaves floating-link geometry entries behind because removeFloatingLink does not clear linkSegmentLayouts or linkSegmentSpatialIndex. Update removeFloatingLink to remove the affected floating link’s segment layout and spatial-index entries, performing this cleanup unconditionally when no whole-link layout exists; leave whole-link layout handling unchanged.docs/architecture/reroute-chain-store.md (1)
111-118: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the reroute geometry scope statement.
Reroute.posno longer mirrors a class-owned position field.layoutStoreowns reroute position. State that reroute geometry is out of scope because it has already migrated, not because duplication remains.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/architecture/reroute-chain-store.md` around lines 111 - 118, Update the Scope section to state that reroute geometry is out of scope because reroute position has already migrated to layoutStore ownership; remove the inaccurate claim that Reroute.pos mirrors a class field and that pre-existing duplication remains.src/lib/litegraph/src/LGraphCanvas.ts (1)
2530-2572: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReroute hit-testing on left-click duplicates the extracted
findRerouteAtPointhelper.
processMouseDown's right-click path resolves reroutes throughfindRerouteAtPoint(graph, x, y, this._visibleReroutes)._processPrimaryButtonre-implements the same "query layout store, then fall back" pattern inline instead of reusing that helper. Keep the two click paths on one hit-testing implementation. Otherwise, future changes tofindRerouteAtPoint's fallback behavior will not propagate to the left-click path, and the two paths can silently diverge on edge positions.Consider extracting the "resolve reroute by id from a layout hit" step into a small helper that both
findRerouteAtPointand this loop call, so the layout-store resolution logic stays in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/litegraph/src/LGraphCanvas.ts` around lines 2530 - 2572, Update the reroute hit-testing in _processPrimaryButton to reuse findRerouteAtPoint instead of duplicating the layout-store query and visible-reroute fallback. If needed, extract the shared layout-hit ID resolution into a small helper used by both findRerouteAtPoint and the primary-button path, while preserving the existing click, drag, connector, and slot-hover behavior.src/lib/litegraph/src/Reroute.ts (1)
674-685: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winHandle duplicate reroute IDs before replacing the graph entry.
registerReroute()refuses to replace an existing chain but returns the unregistered chain.registerRerouteChain()then assigns that chain to the new reroute, while_addReroute()overwrites the graph map. Unregistering the new reroute cannot remove the old store registration, leaving an orphaned chain.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/litegraph/src/Reroute.ts` around lines 674 - 685, Update the reroute registration flow around registerReroute(), registerRerouteChain(), and _addReroute() so duplicate IDs are handled before the graph map is overwritten. Preserve the existing registered chain when an ID already exists, and ensure the new reroute is not assigned an unregistered chain or left able to orphan the old store registration.src/renderer/core/layout/operations/layoutMutations.ts (1)
236-285: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd existence guards to
createGroupanddeleteGroupfor consistency.
setGroupBoundsreturns early when the group layout is missing.createGroupanddeleteGroupalways apply an operation. AdeleteGroupcall for an unknown group still appends an entry to the operation log and notifies listeners withchange.type = 'delete', although the Yjs delete is a no-op. Node mutations (deleteNode) use the same guard pattern in this file.♻️ Proposed guard for `deleteGroup`
const deleteGroup = (rootGraphId: UUID, groupId: GroupId): void => { + if (!layoutStore.getGroupLayout(rootGraphId, groupId)) return + layoutStore.applyOperation({ type: 'deleteGroup', entity: 'group',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/core/layout/operations/layoutMutations.ts` around lines 236 - 285, Update createGroup and deleteGroup to check whether the group already exists using layoutStore.getGroupLayout(rootGraphId, groupId) before applying operations, matching the guard pattern in setGroupBounds and deleteNode. Preserve createGroup’s existing behavior for valid groups and return early when the requested group state does not satisfy the intended existence condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@browser_tests/tests/copyPaste.spec.ts`:
- Around line 33-56: Update the test around the pinned node copy/paste flow to
capture the original node’s distinct ID and position before pasting, then
identify the newly pasted node by ID rather than relying on getNodeRefsByType
ordering. After confirming the node count, derive the expected graph coordinate
from the cursor position and assert the pasted node’s position matches it, while
retaining the original-node identity checks.
In `@src/lib/litegraph/src/LGraph.ts`:
- Around line 508-524: Update the subgraph-definition cleanup in remove() to
delete each released definition’s groups and reroutes through
useLayoutMutations(), using the root graph ID and the same LayoutSource.Canvas
setup as clear(). Ensure this runs before the definition becomes unreachable,
while preserving the existing topology and node-state cleanup.
In `@src/lib/litegraph/src/LGraphCanvas.ts`:
- Around line 8944-8950: Update applyNodePositions to build the corresponding
node-position updates and commit them through
layoutMutations.batchMoveNodes(updates) once, instead of calling node.setPos for
each node. Preserve the existing positions mapping and observable node
coordinates while batching the layout-store write.
In `@src/lib/litegraph/src/LGraphGroup.test.ts`:
- Around line 171-182: The mutation tests around the `test.for(mutations)` block
and the additional cases at lines 204–221 currently derive expected geometry
from `group.pos` and `group.size`, making the assertions self-referential.
Replace those fields with the independently known literal x, y, width, and
height values for each mutation case, matching the established legacy geometry
buffer test pattern.
In `@src/lib/litegraph/src/LGraphGroup.ts`:
- Around line 168-176: Remove the explicit syncBoundsFromStore() calls from both
the boundingRect getter and getBounding() method in LGraphGroup, returning
this._bounding directly. Preserve the Proxy-based synchronization performed when
callers access geometry properties.
In `@src/lib/litegraph/src/LGraphNode.ts`:
- Around line 650-659: Scope node layout refs, Y nodes, indexes, listeners, and
lifecycle operations by rootGraph.id rather than nodeId alone, updating
LGraphNode methods _positionUpdated and the related layout-operation site at
LGraphNode.ts lines 680-690 while preserving existing behavior. Add an isolation
test in LGraphNode.test.ts lines 794-864 using identical node IDs in two root
graphs and verify position and size updates remain isolated.
In `@src/platform/workflow/core/utils/workflowToClipboardItems.test.ts`:
- Around line 101-140: Add a cyclic-subgraph test alongside the existing nesting
test, using the subgraph helper to create A nesting B and B nesting A, then pass
the resulting workflow through workflowToClipboardItems and assert flattening
terminates with each subgraph ID included exactly once and no nested definitions
retained.
In `@src/platform/workflow/core/utils/workflowToClipboardItems.ts`:
- Around line 12-27: The workflow insertion path must preserve root-level
floating links: update workflowToClipboardItems and the corresponding
_deserializeItems processing so graph.floatingLinks are carried through and
inserted alongside parsed.links. Add a regression test covering a workflow with
root-level floating links, rather than only extending ClipboardItems.
In `@src/renderer/core/layout/store/layoutStore.ts`:
- Around line 1000-1003: Update the workflow close cleanup in
initializeFromLiteGraph to remove discarded root graph IDs from both
rerouteLayouts and rerouteSpatialIndex, while preserving reroute geometry during
subgraph navigation and retaining the existing link, segment, and slot layout
cleanup.
In `@src/renderer/core/layout/sync/useLayoutSync.test.ts`:
- Around line 111-146: Replace the `canvas as never` cast in the `startSync`
call with a typed partial mock created via `fromPartial` from
`@total-typescript/shoehorn`; use the same helper for the `liteNode` mock so
both objects are checked against their expected contracts while preserving the
existing position-setter assertion.
In `@src/renderer/core/layout/utils/mappers.ts`:
- Around line 56-96: Share the rectangle serialization and deserialization logic
between group and node layouts by extracting or reusing the existing helpers
used by layoutToYNode and yNodeRect. Update layoutToYGroup and setYGroupRect to
use the shared rect writer, and update yGroupToLayout to use the shared rect
reader so the StoredRect cast is centralized.
In `@src/renderer/extensions/minimap/data/LayoutStoreDataSource.ts`:
- Around line 75-77: Update getNodeCount() to count entries from viewedNodes()
that have a layout, rather than calling getNodes() and constructing full
MinimapNodeData objects; preserve the same layout-presence filtering used by
getNodes().
In `@src/renderer/extensions/vueNodes/layout/useNodeDrag.test.ts`:
- Line 114: Update the isLGraphNode stub used by the startDrag tests to
distinguish actual LGraphNode instances, preferably using the real LiteGraph
class or shared factory instead of a constant mock. Add a selected LGraphNode to
the relevant selectedItems case and assert it is excluded from
nonNodeStartPositions, ensuring the !isLGraphNode filter cannot be inverted or
omitted without failing.
---
Outside diff comments:
In `@docs/architecture/reroute-chain-store.md`:
- Around line 111-118: Update the Scope section to state that reroute geometry
is out of scope because reroute position has already migrated to layoutStore
ownership; remove the inaccurate claim that Reroute.pos mirrors a class field
and that pre-existing duplication remains.
In `@src/extensions/core/groupNode.ts`:
- Around line 926-931: Update reconnectInputs() and reconnectOutputs() to use
the graph owning the group node, rather than app.rootGraph, when resolving
links, origins, and output links. Before reconnecting, explicitly map the
resolved endpoints from that owning graph into the root graph so nested Subgraph
topology is preserved during convertToNodes().
In `@src/extensions/core/widgetInputs.ts`:
- Around line 348-354: In the link-processing loop within _mergeWidgetConfig,
guard the theirNode.inputs[link.target_slot] lookup and skip the link when the
target slot is missing, before calling _isValidConnection. Preserve processing
for links with an existing target input.
In `@src/lib/litegraph/src/LGraphCanvas.ts`:
- Around line 2530-2572: Update the reroute hit-testing in _processPrimaryButton
to reuse findRerouteAtPoint instead of duplicating the layout-store query and
visible-reroute fallback. If needed, extract the shared layout-hit ID resolution
into a small helper used by both findRerouteAtPoint and the primary-button path,
while preserving the existing click, drag, connector, and slot-hover behavior.
In `@src/lib/litegraph/src/Reroute.ts`:
- Around line 674-685: Update the reroute registration flow around
registerReroute(), registerRerouteChain(), and _addReroute() so duplicate IDs
are handled before the graph map is overwritten. Preserve the existing
registered chain when an ID already exists, and ensure the new reroute is not
assigned an unregistered chain or left able to orphan the old store
registration.
In `@src/renderer/core/layout/operations/layoutMutations.ts`:
- Around line 236-285: Update createGroup and deleteGroup to check whether the
group already exists using layoutStore.getGroupLayout(rootGraphId, groupId)
before applying operations, matching the guard pattern in setGroupBounds and
deleteNode. Preserve createGroup’s existing behavior for valid groups and return
early when the requested group state does not satisfy the intended existence
condition.
In `@src/renderer/core/layout/store/layoutStore.ts`:
- Around line 239-242: Update the layout ref setter in the layout store to avoid
silently ignoring null assignments: either expose a read-only Ref<NodeLayout |
null> type so callers cannot write null, or retain the setter and emit a debug
log when newLayout is null. Preserve layoutMutations.deleteNode as the deletion
path for valid node deletions.
- Around line 1136-1158: The node deletion path leaves floating-link geometry
entries behind because removeFloatingLink does not clear linkSegmentLayouts or
linkSegmentSpatialIndex. Update removeFloatingLink to remove the affected
floating link’s segment layout and spatial-index entries, performing this
cleanup unconditionally when no whole-link layout exists; leave whole-link
layout handling unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4a7d20ec-7efd-422c-9d8a-7a2840f3d714
⛔ Files ignored due to path filters (3)
browser_tests/tests/saveImageAndWebp.spec.ts-snapshots/save-image-and-webm-preview-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/vueNodes/nodeStates/colors.spec.ts-snapshots/vue-node-custom-colors-dark-all-colors-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/vueNodes/nodeStates/colors.spec.ts-snapshots/vue-node-custom-colors-light-all-colors-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (59)
browser_tests/fixtures/helpers/CanvasHelper.tsbrowser_tests/tests/copyPaste.spec.tsbrowser_tests/tests/vueNodes/interactions/node/move.spec.tsbrowser_tests/tests/vueNodes/rerouteGeometry.spec.tsdocs/architecture/ecs-migration-plan.mddocs/architecture/ecs-target-architecture.mddocs/architecture/proto-ecs-stores.mddocs/architecture/reroute-chain-store.mdsrc/composables/graph/useNodeArrangement.tssrc/composables/graph/useVueNodeLifecycle.tssrc/core/graph/widgets/dynamicWidgets.tssrc/extensions/core/groupNode.tssrc/extensions/core/widgetInputs.tssrc/lib/litegraph/src/LGraph.test.tssrc/lib/litegraph/src/LGraph.tssrc/lib/litegraph/src/LGraphCanvas.cloneZIndex.test.tssrc/lib/litegraph/src/LGraphCanvas.deprecated.test.tssrc/lib/litegraph/src/LGraphCanvas.groupSelection.test.tssrc/lib/litegraph/src/LGraphCanvas.tssrc/lib/litegraph/src/LGraphGroup.test.tssrc/lib/litegraph/src/LGraphGroup.tssrc/lib/litegraph/src/LGraphNode.test.tssrc/lib/litegraph/src/LGraphNode.tssrc/lib/litegraph/src/Reroute.store.test.tssrc/lib/litegraph/src/Reroute.tssrc/lib/litegraph/src/canvas/findRerouteAtPoint.tssrc/lib/litegraph/src/canvas/getCanvasContextMenuTarget.test.tssrc/lib/litegraph/src/canvas/getCanvasContextMenuTarget.tssrc/lib/litegraph/src/infrastructure/createGeometryView.test.tssrc/lib/litegraph/src/infrastructure/createGeometryView.tssrc/lib/litegraph/src/interfaces.tssrc/lib/litegraph/src/linkDeduplication.tssrc/lib/litegraph/src/subgraph/SubgraphIONodeBase.tssrc/lib/litegraph/src/subgraph/subgraphDeduplication.test.tssrc/lib/litegraph/src/subgraph/subgraphDeduplication.tssrc/platform/workflow/core/services/workflowService.insertWorkflow.test.tssrc/platform/workflow/core/services/workflowService.tssrc/platform/workflow/core/utils/workflowToClipboardItems.integration.test.tssrc/platform/workflow/core/utils/workflowToClipboardItems.test.tssrc/platform/workflow/core/utils/workflowToClipboardItems.tssrc/renderer/core/layout/operations/layoutMutations.tssrc/renderer/core/layout/store/layoutStore.test.tssrc/renderer/core/layout/store/layoutStore.tssrc/renderer/core/layout/sync/syncLayoutStoreFromGraph.test.tssrc/renderer/core/layout/sync/syncLayoutStoreFromGraph.tssrc/renderer/core/layout/sync/useLayoutSync.test.tssrc/renderer/core/layout/sync/useLayoutSync.tssrc/renderer/core/layout/types.tssrc/renderer/core/layout/utils/mappers.test.tssrc/renderer/core/layout/utils/mappers.tssrc/renderer/extensions/minimap/data/LayoutStoreDataSource.tssrc/renderer/extensions/minimap/data/MinimapDataSource.test.tssrc/renderer/extensions/vueNodes/composables/useSlotLinkInteraction.tssrc/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.test.tssrc/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.tssrc/renderer/extensions/vueNodes/layout/useNodeDrag.test.tssrc/renderer/extensions/vueNodes/layout/useNodeDrag.tssrc/scripts/app.tssrc/utils/vintageClipboard.ts
💤 Files with no reviewable changes (3)
- src/renderer/core/layout/sync/syncLayoutStoreFromGraph.ts
- src/renderer/core/layout/sync/syncLayoutStoreFromGraph.test.ts
- src/composables/graph/useVueNodeLifecycle.ts
Comment on lines
+33
to
+56
| test('Pasted copy of a pinned node lands at the cursor', async ({ | ||
| comfyPage | ||
| }) => { | ||
| const node = (await comfyPage.nodeOps.getNodeRefsByType('KSampler'))[0] | ||
| await node.clickContextMenuOption('Pin') | ||
| await comfyPage.contextMenu.waitForHidden() | ||
| await expect.poll(() => node.isPinned()).toBe(true) | ||
|
|
||
| await node.click('title') | ||
| await comfyPage.page.mouse.move(10, 10) | ||
| await comfyPage.nextFrame() | ||
| await comfyPage.clipboard.copy(comfyPage.canvas) | ||
| await comfyPage.clipboard.paste(comfyPage.canvas) | ||
|
|
||
| await expect | ||
| .poll( | ||
| async () => | ||
| (await comfyPage.nodeOps.getNodeRefsByType('KSampler')).length | ||
| ) | ||
| .toBe(2) | ||
| const [original, pasted] = | ||
| await comfyPage.nodeOps.getNodeRefsByType('KSampler') | ||
| expect(await pasted.getPosition()).not.toEqual(await original.getPosition()) | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert cursor placement with a stable node identity.
Lines 53-55 infer original and pasted from list order. This is not stable. The assertion also passes when paste uses any position that differs from the original position.
Capture the original node ID and position before paste. Find the new node by its distinct ID after the count assertion. Compare its position with the graph coordinate derived from the cursor position.
Based on learnings, browser test helpers must not depend on graph-node ordering. As per path instructions, tests must verify behavior rather than implementation details.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@browser_tests/tests/copyPaste.spec.ts` around lines 33 - 56, Update the test
around the pinned node copy/paste flow to capture the original node’s distinct
ID and position before pasting, then identify the newly pasted node by ID rather
than relying on getNodeRefsByType ordering. After confirming the node count,
derive the expected graph coordinate from the cursor position and assert the
pasted node’s position matches it, while retaining the original-node identity
checks.
Sources: Path instructions, Learnings
| @@ -111,7 +111,7 @@ vi.mock('@/renderer/core/layout/transform/useTransformState', () => ({ | |||
| })) | |||
|
|
|||
| vi.mock('@/utils/litegraphUtil', () => ({ | |||
| isLGraphGroup: () => false | |||
| isLGraphNode: () => false | |||
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
A constant isLGraphNode: () => false stub cannot detect an inverted filter.
startDrag keeps selected items with !isLGraphNode(item). With the stub always returning false, every selected item passes the filter, so the new non-node tests also pass if the predicate is inverted or dropped. Make the stub discriminate, for example by returning item instanceof LGraphNode, and add a selected node to selectedItems in one case so that nodes are proven excluded from nonNodeStartPositions.
As per path instructions: "Prefer real LiteGraph instances/shared factories, minimize mocks."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/extensions/vueNodes/layout/useNodeDrag.test.ts` at line 114,
Update the isLGraphNode stub used by the startDrag tests to distinguish actual
LGraphNode instances, preferably using the real LiteGraph class or shared
factory instead of a constant mock. Add a selected LGraphNode to the relevant
selectedItems case and assert it is excluded from nonNodeStartPositions,
ensuring the !isLGraphNode filter cannot be inverted or omitted without failing.
Source: Path instructions
This was referenced Jul 31, 2026
Regression: Subgraph exposed widgets eye icons broken in frontend 1.47.11 (worked in 1.45.21)
#14536
Closed
Contributor
Original claim: loading a saved workflow with a connected rgthree Power Prompt deletes its output What is real is the underlying asymmetry, and it is worth fixing before this API reaches more The genuine victim is node.outputs[slot].links.push(linkId) // repair arm -> DISCARDED
node.outputs[slot].links.splice(linkIdIndex, 1) // destroy arm -> HONOUREDA repair tool that silently repairs nothing while its delete path still works is a bad failure Worth noting for the fix decision: ADR 0008:197 promises the safe endpoint ("assignments are Corpus reachability, live control ( |
This was referenced Aug 22, 2026
Open
Open
christian-byrne
left a comment
christian-byrne
left a comment
Contributor
There was a problem hiding this comment.
C3 review — layout / geometry / spatial state. Reviewed at 5002fae1b12d44831a21367afa7c0f798f7e7a2c against merge base 6532665db947acb61ed044fe91a1d4fe1fb84c8b.
There is no pos/size analogue of the NodeOutputSlot asymmetry behind #15620. I went looking for a half-commit and did not find one. All ten mutation idioms I could construct commit to layoutStore, and reads re-synchronise from it. Verified by execution, 20/21 assertions, both control arms live:
| idiom | result |
|---|---|
node.pos = [x,y] / node.size = [w,h] |
commits |
node.pos[0] = x / node.size[0] = w |
commits |
node.pos.set([x,y]) / .fill(v) / += |
commits |
Object.assign(node, { pos }) |
commits |
| read after an external store move, no prior read | reflects the store |
Reroute.pos whole-array and in-place |
both commit |
In-place element mutation and whole-array assignment were tested separately. They do not differ. createMutationView calls synchronize() before capturing the previous value on every trap, so a write always lands on a fresh base — that generality is why this surface has no gaps where the hand-enumerated slot views did.
Corpus check, 157 frontend files / 29 packs, control arm /registerExtension/ live at 84 files: 68 call sites across 6 packs mutate node geometry (.pos[i] = 13 sites, .size[i] = 18, .pos =/.size = 8, setPos/setSize 29). All of them work.
ADR 0003's own 2026-08-04 decision is implemented, which nothing on the branch says out loud. Verified 7/7 with a live control: reportContentSize does not change node.size and does not leak into serialize(); renderingSize is max(requested, content); collapsing does not overwrite requested size. The three defects that amendment lists as motivation are fixed on the serialize axis.
Three defect hypotheses I formed while reading died under execution, and I am withdrawing them rather than softening them:
- "Reroute layouts leak across
clearGraph" — wrong.projectReroute(layoutStore.ts:1218) cleansrerouteLayoutsand the spatial index off theydocobserver. Probe confirmsqueryRerouteAtPointreturns null afterclearGraph, with a live pre-clear control. - "
LGraphNode.move()andsnapToGrid()read a raw unsynchronised_pos" — wrong. Both callrefreshGeometry()on the immediately preceding line (LGraphNode.ts:2310,:3634). My grep showed the read and not its predecessor. - "The minimap digest mixes a
nodeGeometryVersionthat does not exist" — wrong, it is atlayoutStore.ts:248.
One thing worth calling out as a strength, because it is load-bearing and untested: the clear() ordering. teardownOwnedGraphs passes removeLayouts: !owner.isRootGraph so a root graph's entries survive the detach, and resetAfterClear removes them via layoutStore.clearGraph(graphId) before reassigning this.id. Reverse those two and every workflow reload inherits the previous session's node positions. Nothing pins that order — suggested as a regression test, not a change request.
Three inline comments below. Filed separately: #15623 (document the mirror contract), #15624, #15625, #15626, plus two pre-existing ones I verified against the merge base and am not attributing to this PR: #15627, #15628.
Full writeup, denominators and the withdrawn-claims section: research/review/c3-layout-geometry.md in the ECS review workspace.
Not run and not cited: pnpm lint (eslint reports nothing on src/lib/litegraph/, where most of this slice lives, so an eslint-clean claim there would be vacuous), pnpm typecheck, and Playwright. rendererToggleGeometry.spec.ts and resize.spec.ts changed here and are unexercised by me.
| commit() | ||
| } | ||
|
|
||
| return new Proxy(target, { |
Contributor
There was a problem hiding this comment.
createMutationView is the reason node.pos[0] = x still works after geometry moved into the CRDT store, and it is the best-built mirror on the branch — trapping set, deleteProperty and defineProperty as well as get, and calling synchronize() before snapshotting the previous value so writes land on a fresh base. That generality is why I could not find a half-commit here.
It is also documented nowhere: git grep -c createMutationView 5002fae1b1 -- docs returns zero files. Five other ECS concerns got a per-concern doc on this branch; layout got none, and ADR 0003 carries the reasoning without ever naming this mechanism.
That matters for one row an extension author cannot guess: {...node} drops pos and size (prototype accessors) while keeping _pos/_size/_posSize — the same shape as #15594. I checked reachability and found none: 0 of 29 packs spread a node (control live at 84/157 files), and of 13 candidate spread sites in src/ against a 238-spread control, none spreads an LGraphNode.
Filed as #15623. Not blocking.
Contributor
Author
There was a problem hiding this comment.
The code is the documentation 😛
| if (nodeAttachments.has(node)) detachNodeLayout(node) | ||
|
|
||
| const graphId = graph.rootGraph.id | ||
| if (layoutStore.getNodeLayout(graphId, node.id)) { |
Contributor
There was a problem hiding this comment.
This branch adopts the store's entry and discards the node's own pos/size, silently. adoptNodeAttachment below does readNodeRect(graphId, node.id, node._posSize), writing the store rect straight over _pos/_size.
Verified: seed a store entry at (-500,-600,11,12), then graph.add(node) with node geometry (10,20,200,100) — both end at the store's values.
This is the only place on the geometry surface where a node-vs-store disagreement resolves store-first, and it reads like a fast path rather than a precedence rule.
I could not construct a production path that reaches it, so this is a comment-or-assert ask, not a bug report. The guards: remove deletes via detachNodeLayout (LGraph.ts:1313), root clear() deletes via clearGraph (:582), subgraph release deletes via detachGraphLayouts (:1306), and replacement goes through transferLayoutAttachment, which adopts deliberately. Undo/redo is snapshot-based through configure, which clears first — reasoning, not something I ran.
Filed as #15624.
| size: [number, number] | ||
| }> | ||
| ): void { | ||
| clearViewGeometry(): void { |
Contributor
There was a problem hiding this comment.
clearViewGeometry() clears nodeChangeListeners two lines below, which drops every onNodeChange subscription without running any of the unsubscribe closures it handed out. Consumers keep a live-looking stop-closure and go deaf. This runs on every renderer toggle (GraphCanvas.vue:272, :281).
Verified — and the control needed repairing first, which is worth flagging: layoutStore dispatches through queueMicrotask (:1269), so nothing fires synchronously and a synchronous "listener did not fire" assertion passes for the wrong reason. With await setTimeout(0), the control fires and the post-clear case does not.
Latent, not live: onNodeChange has zero non-test consumers at this SHA — the only layoutStore subscribe method without one. onChange and onGeometryChange are both used by notifyLayoutChanges.ts and both are correctly released on unmount and on the mid-life renderer toggle.
So the ask is to pick one now rather than later: leave nodeChangeListeners alone here (a subscription is not view-scoped data), or delete onNodeChange as dead API. Either is fine; the failure mode for the next person to add a consumer only reproduces after a renderer toggle.
Filed as #15625.
This was referenced Aug 22, 2026
…path (#15595) ## Summary comfyui-promptchain does not break through `slotLinks.ts:192`. Its break is a read divergence: `{...slot}` cannot carry `link` because `link` is a prototype accessor, so the pack's own later reads of `slot.link` return `undefined` and it acts on "nothing is connected". This pins that mechanism, and pins the `:192` early return separately so the two stop being conflated. Tests only. No production change. ## Changes - **What**: five tests in `legacySlotLinkMutations.test.ts`, replaying the two functions the pack actually runs — `mobcat40/ComfyUI-PromptChain` `js/lib/order-chain.js:123` (`updateInputLabels`, the `node.inputs[i] = {...slot}` site) and `js/main.js:507` (`trimEmptyAutogrowSlots`, a raw `inputs.splice` gated on `input.link != null`). Three pass, two are `it.fails` asserting the wanted behaviour: | Test | Kind | Pins | | --- | --- | --- | | `keeps the link store correct when the pack replaces every slot` | pass | The indexed overwrite does not corrupt the store. `removeInput` after it reindexes correctly. | | `never rejects the endpoint batch, though a rejection is detectable` | pass | Two arms. Real promptchain sequence: 0 rejections. Forced rejection: 1, and the node keeps its old input layout while `removeInput` returns normally — #15593's claim, asserted. | | `reads the live link id back through a spread copy` | `it.fails` | `copy.link` should return the live id. Returns `undefined`. | | `keeps connected inputs when the pack re-reads slot.link` | `it.fails` | After the pack's label pass, its own trim reads `.link` as `undefined` on every slot, computes `keepCount = 1`, and splices off 3 of 4 inputs including all 3 connected ones. | | `keeps every input when the same trim reads live slots` | pass | Control for the row above. Same fixture, same trim, live class slots: removes 0. | The control is the point: 3 slots destroyed vs 0, same function, same fixture, one arm known non-zero. `graph.serialize()` after that trim still lists all three links targeting input slots the node no longer has, so this reaches the saved workflow. ## Review Focus **The `:192` early return is real but is not promptchain's bug.** Across every sequence tried — reentrant slot substitution from `onConnectionsChange` during `removeInput`, raw `inputs.splice` followed by `removeInput`, and reconnecting after both — `updateEndpoints` returned `ok` every time and `console.error('Failed to replace node inputs')` fired 0 times. It stays worth fixing on its own merits; it is not the pack break. **Consequence for the shim.** A shim that intercepts the write and routes `link`/`links` mutations does not help here, because promptchain never writes `link` — it reads it. Mutation M1 below simulates a shim that rehydrates the assigned plain object into a `NodeInputSlot`; that one does fix it. ## Mutation verification Baseline for this file: **8 passed | 6 expected fail (14)**. | Mutation | Result | | --- | --- | | M1 — rehydrate plain objects into `NodeInputSlot` on indexed assignment to `node.inputs` (a shim simulation) | **3 failed \| 8 passed \| 3 expected fail**. All three failures are `it.fails` markers flipping to pass: both new ones plus the pre-existing `disconnects through a plain-object input slot`. Widened to `src/lib/litegraph/src/node/` + `linkStore.test.ts` + `LLink.store.test.ts`: **3 failed \| 124 passed \| 3 expected fail (130)** — same three markers, no genuine regression. | | M2 — drop the `return []` at `slotLinks.ts:194` so the rejection falls through | **1 failed \| 7 passed \| 6 expected fail** — `never rejects the endpoint batch`, on the layout assertion. | | M3 — drop `removals` from the endpoint batch in `replaceNodeInputs` | **1 failed \| 7 passed \| 6 expected fail** — `keeps the link store correct`. | Each reverted; baseline restored and re-run green after each. Both `it.fails` markers were checked by flipping them to `it`: both fail on `AssertionError`, never on a `TypeError`. The first draft of one of them passed on `TypeError: source.getOutputLinks is not a function` and was rewritten. ## Gates `oxfmt --check`, `oxlint --type-aware` (verified non-vacuous against a deliberately bad file: exit 1), `knip` exit 0, `vue-tsc --noEmit` at 8GB: 0 errors on any `src/lib/litegraph/` path. The 1418 `TS2688` plus 68 implicit-any errors it reports are the known symlinked-`node_modules` artifact, all in unrelated `.vue` files. - Refs #15593 --------- Co-authored-by: Amp <amp@ampcode.com>
## Summary
`{ ...graph.links[id] }` loses every topology field on this branch, and
there was no test for it. This adds the `LLink` half of the failure
class `node/legacySlotLinkMutations.test.ts` already pins for slots.
## Changes
- **What**: `src/lib/litegraph/src/LLink.spreadCopy.test.ts` — two
`it.fails` cases asserting the behaviour we want, so they flip green
when a fix lands.
1. A spread copy of a link carries `id`, `type`, `origin_id`,
`origin_slot`, `target_id`, `target_slot`.
2. The realistic consequence: snapshot outgoing links as spread copies,
`disconnectOutput`, reconnect the far ends from the copies, and the
downstream nodes are still wired.
`LLink` went from 16 own-field declarations at merge-base `6532665db9`
to 12 at head, with the six topology fields becoming prototype accessors
over `_state` and zero `defineProperty` calls. Prototype accessors are
not own properties, so spread copies nothing.
Case 2 is the shape ComfyUI-Custom-Scripts uses in "Add Clip Skip" and
"Add LoRA" (`web/js/quickNodes.js:132`, `:156-157`): the user's
downstream wiring is disconnected and never restored. Filed as #15594.
Only case 1 proves the mechanism. Case 2 is what a user reports.
## Review Focus
**Mutation numbers.** Scratch edit defining the six fields as own
enumerable accessors in the `LLink` constructor, then reverted:
| run | test files | tests |
| --- | --- | --- |
| baseline | 85 passed | 1261 passed, 6 expected fail |
| mutated | 1 failed, 84 passed | 2 failed, 1261 passed, 4 expected fail
|
| reverted | 85 passed | 1261 passed, 6 expected fail |
The 2 failures are exactly the 2 new cases, failing with `Expect test to
fail`. Nothing else moved.
**Corpus: 1 of 29 local packs, 3 call sites — all in
ComfyUI-Custom-Scripts.** Measured over 157 frontend pack files, control
`/registerExtension/` → 84 files. Widened to `Object.assign({}, link)`,
`structuredClone`, and `JSON.parse(JSON.stringify(...))` on link-named
identifiers: 0 additional. This bounds #15594 rather than broadening it
— the idiom is rare, but the one pack using it is widely installed.
**`vue-tsc` already knows.** The first draft typed the copies as the
spread's inferred type and produced 8 `TS2339` errors — TypeScript
models the accessors as dropped. The committed version uses
`Partial<LLink>`, which is what a JS caller actually holds. Typecheck at
8GB: 0 errors in the new file (1494 → 1486 total, the 8 removed are
mine).
The file carries `// oxlint-disable no-misused-spread` — that rule fires
on exactly the pattern under test.
Refs #15594
---------
Co-authored-by: Amp <amp@ampcode.com>
Both #15577 and #15581 reproduce on this branch, and #15577 is reachable through files today's shipping frontend already saves. Seven `it.fails` assertions state the wanted behaviour; ten controls keep them from passing for the wrong reason. ## #15577 — reachable, and it re-wires the node `normalizeConfiguredTopology` keys duplicates on `target_id:target_slot` and keeps `data.links[first]` rather than the link `input.link` names. With two links into one input from different origins, the node comes up fed by a different upstream node than the file names, and nothing is logged. Reachability, measured rather than argued. Driving rgthree `link_fixer`'s mirror-only creation idiom (`output.links.push(id)` + `input.link = id`, no `connect()`) on `main` at `32d0b6e202` and serializing produces: ``` "links":[[1,1,0,3,0,"number"],[2,2,0,3,0,"number"]] "inputs":[{"name":"input_0","type":"number","link":2}] ``` That is `conflictingOriginLinksRoot` byte for byte. The shape is not hypothetical and it persists across every save on `main`. Same fixture, both branches: | | `main` `32d0b6e202` | this branch `9f6ab9adda` | | --- | --- | --- | | `getInputLink(0).origin_id` | 2, the origin `input.link` names | 1 | | links at `3:0` | 2 | 1 | Dropping to one link is correct, the store enforces one link per input. Which one survives is the regression. This branch closes the producer: the `input.link` setter ignores id assignment and the `output.links` view discards additions, so the same idiom now serializes one link. Pinned by `discards a link written only through the legacy slot mirrors`. Forward-safe, but files saved before the branch ships still load wrong. Mutation: preferring the `input.link`-referenced survivor and warning on the drop flips both `it.fails` to pass, controls unchanged. The negative control `test-cases.md` specified does not exist. Re-narrowing the key to the 4-tuple was predicted to leave two links registered and fail the degeneracy control. It does not: the store rejects the second link downstream and the terminal graph state is identical, link count and all. The 4-tuple key and the 2-tuple key are indistinguishable by any assertion on the resulting graph for this fixture. The control that remains proves the fixture is non-degenerate, which is what it is for. ## #15581 — reproduces, and the issue's suggested fallback does not fix it A serialized input whose name has no live counterpart is skipped, keeps its stale `target_slot`, and the resulting `occupied-target` rejection `break`s the loop, abandoning every remaining move for that node. Three cases in `LGraph.inputSlotRealign.test.ts`: `configure` drops an input, `configure` renames one, and `realignInputLinkSlots` called directly on a pre-seeded incumbent. Net effect in cases 1 and 2 is that `in_a` reports `in_c`'s link and `in_b` reports `in_a`'s. No `has_errors`, `console.error` only. Two mutations, and they disagree: | Mutation | `it.fails` flipped | Controls | | --- | --- | --- | | Per-link retry after a rejected batch, the issue's fallback | 1 of 4 | 8 green | | Blocking incumbents passed to `updateEndpoints`'s `removals` | 4 of 4 | 8 green | The per-link fallback fails because the collision cascades: link 3 squats slot 0, so link 1 cannot move off slot 1, so link 2 cannot move onto it. Retrying one at a time hits the same wall. Only evicting the stale incumbent clears it. Worth deciding before anyone implements the issue's first suggestion. ## Counts 17 tests across the two files: 10 pass, 7 expected fail. The eight pre-existing `#3348` tests stay green under both mutations, which is the negative control that the fix does not break what #3348 was filed for. `oxfmt` and `oxlint --type-aware` clean. `vue-tsc --noEmit` clean for the three files in this diff; the repo-wide count is not meaningful from this worktree, see below. On typecheck, because it will bite the next person working in a worktree with a borrowed `node_modules`. The first two pushes carried 10 real type errors that `vitest`, `oxfmt` and `oxlint --type-aware` all passed: nine branded-id mismatches (`TS2339`, `TS2322` on `LinkId`) and one wrong import path (`TS2459`). Fixed in `4651215809` and `20039d9e42`. They got through because my local typecheck was lying, in two different ways: 1. `vue-tsc` **OOMs at the 2GB default heap** — `FATAL ERROR: Ineffective mark-compacts near heap limit`, exit 134, after 18 lines. Filtered for errors that reads as zero. Silence from a dead process is indistinguishable from a pass. 2. The phantom errors are **not only `TS2688`**. With `NODE_OPTIONS=--max-old-space-size=8192` the run completes at exit 2 with 1418 `TS2688` plus 68 `TS7006`/`TS7031`/`TS7053` implicit-any errors in `.vue` and `.stories.ts` — downstream fallout of the broken type roots, not real. Working recipe: larger heap, confirm exit 2 rather than 134, then grep for your own changed files. Zero for all three files here. ## Wider class, filed separately All three `updateEndpoints` call sites on this branch swallow rejection into `console.error` and degrade silently, none marks `has_errors`, none uses the repo's unified `reportError`. `slotLinks.ts:192` is the worst of them: on rejection it returns `[]` before `node.inputs.splice`, so the node keeps its old input layout and the empty return is indistinguishable from "nothing to replace". Follow-ups tracked in the workspace todo. - Repros #15577 - Repros #15581 --------- Co-authored-by: Amp <amp@ampcode.com>
Contributor
Current state: head What actually caused the red, by bisect over 21 commits with the test file pinned to known-good Not Correction to my The branch also improves parity — One genuine defect did fall out, filed as #15662: Apologies for the noise on a green PR — the symptom was real when I posted it, the attribution was |
#15596) `WidgetValue` and `NodeProperty` both admit `null`, `setProperty(name, null)` propagates it to the bound widget, and `LGraphNode.serialize` actively mints it — `val ?? null` turns an `undefined` widget value into `null` on the way into `widgets_values`. So `null` is already in first-party workflow JSON with no extension involved. Nothing asserted that any persistence path round-trips it, and on `main` one path does not: `SubgraphNode.serialize` filters through `isWidgetValue`, whose `main` predicate rejects `null`, so a null promoted value becomes `undefined` and — when every promoted widget is null — the whole `widgets_values` key is deleted. This branch already fixes the behaviour by widening `isWidgetValue` to `value == null`. This PR adds the assertions that were missing on both sides. I enumerated every persistence path that filters widget values, not just the subgraph one. Verdicts: | Path | Filter | Round-trips null? | | --- | --- | --- | | `LGraphNode.serialize` | `widget.serialize === false`; `val ?? null` | yes — and mints null from undefined | | `LGraphNode.configure` (indexed + named-values) | `serialize === false`, `name in namedValues` | yes | | `SubgraphNode.serialize` | `isWidgetValue` | yes on this branch, **no on `main`** | | `SubgraphNode._applyPromotedWidgetValues` | `value !== undefined` | yes | | `executionUtil` API prompt | `widget.options?.serialize === false` | yes | | draft / autosave | none of its own | transparent | | `workflowSchema` load validation | `z.any()` | yes | | clipboard `_serializeItems` | none of its own | transparent | | `proxyWidgetMigration` | `isWidgetValue` | yes on this branch, **no on `main`** (null classed as a hole) | `isWidgetValue` has two call sites, not one — the same predicate change fixes both. New coverage, all mutation-verified (mutating the corresponding filter to drop null fails the test; unmutated control passes): - `widgetValueNullContract.test.ts` — workflow write, workflow read via both restore branches, draft-cache JSON text, two-pass idempotence, plus a `widget.serialize === false` control arm and a stored-null-vs-absent control arm - `executionUtil.test.ts` — prompt path emits null, with an `options.serialize` control arm - `workflowSchema.test.ts` — array and object forms preserve null One trap worth flagging for reviewers: widget state is keyed `graphId:nodeId:name` and `configure()` restores the original graph id, so a naive save/load test re-adopts the live `widgetValueStore` entry and passes **without reading the serialized JSON at all**. My first draft did exactly that. `roundTripGraph` drops the store between save and load; without that line four of these tests are vacuous. ADR 0016 records the contract and the evidence, including the reachability measurement (nine public extension repos assign null to a widget value, against a 4,280-file control arm) and the blast radius of the alternative (removing `null` from `WidgetValue` yields 19 `vue-tsc` errors across 11 files, against a 0-error control). Gates run by hand — worktrees run no hooks: `oxfmt`, `oxlint --type-aware` (clean, verified against a control that emits), `vue-tsc --noEmit` exit 0 / 0 errors, 52 tests passing. --------- Co-authored-by: GitHub Action <action@github.com> Co-authored-by: Amp <amp@ampcode.com>
## Summary Add behavioral coverage for the remaining ECS migration bridge risks and update the migration audit to reflect the new evidence. ## Changes - **What**: Adds Playwright coverage for promoted-subgraph delete/undo/redo and geometry across navigation, renderer switching, history, and reload; adds Vitest regressions for virtual-consumer role inference and root-scoped, non-creating coach-target layout reads; updates the migration plan and verification audit with covered and remaining gaps. ## Review Focus Confirm the bridge-history scenarios and audit assessments match the intended ECS authority boundaries. Verification: `pnpm test:browser:local browser_tests/tests/vueNodes/layout/ecsBridgeHistory.spec.ts --repeat-each 2` (4 passed); `pnpm test:unit src/renderer/extensions/firstRunTour/roles/heuristicRoles.test.ts src/renderer/extensions/firstRunTour/tour/canvasCoachTarget.test.ts` (60 passed); `pnpm typecheck`; `pnpm typecheck:browser`; targeted ESLint, oxlint, and oxfmt checks. Child of #15443. --------- Co-authored-by: Amp <amp@ampcode.com>
Contributor
|
Root cause: not a badge regression, and the PR is green now. It does not block Monday. Following up on my comment above, which I need to correct in two places. 1. The head moved and the failure is already fixed. I measured 2.
Why it went red. Badges still reach the canvas — both sets, composed in const badgeInstances = [
...badgeDrawObjects(this, badgeRows(this)), // derived core/credits rows
...this.badges.map(...) // extension badges, API unchanged
]
I checked the fix is not vacuous, since the new The branch improves parity by one case: On
42: const node = rootGraph ? resolveNode(nodeData.id, rootGraph) : undefined
43: for (const row of node ? nodeBadges(node) : []) // core + pricing
54: for (const badge of (node?.badges ?? []).map(toValue)) // extension badges
One real defect, pre-existing and not a blocker. The 3 remaining
Scope: Two things I did not file as blockers but that are worth a look:
|
This was referenced Aug 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Still-draft feature branch for migrating entity state from LiteGraph objects and runtime mirrors into dedicated stores with derived, reactive views.
This consolidates the reviewed ECS migration slices, strengthens their coverage, incorporates the layout-state follow-up, and documents the current migration status and remaining work.
Changes
Reviewed PRs merged into this feature branch:
Review Focus
Status
Still draft. The branch is rebased onto
main; CI is running against the rebased history.