fix(web-desktop): copy messages to the browser's machine, not the host#1143
fix(web-desktop): copy messages to the browser's machine, not the host#1143chr1syy wants to merge 2 commits into
Conversation
Clicking Copy on a message in web-desktop wrote to the clipboard of the
machine running the Electron host, not the browser the user is on. The
shared clipboard helper always preferred window.maestro.shell.copyText-
ToClipboard, which routes over IPC to the host's Electron clipboard, and
that path is always present in web-desktop via the preload shim.
Guard the Electron IPC path with isWebDesktop() so web-desktop uses the
browser Clipboard API instead. Applied to text, image-write and image-
read helpers.
Add a legacy execCommand('copy') fallback for the text path: the async
Clipboard API is gated to secure contexts, so it is undefined when web-
desktop is served over plain HTTP (e.g. a Tailscale/LAN IP without TLS).
The fallback runs when navigator.clipboard is missing or the async write
throws, so the copy still lands on the user's own machine.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesClipboard Routing and Fallback
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed due to a network error. 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 |
Greptile SummaryThis PR changes clipboard routing so web-desktop copies use the browser machine. The main changes are:
Confidence Score: 4/5The changed clipboard flow needs fixes for image copy and image read over plain HTTP web-desktop.
src/renderer/utils/clipboard.ts Important Files Changed
Reviews (1): Last reviewed commit: "fix(web-desktop): copy to the browser's ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/__tests__/renderer/utils/clipboard.test.ts (1)
21-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the image routing guards.
Lines 100-123 in
src/renderer/utils/clipboard.tsalso changedsafeClipboardWriteImageandsafeClipboardReadImageto skip host IPC in web-desktop, but this suite only exercisessafeClipboardWrite. Please add one web-desktop case for each image helper so a future regression there does not silently copy images on the Electron host again.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/renderer/utils/clipboard.test.ts` around lines 21 - 92, Add regression coverage in the clipboard tests for the image routing guards introduced in safeClipboardWriteImage and safeClipboardReadImage. Extend the existing safeClipboardWrite suite with one web-desktop test for each helper that verifies mockIsWebDesktop() routes to the browser path and does not call the host IPC clipboard on window.maestro.shell. Keep the tests close to the existing safeClipboardWrite cases so the behavior of safeClipboardWriteImage and safeClipboardReadImage is locked down against future regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/__tests__/renderer/utils/clipboard.test.ts`:
- Around line 21-92: Add regression coverage in the clipboard tests for the
image routing guards introduced in safeClipboardWriteImage and
safeClipboardReadImage. Extend the existing safeClipboardWrite suite with one
web-desktop test for each helper that verifies mockIsWebDesktop() routes to the
browser path and does not call the host IPC clipboard on window.maestro.shell.
Keep the tests close to the existing safeClipboardWrite cases so the behavior of
safeClipboardWriteImage and safeClipboardReadImage is locked down against future
regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 55632a28-2348-4211-8133-19d221ca438f
📒 Files selected for processing (2)
src/__tests__/renderer/utils/clipboard.test.tssrc/renderer/utils/clipboard.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a69ab91add
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Hi @chr1syy, thanks a lot for this fix! 🙏 Routing web-desktop clipboard writes to the browser instead of the Electron host is exactly the right call, and the insecure-context Before we merge, there are a few things worth addressing. These surfaced in the automated reviews (Greptile / CodeRabbit / Codex) and I agree they're worth handling: Must fix
Should fix (this is the plain-HTTP case the PR targets)
Nice to have
Good news: no merge conflicts, the branch is clean against |
Address PR review feedback on the web-desktop clipboard fix: - legacyExecCommandCopy now restores focus to the previously focused element after the hidden textarea is removed, so the next keystroke returns to the control that initiated the copy. - Add web-desktop regression tests for safeClipboardWriteImage and safeClipboardReadImage proving they route to the browser API and never hit the host IPC clipboard. - Replace an em-dash in the test header comment per the repo writing rule. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai Addressed the image-routing test coverage nitpick in ac5e9a2: added two new web-desktop regression suites in Also addressed greptile's P2 (legacy copy now restores focus to the previously focused element) and codex's em-dash nit in the test comment. The two greptile P1 image threads are inherent insecure-context browser limitations (no browser API for binary clipboard data over plain HTTP); replied inline with rationale. |
|
Tip For best results, initiate chat on the files or code changes.
Your note on the remaining plain-HTTP image clipboard limitations also makes sense. Given the browser restrictions there, an inline rationale is a reasonable way to document the constraint. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
2 participants
Problem
Clicking Copy on a message while accessing Maestro through web-desktop (the browser interface) copied the text to the clipboard of the machine running the Electron host, not the PC the user is browsing from.
Root cause
safeClipboardWrite()insrc/renderer/utils/clipboard.tsalways preferredwindow.maestro.shell.copyTextToClipboard, which routes over IPC to the host's Electronclipboard.writeText(). In web-desktop the renderer runs in the browser but the preload shim still exposes that method, so the browser-native fallback was never reached. Result: the copy landed on the server's machine.Fix
Route web-desktop through the browser clipboard. Guard the Electron IPC path with the existing
isWebDesktop()helper (src/renderer/utils/runtimeContext.ts) so web-desktop usesnavigator.clipboardinstead. Applied to all three helpers that had the same flaw:safeClipboardWrite(text),safeClipboardWriteImage, andsafeClipboardReadImage.Insecure-context fallback. The async Clipboard API is gated to secure contexts, so
navigator.clipboardisundefinedwhen web-desktop is served over plain HTTP (e.g. a Tailscale/LAN IP without TLS). Added a legacyexecCommand('copy')fallback for the text path that runs whennavigator.clipboardis missing or the async write throws, so the copy still lands on the user's own machine.Behavior matrix (text copy)
Tests
Added
src/__tests__/renderer/utils/clipboard.test.ts(5 cases, all passing): desktop uses IPC; web-desktop uses the browser API; insecure context falls back to execCommand; thrown async write falls back to execCommand; returns false only when even the legacy copy fails.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
execCommand) and retry behavior when browser clipboard access fails.Tests