Skip to content

Conversation

christian-byrne
Copy link
Contributor

@christian-byrne christian-byrne commented Oct 14, 2025

Summary

Fixed intermittent copy/paste failures when clicking minimap or canvas control buttons by expanding target whitelist in document-level clipboard handlers.

Changes

  • What: Expanded isTargetInGraph check in useCopy.ts and usePaste.ts to include minimap and canvas controls
  • Added IDs:
    • comfy-minimap on minimap container (src/renderer/extensions/minimap/MiniMap.vue)
    • graph-canvas-controls on canvas controls ButtonGroup (src/components/graph/GraphCanvasMenu.vue)
  • Test Infrastructure: Created Minimap test fixture following existing pattern (Topbar, SidebarTab)

Technical Details

Document-level clipboard handlers (added Feb 2025) replace LiteGraph's canvas-only handlers but require event target to be within graph workspace. Previous implementation only checked:

e.target.id === 'graph-canvas' ||
e.target.classList.contains('litegraph') ||
e.target.classList.contains('graph-canvas-container')

Clicking UI elements (minimap buttons, zoom controls) caused focus loss, making target check fail silently.

┆Issue is synchronized with this Notion page by Unito

Copy link

github-actions bot commented Oct 14, 2025

🎨 Storybook Build Status

Build completed successfully!

⏰ Completed at: 10/18/2025, 07:29:03 PM UTC

🔗 Links


🎉 Your Storybook is ready for review!

Copy link

github-actions bot commented Oct 14, 2025

🎭 Playwright Test Results

Some tests failed

⏰ Completed at: 10/18/2025, 07:43:41 PM UTC

📈 Summary

  • Total Tests: 499
  • Passed: 465 ✅
  • Failed: 1 ❌
  • Flaky: 2 ⚠️
  • Skipped: 31 ⏭️

📊 Test Reports by Browser

  • chromium: View Report • ✅ 456 / ❌ 1 / ⚠️ 2 / ⏭️ 31
  • chromium-2x: View Report • ✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • chromium-0.5x: View Report • ✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • mobile-chrome: View Report • ✅ 6 / ❌ 0 / ⚠️ 0 / ⏭️ 0

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

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Oct 14, 2025
@christian-byrne christian-byrne force-pushed the allow-copy-paste-on-minimap branch from a059806 to 12bdf89 Compare October 17, 2025 09:02
@christian-byrne christian-byrne removed the claude-review Add to trigger a PR code review from Claude Code label Oct 17, 2025
@AustinMroz
Copy link
Collaborator

AustinMroz commented Oct 17, 2025

I've been exploring similar options over in #6087.

Thoughts on broadening the target restrictions to e.target instanceof HTMLTextAreaElement || e.target instanceof HTMLInputElement? IIRC the only reason we don't grab every clipboard event was that supporting undo/redo for text inputs was non-feasible. Are there any cases other than text inputs where we don't want our copy/paste handler to apply?

Edit to clarify: The changes here are an improvement and LGTM, even if further changes will be made down the line.

@benceruleanlu
Copy link
Member

@codex review

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

benceruleanlu
benceruleanlu previously approved these changes Oct 17, 2025
Copy link
Member

@benceruleanlu benceruleanlu left a comment

Choose a reason for hiding this comment

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

LGTM, any ideas on a more robust solution that won't need id upkeep to prevent drifting?

@christian-byrne
Copy link
Contributor Author

LGTM, any ideas on a more robust solution that won't need id upkeep to prevent drifting?

Nothing comes to mind. Is ID drifting common? Would be pretty bad to change IDs and not audit all usage first.

Thankfully since I added tests, we will be able to spot that case immediately when the test fails.

Copy link

Bundle Size Report

App Entry Points

Main application bundles

File Size Gzip Brotli
assets/index-Cu3g1N6G.js 2.02 MB 381 kB 296 kB
assets/index-CuLZTtAC.js
assets/index-CXG_5Ix6.js 9.67 MB 1.99 MB 1.43 MB
assets/index-E0kYOj1H.js
assets/index-PESgPnbc.js 507 B 292 B 249 B

Category Total: 11.7 MB

Core Views

Major application views and screens

File Size Gzip Brotli
assets/GraphView-BLQk4ujW.js
assets/GraphView-BsMVw2gy.js 714 kB 139 kB 108 kB
assets/UserSelectView-DIVk3Vdr.js 7.99 kB 2.4 kB 2.1 kB
assets/UserSelectView-Kp6GcnoT.js

Category Total: 722 kB

UI Panels

Settings and configuration panels

File Size Gzip Brotli
assets/AboutPanel-Bq9ApgWv.js
assets/AboutPanel-gCsLk9Xp.js 10.1 kB 2.58 kB 2.27 kB
assets/CreditsPanel-CVBAywTM.js
assets/CreditsPanel-DtXLBHdt.js 21.9 kB 5.21 kB 4.54 kB
assets/ExtensionPanel-DFmOwtDV.js
assets/ExtensionPanel-gR_9LHV7.js 11.9 kB 2.75 kB 2.4 kB
assets/KeybindingPanel-CYjSSRkS.js 15.1 kB 3.68 kB 3.24 kB
assets/KeybindingPanel-IQI7hKov.js
assets/ServerConfigPanel-BJ7OL5nI.js 8.04 kB 2.09 kB 1.83 kB
assets/ServerConfigPanel-D2PD5OIf.js
assets/UserPanel-BFDcG5jS.js 7.76 kB 1.98 kB 1.72 kB
assets/UserPanel-D0nz-wnp.js

Category Total: 74.8 kB

Services

Business logic and services

File Size Gzip Brotli
assets/keybindingService-Cnd76Vc_.js
assets/keybindingService-Q8YZIFQi.js 7.22 kB 1.75 kB 1.5 kB
assets/serverConfigStore-CB-AA3JS.js
assets/serverConfigStore-D8Rx83Cp.js 2.79 kB 890 B 783 B

Category Total: 10 kB

Utilities

Helper functions and utilities

File Size Gzip Brotli
assets/mathUtil-CTARWQ-l.js 1.07 kB 525 B 452 B

Category Total: 1.07 kB

Other

Uncategorized bundles

File Size Gzip Brotli
assets/UserAvatar.vue_vue_type_script_setup_true_lang-C9bSkTC5.js 1.12 kB 577 B 494 B

Category Total: 1.12 kB


Overall Total Size: 12.5 MB
Total Bundle Count: 15

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

Labels

area:hotkeys area:minimap area:navigation size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants