Add TUI yank-to-clipboard support - #9
Merged
Conversation
📝 WalkthroughWalkthroughAdds system clipboard support and a TUI "y" yank action. Introduces a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TUI as TUI Event Loop
participant Clipboard as LinkClipboard
participant Helper as OS Clipboard Helper<br/>(pbcopy/wl-copy/clip)
participant Kernel as System Clipboard
User->>TUI: Press 'y'
TUI->>TUI: map → EventResult::YankSelected
TUI->>Clipboard: copy_text(url)
Clipboard->>Helper: spawn helper process
Clipboard->>Helper: write URL to stdin
Helper->>Kernel: place text on system clipboard
Helper-->>Clipboard: exit or remain running
Clipboard-->>TUI: return Result
TUI->>User: show "Copied URL for '{link}'"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/clipboard.rs (1)
188-225: Tests duplicate production candidate logic.
linux_candidatesin tests mirrors implementation logic, which can let regressions slip if both change together. Prefer testingclipboard_command_candidates()via a small injectable helper (e.g., a purelinux_candidates_from_flags(...)used by both).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/clipboard.rs` around lines 188 - 225, The test duplicates the production logic in linux_candidates, so extract the decision logic into a pure helper (e.g., linux_candidates_from_flags(wayland: bool, x11: bool) -> Vec<ClipboardCommand>) and have the existing linux_candidates call that helper; update clipboard_command_candidates() (or whatever production entrypoint) to use the helper and make tests call linux_candidates_from_flags instead of reimplementing the rules, ensuring a single source of truth for candidate generation.README.md (1)
46-47: Clarify Linux clipboard prerequisites.Consider explicitly stating that at least one of
wl-copy,xclip, orxselmust be installed; otherwise yank will fail with an error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 46 - 47, Update the README TUI clipboard note to explicitly state that plinks requires at least one of the clipboard utilities (wl-copy, xclip, or xsel) to be installed on Linux for the "y" yank/copy action to work; mention that if none are present the yank will fail with an error and optionally suggest installing one of them (e.g., via the system package manager). Include the utility names (wl-copy, xclip, xsel) and the behavior (yank will fail if none installed) so users know the prerequisite.src/tui/mod.rs (1)
261-265: Consider renamingtake()tocopied()for clarity.The method returns a clone rather than draining the vector. While this works for the current tests (single assertion per test), the name
take()suggests ownership transfer. A name likecopied()orget_copied()would better reflect the behavior.♻️ Suggested rename
impl RecordingClipboard { - fn take(&self) -> Vec<String> { + fn copied(&self) -> Vec<String> { self.copied.borrow().clone() } }And similarly for
RecordingOpener:impl RecordingOpener { - fn take(&self) -> Vec<String> { + fn opened(&self) -> Vec<String> { self.opened.borrow().clone() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/mod.rs` around lines 261 - 265, The method RecordingClipboard::take currently returns a cloned Vec<String> but its name implies it drains/consumes the data; rename it to something like copied() or get_copied() to reflect that it clones rather than transfers ownership, and update all call sites accordingly; do the same for RecordingOpener (rename its take method to copied() or get_copied()) and adjust references to these methods to match the new names to preserve behavior and clarity.tests/tui.rs (1)
50-53: Consider increasing delays for CI stability.The 300ms and 500ms sleeps before sending
yandqmay be tight on slower CI runners or under load. If flakiness is observed, consider increasing these or adding retry logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tui.rs` around lines 50 - 53, Increase the timing margins around the interactive test writes to avoid CI flakiness by lengthening the sleeps or adding retry/wait logic: locate the calls in tests/tui.rs where thread::sleep(Duration::from_millis(300)) and thread::sleep(Duration::from_millis(500)) occur before writer.write_all(b"y").unwrap() and writer.write_all(b"q").unwrap(), and either bump those durations (e.g., to 500–1000ms) or replace them with a small loop that retries/waits for readiness before calling writer.write_all on the writer variable to ensure stable input delivery on slow CI runners.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/clipboard.rs`:
- Around line 85-89: The timeout branch returns early and drops the spawned
clipboard Child without reaping it, causing zombies; before returning Ok(())
when Instant::now() >= deadline, take ownership of the Child (the process handle
created for clipboard helpers) and spawn a background thread to call
child.wait() (or child.wait().ok()) so the OS reaps the process, then detach
that thread and return; reference the existing deadline variable and the Child
instance used when launching xclip/wl-copy and ignore the wait result/errors in
the background thread.
---
Nitpick comments:
In `@README.md`:
- Around line 46-47: Update the README TUI clipboard note to explicitly state
that plinks requires at least one of the clipboard utilities (wl-copy, xclip, or
xsel) to be installed on Linux for the "y" yank/copy action to work; mention
that if none are present the yank will fail with an error and optionally suggest
installing one of them (e.g., via the system package manager). Include the
utility names (wl-copy, xclip, xsel) and the behavior (yank will fail if none
installed) so users know the prerequisite.
In `@src/clipboard.rs`:
- Around line 188-225: The test duplicates the production logic in
linux_candidates, so extract the decision logic into a pure helper (e.g.,
linux_candidates_from_flags(wayland: bool, x11: bool) -> Vec<ClipboardCommand>)
and have the existing linux_candidates call that helper; update
clipboard_command_candidates() (or whatever production entrypoint) to use the
helper and make tests call linux_candidates_from_flags instead of reimplementing
the rules, ensuring a single source of truth for candidate generation.
In `@src/tui/mod.rs`:
- Around line 261-265: The method RecordingClipboard::take currently returns a
cloned Vec<String> but its name implies it drains/consumes the data; rename it
to something like copied() or get_copied() to reflect that it clones rather than
transfers ownership, and update all call sites accordingly; do the same for
RecordingOpener (rename its take method to copied() or get_copied()) and adjust
references to these methods to match the new names to preserve behavior and
clarity.
In `@tests/tui.rs`:
- Around line 50-53: Increase the timing margins around the interactive test
writes to avoid CI flakiness by lengthening the sleeps or adding retry/wait
logic: locate the calls in tests/tui.rs where
thread::sleep(Duration::from_millis(300)) and
thread::sleep(Duration::from_millis(500)) occur before
writer.write_all(b"y").unwrap() and writer.write_all(b"q").unwrap(), and either
bump those durations (e.g., to 500–1000ms) or replace them with a small loop
that retries/waits for readiness before calling writer.write_all on the writer
variable to ensure stable input delivery on slow CI runners.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b1f6117-3cc1-4baa-a4a5-a76837feaba2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
AGENTS.mdCargo.tomlREADME.mdsrc/clipboard.rssrc/lib.rssrc/main.rssrc/tui/events.rssrc/tui/mod.rssrc/tui/view.rstests/cli.rstests/tui.rs
- add a system clipboard abstraction with platform-specific implementations - bind the y key to copy the selected link URL - document the feature and extend CLI/TUI test coverage
adambuchweitz
force-pushed
the
feature/tui-clipboard-yank
branch
from
April 23, 2026 15:42
1ec5288 to
4da0625
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/clipboard.rs (1)
228-249: Add a test for the Wayland+X11 simultaneous case.
linux_candidates_from_flags(true, true)currently has no direct test, so ordering betweenwl-copy,xclip, andxselcould regress silently.Proposed test
+ #[cfg(all(unix, not(target_os = "macos")))] + #[test] + fn prefers_wayland_then_x11_when_both_session_hints_are_present() { + let candidates = linux_candidates_from_flags(true, true); + assert_eq!( + candidates, + vec![ + ClipboardCommand { + program: "wl-copy", + args: &[], + }, + ClipboardCommand { + program: "xclip", + args: &["-selection", "clipboard", "-in"], + }, + ClipboardCommand { + program: "xsel", + args: &["--clipboard", "--input"], + } + ] + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/clipboard.rs` around lines 228 - 249, Add a unit test that verifies linux_candidates_from_flags(true, true) returns the expected ordered candidates (Wayland first then X11 tools) to prevent regressions: create a test (e.g., falls_back_to_wayland_then_x11_when_both_true) with the same #[cfg(all(unix, not(target_os = "macos")))] attribute as the existing test, call linux_candidates_from_flags(true, true), and assert equality against a vec of ClipboardCommand entries in the exact order: program "wl-copy" with empty args, then "xclip" with args ["-selection","clipboard","-in"], then "xsel" with args ["--clipboard","--input"] so ordering is enforced.src/tui/mod.rs (1)
125-146: Consider extracting shared selected-entry resolution.
open_selectedandyank_selectedduplicate the same lookup/error path; a small helper would reduce drift risk.Refactor sketch
+fn selected_entry<'a>(app: &'a App) -> Result<(String, &'a crate::config::Link)> { + let primary = app.selected_primary().context("No link selected")?; + let entry = app + .config + .links + .get(&primary) + .with_context(|| format!("selected link '{primary}' no longer exists"))?; + Ok((primary, entry)) +} + fn open_selected(app: &mut App, opener: &dyn LinkOpener) -> Result<()> { - let primary = app.selected_primary().context("No link selected")?; - let entry = app - .config - .links - .get(&primary) - .with_context(|| format!("selected link '{primary}' no longer exists"))?; + let (primary, entry) = selected_entry(app)?; opener.open(&entry.url)?; app.set_info(format!("Opened '{primary}'")); Ok(()) } fn yank_selected(app: &mut App, clipboard: &dyn LinkClipboard) -> Result<()> { - let primary = app.selected_primary().context("No link selected")?; - let entry = app - .config - .links - .get(&primary) - .with_context(|| format!("selected link '{primary}' no longer exists"))?; + let (primary, entry) = selected_entry(app)?; clipboard.copy_text(&entry.url)?; app.set_info(format!("Copied URL for '{primary}'")); Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/mod.rs` around lines 125 - 146, Extract the duplicated selection-and-lookup logic from open_selected and yank_selected into a small helper (e.g. a function named resolve_selected_entry or get_selected_entry) that takes &App and returns Result<&LinkEntry> or Result<(PrimaryKey, &LinkEntry)> with the same error context ("No link selected" and "selected link '{primary}' no longer exists"); update open_selected to call this helper then call opener.open(&entry.url) and app.set_info(...) and update yank_selected to call the same helper then clipboard.copy_text(&entry.url) and app.set_info(...), preserving all existing error messages and Result propagation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/clipboard.rs`:
- Around line 228-249: Add a unit test that verifies
linux_candidates_from_flags(true, true) returns the expected ordered candidates
(Wayland first then X11 tools) to prevent regressions: create a test (e.g.,
falls_back_to_wayland_then_x11_when_both_true) with the same #[cfg(all(unix,
not(target_os = "macos")))] attribute as the existing test, call
linux_candidates_from_flags(true, true), and assert equality against a vec of
ClipboardCommand entries in the exact order: program "wl-copy" with empty args,
then "xclip" with args ["-selection","clipboard","-in"], then "xsel" with args
["--clipboard","--input"] so ordering is enforced.
In `@src/tui/mod.rs`:
- Around line 125-146: Extract the duplicated selection-and-lookup logic from
open_selected and yank_selected into a small helper (e.g. a function named
resolve_selected_entry or get_selected_entry) that takes &App and returns
Result<&LinkEntry> or Result<(PrimaryKey, &LinkEntry)> with the same error
context ("No link selected" and "selected link '{primary}' no longer exists");
update open_selected to call this helper then call opener.open(&entry.url) and
app.set_info(...) and update yank_selected to call the same helper then
clipboard.copy_text(&entry.url) and app.set_info(...), preserving all existing
error messages and Result propagation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c9f26f3-703f-4111-b8ae-c91bcafaf570
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
AGENTS.mdCargo.tomlREADME.mdsrc/clipboard.rssrc/lib.rssrc/main.rssrc/tui/events.rssrc/tui/mod.rssrc/tui/view.rstests/cli.rstests/tui.rs
✅ Files skipped from review due to trivial changes (4)
- src/tui/view.rs
- Cargo.toml
- README.md
- tests/cli.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- AGENTS.md
- src/tui/events.rs
- src/lib.rs
- tests/tui.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
yaction in the TUI to copy the selected link URL to the system clipboard.0.1.7and refresh dependencies for clipboard support.Testing
cargo test./scripts/run-linters.shyfollowed byqusing a fake clipboard tool on UnixSummary by CodeRabbit
New Features
Documentation
Tests
Chores