Skip to content

fix(picker): stash collect warnings until skim releases the terminal - #2627

Merged
max-sixty merged 3 commits into
mainfrom
picker-suppress-warnings
May 7, 2026
Merged

max-sixty merged 3 commits into
mainfrom
picker-suppress-warnings

Conversation

@max-sixty

@max-sixty max-sixty commented May 7, 2026 •

Copy link
Copy Markdown
Owner

Summary

collect::collect emits warnings on stderr (stale default branch, batch-fetch failure, drain timeout, per-row task errors). On the wt list path that's fine. On wt switch, collect runs on a background thread while skim's TUI owns the terminal — eprintln overlays the rendered frame and corrupts skim's clear math, leaving fragments visible after the user picks.

Reproducer (synthetic picker-test repo with a stale worktrunk.default-branch set):

▲ Configured default branch ghost-branch does not exist locally
↳ To reset, run wt config state default-branch clear

…appears overlaid on picker rows mid-render.

Approach

Warnings flow through a new PickerProgressHandler::stash_warning. The picker holds an Arc<Mutex<Vec<String>>> shared with its handler, collect appends from the bg thread, and the picker drains and emits the lines after Skim::run_with returns (and in the dry-run path after the bg thread joins). Late warnings still in flight on the bg thread fall on the floor with the thread, per the existing "don't join after skim" rule.

wt list's stderr behavior is unchanged — when progressive_handler is None, the same closure writes straight to stderr.

The drain-timeout warning + hint that previously hardcoded wt list is now subcommand-agnostic and follows writing-user-outputs patterns: "Listing worktrees timed out after Xs", command at end of clause, semicolon between alternatives, -vv last.

Test infrastructure

Three small extractions made the new code testable end-to-end and brought patch coverage up from 66.7% to 100%:

  • drain_stashed_warnings(&Mutex<Vec<String>>) in picker/mod.rs — both drain call sites collapse to one line; helper body has dedicated unit tests.
  • format_drain_timeout_diag(received_count, &items) in collect/mod.rs — pure formatter; snapshot-tested for the no-blocked and blocked-items paths.
  • handle_drain_timeout(drain_outcome, collect_deadline, &emit) in collect/mod.rs — wraps the previously-untestable DrainOutcome::TimedOut branch (DRAIN_TIMEOUT is 120s with no test seam). Three unit tests synthesize DrainOutcome values directly to cover all branches: timeout-fires, intentional-truncation, complete-outcome.

Plus a new integration test in switch_picker_dry_run.rs that runs the picker in dry-run mode against a stale worktrunk.default-branch and asserts the warning + reset hint reach stderr after the bg thread joins.

Test plan

  • cargo run -- hook pre-merge --yes — 3508 tests pass, pre-commit clean (8 new tests across the helpers above).
  • wt list warning snapshot tests still pass — non-picker stderr unchanged.
  • Manual repro: WORKTRUNK_PICKER_DRY_RUN=1 wt switch --no-cd against picker-test with a stale default branch now surfaces both warning lines on stderr after the picker exits.

This was written by Claude Code on behalf of @max-sixty

`collect::collect` emits four kinds of warnings on stderr (stale default
branch, batch-fetch failure, drain timeout, per-row task errors). On the
`wt list` path, stderr is fine. On the `wt switch` (picker) path, collect
runs on a background thread while skim's TUI owns the terminal — every
eprintln overlays the rendered frame and corrupts skim's clear math, leaving
fragments visible after the user picks.

Pipe warnings through a new `PickerProgressHandler::stash_warning` instead.
The picker holds an `Arc<Mutex<Vec<String>>>` shared with its handler,
collect appends to it from the bg thread, and the picker drains and emits
the lines after `Skim::run_with` returns (and in the dry-run path after the
bg thread joins). Late warnings still in flight on the bg thread fall on
the floor with the thread, per the existing "don't join after skim" rule.

`wt list`'s stderr behavior is unchanged — when `progressive_handler` is
`None`, the same closure writes straight to stderr.

While in the file, generalize the timeout warning + hint that previously
hardcoded `wt list` (`-v` is a global flag and the warning now also fires
under `wt switch`):

- "wt list timed out after Xs" → "Listing worktrees timed out after Xs"
- "run `wt list -v` for details or `wt list -vv` ..." restructured per
  `writing-user-outputs` guidance: command at end of clause, semicolon
  between alternatives, `-vv` last (more useful for sharing a diagnostic).

Co-Authored-By: Claude <noreply@anthropic.com>
worktrunk-bot
worktrunk-bot previously approved these changes May 7, 2026

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

codecov/patch failed at 66.7%. The misses split into three groups:

src/commands/list/collect/mod.rs:641-642 — the picker-handler branch of the new emit_warning closure (h.stash_warning(line)). The non-picker branch on line 643 is covered, but no test runs collect::collect with progressive_handler: Some(...) and a warning trigger. This is the most reachable gap — a focused unit test that wires a mock PickerProgressHandler through collect with a stale-default-branch repo would cover it.

src/commands/list/collect/mod.rs:1419-1424 — the drain-timeout warning emission (Listing worktrees timed out after Xs + the -v/-vv hint). DRAIN_TIMEOUT is 120s with no test seam, so this path was already untested on main; codecov flags it now because the lines were rewritten from eprintln! to emit_warning(...) (new lines from its perspective).

src/commands/picker/mod.rs:672, 693 — the dry-run and post-skim drain loops. Picker integration tests use WORKTRUNK_PICKER_DRY_RUN=1 and don't trigger any warning-producing condition during collect, so the loop body never executes. Difficult to cover without a heavier picker harness.

Dismissing approval per CLAUDE.md (codecov/patch is mandatory). Happy to push a unit test for the first gap if useful.

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

codecov/patch failed at 66.7%. The misses split into three groups:

src/commands/list/collect/mod.rs:641-642 — the picker-handler branch of the new emit_warning closure (h.stash_warning(line)). The non-picker branch on line 643 is covered, but no test runs collect::collect with progressive_handler: Some(...) and a warning trigger. This is the most reachable gap — a focused unit test that wires a mock PickerProgressHandler through collect with a stale-default-branch repo would cover it.

src/commands/list/collect/mod.rs:1419-1424 — the drain-timeout warning emission (Listing worktrees timed out after Xs + the -v/-vv hint). DRAIN_TIMEOUT is 120s with no test seam, so this path was already untested on main; codecov flags it now because the lines were rewritten from eprintln! to emit_warning(...) (new lines from its perspective).

src/commands/picker/mod.rs:672, 693 — the dry-run and post-skim drain loops. Picker integration tests use WORKTRUNK_PICKER_DRY_RUN=1 and don't trigger any warning-producing condition during collect, so the loop body never executes. Difficult to cover without a heavier picker harness.

Dismissing approval per CLAUDE.md (codecov/patch is mandatory). Happy to push a unit test for the first gap if useful.

@worktrunk-bot
worktrunk-bot dismissed their stale review May 7, 2026 02:46

codecov/patch failed — see follow-up review for analysis

Patch coverage was 77% on #2627, with three uncovered groups all in code
the PR introduced or rewrote. Address the two reachable groups directly
and refactor the third (drain-timeout warning) so its body becomes
testable.

- Extract `drain_stashed_warnings` from the dry-run and post-skim drain
  loops in `picker/mod.rs`. Both call sites now share one line; the
  helper body has dedicated unit tests (empty stash, populated stash).
- Extract `format_drain_timeout_diag` from collect's drain-timeout
  branch. The text construction (the bulk of the previously uncovered
  block) is now a pure function with two snapshot tests covering the
  no-blocked-items and blocked-items paths. The `if collect_deadline …
  let DrainOutcome::TimedOut { … }` block remains uncovered (no test
  seam for the 120s `DRAIN_TIMEOUT`), but it shrinks to ~10 lines.
- Add an integration test in `switch_picker_dry_run.rs` that runs the
  picker against a TestRepo with a stale `worktrunk.default-branch`.
  Asserts the stale-default warning + reset hint reach stderr — covers
  `emit_warning`'s `h.stash_warning(line)` branch in collect and the
  dry-run drain call site in picker/mod.rs end-to-end.

3505 tests pass (3 new); pre-commit clean.

Co-Authored-By: Claude <noreply@anthropic.com>
worktrunk-bot
worktrunk-bot previously approved these changes May 7, 2026

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

codecov/patch failed at 92.85% (5 misses out of 70 changed lines). All remaining uncovered lines are the call site of format_drain_timeout_diag inside the DrainOutcome::TimedOut branch (src/commands/list/collect/mod.rs:1428-1434):

1428: let diag = format_drain_timeout_diag(received_count, &items_with_missing);
1429: emit_warning(warning_message(&diag).to_string());
1430: emit_warning(
1432:     "A git command likely hung; for details, re-run with -v; ..."
1434:     .to_string(),

The pure function itself is now covered by the two new snapshot tests; the gap is the same pre-existing untestable path the prior review flagged — DRAIN_TIMEOUT is 120s with no seam to fire the branch synthetically. Picker (drain_stashed_warnings, stash_warning) and the picker-handler branch of emit_warning are at 100%.

Dismissing my approval per CLAUDE.md (codecov/patch is mandatory). Options to clear the check: refactor the drain-timeout block into a helper that takes drain_outcome + emit_warning and unit-test it against a synthetic DrainOutcome::TimedOut, or accept the gap and merge with explicit approval.

@worktrunk-bot
worktrunk-bot dismissed their stale review May 7, 2026 03:20

codecov/patch failed at 92.85% — 5 lines uncovered in the drain-timeout call site

… helper

Take-2 left codecov/patch at 95.09%, blocked by the 5 lines inside the
`if collect_deadline.is_none() && let DrainOutcome::TimedOut { .. } = drain_outcome`
block in `collect::collect`. There's no test seam for the 120s
`DRAIN_TIMEOUT`, so the live drain never enters this branch under
test.

Extract the whole branch into `handle_drain_timeout(drain_outcome,
collect_deadline, &emit_warning)` and unit-test it with synthetic
`DrainOutcome` values. Three tests cover the branches:

- `TimedOut + collect_deadline=None` → emits warning + hint
- `TimedOut + collect_deadline=Some(_)` → silent (intentional truncation)
- `Complete + collect_deadline=None` → silent

`emit` is taken as `&dyn Fn(String)` so the test passes a capturing
closure that stashes lines into a `Mutex<Vec<String>>` instead of
touching real stderr.

Co-Authored-By: Claude <noreply@anthropic.com>
@max-sixty
max-sixty merged commit 6b41433 into main May 7, 2026
28 checks passed
@max-sixty
max-sixty deleted the picker-suppress-warnings branch May 7, 2026 03:39
@max-sixty max-sixty mentioned this pull request May 10, 2026
3 tasks done
max-sixty added a commit that referenced this pull request May 10, 2026
Bump worktrunk to **0.49.0** — minor bump (semver-checks reports a
breaking change in `GitError::UncommittedChanges`, which gained a
`dirty_files: Vec<String>` field).

## Highlights

**Improved**
- New `codename(n)` template filter — deterministic friendly worktree
names from a ~1.26M-combo pool (`{{ branch | codename(2) }}` →
`malleable-opah`).
[#2641](#2641), thanks
@endigma
- Picker preview disk cache for Log / BranchDiff / UpstreamDiff, with
background refresh for stale Log decorations.
[#2628](#2628),
[#2646](#2646)
- Picker shares one `default_branch_sha` lookup across BranchDiff items
instead of forking N rev-parse subprocesses.
[#2658](#2658)
- "X has uncommitted changes" errors now list the dirty files inline.
[#2653](#2653)

**Fixed**
- `wt switch` integrates with `cd` aliases like zoxide (uses `builtin
cd`). [#2644](#2644), thanks
@xkumiyu for reporting
- Empty hook tables no longer panic during `wt switch --create`.
[#2635](#2635), thanks @topit
for reporting
- `Repository::current_worktree()` and five sibling helpers no longer
silently leak the process CWD when called via `Repository::at(p)`.
[#2652](#2652)
- `WorkingTree::is_linked` tolerates non-git CWDs (e.g. Nix sandbox).
[#2625](#2625), thanks
@DArtagan for reporting
- Picker no longer overlays `collect` warnings on the active TUI; clears
its frame on exit in inline mode.
[#2627](#2627),
[#2626](#2626)
- Failed `git log` no longer poisons the picker's preview disk cache.
[#2651](#2651)
- `wt step prune` says "removing branch" for branch-only candidates.
[#2619](#2619)
- Claude Code Windows integration uses a `wt.sh` wrapper to avoid
colliding with Windows Terminal's `wt.exe`.
[#1754](#1754), thanks
@lucaspimentel

**Documentation**
- cmux workspace integration recipe.
[#1907](#1907), thanks
@alvistar

See
[CHANGELOG.md](https://github.com/max-sixty/worktrunk/blob/release/CHANGELOG.md)
for the full list including internal CI / test-isolation changes.

## Test plan

- [x] `cargo run -- hook pre-merge --yes` (tests + lints clean)
- [x] `cargo semver-checks check-release -p worktrunk` (one breaking
change confirmed → minor bump)
- [x] CHANGELOG entries verified by subagent against actual diffs

🤖 Generated with [Claude Code](https://claude.com/claude-code)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants