feat(tui): add linux_clipboard_selection config for primary buffer support - #32370
feat(tui): add linux_clipboard_selection config for primary buffer support#32370bornmw wants to merge 15 commits into
Conversation
|
Hey! Your PR title Please update it to start with one of:
Where See CONTRIBUTING.md for details. |
|
The following comment was made by an LLM, it may be inaccurate: Potential Duplicate PRs Found
Note: PR #6370 appears to be the most relevant, as it specifically addresses enabling PRIMARY clipboard copy for Linux middle-click paste functionality—the exact feature being implemented in PR #32370. |
I reviewed the potential duplicates flagged by the bot, and here is how this PR compares: PR #31252 and PR #27861 are not duplicates. They handle clipboard failure reporting and error validation rather than the primary selection buffer. PR #6370 attempts to solve the same underlying issue (adding PRIMARY selection for Linux middle-click paste), but there are a few key differences. First, that PR has been stalled since December. Second, my implementation introduces a more flexible configuration approach—letting the user explicitly choose between Clipboard, Primary, or Both (defaulting to Both)—whereas #6370 uses a simple boolean toggle. Finally, this PR optimizes the copy operation under the hood by utilizing Promise.allSettled to update the buffers in parallel. Given these architectural differences and the inactivity on #6370, I believe this PR provides a more robust and complete solution to closing #29963. Let me know if any changes are needed! |
b1cb38a to
c6e0c83
Compare
693c292 to
f5bfa85
Compare
bornmw
left a comment
There was a problem hiding this comment.
PR #32370 — feat(opencode): add linux_clipboard_selection config for primary buffer support
Verdict: Request changes (small)
The feature is sound and the bundled fix is the real value. The issues are formatting, one behavioral gap, and some type hygiene.
What is good
- The
wl-copy --type text/plain;charset=utf-8fix is the most important line in this PR. Without it, pipedwl-copyoffers onlyapplication/octet-stream(wl-copy dumps stdin to a temp file namedstdin, andxdg-mimecontent-sniffing on an extensionless file returnsoctet-stream), which text editors reject. This is arguably a hidden bug fix inside a feature PR — worth calling out explicitly in the description. - Clean separation:
copyCommandstays pure,getCopyMethodroutes by selection,Promise.allSettledfor "both" is correct. - Schema annotation carries the description into the generated
tui.jsonschema. - Tests cover all
copyCommandbranches including the MIME fix.
Issues
-
OSC52 bypasses the selection setting (most substantive).
write()callswriteOsc52(text)unconditionally before routing to the selection-specific method (clipboard.ts). Withlinux_clipboard_selection: "primary", the regular clipboard is still written via OSC52 whenever the terminal supports it (tmux, kitty, wezterm...). OSC52 also cannot target the primary buffer. "primary" mode is therefore not primary-only. Suggest skipping OSC52 for"primary"(or threadingselectioninto it). -
Formatting: the PR introduces Prettier violations in all 4 modified source files. The repo uses
prettier(printWidth 120, semi: false) and basedevis clean, but this PR failsbunx prettier --checkonclipboard.ts(xsel ternary,copyMethodtype),clipboard.tsx(138-charClipboardProvidersignature),config/index.tsx(ResolvedOmit), andclipboard.test.ts(longtoEqualarrays). CI only runstsgotypecheck so this will not block, but it deviates from repo convention — runbunx prettier --writeon the changed files. -
Union types duplicated in three files; exported
ClipboardSelectionis unused."clipboard" | "primary" | "both"appears inclipboard.ts,config/index.tsx(Resolved), andclipboard.tsx(ClipboardConfig);copyCommanduses an inline"clipboard" | "primary"instead of the exportedClipboardSelection. Define one shared type and reuse it. -
Non-Linux platforms silently ignore the selection. The osascript (macOS) and PowerShell (Windows) branches do not read
selection, soprimary/bothdegrade to clipboard-only writes there. Defensible given thelinux_scope, but worth documenting (or gating the option inresolve()). -
Minor:
props.linuxClipboardSelection!non-null assertion inClipboardProvideris avoidable; PR history has 2 merge commits + "trigger title check" (squash on merge); PR body says "Closes #43176" while the duplicate-PR comment references #29963 — keep the linked issue consistent.
Note (not a blocker)
- Runtime
write()routing ("both"/allSettled) has no test coverage — acceptable givenspawnmocking cost, but a natural follow-up.
|
Addressed all review comments:
|
|
Verified the changes against the latest head (
One item is not actually done:
|
|
Fixes applied. Prettier formatting now complete. Commit cb7bf05 updates all 4 files. |
|
Confirmed — all review items are now closed. LGTM from the review side. Good to merge. |
There was a problem hiding this comment.
PR #32370 — feat(tui): add linux_clipboard_selection config for primary buffer support
Verdict: Request changes (medium)
The previous round's asks are all closed and verified on the current head. The remaining gaps: the selection contract is write-only, a silent no-op failure mode claims success, and the option is missing from tests and docs.
What is good (verified on head 2fdfe1c9b2)
- All five items from the last review round are done. OSC52 is skipped for
"primary"(thewrite()guard), prettier is clean on all 5 changed files (re-ranbunx prettier --check),ClipboardSelectionis unified, the non-null assertion is gone, and the title is conventional. - The verification claims hold.
bun typecheckis clean and all 196 TUI tests pass (195 pass / 1 skip). - Config pipeline integration is sound. The key flows through
TuiConfig.Info→resolve()at both call sites, a typo fails loudly like any other key, no committed schema artifact needs regeneration (thetui.jsonschema is generated fromInfoat web build time), andtui-migratecorrectly needs no change. - All write paths route through the context service (
useClipboard), so selection routing applies everywhere — nothing bypasses it with a directwriteimport.
Issues
-
The selection contract is write-only:
read()ignores the selection (most substantive).ClipboardProvideronly wrapswritewith the selection;readis passed through bare. The defaultctrl+v(input_paste) routes toprompt.paste→clipboard.read?.(), andread()only ever reads the clipboard buffer (wl-pastedefault,xclip -selection clipboard,clipboardy). With"primary", every opencode copy lands in the primary buffer only, then opencode's own paste reads the stale clipboard. Middle-click itself is fine (the TUI has no middle-click handler; with mouse capture off the terminal pastes natively from primary), so the gap is specifically opencode's ownctrl+vpath. Suggest makingreadselection-aware (wl-paste --primary/xclip -selection primaryfor"primary"), or explicitly documenting that the paste keybind is unaffected by the setting. -
Silent no-op and unconditional success toast.
write()never rejects (.catch(() => undefined)everywhere,Promise.allSettledfor"both"), and the clipboardy fallback doesif (selection === "primary") return— a silent no-op. Both resolve "successfully" into the "Copied to clipboard" toast, so a failed or skipped copy claims success. At minimum the fallback should fall back to the clipboard instead of no-op'ing, or the failure should surface. -
Config tests missing in
config.test.tsx. That file already has the pattern: "validates config constraints" decodesInfoand asserts invalid values throw (e.g.cursor: { style: "beam" }), and "resolves host-neutral defaults" asserts defaults likemouse: true. Nothing coverslinux_clipboard_selection— no valid-value decode, no invalid-value throw, noresolve({}).linux_clipboard_selection === "both". A few lines in an existing file. -
Docs:
linux_clipboard_selectionis missing from the TUI options reference.tui.mdxdocumentsdiff_style,cursor,mouse,attentionand the rest; this option appears nowhere in the repo's docs. Every user-facing option is listed there. -
Non-Linux platforms silently ignore the selection (previous item #4, still open). The osascript branch ignores it; the win32
copyCommandbranch returns the same PowerShell command for every selection, so"primary"writes the normal clipboard and"both"spawns the identical command twice in parallel. Defensible given thelinux_scope, but it's still undocumented and un-gated.
Note (not a blocker)
- The
"both"default is a silent behavior change for all Linux users (copies now also write the primary selection). Fine, but the PR body frames it as a new option — one sentence about the default changing existing copy behavior would help, alongside calling out thewl-copy --type text/plainfix. copyCommandaccepts"both"but implicitly behaves as"clipboard"—Exclude<ClipboardSelection, "both">would make the contract honest.Resolvedalso still inlines the union instead of importingClipboardSelection(previous item #3 half-closed).- PR body: "Also includes a new
cursorconfig" —Cursoralready exists ondev; stale from an earlier iteration. - History still carries 3 merge commits + "trigger title check" — squash on merge.
- No test exercises the OSC52 guard or the
both/allSettledpath — acceptable follow-up as previously noted.
|
cannot wait for this to be merged in. |
|
All addressed in
Notes addressed as well:
Verified: |
Issue for this PR
Closes #43176
Type of change
What does this PR do?
Adds Linux clipboard selection support with a new
linux_clipboard_selectionconfig option (tui.json):clipboard: Standard clipboard only (Ctrl+V)primary: Targets the middle-click buffer; TUI paste reads from it tooboth: Updates both buffers simultaneously (default)With the default
both, copies on Linux now also write the primary selection — this extends existing copy behavior rather than replacing it.Also fixes a Linux clipboard bug found while testing this: text copied via
wl-copywould not paste into other applications becausewl-copyoffers piped content asapplication/octet-stream(its MIME auto-detection runs on an extensionless temp file). The fix passes an explicit--type text/plain;charset=utf-8.The option is Linux-only; on other platforms the regular clipboard is always used.
How did you verify your code works?
I select text and the selection goes to the configured destination - Clipboard / Primary / Both (default). Verified against
wl-pasteandwl-paste -pon a Wayland session,bun typecheck, and the TUI test suite.Checklist
_If you do not follow this template your PR will be automatically rejected.