Fix mask editor sometimes showing wrong image#12413
Conversation
📝 WalkthroughWalkthroughThis PR syncs persisted node image outputs into legacy litegraph nodes by assigning ChangesStale Preview Prevention
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎨 Storybook: ✅ Built — View Storybook |
🎭 Playwright: ✅ 1638 passed, 0 failed · 2 flaky📊 Browser Reports
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/maskEditor.spec.ts`:
- Around line 312-320: The test currently hardcodes node id '1' in the
dispatched event and the getNodeOutput lookup; instead capture the created
node's id (e.g., assign the returned node's id to a variable like createdNodeId)
and use that variable when building the detail object passed to
app!.api.dispatchCustomEvent('executed', ...) and when querying the page.
Replace graph!.getNodeById('1') with a dynamic lookup that uses createdNodeId;
if calling page.evaluate, pass createdNodeId as an argument into the evaluate
callback to avoid relying on closure scope.
- Line 313: Remove the stray debug console.error call
(console.error(JSON.stringify(detail))) from the test; locate it in
maskEditor.spec.ts (the test path containing the JSON.stringify(detail) call)
and either delete the line or replace it with a non-error logging mechanism
(e.g., console.debug or a test-only logger) so test success runs do not emit
error-level console output that can break CI.
🪄 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: CHILL
Plan: Pro
Run ID: c02c5c72-edfa-449d-bbf1-1bfb8f1c1aba
📒 Files selected for processing (2)
browser_tests/tests/maskEditor.spec.tssrc/stores/nodeOutputStore.ts
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #12413 +/- ##
===========================================
- Coverage 74.35% 60.12% -14.23%
===========================================
Files 1522 1415 -107
Lines 85827 72453 -13374
Branches 23516 20130 -3386
===========================================
- Hits 63814 43562 -20252
- Misses 21198 28418 +7220
+ Partials 815 473 -342
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1027 files with indirect coverage changes 🚀 New features to boost your workflow:
|
dosubot
Bot
added
the
size:XS
| ) | ||
| }) | ||
|
|
||
| test('Will not use stale litegraph previews', async ({ comfyPage }) => { |
There was a problem hiding this comment.
This test isnt inside a describe and doesn't actually test any mask editor functionality so probably better elsewhere or open the mask editor at the end to validate that it is the correct image
There was a problem hiding this comment.
Comment got eaten 😭
I agree. There's not a (non screenshot/route mock) way to test the image loaded by the mask editor, but the prior structure of the test actually worked out nicely here.
Tragically, the existing mask editor fixture code isn't helpful since it forcibly loads a different workflow.
| await comfyPage.menu.topbar.newWorkflowButton.click() | ||
| await comfyPage.searchBoxV2.addNode('Preview Image') | ||
|
|
||
| async function simulateExecuted(image: object) { |
There was a problem hiding this comment.
maybe worth adding this to ExecutionHelper?
There was a problem hiding this comment.
Looks like ExecutionHelper already had a function to do this and my grep fu had failed me. The fixture code feels a good bit clunkier to use here, but tests an even broader scope which is appreciated.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
browser_tests/tests/maskEditor.spec.ts (1)
316-330:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the created node id instead of hardcoded
'1'.Line 318, Line 322, and Line 329 hardcode the node id, which makes this regression test brittle if id allocation changes. Capture the created Preview Image node id and reuse it for both
executed(...)andgetNodeById(...).🤖 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/maskEditor.spec.ts` around lines 316 - 330, The test hardcodes node id '1' in getNodeOutput (which calls graph!.getNodeById), executionHelper.executed, and the second executed call; instead capture the id of the created Preview Image node when you create it (store it in a variable like previewNodeId) and reuse that variable in getNodeOutput, executionHelper.executed, and any other place that referenced '1' so the test is resilient to id allocation changes.
🤖 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.
Duplicate comments:
In `@browser_tests/tests/maskEditor.spec.ts`:
- Around line 316-330: The test hardcodes node id '1' in getNodeOutput (which
calls graph!.getNodeById), executionHelper.executed, and the second executed
call; instead capture the id of the created Preview Image node when you create
it (store it in a variable like previewNodeId) and reuse that variable in
getNodeOutput, executionHelper.executed, and any other place that referenced '1'
so the test is resilient to id allocation changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dd1dfcb4-1042-4f5d-a1b8-13df57f7276f
📒 Files selected for processing (1)
browser_tests/tests/maskEditor.spec.ts
2 participants
Mask editor checks
node.imagesto determine the image which is edited. If the user generates an output image in litegraph mode, swaps to vue mode, then generates a new image, the mask editor will incorrectly display the image last shown in litegraph mode.This is resolved by having
syncLegacyNodeImgsalso synchronize node outputs tonode.images.