feat(switch): picker shortcuts — copy branch, open PR, refresh, remap remove to alt-x#3233
Conversation
…ap remove) Add four keyboard shortcuts to the interactive `wt switch` picker: - alt-y: copy the selected branch name to the system clipboard - alt-o: open the selected row's PR/MR in the browser - alt-r: refresh the worktree list - alt-x: remove the selected worktree (remapped from alt-r) copy and open are native skim Action::Custom callbacks that read the selected row off `App.item_list`, so they run without a reload — no cursor reset and no dropping of `--prs` rows (which aren't in `shared_items`). A side table keyed by each row's `output()` token maps to the row's branch and URL, avoiding unreliable cross-thread downcasting. refresh re-enters the collect pipeline via a new PipelineFactory, extracted so startup and refresh build the pipeline identically. Clipboard (arboard) and browser-launch (open) are new cross-platform deps, gated behind the cli feature; arboard drops its image feature. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reconciles the picker shortcut remap with #3211, #3214, and #3195, which landed on main after this branch forked and rewrote large parts of the same picker files. The substantive conflict was semantic: #3211 ("keep the alt-r picker row when its target isn't actually removed") was written assuming alt-r = remove, but this branch remapped remove to alt-x and made alt-r refresh. Resolution: - Kept #3211's full remove dispatch (removal_will_remove_target, keep_unremovable_row, restore_failed_removal, the stashed_warnings backstop) and bound it to alt-x; alt-r stays the refresh key. - invoke() now routes `refresh` to PipelineFactory::spawn ahead of #3211's remove path; the `restore` reload still falls through to the re-stream path. - PickerCollector gains both this branch's `factory` and #3211's `stashed_warnings`; test helpers share one factory via a new `test_factory`. - Retargeted #3211's docstrings, comments, and tests from alt-r to alt-x wherever they describe the remove action (refresh references stay alt-r), including the PTY test test_switch_picker_alt_x_keeps_unmerged_branch_row. Gate green: 4202 tests pass, clippy and fmt clean, snapshots and docs in sync. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pulls in #3225, #3221, #3222, #3226. Reconciles the picker shortcut remap with the two picker changes: - #3225 ("repaint the picker preview after the alt-r sticky reposition") rewrote drive_alt_r_then_switch to gate on the cursor pointer instead of the preview pane (fixing a preview-timing flake). Took that more robust assertion but kept this branch's alt-x key; retargeted #3225's docstrings from alt-r to alt-x where they describe the remove path (its production Event::RunPreview repaint merged cleanly and serves alt-x removal). - #3226 added alt-h:ignore / alt-l:ignore binds; these merged alongside the alt-x/alt-r/alt-y/alt-o binds with no conflict. Regenerated the alt_l hscroll snapshot: this branch deterministically renders the picker list with the `Remote⇅ CI` column layout (verified stable over 5 runs), matching the identically-set-up test_switch_picker_with_multiple_worktrees snapshot. #3226's `CI Age` capture was the outlier — main already carries both layouts for the same fixture, a pre-existing timing-sensitivity in column selection that this merge surfaces but does not introduce. Gate green: 4205 tests pass, clippy and fmt clean, docs in sync. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drop "Alt-y and Alt-o run in the background — the picker stays open" (implicit for in-picker shortcuts) and the "Discrete actions live on Alt" lead-in (redundant with the keybindings table and the existing "plain digits go to the filter" paragraph). Keeps only the non-obvious Alt-o no-op caveat. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
worktrunk-bot
left a comment
There was a problem hiding this comment.
One edge case on the new alt-y copy, inline. Otherwise this looks solid — the alt-r→alt-x remove remap leaves the destructive primitive (SafeDelete via delete_branch_if_safe) untouched and moves remove to a less-reflexive key, so an old-habit alt-r now refreshes rather than removes, which is the safe direction.
…rees Merge: pulls in #3231, #3223, #3229, #3227, #3157. #3223 (unify preview tabs + dim empty diff tabs) added `comments_fetched` / `local_content_slots` to `PickerHandler`; threaded both through `PipelineFactory::spawn`'s handler construction and combined the `items` import with this branch's shortcut types. Regenerated the `empty_all_local_diffs` preview snapshot (#3223's new snapshot captured the pre-shortcut controls line). Fix (review feedback from worktrunk-bot on #3233): `alt-y` on a detached worktree row copied the literal `"(detached)"` — the `branch_name()` fallback the side table stored. `RowShortcutData.branch` is now `Option<String>`, carried from the row's raw `ListItem.branch` (and `Some(head_branch)` for `--prs` rows); the `alt-y` callback `.and_then`s it, so a detached row no-ops just as `alt-o` no-ops on a row with no PR. Covered by `shortcut_table_branch_is_none_for_detached_worktree`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
codecov/patch is failing on this commit — per CLAUDE.md, patch coverage gates merge. Dismissing until the coverage gap is addressed or merging-with-gap is explicitly approved. See follow-up comment for the breakdown.
worktrunk-bot
left a comment
There was a problem hiding this comment.
codecov/patch is failing on this commit, so I'm dismissing my approval — per CLAUDE.md, patch coverage gates merge even though GitHub marks it non-required, and an empty-body approval over a red patch check reads as a rubber-stamp. codecov/project passes; this is purely the patch (new-lines) gate.
The gap is concentrated in picker/os.rs and picker/mod.rs, almost entirely in the interactive/OS-effect surface you already flagged in the PR description as headless-hostile:
| File | Misses | What's uncovered |
|---|---|---|
picker/os.rs |
9 | all of copy_to_clipboard / open_url |
picker/mod.rs |
43 | the alt-y / alt-o Action::Custom closures, spawn_shortcut, and the refresh → factory.spawn() arm |
picker/prs.rs |
1 | the GitLab arm of spawn_worktree_comments_fetch |
Most of this is genuinely irreducible — os::copy_to_clipboard / os::open_url and the spawn_shortcut thread wrapper can't run in CI, exactly as the description says. But the bulk of the mod.rs miss is the row → branch/URL resolution inside the two closures, which is pure logic that doesn't need a live App:
table.lock().unwrap().get(m.item.output().as_ref()).and_then(|d| d.branch.clone())Lifting that lookup out of each closure into a small free fn (e.g. resolve_shortcut_branch(&table, token) -> Option<String> and the url.resolve() equivalent) would let a unit test cover the part that actually matters — the side ShortcutTable keyed by output() token, and the detached-worktree no-op — leaving only the thin spawn_shortcut(os::…) call uncovered. That's likely enough to clear the patch threshold, and it directly tests the lookup the shortcut_table_branch_is_none_for_detached_worktree test only checks one half of today.
I can push that refactor + tests if you'd like — say the word and I'll add it to the branch. Alternatively, if you'd rather merge with the OS surface deliberately untested, that's your call to make explicitly (the codecov gate exists to force exactly that decision); I just won't leave a silent approval standing over the red check.
Uncovered lines (from the Codecov compare API)
os.rs:copy_to_clipboard+open_urlbodiesmod.rs: thealt-yclosure (.and_then(|d| d.branch.clone())→spawn_shortcut("picker-copy", …)), thealt-oclosure (url.resolve()→spawn_shortcut("picker-open", …)),spawn_shortcutitself, andmatch self.factory.spawn() { Ok(SpawnedPipeline { rx, .. }) => … }in therefreshbranchprs.rs:Some(CiPlatform::GitLab) => RefKind::Mr
…nd closures Addresses codecov/patch on #3233 (worktrunk-bot review). The `alt-y` / `alt-o` `Action::Custom` closures held their table lookup inline, so the row → branch/URL resolution — pure logic that needs no live skim `App` — was uncovered along with the genuinely-untestable OS calls. Lift the lookups into `resolve_shortcut_branch` / `resolve_shortcut_url` free functions and unit-test them across present / absent-field (detached row, no-URL row) / missing-token cases. Only the irreducible OS surface (`os::copy_to_clipboard` / `os::open_url`, the `spawn_shortcut` thread wrapper) stays uncovered — headless CI can't drive a clipboard or browser. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Implemented your suggested refactor in d20aadb — pulled the row → branch/URL lookup out of the
|
worktrunk-bot
left a comment
There was a problem hiding this comment.
The refactor did its job — resolve_shortcut_branch / resolve_shortcut_url are now fully covered (they've dropped out of the patch miss list), so the row → branch/URL lookup the alt-y / alt-o closures delegate to is unit-tested across the present / detached / missing-token cases.
codecov/patch is still red, though, so I'm dismissing my approval again — same reason as before (CLAUDE.md treats patch coverage as a merge gate despite the non-required mark, and an empty-body approval over a red check reads as a rubber-stamp). What's left (42 missed patch lines) is now confined to the surface headless CI genuinely can't drive, exactly as the PR description called out:
| File | Misses | What's uncovered |
|---|---|---|
picker/os.rs |
9 | copy_to_clipboard / open_url — clipboard + browser |
picker/mod.rs |
30 | the alt-o closure body, spawn_shortcut thread wrapper, and the refresh → factory.spawn() arm — all need a live skim App / pipeline |
picker/progressive_handler.rs |
1 | the comments_fetched.get() → return guard in maybe_spawn_comments (async slot state) |
picker/prs.rs |
1 | the GitLab arm of spawn_worktree_comments_fetch — needs a live PreviewOrchestrator + glab |
So this is now the deliberate "merge with the OS/interactive surface untested" call the codecov gate exists to force — your decision to make explicitly. I just won't leave a silent approval standing over the red check. If you'd rather I take a swing at any of the residual (e.g. threading the ci_platform branch in spawn_worktree_comments_fetch through a unit test for the GitLab arm), say the word.
Codecov compare API, pullid=3233
Overall patch: 906 / 948 hits (≈95.6%). Misses by file from api.codecov.io/.../compare/?pullid=3233: os.rs 9, mod.rs 30, progressive_handler.rs 1, prs.rs 1, items.rs 1.
codecov/patch failing on d20aadb — patch coverage gates merge per CLAUDE.md. Residual gap is the irreducible OS/interactive surface (clipboard/browser, keybind closures, GitLab orchestrator arm); merging over it is the author's explicit call.
Adds four keyboard shortcuts to the interactive
wt switchpicker:alt-y— copy the selected branch name to the system clipboardalt-o— open the selected row's PR/MR in the browseralt-r— refresh the worktree list (pick up worktrees created elsewhere)alt-x— remove the selected worktree/branch (remapped fromalt-r)Approach
alt-yandalt-oare native skimAction::Customcallbacks that read the selected row offApp.item_listand run the OS action on a background thread — noreload, so the cursor doesn't reset and--prsrows aren't dropped. A side table keyed by each row'soutput()token maps to the row's branch and URL, which avoids skim's unreliable cross-thread downcasting.alt-rrefresh re-enters the collect pipeline via a newPipelineFactory, extracted so startup and refresh build the pipeline identically; worktrees and branches created outside the session then appear without reopening the picker.The one breaking change is the
alt-r→alt-xremap for remove. It fails safe — an old-habitalt-rnow refreshes rather than removes.Key files
src/commands/picker/mod.rs—PipelineFactory,install_shortcut_keybindings, the refresh branch ininvoke, and the bind blocksrc/commands/picker/os.rs(new) —copy_to_clipboard/open_urlviaarboard+opensrc/commands/picker/items.rs— the row-shortcut side table and the controls linesrc/cli/mod.rs— the keybindings table and help textNew cross-platform dependencies:
arboard(clipboard, with itsimagefeature dropped) andopen(browser launch), both gated behind theclifeature.Testing
Unit tests cover the keybinding wiring, the row → branch/URL resolution, and the refresh/remove dispatch; PTY tests cover the alt-x sticky-cursor reposition and the unmerged-branch-keep path. The OS side effects themselves — the clipboard write and the browser launch — are not asserted (both are headless-hostile in CI), so they want a manual check on a desktop; they work on macOS, with the one documented limitation being X11 without a clipboard manager.