-
Notifications
You must be signed in to change notification settings - Fork 458
feat: when restored position has no nodes in viewport, automatically fit to view #7435
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
When loading a workflow with a saved viewport position (extra.ds), check if any nodes are visible in that viewport. If not, fit view to ensure the user can see the workflow.
📝 WalkthroughWalkthroughA new utility function checks if nodes overlap a visible rectangle, enabling conditional viewport auto-fitting during graph restoration. When no nodes occupy the saved viewport, the view is automatically fitted. A test verifies the behavior with visual regression testing. Changes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🧰 Additional context used📓 Path-based instructions (8)**/*.{ts,tsx,js,jsx,vue,json}📄 CodeRabbit inference engine (AGENTS.md)
Files:
src/**/*.{vue,ts}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
src/**/*.ts📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
src/**/*.{ts,tsx,vue}📄 CodeRabbit inference engine (src/CLAUDE.md)
Files:
src/**/*.{vue,ts,tsx}📄 CodeRabbit inference engine (src/CLAUDE.md)
Files:
**/*.{ts,tsx,vue}📄 CodeRabbit inference engine (AGENTS.md)
Files:
browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (browser_tests/CLAUDE.md)
Files:
browser_tests/**/*.spec.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (17)📚 Learning: 2025-11-24T19:47:56.371ZApplied to files:
📚 Learning: 2025-11-24T19:47:56.371ZApplied to files:
📚 Learning: 2025-12-09T03:39:54.501ZApplied to files:
📚 Learning: 2025-12-11T12:25:15.470ZApplied to files:
📚 Learning: 2025-11-24T19:47:22.909ZApplied to files:
📚 Learning: 2025-12-09T20:22:23.620ZApplied to files:
📚 Learning: 2025-12-09T20:22:23.620ZApplied to files:
📚 Learning: 2025-11-24T19:47:22.909ZApplied to files:
📚 Learning: 2025-11-24T19:47:22.909ZApplied to files:
📚 Learning: 2025-11-24T19:48:03.270ZApplied to files:
📚 Learning: 2025-12-09T20:22:23.620ZApplied to files:
📚 Learning: 2025-11-24T19:47:22.909ZApplied to files:
📚 Learning: 2025-12-09T20:22:23.620ZApplied to files:
📚 Learning: 2025-12-13T05:54:28.225ZApplied to files:
📚 Learning: 2025-12-13T05:34:15.488ZApplied to files:
📚 Learning: 2025-12-09T20:22:23.620ZApplied to files:
📚 Learning: 2025-11-24T19:47:56.371ZApplied to files:
🧬 Code graph analysis (2)src/utils/mathUtil.ts (2)
src/scripts/app.ts (1)
🔇 Additional comments (6)
Comment |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 12/13/2025, 06:17:19 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Test Results❌ Some tests failed ⏰ Completed at: 12/13/2025, 06:25:58 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.25 MB (baseline 3.24 MB) • 🔴 +1.71 kBMain entry bundles and manifests
Status: 3 added / 3 removed Graph Workspace — 984 kB (baseline 984 kB) • 🟢 -68 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 6.54 kB (baseline 6.54 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 1 added / 1 removed Panels & Settings — 298 kB (baseline 298 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 6 added / 6 removed UI Components — 178 kB (baseline 178 kB) • 🟢 -33 BReusable component library chunks
Status: 7 added / 7 removed Data & Services — 12.5 kB (baseline 12.5 kB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 2 added / 2 removed Utilities & Hooks — 1.86 kB (baseline 3.18 kB) • 🟢 -1.32 kBHelpers, composables, and utility bundles
Status: 1 added / 2 removed Vendor & Third-Party — 8.56 MB (baseline 8.56 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 3.81 MB (baseline 3.81 MB) • ⚪ 0 BBundles that do not match a named category
Status: 18 added / 18 removed |
|
Updating Playwright Expectations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This workflow's viewport data is way down to the right with no nodes in view. This snapshot verified the fit-to-view happened automatically.
AustinMroz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming canvas.computeVisibleNodes isn't used because node.boundingRect isn't initialized yet?
I suppose that creates an edge case where a single node title barely displaying off the bottom of the screen will still trigger fit, but that's not worth solving.
LGTM
…fit to view (Comfy-Org#7435) ## Summary Sometimes the saved position is super far away from any of the nodes, which causes general confusion. This PR changes the `loadGraphData` logic to fit-to-view in those scenarios. Fixes Comfy-Org#7425 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7435-feat-when-restored-position-has-no-nodes-in-viewport-automatically-fit-to-view-2c86d73d36508119bf2ed9d361ec868f) by [Unito](https://www.unito.io) --------- Co-authored-by: github-actions <github-actions@github.com>
#7435 introduced a tricky regression which will cause extremely small levels of zoom with nodes spread far apart when in vue mode. I am able to consistently reproduce this behaviour by - Being in vue mode - Swapping to a different tab so that ComfyUI is in the background - Making a pointless line change to frontend code so that vite forces a reload - Waiting ~1 minute to ensure the reload completes - Swapping back to the ComfyUI tab From testing, if a reload occurs while the tab is backgrounded, the canvas has an uninitialized size of 300x150. This PR proposes falling back to a more sane default width and height of 1920x1080 if it is detected that the canvas element is unitialized. | Before | After | | ------ | ----- | | <img width="360" alt="before" src="https://github.com/user-attachments/assets/8e19fc98-7187-4008-98cc-fb5ea3bcdce2"/> | <img width="360" alt="after" src="https://github.com/user-attachments/assets/add88614-3451-44df-ae9a-b0b867486459" />| This appears to have consistently good results, but second opinions or further testing would be appreciated. A more reasonable option (like skipping this automatic fitView if the canvas has uninitialized size) is likely to be safer, even if it results in a return of edge cases resulting in a graph having no nodes in view after load. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7751-Workaround-for-reload-causing-node-spread-2d36d73d365081b9ae74d5f0e6f436f5) by [Unito](https://www.unito.io)
…fit to view (#7435) ## Summary Sometimes the saved position is super far away from any of the nodes, which causes general confusion. This PR changes the `loadGraphData` logic to fit-to-view in those scenarios. Fixes #7425 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7435-feat-when-restored-position-has-no-nodes-in-viewport-automatically-fit-to-view-2c86d73d36508119bf2ed9d361ec868f) by [Unito](https://www.unito.io) --------- Co-authored-by: github-actions <github-actions@github.com>
#7435 introduced a tricky regression which will cause extremely small levels of zoom with nodes spread far apart when in vue mode. I am able to consistently reproduce this behaviour by - Being in vue mode - Swapping to a different tab so that ComfyUI is in the background - Making a pointless line change to frontend code so that vite forces a reload - Waiting ~1 minute to ensure the reload completes - Swapping back to the ComfyUI tab From testing, if a reload occurs while the tab is backgrounded, the canvas has an uninitialized size of 300x150. This PR proposes falling back to a more sane default width and height of 1920x1080 if it is detected that the canvas element is unitialized. | Before | After | | ------ | ----- | | <img width="360" alt="before" src="https://github.com/user-attachments/assets/8e19fc98-7187-4008-98cc-fb5ea3bcdce2"/> | <img width="360" alt="after" src="https://github.com/user-attachments/assets/add88614-3451-44df-ae9a-b0b867486459" />| This appears to have consistently good results, but second opinions or further testing would be appreciated. A more reasonable option (like skipping this automatic fitView if the canvas has uninitialized size) is likely to be safer, even if it results in a return of edge cases resulting in a graph having no nodes in view after load. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7751-Workaround-for-reload-causing-node-spread-2d36d73d365081b9ae74d5f0e6f436f5) by [Unito](https://www.unito.io)
Summary
Sometimes the saved position is super far away from any of the nodes, which causes general confusion. This PR changes the
loadGraphDatalogic to fit-to-view in those scenarios.Fixes #7425
┆Issue is synchronized with this Notion page by Unito