fix: track nodePreviewImages in usePromotedPreviews for GLSL live preview propagation#9461
Open
christian-byrne wants to merge 4 commits intomainfrom
Open
fix: track nodePreviewImages in usePromotedPreviews for GLSL live preview propagation#9461christian-byrne wants to merge 4 commits intomainfrom
christian-byrne wants to merge 4 commits intomainfrom
Conversation
dosubot
bot
added
the
size:M
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 (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a reactive Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Comment |
🎨 Storybook: ✅ Built — View Storybook |
🎭 Playwright: ✅ 553 passed, 0 failed · 4 flaky📊 Browser Reports
|
📦 Bundle: 4.58 MB gzip 🟢 -94 BDetailsSummary
Category Glance App Entry Points — 17.7 kB (baseline 17.7 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.01 MB (baseline 1.01 MB) • 🔴 +147 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 72.5 kB (baseline 72.5 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 438 kB (baseline 438 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.79 MB (baseline 2.79 MB) • 🔴 +132 BStores, services, APIs, and repositories
Status: 15 added / 15 removed Utilities & Hooks — 57.2 kB (baseline 57.2 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.04 MB (baseline 8.04 MB) • ⚪ 0 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.
🧹 Nitpick comments (1)
src/composables/node/usePromotedPreviews.test.ts (1)
217-240: Add one reactive-update test to lock in the original regression path.This new case is good for preview-only data, but it seeds data before first
evaluation. Consider also asserting recomputation whennodePreviewImagesis
written after initial evaluation.♻️ Suggested test addition
+ it('recomputes when preview images are populated after first evaluation', () => { + const setup = createSetup() + addInteriorNode(setup, { id: 10, previewMediaType: 'image' }) + usePromotionStore().promote( + setup.subgraphNode.rootGraph.id, + setup.subgraphNode.id, + '10', + '$$canvas-image-preview' + ) + + const { promotedPreviews } = usePromotedPreviews(() => setup.subgraphNode) + expect(promotedPreviews.value).toEqual([]) + + const blobUrl = 'blob:http://localhost/glsl-preview' + seedPreviewImages(setup.subgraph.id, [{ nodeId: 10, urls: [blobUrl] }]) + getNodeImageUrls.mockReturnValue([blobUrl]) + + expect(promotedPreviews.value).toEqual([ + { + interiorNodeId: '10', + widgetName: '$$canvas-image-preview', + type: 'image', + urls: [blobUrl] + } + ]) + })As per coding guidelines,
src/**/*.test.tsshould prioritize behavioral
coverage for critical/new features.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/composables/node/usePromotedPreviews.test.ts` around lines 217 - 240, The test covers the preview-only path but misses asserting reactive recomputation when nodePreviewImages are added after first evaluation; add a new test that uses createSetup(), addInteriorNode(... id:10 ...), promote via usePromotionStore().promote(..., '10', '$$canvas-image-preview'), then call usePromotedPreviews(() => setup.subgraphNode) and first assert promotedPreviews.value is empty, then write the preview via seedPreviewImages(setup.subgraph.id, [{ nodeId: 10, urls: [blobUrl] }]) (and ensure getNodeImageUrls.mockReturnValue([blobUrl]) is set), await a tick (or force reactivity), and finally assert promotedPreviews.value equals the expected image entry; reference usePromotedPreviews, seedPreviewImages, getNodeImageUrls, usePromotionStore().promote, createSetup, and addInteriorNode to locate code.
🤖 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/composables/node/usePromotedPreviews.test.ts`:
- Around line 217-240: The test covers the preview-only path but misses
asserting reactive recomputation when nodePreviewImages are added after first
evaluation; add a new test that uses createSetup(), addInteriorNode(... id:10
...), promote via usePromotionStore().promote(..., '10',
'$$canvas-image-preview'), then call usePromotedPreviews(() =>
setup.subgraphNode) and first assert promotedPreviews.value is empty, then write
the preview via seedPreviewImages(setup.subgraph.id, [{ nodeId: 10, urls:
[blobUrl] }]) (and ensure getNodeImageUrls.mockReturnValue([blobUrl]) is set),
await a tick (or force reactivity), and finally assert promotedPreviews.value
equals the expected image entry; reference usePromotedPreviews,
seedPreviewImages, getNodeImageUrls, usePromotionStore().promote, createSetup,
and addInteriorNode to locate code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3584d2dd-8f2e-4c5e-af7a-93ab9babad4f
📒 Files selected for processing (2)
src/composables/node/usePromotedPreviews.test.tssrc/composables/node/usePromotedPreviews.ts
⚡ Performance Report
Raw data{
"timestamp": "2026-03-12T10:49:11.330Z",
"gitSha": "c6fa2eec155fdd403d58ad050a9a0b600eee18fa",
"branch": "fix/glsl-promoted-preview-propagation",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 1994.9770000000058,
"styleRecalcs": 10,
"styleRecalcDurationMs": 8.866999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 347.846,
"heapDeltaBytes": 1164380
},
{
"name": "canvas-idle",
"durationMs": 2012.7949999999828,
"styleRecalcs": 11,
"styleRecalcDurationMs": 9.447000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 361.445,
"heapDeltaBytes": 1110452
},
{
"name": "canvas-idle",
"durationMs": 2045.829000000026,
"styleRecalcs": 11,
"styleRecalcDurationMs": 9.969,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 365.864,
"heapDeltaBytes": 428992
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1885.1600000000417,
"styleRecalcs": 75,
"styleRecalcDurationMs": 36.846,
"layouts": 12,
"layoutDurationMs": 3.8739999999999997,
"taskDurationMs": 835.726,
"heapDeltaBytes": 1257576
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1845.0209999999743,
"styleRecalcs": 78,
"styleRecalcDurationMs": 40.803,
"layouts": 13,
"layoutDurationMs": 3.398,
"taskDurationMs": 778.605,
"heapDeltaBytes": 1784200
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1848.7230000000068,
"styleRecalcs": 77,
"styleRecalcDurationMs": 42.948,
"layouts": 12,
"layoutDurationMs": 3.928,
"taskDurationMs": 787.4250000000001,
"heapDeltaBytes": 187980
},
{
"name": "dom-widget-clipping",
"durationMs": 615.429000000006,
"styleRecalcs": 13,
"styleRecalcDurationMs": 9.528,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 372.85999999999996,
"heapDeltaBytes": 6787708
},
{
"name": "dom-widget-clipping",
"durationMs": 554.3180000000234,
"styleRecalcs": 13,
"styleRecalcDurationMs": 8.833,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 357.26099999999997,
"heapDeltaBytes": 11810324
},
{
"name": "dom-widget-clipping",
"durationMs": 551.7189999999914,
"styleRecalcs": 13,
"styleRecalcDurationMs": 8.517999999999997,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 343.151,
"heapDeltaBytes": 12594096
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 577.9919999999947,
"styleRecalcs": 49,
"styleRecalcDurationMs": 12.200000000000003,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 395.095,
"heapDeltaBytes": -4541684
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 582.5760000000173,
"styleRecalcs": 48,
"styleRecalcDurationMs": 11.642999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 387.953,
"heapDeltaBytes": -4269540
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 626.1739999999918,
"styleRecalcs": 49,
"styleRecalcDurationMs": 12.884999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 424.35100000000006,
"heapDeltaBytes": -4529808
},
{
"name": "subgraph-idle",
"durationMs": 2003.3440000000269,
"styleRecalcs": 12,
"styleRecalcDurationMs": 9.369,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 335.789,
"heapDeltaBytes": 612896
},
{
"name": "subgraph-idle",
"durationMs": 2011.0609999999838,
"styleRecalcs": 13,
"styleRecalcDurationMs": 14.270000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 353.074,
"heapDeltaBytes": 259232
},
{
"name": "subgraph-idle",
"durationMs": 2000.0519999999824,
"styleRecalcs": 11,
"styleRecalcDurationMs": 9.964999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 405.62299999999993,
"heapDeltaBytes": 551996
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1718.629999999962,
"styleRecalcs": 76,
"styleRecalcDurationMs": 36.943,
"layouts": 16,
"layoutDurationMs": 5.032,
"taskDurationMs": 691.5450000000001,
"heapDeltaBytes": -553876
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1748.106000000007,
"styleRecalcs": 80,
"styleRecalcDurationMs": 43.120000000000005,
"layouts": 17,
"layoutDurationMs": 4.8950000000000005,
"taskDurationMs": 710.8879999999999,
"heapDeltaBytes": -158108
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1725.195000000042,
"styleRecalcs": 78,
"styleRecalcDurationMs": 38.981,
"layouts": 16,
"layoutDurationMs": 4.468,
"taskDurationMs": 697.088,
"heapDeltaBytes": -1216132
}
]
} |
AustinMroz
previously approved these changes
Mar 6, 2026
dosubot
bot
added
size:L
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/composables/node/usePromotedPreviews.test.ts (1)
17-40:⚠️ Potential issue | 🟡 MinorReset the hoisted
getNodeImageUrlsmock between tests to prevent test order dependency.
vi.clearAllMocks()clears call history but does not resetmockReturnValue/mockImplementation, causing per-test stubs to persist across tests and create hidden test order dependencies.🧪 Suggested change
beforeEach(() => { setActivePinia(createTestingPinia({ stubActions: false })) vi.clearAllMocks() + getNodeImageUrls.mockReset() nodeOutputStore = createMockNodeOutputStore() useNodeOutputStoreMock.mockReturnValue(nodeOutputStore) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/composables/node/usePromotedPreviews.test.ts` around lines 17 - 40, The hoisted mocks created with vi.hoisted (getNodeImageUrls and useNodeOutputStoreMock) keep their mock implementations between tests, causing test-order dependency; add a per-test reset (e.g., in a beforeEach or afterEach) that calls mockReset()/mockClear() on getNodeImageUrls and useNodeOutputStoreMock to wipe any mockReturnValue/mockImplementation set by a previous test so createMockNodeOutputStore returns clean stubs each test and tests remain isolated.
🤖 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/subgraphPromotion.spec.ts`:
- Around line 557-597: The test "Promoted preview renders when outputs are
injected for interior node" currently simulates backend results by dispatching
an 'executed' event and seeding output.images, which bypasses the preview-image
propagation path; change the test to drive the preview path instead by writing
into the app's preview store used by GLSL/live previews (e.g.
window.app!.nodePreviewImages or the API used to set preview images) for the
interior node locator "5:10" (using the same NodeLocatorId conversion the app
uses), or dispatch the specific preview update event the app listens for, so the
reactive preview propagation code runs and the promoted preview in the
SubgraphNode (node locator '5') becomes visible; keep the final assertion on
subgraphVueNode.locator('.image-preview img') as-is.
---
Outside diff comments:
In `@src/composables/node/usePromotedPreviews.test.ts`:
- Around line 17-40: The hoisted mocks created with vi.hoisted (getNodeImageUrls
and useNodeOutputStoreMock) keep their mock implementations between tests,
causing test-order dependency; add a per-test reset (e.g., in a beforeEach or
afterEach) that calls mockReset()/mockClear() on getNodeImageUrls and
useNodeOutputStoreMock to wipe any mockReturnValue/mockImplementation set by a
previous test so createMockNodeOutputStore returns clean stubs each test and
tests remain isolated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 067f0319-2465-47c6-af3a-776f18d26161
📒 Files selected for processing (3)
browser_tests/tests/subgraphPromotion.spec.tssrc/composables/node/usePromotedPreviews.test.tssrc/stores/promotionStore.ts
Comment on lines
+557
to
+597
| test('Promoted preview renders when outputs are injected for interior node', async ({ | ||
| comfyPage | ||
| }) => { | ||
| await comfyPage.workflow.loadWorkflow( | ||
| 'subgraphs/subgraph-with-preview-node' | ||
| ) | ||
| await comfyPage.vueNodes.waitForNodes() | ||
|
|
||
| const subgraphVueNode = comfyPage.vueNodes.getNodeLocator('5') | ||
| await expect(subgraphVueNode).toBeVisible() | ||
|
|
||
| // Simulate backend sending execution results for interior SaveImage | ||
| // node (id 10) inside the SubgraphNode (id 5). | ||
| // The executed event handler converts execution ID "5:10" to a | ||
| // NodeLocatorId using the subgraph UUID, then sets both | ||
| // app.nodeOutputs and the reactive nodeOutputs ref. | ||
| await comfyPage.page.evaluate(() => { | ||
| const api = window.app!.api | ||
| api.dispatchEvent( | ||
| new CustomEvent('executed', { | ||
| detail: { | ||
| node: '5:10', | ||
| display_node: '5:10', | ||
| output: { | ||
| images: [ | ||
| { | ||
| filename: 'example.png', | ||
| subfolder: '', | ||
| type: 'input' | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| }) | ||
| ) | ||
| }) | ||
|
|
||
| // The promoted preview should render an image inside the SubgraphNode | ||
| const previewImage = subgraphVueNode.locator('.image-preview img') | ||
| await expect(previewImage).toBeVisible({ timeout: 5_000 }) | ||
| }) |
Contributor
There was a problem hiding this comment.
This E2E misses the regression path.
The bug here is promoted previews updating from nodePreviewImages/GLSL live previews, but this test dispatches executed and seeds output.images. That means it can stay green even if preview-image propagation is still broken. Please drive the preview-image path here instead of the executed-output path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@browser_tests/tests/subgraphPromotion.spec.ts` around lines 557 - 597, The
test "Promoted preview renders when outputs are injected for interior node"
currently simulates backend results by dispatching an 'executed' event and
seeding output.images, which bypasses the preview-image propagation path; change
the test to drive the preview path instead by writing into the app's preview
store used by GLSL/live previews (e.g. window.app!.nodePreviewImages or the API
used to set preview images) for the interior node locator "5:10" (using the same
NodeLocatorId conversion the app uses), or dispatch the specific preview update
event the app listens for, so the reactive preview propagation code runs and the
promoted preview in the SubgraphNode (node locator '5') becomes visible; keep
the final assertion on subgraphVueNode.locator('.image-preview img') as-is.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
browser_tests/tests/subgraphPromotion.spec.ts (1)
568-589:⚠️ Potential issue | 🟠 MajorThis still validates the fallback path, not the regression fixed by this PR.
Dispatching
executedwithoutput.imagesonly proves promoted previews render fromnodeOutputs. The bug here was broken reactivity fromnodePreviewImages/GLSL live previews, so this test can stay green even if that path is still broken. Please seed the preview-image update path for the interior node instead and keep the final DOM assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@browser_tests/tests/subgraphPromotion.spec.ts` around lines 568 - 589, The test currently only dispatches an 'executed' event which updates app.nodeOutputs (validating the fallback) but not the GLSL/live-preview reactivity; instead seed the preview-image update path for the interior node (locator "5:10") by either dispatching the same custom event your app listens for to update nodePreviewImages/preview-image state or directly mutating the reactive ref app.nodePreviewImages['5:10'] (and any associated preview metadata the live-preview logic expects) before asserting the DOM; keep the final DOM assertion as-is but ensure you target the nodePreviewImages/reactive preview update path (reference symbols: nodePreviewImages, nodeOutputs, locator "5:10", and the 'executed'/'preview-image' custom event your app handles).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@browser_tests/tests/subgraphPromotion.spec.ts`:
- Around line 568-589: The test currently only dispatches an 'executed' event
which updates app.nodeOutputs (validating the fallback) but not the
GLSL/live-preview reactivity; instead seed the preview-image update path for the
interior node (locator "5:10") by either dispatching the same custom event your
app listens for to update nodePreviewImages/preview-image state or directly
mutating the reactive ref app.nodePreviewImages['5:10'] (and any associated
preview metadata the live-preview logic expects) before asserting the DOM; keep
the final DOM assertion as-is but ensure you target the
nodePreviewImages/reactive preview update path (reference symbols:
nodePreviewImages, nodeOutputs, locator "5:10", and the
'executed'/'preview-image' custom event your app handles).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 94ab191f-4c79-4eb6-bef8-0b437924e907
📒 Files selected for processing (1)
browser_tests/tests/subgraphPromotion.spec.ts
christian-byrne
added
the
preview-gpu
Add version counter to promotionStore that guarantees computed properties re-evaluate when promotions change. The nested Map<UUID, Map<NodeId, ...>> structure can miss reactive notifications when inner Maps are lazily initialized and returned as raw references. - Add _version ref incremented on setPromotions/movePromotion/clearGraph - Read _version in getPromotionsRef to establish reactive dependency - Re-read inner Maps through reactive proxy after lazy init - Add unit test for late-promotion reactivity scenario - Add E2E test for promoted preview rendering via synthetic execution
christian-byrne
force-pushed
the
fix/glsl-promoted-preview-propagation
branch
from
c8f09b9 to
549dd5d
Compare
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
Fix GLSL shader live preview propagation to SubgraphNode parents, broken after the promoted widget rewrite in #8856.
Changes
nodePreviewImagesas a reactive dependency inusePromotedPreviews's computed property. The GLSL live preview renderer writes tonodePreviewImages(viasetNodePreviewsByNodeId), notnodeOutputs, so the computed never re-evaluated. The gate now passes if either store has data.getNodeImageUrlsalready handles the fallback correctly (checks previews first, then outputs).Review Focus
The fix is a one-line gate change (
||instead of only checkingnodeOutputs). Both reactive refs are read to establish Vue dependency tracking, matching the existing pattern.Screenshots (if applicable)
N/A — preview rendering is identical to pre-#8856 behavior; this restores broken reactivity.
┆Issue is synchronized with this Notion page by Unito