feat(ui): Add y shortcut to yank selection to clipboard#262
Conversation
Press y to copy the active terminal text selection to the clipboard. Uses pbcopy on macOS and falls back to OSC52 elsewhere. The action also appears in the Navigate menu and the help dialog. Routes the acknowledgement through a new useTransientNotice channel so the yank message and the startup update notice share one status-bar slot with one dismiss timer instead of competing.
Greptile SummaryThis PR adds a
Confidence Score: 5/5Safe to merge. The yank feature is self-contained, the clipboard fallback chain is correct, and the notice-channel refactor is well-isolated in AppHost. All changed paths are well-tested end-to-end (unit tests for clipboard, integration test for the full yank flow). The useTransientNotice refactor cleanly centralises timer ownership without changing observable behaviour for either the yank notice or the startup update notice. No data loss or correctness risk was found in the new code. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "feat(ui): add y shortcut to yank selecti..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (key.name === "y" || key.sequence === "y") { | ||
| yankSelection(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
handlePagerShortcut ends with an explicit return. The y handler omits it, which is harmless today because it is the last branch, but will silently fall through if a future case is appended below it.
| if (key.name === "y" || key.sequence === "y") { | |
| yankSelection(); | |
| } | |
| }; | |
| if (key.name === "y" || key.sequence === "y") { | |
| yankSelection(); | |
| return; | |
| } | |
| }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/hooks/useAppKeyboardShortcuts.ts
Line: 183-186
Comment:
Every other branch in `handlePagerShortcut` ends with an explicit `return`. The `y` handler omits it, which is harmless today because it is the last branch, but will silently fall through if a future case is appended below it.
```suggestion
if (key.name === "y" || key.sequence === "y") {
yankSelection();
return;
}
};
```
How can I resolve this? If you propose a fix, please make it concise.| const result = spawnSync("pbcopy", [], { | ||
| input: text, | ||
| stdio: ["pipe", "ignore", "ignore"], | ||
| }); |
There was a problem hiding this comment.
spawnSync has no timeout option, so if pbcopy ever stalls (e.g. macOS clipboard service unresponsive), the call blocks the Node.js main thread indefinitely, freezing the terminal UI with no escape path. A short timeout lets the OSC52 fallback kick in instead.
| const result = spawnSync("pbcopy", [], { | |
| input: text, | |
| stdio: ["pipe", "ignore", "ignore"], | |
| }); | |
| const result = spawnSync("pbcopy", [], { | |
| input: text, | |
| stdio: ["pipe", "ignore", "ignore"], | |
| timeout: 2000, | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/lib/clipboard.ts
Line: 36-39
Comment:
`spawnSync` has no `timeout` option, so if `pbcopy` ever stalls (e.g. macOS clipboard service unresponsive), the call blocks the Node.js main thread indefinitely, freezing the terminal UI with no escape path. A short timeout lets the OSC52 fallback kick in instead.
```suggestion
const result = spawnSync("pbcopy", [], {
input: text,
stdio: ["pipe", "ignore", "ignore"],
timeout: 2000,
});
```
How can I resolve this? If you propose a fix, please make it concise.
|
Hey can you help me understand the use case? With iTerm or Ghostty I just drag-select and copy using the native terminal. |
I find it very annoying that Cmd+C does not work. What's the point of allowing text selection if it's not possible to copy it? |
Okay sounds like we just need to fix that then. Maybe related to #312. |
|
Closing this as superseded by the copy-selection work that landed in #306. The original copying gap tracked in #312 is now closed: diff/pager views support mouse-drag selection and clipboard copy via OSC 52, with copy-decoration handling in the current render path. A future keyboard yank command could still be proposed separately, but this branch predates the current copy-selection implementation and would need a fresh design on top of it. This comment was generated by Pi using OpenAI GPT-5 |
|
Reopening — #306 fixed mouse-drag copy, but it did not add the keyboard This comment was generated by Pi using OpenAI GPT-5 |
3 participants
Context
Press
yto copy the active terminal text selection to the clipboard. Usespbcopyon macOS and falls back to OSC52 elsewhere. Action also surfaces in the Navigate menu and the help dialog.What was changed
ykeybinding wired throughuseAppKeyboardShortcuts, the Navigate menu, and the help dialogsrc/ui/lib/clipboard.tswithyankActiveSelection(selection read + copy + status mapping) andcopyTextToClipboard(pbcopy → OSC52 fallback)useTransientNoticehook hoisted toAppHostso yank acknowledgements and the startup update notice share one status-bar slot and one dismiss timer instead of competinguseStartupUpdateNoticerefactored to publish through the shared channelOut of scope