Repository navigation
Keep synthetic Copy link commands inside the clipboard fence - #1346
Conversation
Preserve current main and pin the reviewed fork dependency family. Qualify bundled and system-font configurations, plugins and regression tests.
Reconcile upstream: keep both roadmap entries, main's Copy as submenu marker and copy-link toast, and the migration's explicit join-order switch in the live session test. Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Session switch ordering crates/collab-net/src/sessions_tests.rs |
The test performs one switch to file B after noting that join completion order is nondeterministic. It checks that B is in front while edits arrive in A. |
Copy-link clipboard fencing
| Layer / File(s) | Summary |
|---|---|
Copy-link invocation and clipboard behavior crates/ui-egui/src/menus.rs, crates/ui-egui/src/clipboard_fence_tests.rs |
The edit.copyLink branch calls app.clipboard_set_text. The test checks that a synthetic invocation leaves the system clipboard unchanged and that a user invocation writes the selected layer’s link once. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Suggested reviewers: watchakorn-18k, bflatastic, aiongg
Merge Risk: 
b7a0d
-
Copy link should leave the clipboard unchanged for synthetic requests while continuing to copy links for users. No issue identified here needs resolution before merge.
Architecture risk: 
b7a0d
-
The change affects 1 system.
Changed systems:
cratesArchitecture concerns
No architecture-level concerns identified.Review details
-
Systems and components
- observed — crates (service) was modified; 3 changed files map to changed impact.
Before / after behavior
- observed — Modified behavior in crates/collab-net/src/sessions_tests.rs: The test removes a redundant earlier switch to B and places its single switch after the note that joins may complete in either order. It then asserts B is in front before testing edits to A.
- observed — Modified behavior in crates/ui-egui/src/clipboard_fence_tests.rs: Added a test covering synthetic and user
edit.copyLinkinvocations: the synthetic control request is acknowledged, leaves the system clipboard unchanged, and ends its fence with the frame; the user invocation writes the selected layer’s link once. - observed — Modified behavior in crates/ui-egui/src/menus.rs: The
edit.copyLinkbranch now callsapp.clipboard_set_textrather than the clipboard service directly; its comment identifies this as the control-channel-fenced path. The selected-layer condition remains unchanged.
Reliability and maintainability
- inferred — Risk-relevant change factors for crates: blast_radius_1; direct_dependents_1
-
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: routing synthetic Copy link commands through the clipboard fence. Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
FredApps
left a comment
FredApps
left a comment
There was a problem hiding this comment.
Approving at b7a0d89. The gate and SonarCloud pass at this head.
Copy link in menus.rs now writes through app.clipboard_set_text (the control-channel fence) instead of app.services.clipboard_set_text, so a synthetic ui.menu.invoke no longer replaces the user's clipboard. The new test sends a real ui.menu.invoke for edit.copyLink and checks no clipboard write, the toast and the fence ending with the frame, then a user invocation writing exactly the figcraft://node/<id> text. The collab test change only drops a duplicated doc.switch and moves a comment.
Limits: the OS clipboard itself was not observed; the test uses the recording window.
I read the code and the CI results; I did not run the app.
|
Merged, thank you @skylartaylor! With this, synthetic Copy link commands stay inside the clipboard fence. It landed with your commits as they were. |
Synthetic
ui.menu.invokecalls toedit.copyLinknow use the app’s clipboard fence, preserving the user’s system clipboard. A normal user invocation still copies the selected node’s link. This follows merged #1312 and also removes a redundant document switch from a collaboration test.The regression test sends a real control request through a UI frame, verifies that Copy link ran without writing to the clipboard, then verifies that a normal invocation writes the exact link once. Restoring the original direct clipboard call makes the test fail at the clipboard assertion.
Validation: full local CI passed with 3,329 passing test executions, zero failures and nine ignored, including WASM and three plugin bundles. Independent review accepted the change; the final committed files match the reviewed and tested source. Base:
236f3185d68512e0f4af0eefe26d24c2e1c4ddd7.Remaining checks: remote CI and CodeRabbit on this new PR; native clipboard and GUI workflows, platform/IME coverage, final packaging and paired application performance. The regression uses a recording clipboard service and does not establish native OS clipboard behavior. Human review and merge are required.
Summary by CodeRabbit
Bug Fixes
Tests