Skip to content

fix: prevent splitter overlay from loading in focus mode #5670

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix: prevent splitter overlay from loading in focus mode #5670

wants to merge 5 commits into from
+5 −9

Conversation

Myestery
Copy link
Collaborator

@Myestery Myestery commented Sep 19, 2025 •
edited by sync-by-unito bot
Loading

This pull request updates the display logic for the LiteGraphCanvasSplitterOverlay component in GraphCanvas.vue. The change ensures that the splitter overlay is only shown when the application is ready, the beta menu is enabled, and the workspace is not in focus mode.

Conditional rendering improvement:

Screen.Recording.2025-09-19.at.21.41.46.mov

┆Issue is synchronized with this Notion page by Unito

@Myestery Myestery requested a review from a team as a code owner September 19, 2025 20:42
@Myestery Myestery linked an issue Sep 19, 2025 that may be closed by this pull request
Bug: "Focus Mode(F)" Front-end performance is abnormal #5658
Open
3 tasks
@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Sep 19, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 19, 2025 •
edited
Loading

🎭 Playwright Test Results

❌ Some tests failed

⏰ Completed at: 09/22/2025, 07:47:51 PM UTC

📈 Summary

  • Total Tests: 455
  • Passed: 421 ✅
  • Failed: 5 ❌
  • Flaky: 0
  • Skipped: 29 ⏭️

📊 Test Reports by Browser

  • ✅ chromium: View Report • ✅ 414 / ❌ 5 / ⚠️ 0 / ⏭️ 29
  • ✅ chromium-2x: View Report • ✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • ✅ chromium-0.5x: View Report • ✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • ✅ mobile-chrome: View Report • ✅ 4 / ❌ 0 / ⚠️ 0 / ⏭️ 0

🎉 Click on the links above to view detailed test results for each browser configuration.

@Myestery Myestery requested a review from DrJKL September 19, 2025 20:46
DrJKL
DrJKL previously approved these changes Sep 19, 2025
View reviewed changes
Copy link
Contributor

@DrJKL DrJKL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the minimap/controls stuff will be a follow-up?

@DrJKL DrJKL assigned Myestery and unassigned DrJKL Sep 19, 2025
@Myestery
Copy link
Collaborator Author

  • View Report

Let me confirm the expected behaviour internally and revert

So the minimap/controls stuff will be a follow-up?

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Sep 19, 2025
@Myestery Myestery added the New Browser Test Expectations New browser test screenshot should be set by github action label Sep 22, 2025
@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Sep 22, 2025
christian-byrne
christian-byrne reviewed Sep 24, 2025
View reviewed changes
Copy link
Contributor

@christian-byrne christian-byrne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the massive number of regenerated snapshots, I wonder if it is feasible to apply my suggestions (assuming you agree that they are valid) then open a new PR (given it's only 5 or so lines changes)? WDYT?

src/components/graph/GraphCanvas.vue
/>
</template>
</LiteGraphCanvasSplitterOverlay>
<GraphCanvasMenu v-if="!betaMenuEnabled && canvasMenuEnabled" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we accidentally dropped the !betaMenuEnabled check here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I dropped it because we had 2 Graphcanvas components and thebetaMenuEnabled check was logically irrelevant

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 GraphCanvasMenu components? Apologies for not understanding

src/components/graph/GraphCanvas.vue
</LiteGraphCanvasSplitterOverlay>
<GraphCanvasMenu v-if="!betaMenuEnabled && canvasMenuEnabled" />
<GraphCanvasMenu v-if="canvasMenuEnabled" class="pointer-events-auto" />
<MiniMap v-if="comfyAppReady && minimapEnabled" class="pointer-events-auto" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above, the MiniMap was previously inside of a parent that only rendered when !betaMenuEnabled, so now we may need to move that condition directly into the v-if here.

@christian-byrne
Copy link
Contributor

There seems to be merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christian-byrne christian-byrne christian-byrne left review comments

@DrJKL DrJKL DrJKL left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@Myestery Myestery

Labels
New Browser Test Expectations New browser test screenshot should be set by github action size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: "Focus Mode(F)" Front-end performance is abnormal
3 participants
@Myestery @christian-byrne @DrJKL