fix(tui): surface clipboard write failures instead of false success - #41924
fix(tui): surface clipboard write failures instead of false success#41924aishangwuji wants to merge 1 commit into
Conversation
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
|
I ran into this on Kubuntu 24.04 using X11 while in Konsole. What I want to add is that the error you're trying to show already exists and is getting thrown away. The reason turned out to be simple. I had no clipboard program installed at all. No xclip, no xsel, no wl-copy. Nothing told me that. Here's what the code does. In packages/tui/src/clipboard.ts, copyCommand looks for wl-copy, xclip and xsel. If it finds none of them it returns nothing. getCopyMethod then falls back to clipboardy. All three of those paths end in .catch(() => undefined), so the error disappears. Here's the part that matters. clipboardy doesn't fail quietly. Its linux backend throws this exact message. "Couldn't find the xsel binary and fallback didn't work. On Debian/Ubuntu you can install xsel with: sudo apt install xsel. The bundled xsel fallback was not found. This can happen when running in a bundled environment (e.g. Node.js SEA)." clipboardy ships its own copy of xsel and checks whether that file is really there, because in a bundled build it isn't. opencode is a single Bun binary and that file isn't in the installed package, so the second line is the one you hit here. So the fix is smaller than writing a new error message. Show clipboardy's message instead of a generic failure. It already tells the user what to install. One catch. That message says to install xsel, but image paste only uses xclip. read() runs xclip -selection clipboard -t image/png -o for it. So someone who installs xsel gets text copy working and image paste still broken. Pointing people at xclip fixes both. The troubleshooting page has the same problem. It lists apt install -y xclip # or apt install -y xsel as if either one works. Something this fix won't reach. Once xclip is installed, copy stops failing and starts hanging instead. xclip has to stay alive to own the X selection, and the background process it forks holds the stdout pipe open, so command() never sees close and write() never settles. I had one sit pending for over eight minutes on a copy that actually worked. A pending promise never rejects, so surfacing errors doesn't help there. I'm filing that separately. Two smaller things. writeOsc52 sends the escape through process.stdout.write, which is buffered and can get mixed in with the TUI drawing instead of reaching the terminal. That was part of #35289 too. And Konsole didn't support OSC 52 at all until it was added in July 2024 (KDE MR !767). Kubuntu 24.04 ships Konsole 23.08, which is a year older than that, so on this setup both ways of copying fail at the same time and you still get the success message. It breaks in the quietest way possible. |
|
In the next version of OpenCode, we don't require the xclip, excel, wl-copy, etc. anymore, but instead I'm linking against Wayland and X libraries for managing the clipboard. |
Issue for this PR
Closes #41470
Type of change
What does this PR do?
The TUI shows a "Copied to clipboard" toast even when the clipboard write silently failed, leaving the system clipboard empty. On Linux (e.g. Ubuntu), copy-on-select fires on mouse-up, but when no clipboard tool (
wl-copy/xclip/xsel) is available or the write errors out,packages/tui/src/clipboard.tsswallowed the error with.catch(() => undefined)in both the native command andclipboardyfallback paths.write()therefore always resolved, and every caller (Selection.copy,DialogProvider.copySelection, message/session copy, etc.) showed the success toast regardless of the actual outcome.The fix removes the error swallowing so real failures propagate to the callers' existing
toast.errorhandlers. To make the write path unit-testable, the native/clipboardy logic was extracted into an injectablewriteWith(deps, text).How did you verify your code works?
packages/tui/test/clipboard.test.tscases assertingwriteWithrejects when the native command or theclipboardyfallback fails (these fail against the old code), plus coverage for the success path and osascript escaping.bun test test/clipboard.test.tsinpackages/tui: 8/8 pass.bun typecheckinpackages/tui: passes.Screenshots / recordings
Not a visual change; behavior of the failure toast only.
Checklist