fix(ui): improve clipboard reliability across terminals - #320
Conversation
Add a clipboard adapter that uses OSC 52 with native command fallbacks and Apple Terminal preference for more consistent copy behavior. Route selection copy paths through the adapter and handle Cmd+C like Ctrl+C when text is selected. Assistant-model: openai/gpt-5.3-codex
PR Review: fix(ui): improve clipboard copy reliability across terminalsThanks for this PR! The clipboard adapter implementation is well-structured and addresses real cross-terminal compatibility issues. Here's my review: ✅ Strengths
🔧 Suggestions1. Clipboard ref initialization pattern (chat.tsx:2258-2262)The current pattern works but could be cleaner using a lazy ref pattern: // Current
const clipboardRef = useRef<ClipboardAdapter | null>(null);
if (!clipboardRef.current) {
clipboardRef.current = createClipboardAdapter(renderer);
}
const clipboard = clipboardRef.current;
// Suggested (idiomatic lazy ref)
const clipboard = useMemo(() => createClipboardAdapter(renderer), [renderer]);Since 2. Comment mentions VS Code but doesn't detect it (clipboard.ts:108-110)The comment for // Comment says: "macOS Terminal.app, VS Code terminal"
// But code only checks:
return termProgram === "apple_terminal";Consider either:
3. Minor: Extra blank lines addedThere are two extra blank lines that look accidental:
🧪 Test Coverage GapsConsider adding tests for:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a platform-aware clipboard adapter to improve copy reliability in the TUI across terminals by preferring OSC 52 when available and falling back to native clipboard commands when needed, then routes selection/copy paths through that adapter.
Changes:
- Added
ClipboardAdapterimplementation with OSC 52 primary strategy and native-command fallbacks (pbcopy/wl-copy/xclip/xsel), including an Apple Terminal preference. - Updated
ChatAppcopy-on-selection and explicit copy handling to use the adapter; expanded key handling to treat Cmd+C similarly to Ctrl+C. - Added Bun tests for strategy resolution and fallback behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/ui/utils/clipboard.ts | New clipboard adapter with OSC 52 + native command strategy selection and fallback chaining. |
| src/ui/utils/clipboard.test.ts | New tests covering adapter behavior and resolution logic (currently includes OS-dependent assumptions). |
| src/ui/index.ts | Updated renderer/mouse tracking comment to better describe selection/copy behavior. |
| src/ui/chat.tsx | Routes selection copy and Ctrl/Cmd+C copy paths through the adapter and adjusts key handling. |
Comments suppressed due to low confidence (1)
src/ui/chat.tsx:6246
- This change makes Cmd+C (
event.meta) go through the same interrupt/exit path as Ctrl+C whenever there is no selection. The PR description only mentions treating Cmd+C like Ctrl+C when text is selected; as written, a habitual Cmd+C can now abort/exit even when nothing is selected. Consider gating theevent.metahandling to only the copy-with-selection branch (and otherwise ignore/meta-pass-through), while keeping Ctrl+C semantics unchanged.
// Ctrl+C / Cmd+C handling must work everywhere (even in dialogs) for double-press exit
if ((event.ctrl || event.meta) && event.name === "c") {
const textarea = textareaRef.current;
// If textarea or renderer has selection and no dialog is active, copy instead of interrupt/exit
const hasRendererSelection = !!renderer.getSelection()?.getSelectedText();
if (!activeQuestion && !showModelSelector && (textarea?.hasSelection() || hasRendererSelection)) {
void handleCopy();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }); | ||
|
|
||
| afterEach(() => { | ||
| process.env.TERM_PROGRAM = originalTermProgram; |
There was a problem hiding this comment.
In afterEach, assigning process.env.TERM_PROGRAM = originalTermProgram will set the env var to the string "undefined" when it was originally unset (common in Node/Bun). This can leak state into later tests. Restore by delete process.env.TERM_PROGRAM when originalTermProgram === undefined, otherwise set it back to the original value (same pattern used in src/utils/detect.test.ts).
| process.env.TERM_PROGRAM = originalTermProgram; | |
| if (originalTermProgram === undefined) { | |
| delete process.env.TERM_PROGRAM; | |
| } else { | |
| process.env.TERM_PROGRAM = originalTermProgram; | |
| } |
| test("falls back to native clipboard when OSC 52 write fails", () => { | ||
| const renderer = makeMockRenderer({ osc52Supported: true, copyResult: false }); | ||
| const whichSpy = spyOn(Bun, "which").mockImplementation((cmd: string) => { | ||
| if (cmd === "pbcopy") return "/usr/bin/pbcopy" as ReturnType<typeof Bun.which>; | ||
| return null as ReturnType<typeof Bun.which>; | ||
| }); | ||
| const spawnSpy = spyOn(Bun, "spawnSync").mockReturnValue({ | ||
| success: true, | ||
| } as ReturnType<typeof Bun.spawnSync>); | ||
|
|
||
| const adapter = createClipboardAdapter(renderer); | ||
| const result = adapter.copy("hello"); | ||
|
|
||
| expect(result).toBe(true); | ||
| expect(renderer.copyToClipboardOSC52).toHaveBeenCalledWith("hello"); | ||
| expect(spawnSpy).toHaveBeenCalled(); | ||
|
|
There was a problem hiding this comment.
This test hard-codes the macOS-only pbcopy native command. CI runs on ubuntu-latest, where detectNativeClipboardCommand() never checks pbcopy, so the native strategy will be null and spawnSync won't be called—making this test fail/flaky across platforms. Adjust the mock to return a native command that matches process.platform (e.g. wl-copy/xclip on linux), or refactor detectNativeClipboardCommand/platform detection to be injectable so the test can force a deterministic command.
| test("prefers native clipboard on Apple Terminal even if OSC 52 is reported", () => { | ||
| process.env.TERM_PROGRAM = "Apple_Terminal"; | ||
|
|
||
| const renderer = makeMockRenderer({ osc52Supported: true, copyResult: true }); | ||
| const whichSpy = spyOn(Bun, "which").mockImplementation((cmd: string) => { | ||
| if (cmd === "pbcopy") return "/usr/bin/pbcopy" as ReturnType<typeof Bun.which>; | ||
| return null as ReturnType<typeof Bun.which>; | ||
| }); | ||
| const spawnSpy = spyOn(Bun, "spawnSync").mockReturnValue({ | ||
| success: true, | ||
| } as ReturnType<typeof Bun.spawnSync>); | ||
|
|
||
| const adapter = createClipboardAdapter(renderer); | ||
| const result = adapter.copy("hello"); | ||
|
|
||
| expect(result).toBe(true); | ||
| expect(spawnSpy).toHaveBeenCalled(); | ||
| expect((renderer.copyToClipboardOSC52 as ReturnType<typeof mock>).mock.calls.length).toBe(0); |
There was a problem hiding this comment.
Same issue as above: this test assumes pbcopy is the native clipboard command even though the suite runs on Linux in CI. On non-darwin platforms detectNativeClipboardCommand() won’t select pbcopy, so spawnSync will not be invoked and the expectations will fail. Make the command mocked by Bun.which conditional on process.platform (linux: wl-copy/xclip/xsel, darwin: pbcopy) or inject platform/command resolution so the test can force the native strategy.
| const adapter = createClipboardAdapter(renderer); | ||
|
|
||
| // On macOS (our test platform), this will attempt pbcopy as the native fallback. | ||
| // The copy result depends on platform availability, but OSC 52 should NOT be the | ||
| // primary path. We verify by checking that the adapter was created successfully | ||
| // and doesn't throw. | ||
| const result = adapter.copy("hello"); | ||
| // On macOS with pbcopy available, this should succeed via native fallback | ||
| if (process.platform === "darwin") { | ||
| expect(result).toBe(true); | ||
| } | ||
| // Regardless of platform, the adapter should not throw | ||
| expect(typeof result).toBe("boolean"); |
There was a problem hiding this comment.
This test name claims "does not call OSC 52 as primary", but it never asserts whether copyToClipboardOSC52 was called and it relies on whatever native tools happen to exist on the host. To actually validate the resolution logic deterministically, mock Bun.which/Bun.spawnSync to force a native strategy on the current platform, then assert that renderer.copyToClipboardOSC52 is not called (or only called as fallback) when osc52Supported is false.
| const adapter = createClipboardAdapter(renderer); | |
| // On macOS (our test platform), this will attempt pbcopy as the native fallback. | |
| // The copy result depends on platform availability, but OSC 52 should NOT be the | |
| // primary path. We verify by checking that the adapter was created successfully | |
| // and doesn't throw. | |
| const result = adapter.copy("hello"); | |
| // On macOS with pbcopy available, this should succeed via native fallback | |
| if (process.platform === "darwin") { | |
| expect(result).toBe(true); | |
| } | |
| // Regardless of platform, the adapter should not throw | |
| expect(typeof result).toBe("boolean"); | |
| // Force native clipboard strategy resolution deterministically by mocking | |
| // Bun.which / Bun.spawnSync, so the test does not depend on host tools. | |
| const whichSpy = spyOn(Bun, "which").mockReturnValue("/usr/bin/fake-clipboard" as any); | |
| const spawnSpy = spyOn(Bun, "spawnSync").mockReturnValue({ | |
| success: true, | |
| exitCode: 0, | |
| stdout: new TextEncoder().encode(""), | |
| stderr: new TextEncoder().encode(""), | |
| } as any); | |
| const adapter = createClipboardAdapter(renderer); | |
| const result = adapter.copy("hello"); | |
| // Native path should be used as the primary strategy when OSC 52 is not supported. | |
| expect(result).toBe(true); | |
| expect(whichSpy.mock.calls.length).toBeGreaterThan(0); | |
| expect(spawnSpy.mock.calls.length).toBeGreaterThan(0); | |
| // OSC 52 must not be used as the primary copy mechanism in this scenario. | |
| expect( | |
| (renderer.copyToClipboardOSC52 as ReturnType<typeof mock>).mock.calls.length | |
| ).toBe(0); | |
| whichSpy.mockRestore(); | |
| spawnSpy.mockRestore(); |
Rely on renderer OSC 52 capability checks for the primary copy path and keep a narrow pbcopy fallback for macOS. This removes custom multi-platform strategy resolution while preserving Apple Terminal reliability via native copy first. Assistant-model: openai/gpt-5.3-codex
PR Review: fix(ui): improve clipboard copy reliability across terminalsThanks for this PR! The Strategy pattern approach for clipboard operations is well-designed and the lazy initialization is a nice touch. Here's my feedback: ✅ Strengths
|
PR Review: fix(ui): improve clipboard copy reliability across terminalsOverall, this is a well-structured PR with clean implementation and excellent test coverage. The Strategy pattern is applied appropriately, and the code leverages OpenTUI's existing APIs rather than reinventing the wheel. ✅ StrengthsCode Quality
Test Coverage
Design Decisions
🔧 Minor Suggestions1. Extra blank lines in
Consider removing these to keep formatting consistent. 2. Test assertion style expect((renderer.isOsc52Supported as ReturnType<typeof mock>).mock.calls.length).toBe(1);Consider using expect(renderer.isOsc52Supported).toHaveBeenCalledTimes(1);3. Consider testing the pbcopy-fails-then-OSC52 fallback path test("falls back to OSC 52 when pbcopy fails on Apple Terminal", () => {
process.env.TERM_PROGRAM = "Apple_Terminal";
setPlatform("darwin");
// ... mock pbcopy to fail, OSC 52 to succeed
});🔒 SecurityNo concerns - the implementation only writes to clipboard (no reads), and ⚡ PerformanceGood implementation:
📝 Notes
VerdictLGTM 👍 - This PR improves clipboard reliability across terminals with clean, well-tested code. The minor suggestions above are non-blocking. |
Assistant-model: openai/gpt-5.3-codex
PR Review: Clipboard Adapter ImplementationThanks for this PR! The platform-aware clipboard adapter is a solid improvement for cross-terminal compatibility. Here is my review: Strengths
Security Concern
The escaping only handles backslashes and double quotes, but AppleScript has additional special characters that could cause issues (e.g., Recommendation: Consider making Potential Issues1. PR description vs implementation mismatch The PR summary states: "Special handling for Apple Terminal.app (prefers pbcopy over OSC 52 due to observed inconsistencies)" But the code at lines 77-82 actually prefers 2. When bracketed paste contains empty content, this now reads from the clipboard. This changes existing behavior - if a user intentionally pastes empty content, the app now inserts existing clipboard contents instead. Is this intentional? 3. Synchronous clipboard reads could block UI
4. The Test Coverage GapsThe tests are a good start but could be expanded:
Minor Suggestions
Summary
Overall this is a solid improvement. The main item to address is the osascript string escaping security concern - I would recommend using |
claude
Bot
changed the title
* fix(ui): improve clipboard copy reliability across terminals Add a clipboard adapter that uses OSC 52 with native command fallbacks and Apple Terminal preference for more consistent copy behavior. Route selection copy paths through the adapter and handle Cmd+C like Ctrl+C when text is selected. Assistant-model: openai/gpt-5.3-codex * refactor(ui): simplify clipboard adapter around OpenTUI APIs Rely on renderer OSC 52 capability checks for the primary copy path and keep a narrow pbcopy fallback for macOS. This removes custom multi-platform strategy resolution while preserving Apple Terminal reliability via native copy first. Assistant-model: openai/gpt-5.3-codex * fix CI errors * fix(ui): improve paste fallback with native clipboard reads Assistant-model: openai/gpt-5.3-codex
* fix(ui): improve clipboard copy reliability across terminals Add a clipboard adapter that uses OSC 52 with native command fallbacks and Apple Terminal preference for more consistent copy behavior. Route selection copy paths through the adapter and handle Cmd+C like Ctrl+C when text is selected. Assistant-model: openai/gpt-5.3-codex * refactor(ui): simplify clipboard adapter around OpenTUI APIs Rely on renderer OSC 52 capability checks for the primary copy path and keep a narrow pbcopy fallback for macOS. This removes custom multi-platform strategy resolution while preserving Apple Terminal reliability via native copy first. Assistant-model: openai/gpt-5.3-codex * fix CI errors * fix(ui): improve paste fallback with native clipboard reads Assistant-model: openai/gpt-5.3-codex
Summary
Introduces a robust, platform-aware clipboard adapter that improves both copy and paste reliability across different terminal emulators using OSC 52 escape sequences with native command fallbacks.
Key Changes
Core Implementation
src/ui/utils/clipboard.ts)UI Improvements
Testing
Technical Details
The adapter uses a dual-strategy approach:
Both strategies are attempted for copy operations, with automatic fallback if one fails.
Platform Support
User-Facing Changes
Copy Behavior
Paste Behavior
Files Changed
Backwards Compatibility
✅ No breaking changes — Fully backwards compatible with existing clipboard behavior. The adapter transparently enhances reliability without changing the user-facing API.
Testing Instructions