Always wait for next tick before layout init#7591
Conversation
📝 WalkthroughWalkthroughAdded guards and timing changes for node layout initialization: abort if a node is removed mid-sequence, re-read node data after workflow/configure completes before initializing layout, and defer layout init with requestAnimationFrame on the normal path. Public API unchanged. Changes
Sequence Diagram(s)(omitted — changes do not introduce a new multi-component control flow requiring visualization) Suggested reviewers
✨ Finishing touches
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 |
🎭 Playwright Tests:
|
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 01/21/2026, 02:07:39 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 22.4 kB (baseline 22.4 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.02 MB (baseline 1.02 MB) • 🔴 +55 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 80.7 kB (baseline 80.7 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Panels & Settings — 430 kB (baseline 430 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
User & Accounts — 3.94 kB (baseline 3.94 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Editors & Dialogs — 2.8 kB (baseline 2.8 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
UI Components — 32.8 kB (baseline 32.8 kB) • ⚪ 0 BReusable component library chunks
Data & Services — 3.04 MB (baseline 3.04 MB) • ⚪ 0 BStores, services, APIs, and repositories
Utilities & Hooks — 18.8 kB (baseline 18.8 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Vendor & Third-Party — 10.4 MB (baseline 10.4 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 6.25 MB (baseline 6.25 MB) • ⚪ 0 BBundles that do not match a named category
|
6b36d8d to
ca95d4c
Compare
Fixes a bug where swapping to a different workflow from the inside of a subgraph would cause nodes to be in an incorrect position after swapping back. in vue mode Prior to an unknown-but-recent PR, all nodes would would stack on the origin. This PR instead solves the remaining issue where having `ComfyEnableWorkflowViewRestore` would cause incorrect node positions. This is done by not delaying the fitView by a frame (which causes it to occur after the graph is no longer in the configuring state). In order to accomplish this, the code in LGraphNode has been updated to allow measuring node bounds without requiring a ctx argument. This arg is only used to ensure sufficient width for a node's title and is irrelevant when loading an existing graph. | Before | After | | ------ | ----- | | <img width="360" alt="before" src="https://github.com/user-attachments/assets/7f73817b-36e9-4400-8342-9e660cb36628"/> | <img width="360" alt="after" src="https://github.com/user-attachments/assets/c7ab4b99-2797-4276-9703-58d489cc3eaf" />| See also #7591, which solves similar issues, but does not resolve this bug. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7645-Do-not-delay-fit-to-view-on-graph-restore-2ce6d73d36508153972cc7b5948ce375) by [Unito](https://www.unito.io)
Fixes a bug where swapping to a different workflow from the inside of a subgraph would cause nodes to be in an incorrect position after swapping back. in vue mode Prior to an unknown-but-recent PR, all nodes would would stack on the origin. This PR instead solves the remaining issue where having `ComfyEnableWorkflowViewRestore` would cause incorrect node positions. This is done by not delaying the fitView by a frame (which causes it to occur after the graph is no longer in the configuring state). In order to accomplish this, the code in LGraphNode has been updated to allow measuring node bounds without requiring a ctx argument. This arg is only used to ensure sufficient width for a node's title and is irrelevant when loading an existing graph. | Before | After | | ------ | ----- | | <img width="360" alt="before" src="https://github.com/user-attachments/assets/7f73817b-36e9-4400-8342-9e660cb36628"/> | <img width="360" alt="after" src="https://github.com/user-attachments/assets/c7ab4b99-2797-4276-9703-58d489cc3eaf" />| See also #7591, which solves similar issues, but does not resolve this bug. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7645-Do-not-delay-fit-to-view-on-graph-restore-2ce6d73d36508153972cc7b5948ce375) by [Unito](https://www.unito.io)
Fixes a bug where swapping to a different workflow from the inside of a subgraph would cause nodes to be in an incorrect position after swapping back. in vue mode Prior to an unknown-but-recent PR, all nodes would would stack on the origin. This PR instead solves the remaining issue where having `ComfyEnableWorkflowViewRestore` would cause incorrect node positions. This is done by not delaying the fitView by a frame (which causes it to occur after the graph is no longer in the configuring state). In order to accomplish this, the code in LGraphNode has been updated to allow measuring node bounds without requiring a ctx argument. This arg is only used to ensure sufficient width for a node's title and is irrelevant when loading an existing graph. | Before | After | | ------ | ----- | | <img width="360" alt="before" src="https://github.com/user-attachments/assets/7f73817b-36e9-4400-8342-9e660cb36628"/> | <img width="360" alt="after" src="https://github.com/user-attachments/assets/c7ab4b99-2797-4276-9703-58d489cc3eaf" />| See also #7591, which solves similar issues, but does not resolve this bug. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7645-Do-not-delay-fit-to-view-on-graph-restore-2ce6d73d36508153972cc7b5948ce375) by [Unito](https://www.unito.io)
Fixes a bug where swapping to a different workflow from the inside of a subgraph would cause nodes to be in an incorrect position after swapping back. in vue mode Prior to an unknown-but-recent PR, all nodes would would stack on the origin. This PR instead solves the remaining issue where having `ComfyEnableWorkflowViewRestore` would cause incorrect node positions. This is done by not delaying the fitView by a frame (which causes it to occur after the graph is no longer in the configuring state). In order to accomplish this, the code in LGraphNode has been updated to allow measuring node bounds without requiring a ctx argument. This arg is only used to ensure sufficient width for a node's title and is irrelevant when loading an existing graph. | Before | After | | ------ | ----- | | <img width="360" alt="before" src="https://github.com/user-attachments/assets/7f73817b-36e9-4400-8342-9e660cb36628"/> | <img width="360" alt="after" src="https://github.com/user-attachments/assets/c7ab4b99-2797-4276-9703-58d489cc3eaf" />| See also #7591, which solves similar issues, but does not resolve this bug. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7645-Do-not-delay-fit-to-view-on-graph-restore-2ce6d73d36508153972cc7b5948ce375) by [Unito](https://www.unito.io)
|
Updating Playwright Expectations |
f5e1ec0 to
764b780
Compare
764b780 to
0fceb13
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (7)
browser_tests/tests/vueNodes/groups/groups.spec.ts-snapshots/vue-groups-create-group-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/vueNodes/interactions/canvas/zoom.spec.ts-snapshots/zoomed-in-ctrl-shift-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/vueNodes/interactions/node/move.spec.ts-snapshots/vue-node-moved-node-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/vueNodes/nodeStates/bypass.spec.ts-snapshots/vue-node-bypassed-state-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/vueNodes/nodeStates/colors.spec.ts-snapshots/vue-node-custom-color-blue-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/vueNodes/nodeStates/mute.spec.ts-snapshots/vue-node-muted-state-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/widget.spec.ts-snapshots/image-preview-changed-by-combo-value-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (1)
src/composables/graph/useGraphNodeManager.ts
🧰 Additional context used
📓 Path-based instructions (10)
src/**/*.{vue,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{vue,ts}: Leverage VueUse functions for performance-enhancing styles
Implement proper error handling
Use vue-i18n in composition API for any string literals. Place new translation entries in src/locales/en/main.json
Files:
src/composables/graph/useGraphNodeManager.ts
src/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.ts: Use es-toolkit for utility functions
Use TypeScript for type safety
src/**/*.ts: Derive component types usingvue-component-type-helpers(ComponentProps,ComponentSlots) instead of separate type files
Use es-toolkit for utility functions
Minimize the surface area (exported values) of each module and composable
Favor pure functions, especially testable ones
Files:
src/composables/graph/useGraphNodeManager.ts
src/**/{services,composables}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/{services,composables}/**/*.{ts,tsx}: Useapi.apiURL()for backend endpoints instead of constructing URLs directly
Useapi.fileURL()for static file access instead of constructing URLs directly
Files:
src/composables/graph/useGraphNodeManager.ts
src/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.{ts,tsx,vue}: Sanitize HTML with DOMPurify to prevent XSS attacks
Avoid using @ts-expect-error; use proper TypeScript types instead
Use es-toolkit for utility functions instead of other utility libraries
Implement proper TypeScript types throughout the codebase
src/**/*.{ts,tsx,vue}: Use separateimport typestatements instead of inlinetypein mixed imports
Apply Prettier formatting with 2-space indentation, single quotes, no trailing semicolons, 80-character width
Sort and group imports by plugin, runpnpm formatbefore committing
Never useanytype - use proper TypeScript types
Never useas anytype assertions - fix the underlying type issue
Write code that is expressive and self-documenting - avoid unnecessary comments
Do not add or retain redundant comments - clean as you go
Avoid mutable state - prefer immutability and assignment at point of declaration
Watch out for Code Smells and refactor to avoid them
Files:
src/composables/graph/useGraphNodeManager.ts
src/**/{composables,components}/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (src/CLAUDE.md)
Clean up subscriptions in state management to prevent memory leaks
Files:
src/composables/graph/useGraphNodeManager.ts
src/**/*.{vue,ts,tsx}
📄 CodeRabbit inference engine (src/CLAUDE.md)
Follow Vue 3 composition API style guide
Files:
src/composables/graph/useGraphNodeManager.ts
src/**/{components,composables}/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (src/CLAUDE.md)
Use vue-i18n for ALL user-facing strings by adding them to
src/locales/en/main.json
Files:
src/composables/graph/useGraphNodeManager.ts
src/composables/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Name composables as
useXyz.ts(e.g.,useForm.ts)
Files:
src/composables/graph/useGraphNodeManager.ts
src/**/*.{ts,vue}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,vue}: Usereffor reactive state,computed()for derived values, andwatch/watchEffectfor side effects in Composition API
Avoid usingrefwithwatchif acomputedwould suffice - minimize refs and derived state
Useprovide/injectfor dependency injection only when simpler alternatives (Store or shared composable) won't work
Leverage VueUse functions for performance-enhancing composables
Use VueUse function for useI18n in composition API for string literals
Files:
src/composables/graph/useGraphNodeManager.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx}: Keep functions short and functional
Minimize nesting (if statements, for loops, etc.)
Use function declarations instead of function expressions when possible
Files:
src/composables/graph/useGraphNodeManager.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7415
File: browser_tests/tests/mobileBaseline.spec.ts:17-22
Timestamp: 2025-12-13T05:54:35.779Z
Learning: In browser_tests tests for the Comfy-Org/ComfyUI_frontend repository, the `comfyPage.loadWorkflow()` method already handles all necessary synchronization and waiting. No additional `await comfyPage.nextFrame()` call is needed before taking screenshots after loading a workflow.
📚 Learning: 2025-12-09T03:39:54.501Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7169
File: src/platform/remote/comfyui/jobs/jobTypes.ts:1-107
Timestamp: 2025-12-09T03:39:54.501Z
Learning: In the ComfyUI_frontend project, Zod is on v3.x. Do not suggest Zod v4 standalone validators (z.uuid, z.ulid, z.cuid2, z.nanoid) until an upgrade to Zod 4 is performed. When reviewing TypeScript files (e.g., src/platform/remote/comfyui/jobs/jobTypes.ts) validate against Zod 3 capabilities and avoid introducing v4-specific features; flag any proposal to upgrade or incorporate v4-only validators and propose staying with compatible 3.x patterns.
Applied to files:
src/composables/graph/useGraphNodeManager.ts
📚 Learning: 2025-12-13T11:03:11.264Z
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 7416
File: src/stores/imagePreviewStore.ts:5-7
Timestamp: 2025-12-13T11:03:11.264Z
Learning: In the ComfyUI_frontend repository, lint rules require keeping 'import type' statements separate from non-type imports, even if importing from the same module. Do not suggest consolidating them into a single import statement. Ensure type imports remain on their own line (import type { ... } from 'module') and regular imports stay on separate lines.
Applied to files:
src/composables/graph/useGraphNodeManager.ts
📚 Learning: 2025-12-17T00:40:09.635Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7537
File: src/components/ui/button/Button.stories.ts:45-55
Timestamp: 2025-12-17T00:40:09.635Z
Learning: Prefer pure function declarations over function expressions (e.g., use function foo() { ... } instead of const foo = () => { ... }) for pure functions in the repository. Function declarations are more functional-leaning, offer better hoisting clarity, and can improve readability and tooling consistency. Apply this guideline across TypeScript files in Comfy-Org/ComfyUI_frontend, including story and UI component code, except where a function expression is semantically required (e.g., callbacks, higher-order functions with closures).
Applied to files:
src/composables/graph/useGraphNodeManager.ts
📚 Learning: 2025-12-30T22:22:33.836Z
Learnt from: kaili-yang
Repo: Comfy-Org/ComfyUI_frontend PR: 7805
File: src/composables/useCoreCommands.ts:439-439
Timestamp: 2025-12-30T22:22:33.836Z
Learning: When accessing reactive properties from Pinia stores in TypeScript files, avoid using .value on direct property access (e.g., useStore().isOverlayExpanded). Pinia auto-wraps refs when accessed directly, returning the primitive value. The .value accessor is only needed when destructuring store properties or when using storeToRefs().
Applied to files:
src/composables/graph/useGraphNodeManager.ts
📚 Learning: 2025-12-11T12:25:15.470Z
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 7358
File: src/components/dialog/content/signin/SignUpForm.vue:45-54
Timestamp: 2025-12-11T12:25:15.470Z
Learning: This repository uses CI automation to format code (pnpm format). Do not include manual formatting suggestions in code reviews for Comfy-Org/ComfyUI_frontend. If formatting issues are detected, rely on the CI formatter or re-run pnpm format. Focus reviews on correctness, readability, performance, accessibility, and maintainability rather than style formatting.
Applied to files:
src/composables/graph/useGraphNodeManager.ts
📚 Learning: 2026-01-12T17:39:27.738Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7906
File: src/components/sidebar/tabs/AssetsSidebarTab.vue:545-552
Timestamp: 2026-01-12T17:39:27.738Z
Learning: In Vue/TypeScript files (src/**/*.{ts,tsx,vue}), prefer if/else statements over ternary operators when performing side effects or actions (e.g., mutating state, calling methods with side effects). Ternaries should be reserved for computing and returning values.
Applied to files:
src/composables/graph/useGraphNodeManager.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/composables/graph/useGraphNodeManager.ts (1)
418-433: Confirm “always wait a tick” also applies during graph configuration.
RAF deferral is only in the non-configuring path. If the intent is to always delay layout init, consider deferring in the configuring path as well (afteronAfterGraphConfigured).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
browser_tests/fixtures/utils/litegraphUtils.ts (1)
69-101: LGTM! Correctly usesemptySlot.posfor not-yet-created slots.The fix properly addresses the root cause identified in the postmortem — not-yet-created slots are stored under
.emptySlotrather than.slots, so accessingnode.emptySlot.posdirectly is the correct approach.Optional: Consider adding a defensive check for
emptySlotexistence (similar to theslot.posvalidation ingetPosition()) to provide a clearer error message if the property is missing:🔧 Optional defensive check
if (!node) { throw new Error(`No ${type} node found in subgraph`) } + if (!node.emptySlot?.pos) { + throw new Error(`No emptySlot position found on ${type} node`) + } + // Convert from offset to canvas coordinates const canvasPos = window['app'].canvas.ds.convertOffsetToCanvas([ node.emptySlot.pos[0], node.emptySlot.pos[1] ])
e75871f to
fd22c54
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/composables/graph/useGraphNodeManager.ts (1)
422-429: Consider adding guard before re-extracting data.If a node is removed between being added and when
onAfterGraphConfiguredfires, line 426 will re-add data tovueNodeDatafor a node that no longer exists. The guard ininitializeVueNodeLayoutprevents the layout store issue, but leaves an orphan entry invueNodeData.Optional: Add guard before data extraction
node.onAfterGraphConfigured = useChainCallback( node.onAfterGraphConfigured, () => { + // Guard against node removal before configure completes + if (!nodeRefs.has(id)) return + // Re-extract data now that configure() has populated title/slots/widgets/etc. vueNodeData.set(id, extractVueNodeData(node)) initializeVueNodeLayout() } )
| const canvasPos = window['app'].canvas.ds.convertOffsetToCanvas([ | ||
| slotX, | ||
| slotY | ||
| node.emptySlot.pos[0], | ||
| node.emptySlot.pos[1] | ||
| ]) |
There was a problem hiding this comment.
Very nit...
Could you just pass node.emptySlot.pos?
There was a problem hiding this comment.
Yeah, that would have been better. Automerge took the reigns this time, but I'll make sure to fix if I'm ever in the code section again.
A frequent pattern is to add a node to the graph, and then update the nodes position afterwards. Some of these cases (like subgraph unpacking) can set the node position in advance, but others, (like importA1111) require information on nodes in order to perform arranging. Alternatives, like allowing code to either modify `app.configuringGraph` or otherwise set a temporary state were considered, but create the same problem of requiring fixes in many places. As a proposed alternative, when a node is created, an extra tick of delay is always added before initializing layout. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7591-Always-wait-for-next-tick-before-layout-init-2cc6d73d365081f4ababc38020645670) by [Unito](https://www.unito.io) --------- Co-authored-by: DrJKL <DrJKL0424@gmail.com> Co-authored-by: github-actions <github-actions@github.com>
|
@AustinMroz plz fix |
#7591 added a one tick delay to layout initialization in an attempt to resolve some layouting discrepancies. However, it appears to have reintroduced node scaling issues and introduced a new bug that prevents cloning nodes with alt+drag in vue. Alternatives methods of resolving the original issue are being investigated, but this change was causing more harm than good. The prior PR included other changes (like a testing fix). Those changes remain beneficial and do not need to be reverted. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8619-Revert-delay-of-layout-initialization-2fe6d73d365081fc9111c9457ea9752d) by [Unito](https://www.unito.io) --------- Co-authored-by: github-actions <github-actions@github.com>
#7591 added a one tick delay to layout initialization in an attempt to resolve some layouting discrepancies. However, it appears to have reintroduced node scaling issues and introduced a new bug that prevents cloning nodes with alt+drag in vue. Alternatives methods of resolving the original issue are being investigated, but this change was causing more harm than good. The prior PR included other changes (like a testing fix). Those changes remain beneficial and do not need to be reverted. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8619-Revert-delay-of-layout-initialization-2fe6d73d365081fc9111c9457ea9752d) by [Unito](https://www.unito.io) --------- Co-authored-by: github-actions <github-actions@github.com>
A frequent pattern is to add a node to the graph, and then update the nodes position afterwards.
Some of these cases (like subgraph unpacking) can set the node position in advance, but others, (like importA1111) require information on nodes in order to perform arranging.
Alternatives, like allowing code to either modify
app.configuringGraphor otherwise set a temporary state were considered, but create the same problem of requiring fixes in many places.As a proposed alternative, when a node is created, an extra tick of delay is always added before initializing layout.
┆Issue is synchronized with this Notion page by Unito