Skip to content

refactor: strip ANSI via ansi_strip() directly, dropping local helpers - #3035

Merged
max-sixty merged 1 commit into
mainfrom
consolidate-ansi-strip
Jun 11, 2026
Merged

refactor: strip ANSI via ansi_strip() directly, dropping local helpers#3035
max-sixty merged 1 commit into
mainfrom
consolidate-ansi-strip

Conversation

@max-sixty

Copy link
Copy Markdown
Owner

A simplification review found three local ANSI-stripping implementations: identical strip_ansi_codes wrappers in help.rs and diagnostic.rs, and a hand-rolled CSI-only escape parser in the list/collect test module. All three call sites now use ansi_str::AnsiStr::ansi_strip() directly, the idiom already used at the ~45 other stripping sites in the codebase. Net +12/−52, no new API.

The hand-rolled parser handled only CSI sequences; ansi_strip covers the full escape grammar. The unchanged inline snapshots in the collect tests confirm identical output for the strings those tests produce. In help.rs, keeping the returned Cow borrowed also drops a per-line String allocation in the help-text scanning loop.

An earlier iteration added a shared styling::strip_ansi_codes helper instead; it was cut in favor of the trait method since the crate already is the canonical implementation, and a wrapper would have left the codebase with two spellings of the same operation.

This was written by Claude Code on behalf of max

Replace three local implementations (identical strip_ansi_codes wrappers
in help.rs and diagnostic.rs, a hand-rolled CSI-only parser in the
list/collect test module) with direct ansi_str::AnsiStr::ansi_strip()
calls, the idiom already used throughout the codebase. The borrowed Cow
also drops a per-line allocation in help.rs's help-text scanning loop.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@max-sixty
max-sixty merged commit 5f62157 into main Jun 11, 2026
36 checks passed
@max-sixty
max-sixty deleted the consolidate-ansi-strip branch June 11, 2026 15:01
max-sixty added a commit that referenced this pull request Jun 11, 2026
…es (#3039)

A CI monitor watching PR #3035 polled the combined-status API (`gh api
.../commits/<sha>/status`) for `codecov/patch` and concluded codecov had
stopped posting on PR heads. Investigation showed nothing is broken:
codecov's GitHub App posts **check runs** on PR head commits (commit
statuses appear only on main pushes), and check runs never show up in
the legacy status API. This has been the case since at least April
(oldest surviving PR head, #1743). The check run also lands several
minutes after the `code-coverage` job finishes (~9 min on #3035).

This adds one sentence to the Coverage section so merge gating polls `gh
pr checks` rather than the status API.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

> _This was written by Claude Code on behalf of max_

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
max-sixty added a commit that referenced this pull request Jun 12, 2026
The CI column in `wt list --full` (and the statusline) previously showed
a single colored dot. It now shows the branch's open PR/MR reference —
`#3035` on GitHub/Gitea/Azure DevOps, `!3035` on GitLab — colored by CI
status, dimmed when stale, and hyperlinked to the PR. When no number is
available (branch workflows without a PR/MR, pre-number cache entries,
or a number wider than the allocated column), the cell shows a bare `#`
in the same colors. Fetch errors always render `⚠`, even when a number
is known — Error and Conflicts share yellow, so a yellow `#3035` would
read as a conflicted PR.

The branch merges main's review-state feature (#3044): review colors
(magenta/cyan) and draft dimming apply to the number cells exactly as
they did to the dot, and the `--help` legend shows colored `#` samples
for all seven states (the interim version had dropped the colored
samples from the legend entirely).

## The width problem

`wt list` renders skeleton-first: column widths are fixed before any CI
data arrives, and the table never resizes mid-render. The PR number's
width therefore has to be known up front. The solution is a repo-level
ratchet cache (`.git/wt/cache/pr-number/max.json`) holding the largest
PR number any fetch has seen — PR numbers are monotonic per repo, so the
value needs no invalidation. Pre-skeleton, `collect` reads that one file
and sizes the column exactly; on a cold cache the estimate is 5 chars
(`#9999`). A number that outgrows the estimate renders as the bare `#`
for that run and sizes correctly on the next run once the ratchet
records it. The ratchet is deliberately separate from the per-branch
`ci-status/` entries so the width hint isn't coupled to branch-entry
retention, and `detect` re-ratchets on cache hits too, so a deleted or
racily regressed `max.json` heals from locally cached numbers instead of
waiting out the TTL.

## Reviewer's map

- `src/commands/list/ci_status/mod.rs` — `PrRef` (number + forge sigil,
`PrRef::pr`/`PrRef::mr` constructors), `PrStatus.number` (serde-default
so pre-existing cache entries still deserialize, rendering `#` until
their 30–60s TTL expires), `format_cell` width-aware renderer with the
Error guard, ratchet in `detect` (both cache-hit and fetch paths)
- `src/commands/list/ci_status/cache.rs` — `MaxPrNumber` ratchet
(read/ratchet/clear)
- `src/commands/list/ci_status/{github,gitlab,gitea,azure}.rs` — each
fetcher populates the number (`gh --json number`, `iid`, Gitea `number`,
`pullRequestId`); GitLab's mr-view-failure path carries the
iid/URL/review state into the error status so the `⚠` stays clickable
- `src/commands/list/layout.rs`, `collect/mod.rs` — width estimate
threading
- `src/commands/list/render.rs`, `model/item.rs` — table cell and
statusline both go through `format_cell`
- `src/commands/list/json_output.rs` — `ci.number` field
- `src/commands/config/state.rs` — ratchet shown by `state get`/`cache
get` (table + JSON) and swept with the CI cache category, including the
deprecated `ci-status clear --all` path
- `src/md_help.rs`, `src/help.rs` — legend colorization rules rewritten
from `●` to `#` (terminal + website)

Most of the diff is snapshot churn from the column width and glyph
changes plus regenerated docs mirrors.

Known trade-offs: concurrent statusline ratchet writes can transiently
lose an update (monotonic, re-learns on the next render, documented at
the write site); one anomalously high PR number widens the column until
`wt config state cache clear`; an open Azure DevOps PR still shows gray
`NoCI` instead of its pipeline status — a pre-existing gap, now marked
`TODO(azure-pr-pipeline)`.

Testing: unit tests for `format_cell` (including the Error-with-number
and oversized-number link cases)/`pr_ref_width`/ratchet/width estimates;
integration coverage for all four forges with real numbers (the Gitea
mocks now exercise the number path too), review-state × number
composition, the GitLab mr-view-failure `⚠` and branch-pipeline success
paths, cache-TTL expiry → refetch, the statusline number view, and the
`wt config state` surfaces.

> _This was written by Claude Code on behalf of max_

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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