fix: simplify ensureCorrectLayoutScale and fix link sync during Vue node drag#9680
Merged
fix: simplify ensureCorrectLayoutScale and fix link sync during Vue node drag#9680
Conversation
…drift Prefer graph.extra.workflowRendererVersion over the caller-supplied renderer parameter. After scaling sets this metadata, subsequent calls see the graph already matches and return early. Remove default 'LG' fallback — if renderer is unknown and graph has no metadata, do nothing instead of assuming LG and scaling. Amp-Thread-ID: https://ampcode.com/threads/T-019cd488-7093-77ab-b91f-01b2009351b9 Co-authored-by: Amp <amp@ampcode.com>
dosubot
bot
added
the
size:L
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughOne-time graph-wide normalization converts Vue-mutated coordinates back to canonical LiteGraph coordinates; layout store adds synchronous per-node listeners and batched global notifications; layout sync batches writes via microtask/RAF and exposes start/stop; node dragging/resizing use batched move/update mutations; many unit and E2E tests added. Changes
Sequence DiagramsequenceDiagram
rect rgba(200,220,255,0.5)
participant UI as User/UI
participant Sync as useLayoutSync
participant Store as LayoutStore
participant RAF as RAF/microtask
participant Lite as LiteGraph
end
UI->>Sync: emit layout change (nodeId)
activate Sync
Sync->>Sync: collect pendingNodeIds
Sync->>RAF: scheduleFlush (microtask or RAF)
deactivate Sync
Note over UI,RAF: multiple events coalesce
RAF->>Sync: flushPendingChanges
activate Sync
loop for each pendingNodeId
Sync->>Store: graph.getNodeById(nodeId)
Store-->>Sync: liteNode
Sync->>Lite: apply position & size updates
end
Sync->>Store: set source and flush batched writes
Sync->>Sync: clear pendingNodeIds
deactivate Sync
alt Node-scoped listeners
Store->>Store: notifyNodeChange(change) (synchronous)
Store->>UI: invoke per-node callbacks
else Global listeners
Store->>Store: queueGlobalChange(change) (batched)
Store->>Store: flushQueuedGlobalChanges() (microtask)
Store->>UI: invoke global callbacks
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
🎨 Storybook: ✅ Built — View Storybook |
🎭 Playwright: ✅ 546 passed, 0 failed · 8 flaky📊 Browser Reports
|
📦 Bundle: 4.61 MB gzip 🔴 +1.41 kBDetailsSummary
Category Glance App Entry Points — 17.8 kB (baseline 17.8 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.02 MB (baseline 1.01 MB) • 🔴 +2.42 kBGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 72.5 kB (baseline 72.5 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 441 kB (baseline 441 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed User & Accounts — 16.1 kB (baseline 16.1 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 5 added / 5 removed Editors & Dialogs — 78.3 kB (baseline 78.3 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 56.7 kB (baseline 56.7 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed Data & Services — 2.8 MB (baseline 2.79 MB) • 🔴 +2.13 kBStores, services, APIs, and repositories
Status: 15 added / 15 removed Utilities & Hooks — 61.5 kB (baseline 61.5 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 11 added / 11 removed Vendor & Third-Party — 8.9 MB (baseline 8.9 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 8.16 MB (baseline 8.16 MB) • 🔴 +103 BBundles that do not match a named category
Status: 50 added / 50 removed |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.ts (1)
32-33: Consider optional chaining for defensive access.If
graph.extrais ever undefined, this line would throw a TypeError. While LGraph typically always initializesextra, optional chaining would be more defensive.🛡️ Optional defensive fix
- const savedRenderer = graph.extra.workflowRendererVersion ?? renderer + const savedRenderer = graph.extra?.workflowRendererVersion ?? renderer🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.ts` around lines 32 - 33, The access to graph.extra is not defensive; change the retrieval of savedRenderer to use optional chaining so it reads the workflow renderer safely (replace graph.extra.workflowRendererVersion with graph.extra?.workflowRendererVersion) — i.e., update the expression that sets savedRenderer in ensureCorrectLayoutScale to use graph.extra?.workflowRendererVersion ?? renderer and keep the existing if (!savedRenderer) return guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.test.ts`:
- Around line 172-174: The test comment is misleading: with savedExtra = {} and
renderer = undefined the savedRenderer remains undefined so
ensureCorrectLayoutScale runs as a no-op and distances remain equal; update the
comment above the assertions (near the distances expectations) to state that
this test verifies the "no-op when renderer unknown / savedRenderer undefined"
behavior (or alternatively modify the test setup by providing a non-undefined
renderer/savedExtra to actually test the "first load scales up, subsequent loads
do not compound" scenario), referencing savedExtra, renderer, savedRenderer, and
the distances assertions so maintainers can find and understand the change.
---
Nitpick comments:
In `@src/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.ts`:
- Around line 32-33: The access to graph.extra is not defensive; change the
retrieval of savedRenderer to use optional chaining so it reads the workflow
renderer safely (replace graph.extra.workflowRendererVersion with
graph.extra?.workflowRendererVersion) — i.e., update the expression that sets
savedRenderer in ensureCorrectLayoutScale to use
graph.extra?.workflowRendererVersion ?? renderer and keep the existing if
(!savedRenderer) return guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 40d153ee-9911-44e2-9bc2-331b9797084c
📒 Files selected for processing (2)
src/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.test.tssrc/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.ts
src/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.test.ts
Outdated
Show resolved
Hide resolved
⚡ Performance Report
Raw data{
"timestamp": "2026-03-13T08:06:51.774Z",
"gitSha": "84c9a0936651d39af2c2323494288f464f553056",
"branch": "drjkl/dark-energy",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2031.4730000000054,
"styleRecalcs": 11,
"styleRecalcDurationMs": 10.341999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 415.41299999999995,
"heapDeltaBytes": 798080
},
{
"name": "canvas-idle",
"durationMs": 2038.5099999999738,
"styleRecalcs": 11,
"styleRecalcDurationMs": 11.091999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 406.87600000000003,
"heapDeltaBytes": 780036
},
{
"name": "canvas-idle",
"durationMs": 2035.3390000000218,
"styleRecalcs": 10,
"styleRecalcDurationMs": 11.005,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 410.22,
"heapDeltaBytes": 864108
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1954.430000000002,
"styleRecalcs": 80,
"styleRecalcDurationMs": 44.736000000000004,
"layouts": 12,
"layoutDurationMs": 3.908,
"taskDurationMs": 868.593,
"heapDeltaBytes": -5248424
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1995.4099999999926,
"styleRecalcs": 82,
"styleRecalcDurationMs": 47.315999999999995,
"layouts": 12,
"layoutDurationMs": 4.029000000000001,
"taskDurationMs": 941.936,
"heapDeltaBytes": 1476692
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1828.6719999999832,
"styleRecalcs": 75,
"styleRecalcDurationMs": 41.182,
"layouts": 12,
"layoutDurationMs": 3.801,
"taskDurationMs": 795.208,
"heapDeltaBytes": 1550236
},
{
"name": "dom-widget-clipping",
"durationMs": 560.483999999974,
"styleRecalcs": 14,
"styleRecalcDurationMs": 10.382000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 349.87,
"heapDeltaBytes": 13063584
},
{
"name": "dom-widget-clipping",
"durationMs": 608.2260000000019,
"styleRecalcs": 13,
"styleRecalcDurationMs": 9.948,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 378.28999999999996,
"heapDeltaBytes": 12634420
},
{
"name": "dom-widget-clipping",
"durationMs": 582.1530000000052,
"styleRecalcs": 16,
"styleRecalcDurationMs": 14.982000000000003,
"layouts": 1,
"layoutDurationMs": 0.2980000000000001,
"taskDurationMs": 368.093,
"heapDeltaBytes": 13476156
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 595.3460000000064,
"styleRecalcs": 49,
"styleRecalcDurationMs": 14.658,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 427.477,
"heapDeltaBytes": -3819676
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 681.5839999999866,
"styleRecalcs": 48,
"styleRecalcDurationMs": 15.177999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 455.20000000000005,
"heapDeltaBytes": -3806176
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 705.8150000000296,
"styleRecalcs": 51,
"styleRecalcDurationMs": 19.963,
"layouts": 1,
"layoutDurationMs": 0.22500000000000006,
"taskDurationMs": 480.558,
"heapDeltaBytes": 15989916
},
{
"name": "subgraph-idle",
"durationMs": 1997.4500000000148,
"styleRecalcs": 11,
"styleRecalcDurationMs": 10.342,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 348.78,
"heapDeltaBytes": 1283224
},
{
"name": "subgraph-idle",
"durationMs": 1995.1790000000074,
"styleRecalcs": 11,
"styleRecalcDurationMs": 11.468999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 413.62100000000004,
"heapDeltaBytes": 600008
},
{
"name": "subgraph-idle",
"durationMs": 2008.5860000000366,
"styleRecalcs": 12,
"styleRecalcDurationMs": 12.270999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 448.402,
"heapDeltaBytes": 534992
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1695.934999999963,
"styleRecalcs": 77,
"styleRecalcDurationMs": 36.46,
"layouts": 16,
"layoutDurationMs": 4.228,
"taskDurationMs": 680.3019999999999,
"heapDeltaBytes": -1113992
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1722.3199999999679,
"styleRecalcs": 77,
"styleRecalcDurationMs": 42.086000000000006,
"layouts": 16,
"layoutDurationMs": 4.682,
"taskDurationMs": 784.524,
"heapDeltaBytes": -1030808
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1682.2760000000017,
"styleRecalcs": 77,
"styleRecalcDurationMs": 38.12199999999999,
"layouts": 16,
"layoutDurationMs": 4.612,
"taskDurationMs": 777.146,
"heapDeltaBytes": -757448
}
]
} |
- Replace coordinateSpaceVersion + workflowRendererVersion deletion with a single 'Vue-corrected' RendererType value - Make graph parameter required; remove comfyApp fallback - Reuse Point/Bounds types from layout/types instead of local interfaces - Remove no-op ensureCorrectLayoutScale calls from useVueNodeLifecycle - Update callers in app.ts to always pass graph explicitly - Add E2E test for renderer toggle position stability Amp-Thread-ID: https://ampcode.com/threads/T-019cd4e5-bc6d-72ee-8677-554212a98419 Co-authored-by: Amp <amp@ampcode.com>
🎨 Storybook:
|
Remove setTimeout wrapper from notifyChange in finalizeOperation so useLayoutSync pushes positions to LiteGraph within the same RAF frame as the drag update. This fixes links not following Vue nodes during drag. Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019cd504-cbcc-723e-b2c4-06eb6e488101
DrJKL
force-pushed
the
drjkl/dark-energy
branch
from
595a30b to
9e72a6e
Compare
DrJKL
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/scripts/app.ts (1)
1316-1321:⚠️ Potential issue | 🟠 MajorParameter precedence in
ensureCorrectLayoutScale()creates re-normalization risk.The helper uses
rendererVersion ?? graph.extra.workflowRendererVersion, prioritizing the parameter over the graph's current metadata. If the function is called on an already-normalized graph with a stale'Vue'parameter, it will skip the'Vue-corrected'marker and attempt re-normalization—applying the coordinate transform twice, which inverts the correction.For the call at line 1321, the parameter is read immediately before use, so the risk is contained within this operation. However, the design lacks idempotency protection. Consider restructuring to:
- Prefer
graph.extra.workflowRendererVersionand treat the parameter as a true fallback only when graph metadata is missing- Add a guard: return early if
graph.extra.workflowRendererVersion === 'Vue-corrected', regardless of parameter value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scripts/app.ts` around lines 1316 - 1321, The helper ensureCorrectLayoutScale currently prefers the rendererVersion parameter over graph.extra.workflowRendererVersion, which can cause double-renormalization; update ensureCorrectLayoutScale so it first reads graph.extra.workflowRendererVersion (use the parameter rendererVersion only as a fallback when the graph metadata is missing), and add an early guard that returns immediately if graph.extra.workflowRendererVersion === 'Vue-corrected' to make the function idempotent; adjust callers like ensureCorrectLayoutScale(originalMainGraphRenderer, this.rootGraph) only if they rely on the old precedence.
🧹 Nitpick comments (3)
browser_tests/tests/rendererToggleStability.spec.ts (1)
35-41: Load a pinned workflow fixture before asserting positions.This snapshots whatever graph the fixture happens to boot with, so the test's coverage and stability will drift with unrelated startup/default-workflow changes. Load a known JSON from
assets/first, then run the toggle assertions against that fixed graph.As per coding guidelines, use premade JSON workflows in the
assets/directory to load desired graph state in E2E tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@browser_tests/tests/rendererToggleStability.spec.ts` around lines 35 - 41, The test "node positions do not drift when toggling between Vue and LiteGraph renderers" currently snapshots whatever graph is present at startup; before calling getAllNodePositions(comfyPage) load a deterministic workflow JSON from the assets/ directory (e.g., a premade fixture like assets/<desired-workflow>.json) into the app using the test page helper (via comfyPage import/upload or the app's load workflow API), wait for the workflow to finish loading (nodes rendered), then proceed with the TOGGLE_COUNT loop and assertions; update the test body around getAllNodePositions to perform this deterministic load so positions are asserted against a known graph.src/renderer/core/layout/transform/graphRenderTransform.test.ts (1)
89-99: Consider usingsatisfiesfor mock type validation.The
as nevercasts work but lose type safety. Per repository learnings, prefersatisfiesfor mock objects to maintain shape validation while allowing partial implementations.♻️ Optional improvement using satisfies pattern
- const anchor1 = getGraphRenderAnchor(mockGraph as never) + const anchor1 = getGraphRenderAnchor(mockGraph as unknown as LGraph) // Mutate positions — anchor should stay frozen mockGraph.nodes[0].pos = [500, 600] - const anchor2 = getGraphRenderAnchor(mockGraph as never) + const anchor2 = getGraphRenderAnchor(mockGraph as unknown as LGraph)Or define a helper function that creates properly typed mock graphs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/core/layout/transform/graphRenderTransform.test.ts` around lines 89 - 99, Replace the unsafe "as never" casts in the tests with a proper typed mock using TypeScript's "satisfies" so the mock maintains the Graph shape while allowing partial fields; locate uses of getGraphRenderAnchor in this test (e.g., the mockGraph declarations around the anchor1/anchor2 and empty-graph cases) and change those mockGraph objects to use "satisfies Graph" (or create a small helper like createMockGraph(...) that returns a value typed as Graph via satisfies) so the compiler validates the mock shape without forcing full implementation.src/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.test.ts (1)
14-28: Minor semantic note:Pointused forsizeproperty.The
sizeproperty represents width/height dimensions, but it's typed asPoint(which semantically represents x/y coordinates). This works because both are[number, number]tuples, but could be confusing. Consider using aSizetype alias or[number, number]directly for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.test.ts` around lines 14 - 28, The size property is currently typed as Point which semantically implies coordinates; change its type to a Size alias or the explicit tuple [number, number] to make intent clear: update the declaration of size in the returned object (and any related type imports) from "size: [w, h] as Point" to use a Size type or "[number, number]" and ensure getters/setters (width, set width) and boundingRect continue to operate on the new Size type without runtime changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/core/layout/store/layoutStore.ts`:
- Around line 920-922: batchUpdateNodeBounds temporarily sets the shared field
this.currentSource = LayoutSource.Vue and only restores it after
applyOperation() returns, but notifyChange(change) is called while the temporary
context is active so synchronous listeners inherit the wrong source/actor;
before calling notifyChange(change) restore the previous this.currentSource (and
any related actor/context fields) so listeners run with the original context, or
alternatively change the notification to pass an immutable operation context
object to listeners instead of relying on shared mutable fields (refer to
batchUpdateNodeBounds, this.currentSource, LayoutSource.Vue, applyOperation(),
and notifyChange()).
---
Outside diff comments:
In `@src/scripts/app.ts`:
- Around line 1316-1321: The helper ensureCorrectLayoutScale currently prefers
the rendererVersion parameter over graph.extra.workflowRendererVersion, which
can cause double-renormalization; update ensureCorrectLayoutScale so it first
reads graph.extra.workflowRendererVersion (use the parameter rendererVersion
only as a fallback when the graph metadata is missing), and add an early guard
that returns immediately if graph.extra.workflowRendererVersion ===
'Vue-corrected' to make the function idempotent; adjust callers like
ensureCorrectLayoutScale(originalMainGraphRenderer, this.rootGraph) only if they
rely on the old precedence.
---
Nitpick comments:
In `@browser_tests/tests/rendererToggleStability.spec.ts`:
- Around line 35-41: The test "node positions do not drift when toggling between
Vue and LiteGraph renderers" currently snapshots whatever graph is present at
startup; before calling getAllNodePositions(comfyPage) load a deterministic
workflow JSON from the assets/ directory (e.g., a premade fixture like
assets/<desired-workflow>.json) into the app using the test page helper (via
comfyPage import/upload or the app's load workflow API), wait for the workflow
to finish loading (nodes rendered), then proceed with the TOGGLE_COUNT loop and
assertions; update the test body around getAllNodePositions to perform this
deterministic load so positions are asserted against a known graph.
In `@src/renderer/core/layout/transform/graphRenderTransform.test.ts`:
- Around line 89-99: Replace the unsafe "as never" casts in the tests with a
proper typed mock using TypeScript's "satisfies" so the mock maintains the Graph
shape while allowing partial fields; locate uses of getGraphRenderAnchor in this
test (e.g., the mockGraph declarations around the anchor1/anchor2 and
empty-graph cases) and change those mockGraph objects to use "satisfies Graph"
(or create a small helper like createMockGraph(...) that returns a value typed
as Graph via satisfies) so the compiler validates the mock shape without forcing
full implementation.
In `@src/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.test.ts`:
- Around line 14-28: The size property is currently typed as Point which
semantically implies coordinates; change its type to a Size alias or the
explicit tuple [number, number] to make intent clear: update the declaration of
size in the returned object (and any related type imports) from "size: [w, h] as
Point" to use a Size type or "[number, number]" and ensure getters/setters
(width, set width) and boundingRect continue to operate on the new Size type
without runtime changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 27dec7f7-a0ab-495a-9074-1f0a36907218
📒 Files selected for processing (10)
browser_tests/tests/rendererToggleStability.spec.tssrc/composables/graph/useVueNodeLifecycle.tssrc/lib/litegraph/src/LGraph.tssrc/platform/workflow/validation/schemas/workflowSchema.tssrc/renderer/core/layout/store/layoutStore.tssrc/renderer/core/layout/transform/graphRenderTransform.test.tssrc/renderer/core/layout/transform/graphRenderTransform.tssrc/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.test.tssrc/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.tssrc/scripts/app.ts
💤 Files with no reviewable changes (1)
- src/composables/graph/useVueNodeLifecycle.ts
- Remove projectPoint/projectBounds (YAGNI — no production consumers) - Extract unprojectPosSize helper to DRY repeated marshalling - Remove redundant modeChanged guard in useVueNodeLifecycle watch Amp-Thread-ID: https://ampcode.com/threads/T-019cd514-f65d-7569-b1c8-050d57523d7b Co-authored-by: Amp <amp@ampcode.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.ts`:
- Around line 18-30: The helper unprojectPosSize currently replaces item.pos and
item.size with new arrays which breaks the shared-buffer invariant for
Rectangle/graph.groups; instead mutate the existing arrays in place: after
computing c via unprojectBounds(anchor, RENDER_SCALE_FACTOR) assign the computed
values into the existing item.pos and item.size entries (e.g. update
item.pos[0], item.pos[1], item.size[0], item.size[1]) rather than reassigning
the arrays so any shared backing buffer remains intact.
- Around line 55-56: The function ensureCorrectLayoutScale uses rendererVersion
?? graph.extra.workflowRendererVersion which lets a passed-in rendererVersion
override the graph's own 'Vue-corrected' marker; update the logic in
ensureCorrectLayoutScale (and the similar block around the renderer check at the
other occurrence) to first check graph.extra['Vue-corrected'] (or
graph.extra?.['Vue-corrected']) and return false if present before honoring
rendererVersion, so an already-normalized graph is not reprocessed even when a
fallback rendererVersion of 'Vue' is supplied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fc5d8e47-336f-4bb5-9071-e020b29471da
📒 Files selected for processing (4)
src/composables/graph/useVueNodeLifecycle.tssrc/renderer/core/layout/transform/graphRenderTransform.test.tssrc/renderer/core/layout/transform/graphRenderTransform.tssrc/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/renderer/core/layout/transform/graphRenderTransform.test.ts
- src/composables/graph/useVueNodeLifecycle.ts
- src/renderer/core/layout/transform/graphRenderTransform.ts
src/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.ts
Outdated
Show resolved
Hide resolved
src/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.ts
Outdated
Show resolved
Hide resolved
Move min-w to both outer container and inner wrapper via a shared CSS variable (--min-node-width) so overlays and node body stay in sync. Extract MIN_NODE_WIDTH=225 constant from graphRenderTransform.ts and use it across LGraphNode.vue and useNodeResize.ts. Amp-Thread-ID: https://ampcode.com/threads/T-019cd524-d504-764b-b685-2fd29c09a3b6 Co-authored-by: Amp <amp@ampcode.com>
When a node is collapsed in Vue mode, the DOM shrinks to header-only height. The ResizeObserver was writing this small size back to the layout store, permanently overwriting the stored expanded size. When uncollapsing in litegraph mode, the node would appear with minimized height. Add data-collapsed attribute to tracked node elements and skip ResizeObserver bounds updates for collapsed nodes. Amp-Thread-ID: https://ampcode.com/threads/T-019cd539-7557-7248-a7e1-4ea4ff254ed2 Co-authored-by: Amp <amp@ampcode.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/extensions/vueNodes/composables/useVueNodeResizeTracking.ts`:
- Around line 121-123: The early continue when a node is collapsed prevents
adding it to nodesNeedingSlotResync, leaving cached slot offsets stale; in
useVueNodeResizeTracking.ts (inside the loop that checks elementType and
element.dataset.collapsed) keep skipping the bounds write for collapsed nodes
but still enqueue them for slot resync by adding the node's id to
nodesNeedingSlotResync (or calling the same queuing logic used elsewhere) before
continuing — reference elementType, element.dataset.collapsed,
nodesNeedingSlotResync, and the related LGraphNode.vue behavior to ensure
collapsed nodes get their slot offsets recalculated on next sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3c61b872-f4af-49bd-8794-c292f31d4fe4
📒 Files selected for processing (5)
src/renderer/core/layout/transform/graphRenderTransform.tssrc/renderer/extensions/vueNodes/components/LGraphNode.vuesrc/renderer/extensions/vueNodes/composables/useVueNodeResizeTracking.tssrc/renderer/extensions/vueNodes/interactions/resize/useNodeResize.test.tssrc/renderer/extensions/vueNodes/interactions/resize/useNodeResize.ts
src/renderer/extensions/vueNodes/composables/useVueNodeResizeTracking.ts
Outdated
Show resolved
Hide resolved
Adds a concise guide to `docs/release-process.md` explaining how the release workflows interact, with focus on the version semantics that differ between minor and patch bumps. Key sections: - How minor bumps freeze the previous minor into `core/` and `cloud/` branches - How patch bumps on `main` vs `core/X.Y` differ (published vs draft releases) - Why unreleased commits are dual-homed when a minor bump happens - Summary table, backporting, publishing, and bi-weekly automation ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9548-docs-add-release-process-guide-31d6d73d365081f2bdaace48a7cb81ae) by [Unito](https://www.unito.io) --------- Co-authored-by: Alexander Brown <drjkl@comfy.org>
Main targeted, built on #9551 ## Summary Fix Load Image/Load Video input dropdown tabs not showing available input assets in Vue node select dropdown. ## Changes - **What**: Keep combo widget `options` object identity while exposing dynamic `values` for cloud/remote combos. - **What**: Remove temporary debug logging and restore clearer dropdown filter branching. - **What**: Remove stale `searcher`/`updateKey` prop plumbing in dropdown menu/actions and update related tests. ## Review Focus Verify `Load Image` / `Load Video` Inputs tab behavior and confirm cloud/remote combo option values still update correctly. Relates to #9551 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9670-fix-show-load-widget-inputs-in-media-dropdown-31e6d73d36508148b845e18268a60c2a) by [Unito](https://www.unito.io) --------- Co-authored-by: bymyself <cbyrne@comfy.org> Co-authored-by: Amp <amp@ampcode.com>
…ogs (#9672) ## Summary Adds `isGraphReady` getter to `ComfyApp` and uses it in `executionErrorStore` guards to prevent false 'ComfyApp graph accessed before initialization' error logs during early store evaluation. ## Changes - **What**: Added `isGraphReady` boolean getter to `ComfyApp` that safely checks graph initialization without triggering the `rootGraph` getter's error log. Updated 5 guard sites in `executionErrorStore` to use `app.isGraphReady` instead of `app.rootGraph`. - **Why**: The `rootGraph` getter logs an error when accessed before initialization. Computed properties and watch callbacks in `executionErrorStore` are evaluated early (before graph init), causing false error noise in the console. ## Review Focus - `isGraphReady` is intentionally minimal — just `!!this.rootGraphInternal` — to avoid duplicating the error-logging behavior of `rootGraph` - The `watch(lastNodeErrors, ...)` callback now checks `isGraphReady` at the top and early-returns, consistent with the computed property pattern ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9672-fix-add-isGraphReady-guard-to-prevent-premature-graph-access-error-logs-31e6d73d365081be8e1fc77114ce9382) by [Unito](https://www.unito.io) Co-authored-by: Alexander Brown <drjkl@comfy.org>
As a temporary fix for widgets being incorrectly hidden, #9669 allowed all disabled widgets to be displayed. This PR provides a more robust implementation to derive whether the widget, as would be displayed from the root graph, is disabled. Potential regression: - Drag drop handlers are applied on node, not widgets. A subgraph containing a "Load Image" node, does not allow dragging and dropping an image onto the subgraphNode in order to load it. Because app mode widgets would display from the original owning node prior to this PR, these drag/drop handlers would apply. Placing "Load Image" nodes. I believe this change makes behavior more consistent, but it warrants consideration. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9671-Restore-hiding-of-linked-inputs-in-app-mode-31e6d73d365081688e37fbb931f3af68) by [Unito](https://www.unito.io)
## Summary Cloud build dispatch was only triggering on the `labeled` event, not on subsequent pushes to PRs that already had a preview label. ## Changes - **What**: Add `synchronize` to `pull_request` event types and update the `if` condition to support all three preview labels (`preview`, `preview-cpu`, `preview-gpu`). For `labeled` events, check the added label name; for `synchronize` events, check existing PR labels. ## Review Focus The `if` condition now branches on `github.event.action` to use the correct label-checking mechanism for each event type. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9636-fix-dispatch-cloud-build-on-synchronize-for-preview-labeled-PRs-31e6d73d3650814e9069e37d6199ffc9) by [Unito](https://www.unito.io)
Thumbnail downscaling is currently being used in more places than it should be. - Nodes which display images will display incorrect resolution indicators <img width="255" height="372" alt="image" src="https://github.com/user-attachments/assets/674790b6-04c8-4db0-84c2-2fa2dbaf123d" /> <img width="255" height="372" alt="image" src="https://github.com/user-attachments/assets/1dbe751b-7462-4408-9236-9446b005f5fc" /> This is particularly confusing with output nodes, which claim the output is not of the intended resolution - The "Download Image" and "Open Image" context menu actions will incorrectly download the downscaled thumbnail. - The assets panel will incorrectly display the thumbnail resolution as the resolution of the output - The lightbox (zoom) of an image will incorrectly display a downscaled thumbnail. This PR is a quick workaround to staunch the major problems - Nodes always display full previews. - Resolution downscaling is applied on the assert card, not on the assetItem itself - Due to implementation, this means that asset cards will still incorrectly show the resolution of the thumbnail instead of the size of the full image. --------- Co-authored-by: GitHub Action <action@github.com> Co-authored-by: Alexander Brown <drjkl@comfy.org>
- batch node drag updates through a single bounds transaction - coalesce layout-to-litegraph sync work with requestAnimationFrame - add node-scoped layout subscriptions and switch LGraphNode to onNodeChange - add regression tests for batched drag and sync coalescing Amp-Thread-ID: https://ampcode.com/threads/T-019cd561-5831-7688-af27-3701c92948f7 Co-authored-by: Amp <amp@ampcode.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdown.test.ts (1)
22-39: Consider adding v-model props toMockFormDropdownMenufor completeness.The mock component includes most props but omits the
defineModelbindings thatFormDropdownMenuexposes (e.g.,filterSelected,layoutMode,sortSelected,searchQuery,ownershipSelected,baseModelSelected). While the current tests don't exercise these, future tests might benefit from having a more complete mock.This is optional since current tests focus only on items reactivity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdown.test.ts` around lines 22 - 39, MockFormDropdownMenu currently defines props for items and several filters but omits the v-model bindings exposed by the real FormDropdownMenu; add props to MockFormDropdownMenu for filterSelected, layoutMode, sortSelected, searchQuery, ownershipSelected, and baseModelSelected (matching the real component's v-model names and types) so the mock can accept and reflect v-model interactions in future tests, then ensure these props are included in the props object of the MockFormDropdownMenu definition and used/returned as needed in setup to mirror the real component's API.src/renderer/core/layout/sync/useLayoutSync.ts (1)
76-76: Preferfor...ofto avoid lint warning about callback return value.Biome flags this because
Set.add()returns the Set itself, andforEachcallbacks shouldn't return values. While functionally correct, afor...ofloop is cleaner and avoids the lint warning.♻️ Suggested refactor
- change.nodeIds.forEach((nodeId) => pendingNodeIds.add(nodeId)) + for (const nodeId of change.nodeIds) { + pendingNodeIds.add(nodeId) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/core/layout/sync/useLayoutSync.ts` at line 76, Replace the forEach callback with a for...of loop to avoid returning values from the callback; in useLayoutSync.ts, locate the code that iterates change.nodeIds and calling pendingNodeIds.add(nodeId) and rewrite it to use: for (const nodeId of change.nodeIds) { pendingNodeIds.add(nodeId) } so the lint warning about callback return value is eliminated while keeping the same behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/cloud-dispatch-build.yaml:
- Around line 29-40: Update the PR guard expression in the if: condition to also
require that the PR head repo matches the base repo by adding a check for
github.event.pull_request.head.repo.full_name == github.repository so
fork-originated PRs are excluded (this prevents jobs from running on
'synchronize' and 'labeled' events for forks and avoids failing when
secrets.CLOUD_DISPATCH_TOKEN is unavailable). Ensure the new clause is combined
with the existing pull_request-specific branches (the labeled and synchronize
checks) so it only applies when github.event_name == 'pull_request'.
In `@docs/release-process.md`:
- Around line 25-29: The fenced code block containing the ASCII diagram (the
lines starting with "v1.40.1 ── A ── B ── C ── [bump to 1.41.0]" and the
following two lines) needs a language specifier to satisfy markdown linting;
update the opening fence from ``` to ```text so the block is ```text ... ``` and
retains the ASCII art unchanged.
In `@src/renderer/extensions/vueNodes/widgets/composables/useComboWidget.test.ts`:
- Around line 341-348: The local variable named "constructor" shadows the
restricted global name and causes lint failure; rename that binding returned
from useComboWidget() (e.g., to comboConstructor, createComboWidget, or ctor)
and update its usage where you call it with mockNode and inputSpec (the places
around useComboWidget(), mockNode, inputSpec, and widget) so the test no longer
uses the reserved identifier.
In `@src/renderer/extensions/vueNodes/widgets/composables/useComboWidget.ts`:
- Around line 51-66: The setter installed on widget.options.values currently
only accepts arrays and ignores function assignments, so update the
Object.defineProperty setter for 'values' to accept and store either an array or
a function: if the assigned value is an array set fallbackValues, if it's a
function set fallbackValues to that function (or store it in a new variable and
have get() call it via getValues); ensure getValues continues to resolve both
arrays and functions when returning values. Locate the Object.defineProperty for
options.values, the fallbackValues variable, and getValues to implement this
change so subsequent assignments like widget.options.values = fn are preserved.
In `@src/utils/litegraphUtil.ts`:
- Around line 323-346: resolveNodeWidget currently returns [] as soon as a node
is found but the named widget is missing; instead, when a node exists but widget
lookup fails you should still allow the later subgraph-promoted-widget search to
run and, if that also fails, return [node] as the fallback. Modify
resolveNodeWidget so that inside the "if (node) { ... }" block you only return
when you find the widget (return [node, widget]); if not found, do not return
immediately — let the subsequent loop that checks isPromotedWidgetView,
sourceWidgetName and sourceNodeId run; after that loop, if the original node
variable is present return [node], otherwise return [].
---
Nitpick comments:
In `@src/renderer/core/layout/sync/useLayoutSync.ts`:
- Line 76: Replace the forEach callback with a for...of loop to avoid returning
values from the callback; in useLayoutSync.ts, locate the code that iterates
change.nodeIds and calling pendingNodeIds.add(nodeId) and rewrite it to use: for
(const nodeId of change.nodeIds) { pendingNodeIds.add(nodeId) } so the lint
warning about callback return value is eliminated while keeping the same
behavior.
In
`@src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdown.test.ts`:
- Around line 22-39: MockFormDropdownMenu currently defines props for items and
several filters but omits the v-model bindings exposed by the real
FormDropdownMenu; add props to MockFormDropdownMenu for filterSelected,
layoutMode, sortSelected, searchQuery, ownershipSelected, and baseModelSelected
(matching the real component's v-model names and types) so the mock can accept
and reflect v-model interactions in future tests, then ensure these props are
included in the props object of the MockFormDropdownMenu definition and
used/returned as needed in setup to mirror the real component's API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7361deca-04c0-4e52-bc2b-f56ef06db714
📒 Files selected for processing (29)
.github/workflows/cloud-dispatch-build.yamldocs/release-process.mdsrc/components/builder/AppBuilder.vuesrc/composables/graph/useVueNodeLifecycle.tssrc/extensions/core/imageCompare.tssrc/platform/assets/components/MediaAssetCard.vuesrc/platform/assets/composables/media/assetMappers.tssrc/platform/assets/schemas/assetSchema.tssrc/platform/assets/utils/outputAssetUtil.tssrc/renderer/core/layout/operations/layoutMutations.tssrc/renderer/core/layout/store/layoutStore.test.tssrc/renderer/core/layout/store/layoutStore.tssrc/renderer/core/layout/sync/useLayoutSync.test.tssrc/renderer/core/layout/sync/useLayoutSync.tssrc/renderer/core/layout/types.tssrc/renderer/extensions/linearMode/LinearControls.vuesrc/renderer/extensions/vueNodes/components/LGraphNode.vuesrc/renderer/extensions/vueNodes/layout/useNodeDrag.test.tssrc/renderer/extensions/vueNodes/layout/useNodeDrag.tssrc/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdown.test.tssrc/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdown.vuesrc/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdownMenu.vuesrc/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdownMenuActions.vuesrc/renderer/extensions/vueNodes/widgets/composables/useComboWidget.test.tssrc/renderer/extensions/vueNodes/widgets/composables/useComboWidget.tssrc/scripts/app.tssrc/stores/executionErrorStore.tssrc/stores/nodeOutputStore.tssrc/utils/litegraphUtil.ts
💤 Files with no reviewable changes (3)
- src/stores/nodeOutputStore.ts
- src/extensions/core/imageCompare.ts
- src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdownMenuActions.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- src/scripts/app.ts
src/renderer/extensions/vueNodes/widgets/composables/useComboWidget.test.ts
Show resolved
Hide resolved
dosubot
bot
added
size:XXL
AustinMroz
previously approved these changes
Mar 12, 2026
Collaborator
There was a problem hiding this comment.
I'm not seeing any glaring issues from reading over the code.
As mentioned offline, I'm still able to trivially produce errors in node sizing by reloading while the browser tab is not in the foreground. This seems to be a browser specific issue as I can not reproduce in Firefox. Of note useDocumentVisibility reports that the document is visible during resizeObserver trigger.
I am specifically concerned that a 'code simplification PR' is adding ~6x the code it's removing.
In terms of a future ideal, is there ever a time that nodes should resize or move position without either
- Code that mutates the position of the node
- A user releasing a drag handle
Is having a resizeObserver at all actually needed?
|
|
||
| // ResizeObserver emits entries where nothing changed (e.g. initial observe). | ||
| // Skip expensive DOM reads when this exact element/node already measured at | ||
| // the same normalized bounds and size. |
Collaborator
There was a problem hiding this comment.
I've tried multiple browsers, but I'm unable to replicate a scenario where entries which have not actually undergone a resize are included.
Contributor
There was a problem hiding this comment.
note (non-blocking): ResizeObserver does fire on initial observe per the spec (the first callback always includes the observed element even if no resize occurred). The guard here is a perf optimization to skip redundant DOM reads in that case. Harmless if it never triggers in practice, but it does match the spec behavior.
Collaborator
There was a problem hiding this comment.
Needing to make changes to formdropdown tests as a result of node resize code seems really out of scope.
Contributor
There was a problem hiding this comment.
note (non-blocking): the mock simplification (defineComponent -> plain objects) was needed because the resize tracking changes affected how component mounting works in tests. The changes are test-only and add a missing coverage case. Agreed it is scope-adjacent - happy to split out if you feel strongly, but it should not block merge.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdown.vue (1)
143-154: KeepisOpentied to an actual popover action.With the new optional chaining,
isOpenstill flips even when the imperative call is missing, so the component can report “open” or “closed” while the overlay did nothing. Guard the state update behind the method check instead of silently no-oping.♻️ Suggested adjustment
const toggleDropdown = (event: Event) => { if (disabled) return - if (popoverRef.value && triggerRef.value) { - popoverRef.value.toggle?.(event, triggerRef.value) - isOpen.value = !isOpen.value - } + const popover = popoverRef.value + if (!triggerRef.value || !popover?.toggle) return + + popover.toggle(event, triggerRef.value) + isOpen.value = !isOpen.value } const closeDropdown = () => { - if (popoverRef.value) { - popoverRef.value.hide?.() - isOpen.value = false - } + const popover = popoverRef.value + if (!popover?.hide) return + + popover.hide() + isOpen.value = false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdown.vue` around lines 143 - 154, toggleDropdown and closeDropdown update isOpen even when the popover's imperative methods are missing due to optional chaining; change them to first check that popoverRef.value?.toggle (in toggleDropdown) or popoverRef.value?.hide (in closeDropdown) is a real function before calling it and only then update isOpen.value (and still respect disabled and triggerRef). In other words, for toggleDropdown verify popoverRef.value?.toggle exists, call popoverRef.value.toggle(event, triggerRef.value) and then flip isOpen.value; for closeDropdown verify popoverRef.value?.hide exists, call it and then set isOpen.value = false.src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdown.test.ts (1)
49-63: Derive the mount-helper props fromFormDropdowninstead of duplicating them.
MountDropdownOptionsis re-declaring part of the component API here, so a prop signature change can leave the helper stale while the test still compiles. UsingPartial<ComponentProps<typeof FormDropdown>>keeps this in lockstep with the component.♻️ Suggested adjustment
+import type { ComponentProps } from 'vue-component-type-helpers' import FormDropdown from './FormDropdown.vue' import type { FormDropdownItem } from './types' @@ -interface MountDropdownOptions { - searcher?: ( - query: string, - items: FormDropdownItem[], - onCleanup: (cleanupFn: () => void) => void - ) => Promise<FormDropdownItem[]> - searchQuery?: string -} +type MountDropdownOptions = Partial<ComponentProps<typeof FormDropdown>> @@ function mountDropdown( items: FormDropdownItem[], options: MountDropdownOptions = {} ) { return mount(FormDropdown, { - props: { items, ...options }, + props: { items, ...options } as ComponentProps<typeof FormDropdown>,Based on learnings, "In test files (e.g., any file ending with .test.ts or .test.tsx), for test helpers like mountComponent, type the props parameter as Partial<ComponentProps>."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdown.test.ts` around lines 49 - 63, MountDropdownOptions is duplicating FormDropdown's prop types and can go out of sync; replace the custom interface with a derived type using Partial<ComponentProps<typeof FormDropdown>> and update the mountDropdown signature to accept options: Partial<ComponentProps<typeof FormDropdown>> so the helper stays locked to FormDropdown's props (remove MountDropdownOptions and any manual prop declarations and adjust the mount call accordingly).src/renderer/core/layout/sync/useLayoutSync.ts (1)
30-35: Preserve string node IDs when resolving LiteGraph nodes.
LayoutChange.nodeIdsare strings, and LiteGraph'sNodeIdallowsnumber | string.Number.parseInt()is lossy ('foo'→NaN,'12a'→12), so any non-numeric ID will silently stop syncing here. Prefer the raw ID first and only fall back to numeric lookup when needed.💡 Suggested fix
- const liteNode = canvas.graph.getNodeById(Number.parseInt(nodeId, 10)) + const numericNodeId = Number(nodeId) + const liteNode = + canvas.graph.getNodeById(nodeId) ?? + (Number.isNaN(numericNodeId) + ? null + : canvas.graph.getNodeById(numericNodeId))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/core/layout/sync/useLayoutSync.ts` around lines 30 - 35, The loop is converting string node IDs with Number.parseInt which drops non-numeric IDs and breaks canvas.graph.getNodeById lookup; change the lookup in useLayoutSync so it first calls canvas.graph.getNodeById(nodeId) using the raw string from pendingNodeIds (or the value returned by layoutStore.getNodeLayoutRef(nodeId).value), and only if that returns falsy attempt a numeric lookup (e.g., parseInt(nodeId, 10)) as a fallback; update the code paths around pendingNodeIds, layoutStore.getNodeLayoutRef, and canvas.graph.getNodeById to preserve string IDs and only coerce to number when necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/TopMenuSection.test.ts`:
- Around line 569-594: The forEach callbacks in the test implicitly return a
value because they use concise arrow bodies like `(callback) => callback(0)`;
update these to use block arrow functions that call the callback without
returning (e.g., `(callback) => { callback(0); }`) for both the
initialCallbacks.forEach and callbacks.forEach invocations that use rafCallbacks
so the linter/static analysis warning is resolved.
In `@src/renderer/core/layout/sync/useLayoutSync.ts`:
- Around line 65-90: scheduleFlush currently enqueues microtasks via
queueMicrotask that cannot be canceled, so stale microtasks (from a previous
sync session) can call flushPendingChanges and operate on shared state
(pendingNodeIds) after stopSync()/startSync() restart sync; modify the sync
lifecycle to include a generation/epoch token (incremented in
startSync()/stopSync()) stored in a local variable when scheduling (in
scheduleFlush) and checked at the start of the microtask and the RAF callback
before calling flushPendingChanges, and also ensure rafId/isMicrotaskQueued are
reset when sync restarts so stale callbacks see a mismatched generation and bail
out; update references: scheduleFlush, rafId, isMicrotaskQueued, queueMicrotask,
requestAnimationFrame, flushPendingChanges, stopSync, startSync, and
pendingNodeIds.
---
Nitpick comments:
In `@src/renderer/core/layout/sync/useLayoutSync.ts`:
- Around line 30-35: The loop is converting string node IDs with Number.parseInt
which drops non-numeric IDs and breaks canvas.graph.getNodeById lookup; change
the lookup in useLayoutSync so it first calls canvas.graph.getNodeById(nodeId)
using the raw string from pendingNodeIds (or the value returned by
layoutStore.getNodeLayoutRef(nodeId).value), and only if that returns falsy
attempt a numeric lookup (e.g., parseInt(nodeId, 10)) as a fallback; update the
code paths around pendingNodeIds, layoutStore.getNodeLayoutRef, and
canvas.graph.getNodeById to preserve string IDs and only coerce to number when
necessary.
In
`@src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdown.test.ts`:
- Around line 49-63: MountDropdownOptions is duplicating FormDropdown's prop
types and can go out of sync; replace the custom interface with a derived type
using Partial<ComponentProps<typeof FormDropdown>> and update the mountDropdown
signature to accept options: Partial<ComponentProps<typeof FormDropdown>> so the
helper stays locked to FormDropdown's props (remove MountDropdownOptions and any
manual prop declarations and adjust the mount call accordingly).
In
`@src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdown.vue`:
- Around line 143-154: toggleDropdown and closeDropdown update isOpen even when
the popover's imperative methods are missing due to optional chaining; change
them to first check that popoverRef.value?.toggle (in toggleDropdown) or
popoverRef.value?.hide (in closeDropdown) is a real function before calling it
and only then update isOpen.value (and still respect disabled and triggerRef).
In other words, for toggleDropdown verify popoverRef.value?.toggle exists, call
popoverRef.value.toggle(event, triggerRef.value) and then flip isOpen.value; for
closeDropdown verify popoverRef.value?.hide exists, call it and then set
isOpen.value = false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 574e330a-da95-47ab-abf1-0b625a50ff77
📒 Files selected for processing (11)
src/components/TopMenuSection.test.tssrc/components/TopMenuSection.vuesrc/platform/workflow/validation/schemas/workflowSchema.tssrc/renderer/core/layout/store/layoutStore.test.tssrc/renderer/core/layout/store/layoutStore.tssrc/renderer/core/layout/sync/useLayoutSync.test.tssrc/renderer/core/layout/sync/useLayoutSync.tssrc/renderer/extensions/vueNodes/components/LGraphNode.vuesrc/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdown.test.tssrc/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdown.vuesrc/scripts/app.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/platform/workflow/validation/schemas/workflowSchema.ts
- src/scripts/app.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdown.vue (1)
143-156: Optional chaining is redundant but harmless.The
popoverRef.valueexistence is already checked on lines 145 and 152. PrimeVue's Popover should always exposetoggleandhidemethods, so the optional chaining is technically unnecessary. However, this defensive pattern is acceptable for robustness against edge cases or future API changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdown.vue` around lines 143 - 156, The optional chaining on popoverRef.value.toggle and popoverRef.value.hide is redundant because you already check popoverRef.value exists; update toggleDropdown and closeDropdown to call popoverRef.value.toggle(event, triggerRef.value) and popoverRef.value.hide() respectively (remove the ?.), leaving the existing checks for popoverRef.value and triggerRef/value checks and the isOpen state toggles intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdown.vue`:
- Around line 143-156: The optional chaining on popoverRef.value.toggle and
popoverRef.value.hide is redundant because you already check popoverRef.value
exists; update toggleDropdown and closeDropdown to call
popoverRef.value.toggle(event, triggerRef.value) and popoverRef.value.hide()
respectively (remove the ?.), leaving the existing checks for popoverRef.value
and triggerRef/value checks and the isOpen state toggles intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cbe7fa30-a849-4d8c-9fee-c5587eb3aa4f
📒 Files selected for processing (1)
src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdown.vue
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@browser_tests/tests/rendererToggleStability.spec.ts`:
- Around line 39-40: The test is reading graph state via
getAllNodePositions(comfyPage) without loading a deterministic workflow; load a
premade JSON workflow from assets before taking the baseline snapshot. Update
the test to load a known workflow (e.g., by calling a helper that uploads/parses
an assets/*.json file or by using comfyPage.evaluate to call
window.app.loadWorkflow / set window.app.graph) prior to calling
getAllNodePositions(comfyPage), so getAllNodePositions and comfyPage operate on
a stable graph state.
- Around line 52-53: The assertions using toBeCloseTo(initial.x, 1) and
toBeCloseTo(initial.y, 1) are using 1 decimal-place precision, not an absolute
±1 tolerance; update the checks (for variables current and initial in the
rendererToggleStability spec) to explicit absolute-tolerance assertions, e.g.
replace expect(current!.x).toBeCloseTo(initial.x, 1) and
expect(current!.y).toBeCloseTo(initial.y, 1) (and the similar pair later) with
assertions that use Math.abs(current!.x - initial.x) <= 1 and
Math.abs(current!.y - initial.y) <= 1 (or the test framework equivalent to
assert absolute difference <= 1) so the position drift check uses an absolute ±1
tolerance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e9ebad8a-db89-48c0-90b9-91a0106b347a
📒 Files selected for processing (1)
browser_tests/tests/rendererToggleStability.spec.ts
src/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.ts
Outdated
Show resolved
Hide resolved
- clear stale node-scoped listeners during layout reinit - guard null subgraph IO nodes during scale normalization - document graph render anchor cache lifetime - strengthen drag and renderer-toggle regression tests Amp-Thread-ID: https://ampcode.com/threads/T-019ce5d7-168f-736b-bc01-dd46b66b5d71 Co-authored-by: Amp <amp@ampcode.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/renderer/extensions/vueNodes/layout/useNodeDrag.test.ts (1)
9-15: Reuse the real layout type in this mock map.The inline
{ position; size }shape can drift fromNodeLayoutand let the suite compile against the wrong contract. Typing this asMap<string, Pick<NodeLayout, 'position' | 'size'>>keeps the mock coupled to the production API.💡 Suggested cleanup
import { beforeEach, describe, expect, it, vi } from 'vitest' import { ref } from 'vue' import type { Ref } from 'vue' +import type { NodeLayout } from '@/renderer/core/layout/types' const testState = vi.hoisted(() => { return { selectedNodeIds: null as unknown as Ref<Set<string>>, selectedItems: null as unknown as Ref<unknown[]>, - nodeLayouts: new Map< - string, - { - position: { x: number; y: number } - size: { width: number; height: number } - } - >(), + nodeLayouts: new Map<string, Pick<NodeLayout, 'position' | 'size'>>(),Based on learnings, in TypeScript test files, "avoid duplicating interface/type definitions. Import real type definitions from the component modules under test and reference them directly, so there is a single source of truth and to prevent type drift."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/vueNodes/layout/useNodeDrag.test.ts` around lines 9 - 15, The mock nodeLayouts Map is typed with an inline shape that can drift from the real API; import the production NodeLayout type (e.g., import { NodeLayout } from the module under test) and change the declaration of nodeLayouts to use Map<string, Pick<NodeLayout, 'position' | 'size'>> so the test's mock stays tied to the real NodeLayout contract (update the nodeLayouts variable/type accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/core/layout/sync/useLayoutSync.ts`:
- Line 112: The forEach callback returns the Set from
pendingNodeIds.add(nodeId), which trips lint rule
lint/suspicious/useIterableCallbackReturn; update the callback in useLayoutSync
(the change.nodeIds.forEach(...) call) to use a block-bodied function that
explicitly performs pendingNodeIds.add(nodeId) and does not return a value (or
replace the forEach with a simple for...of loop over change.nodeIds that calls
pendingNodeIds.add(nodeId)), ensuring the callback returns void so the linter no
longer flags it.
- Around line 31-63: Replace the numeric coercion when looking up LiteGraph
nodes: stop using Number.parseInt(nodeId, 10) in the loop and pass nodeId
directly to canvas.graph.getNodeById(...) so string UUIDs and numeric string IDs
work correctly; keep the rest of the logic that reads layout from
layoutStore.getNodeLayoutRef(nodeId).value and updates liteNode.pos/size and
calls liteNode.onResize. Also fix the Biome lint issue by ensuring you don't
return from a Set.forEach callback (replace any pendingNodeIds.forEach(...) that
returns pendingNodeIds.add(nodeId) with a loop or a direct
pendingNodeIds.add(nodeId) statement).
---
Nitpick comments:
In `@src/renderer/extensions/vueNodes/layout/useNodeDrag.test.ts`:
- Around line 9-15: The mock nodeLayouts Map is typed with an inline shape that
can drift from the real API; import the production NodeLayout type (e.g., import
{ NodeLayout } from the module under test) and change the declaration of
nodeLayouts to use Map<string, Pick<NodeLayout, 'position' | 'size'>> so the
test's mock stays tied to the real NodeLayout contract (update the nodeLayouts
variable/type accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 769ecfb2-4cec-4d90-bdaf-ad5733581ed9
📒 Files selected for processing (9)
browser_tests/tests/rendererToggleStability.spec.tssrc/renderer/core/layout/operations/layoutMutations.tssrc/renderer/core/layout/store/layoutStore.test.tssrc/renderer/core/layout/store/layoutStore.tssrc/renderer/core/layout/sync/useLayoutSync.tssrc/renderer/core/layout/transform/graphRenderTransform.tssrc/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.test.tssrc/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.tssrc/renderer/extensions/vueNodes/layout/useNodeDrag.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- browser_tests/tests/rendererToggleStability.spec.ts
- src/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.test.ts
Amp-Thread-ID: https://ampcode.com/threads/T-019ce607-2ea1-7609-9f3c-e1eab480d118 Co-authored-by: Amp <amp@ampcode.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/core/layout/sync/useLayoutSync.ts (1)
101-106:⚠️ Potential issue | 🟠 MajorMove teardown before the null-canvas guard in
startSync().If
startSync()is called with a null canvas during a canvas switch/teardown path, the current early return skipsstopSync()and can leave the previous subscription active against a stale canvas instance.💡 Proposed fix
function startSync(canvas: ReturnType<typeof useCanvasStore>['canvas']) { - if (!canvas?.graph) return - // Cancel last subscription stopSync() + if (!canvas?.graph) return // Subscribe to layout changes unsubscribe.value = layoutStore.onChange((change) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/core/layout/sync/useLayoutSync.ts` around lines 101 - 106, The null-canvas guard in startSync skips stopSync() and can leave an old subscription active; move the teardown call so stopSync() is invoked before the early return check. Specifically, in function startSync (and any related startup path that uses useCanvasStore()['canvas']), call stopSync() first to cancel the previous subscription, then check if (!canvas?.graph) return; and only after that proceed to subscribe to layout changes. This ensures previous subscriptions are cleaned up even when startSync is invoked with a null/stale canvas.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/renderer/core/layout/sync/useLayoutSync.ts`:
- Around line 101-106: The null-canvas guard in startSync skips stopSync() and
can leave an old subscription active; move the teardown call so stopSync() is
invoked before the early return check. Specifically, in function startSync (and
any related startup path that uses useCanvasStore()['canvas']), call stopSync()
first to cancel the previous subscription, then check if (!canvas?.graph)
return; and only after that proceed to subscribe to layout changes. This ensures
previous subscriptions are cleaned up even when startSync is invoked with a
null/stale canvas.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 268c7a7c-7da1-451a-8ce1-05ee1778b02e
📒 Files selected for processing (2)
src/renderer/core/layout/sync/useLayoutSync.tssrc/renderer/extensions/vueNodes/layout/useNodeDrag.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/extensions/vueNodes/layout/useNodeDrag.test.ts
Amp-Thread-ID: https://ampcode.com/threads/T-019ce623-c768-76a8-8a9e-5a1aafb0be9f Co-authored-by: Amp <amp@ampcode.com>
christian-byrne
approved these changes
Mar 13, 2026
Contributor
christian-byrne
left a comment
christian-byrne
left a comment
There was a problem hiding this comment.
All blocking feedback addressed and verified in code. Two non-blocking threads from AustinMroz remain open (ResizeObserver guard justification, FormDropdown test scope) - replied with rationale, neither blocks merge.
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
6 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
Fix node layout drift from repeated
ensureCorrectLayoutScalescaling, simplify it to a pure one-time normalizer, and fix links not following Vue nodes during drag.Changes
ensureCorrectLayoutScalesimplified to a one-time normalizer: unprojects legacy Vue-scaled coordinates back to canonical LiteGraph coordinates, marks the graph as corrected, and does nothing else. No longer touches the layout store, syncs reroutes, or changes canvas scale.useVueNodeLifecycle.ts(a renderer version string was passed where anLGraphwas expected).layoutStore.finalizeOperationnow callsnotifyChangesynchronously instead of viasetTimeout. This ensuresuseLayoutSync'sonChangecallback pushes positions to LiteGraphnode.posand callscanvas.setDirty()within the same RAF frame as a drag update, fixing links not following Vue nodes during drag.ensureCorrectLayoutScale(idempotency, round-trip, unknown-renderer no-op) andgraphRenderTransform(project/unproject round-trips, anchor caching).Review Focus
setTimeout(() => this.notifyChange(change), 0)→this.notifyChange(change)change inlayoutStore.tsis the key fix for the drag-link-sync bug. The listener (useLayoutSync) only writes to LiteGraph, not back to the layout store, so synchronous notification is safe.ensureCorrectLayoutScaleno longer has any side effects beyond normalizing coordinates and settingworkflowRendererVersionmetadata.