docs(list): correct main_state priority order in field docstring - #3531
Merged
Conversation
The StatusSymbols.main_state field docstring listed the priority as IsMain > Orphan > WouldConflict > Empty > SameCommit > Integrated > …, which contradicts the canonical MainState order in state.rs (Empty and Integrated both outrank WouldConflict) and this file's own type-level docstring. Correct it to match: IsMain > Orphan > Empty > Integrated > WouldConflict > SameCommit > Diverged > Ahead > Behind.
Collaborator
Author
|
The I didn't rerun it from here: |
Owner
|
please fix flake |
Collaborator
Author
|
Opened #3532 to fix it. The |
max-sixty
pushed a commit
that referenced
this pull request
Jul 25, 2026
#3532) ## Problem `test_switch_picker_prs_shows_loading_marker` intermittently times out on `test (windows)` — it flaked on #3531 (a docs-only change), which is what prompted this fix. The test asserts a *transient* frame: while a mocked, delayed `gh pr list` is in flight, the picker header shows `↳ Loading open PRs…` and the `#42` row has not yet streamed in. The mock held that loading state for a fixed time (originally 3s). On a loaded Windows runner, the PTY boot sequence — `boot_picker_pty`'s skim-ready wait plus its initial `wait_for_stable`, together with the async worktree-column churn (`·` placeholders resolving) — can consume that whole window. By the time the stabilization helper first polls for `Loading open PRs`, the mocked fetch has already returned and the rows have streamed in, so the marker is gone and the wait times out at `STABILIZE_TIMEOUT` (30s). ## Fix Per review feedback ([comment](#3532 (comment))), tie the marker's lifetime to the picker's, with **no timing number at all** — the file's "poll, don't pick a time" rule applied to the producer side. `mock-stub` gains a `hold_until_parent_exit` response mode: the mocked `gh pr list` holds until `wt` exits instead of sleeping a fixed `delay_ms`. Detection needs no platform primitive (`getppid` / `OpenProcess`): - While `wt` lives, its `--prs` fetch thread runs the mock with a piped stdout and blocks reading that pipe to EOF, so as long as the mock neither writes a full response nor exits, the loading marker stays. - When the picker aborts, `wt` detaches the fetch thread (`drop(prs_handle)`) and exits, which closes the read end of the mock's stdout pipe. The mock's next write then fails with `BrokenPipe` (Rust ignores `SIGPIPE`, so the broken write surfaces as an `Err` rather than killing the process), and the mock exits. Polling for that write error is a **causal** parent-death signal — release is tied to `wt` exiting, not to a timer — and it behaves identically on Unix and Windows, so there's no platform-specific FFI to get right. The one-byte probes written into stdout are never observed (this mode's contract is that the parent aborts without parsing the response, and the fetch thread drains the pipe until process exit). A 60s poll cap bounds a stuck orphan if the pipe somehow never breaks. This removes the race the earlier 30s bump only widened: the marker is present for the entire window the poll could observe it, on any boot latency, and no orphaned sleeper is left behind — the mock exits the moment the picker does. ## Testing `cargo test --test integration --features shell-integration-tests test_switch_picker_prs_shows_loading_marker` passes in ~1.3s, confirming the capture-and-abort path stays fast and the mock releases promptly on `wt` exit. The sibling `--prs` tests (`test_switch_picker_prs_{github,gitlab}_list`, `..._rows_survive_alt_x_removal`) still pass through the refactored `mock_forge_env` helper. Reasoning is documented inline at the mock-stub call site and in `wait_for_parent_exit`. --------- Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com>
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
2 participants
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.
Problem
The
StatusSymbols.main_statefield docstring insrc/commands/list/model/status_symbols.rslisted the priority as:This contradicts the canonical order documented on the
MainStateenum instate.rs, whereEmptyandIntegratedboth outrankWouldConflict(a branch already integrated into the default branch is safe to delete regardless of a vacuous re-merge conflict). It also disagreed with this same file's own type-level docstring (^ > _ > ⊂ > ✗ > – > …).A maintainer reading the field doc to reason about an already-integrated branch with a vacuous merge-tree conflict would expect
✗, whereas the code correctly renders⊂.Fix
Documentation only — corrects the field docstring to match the canonical order (
IsMain > Orphan > Empty > Integrated > WouldConflict > SameCommit > Diverged > Ahead > Behind) and points atMainStateinstate.rsas the source of truth. No behavior change, so no test.Found during the nightly code-quality survey.