Skip to content

refactor(list): collapse rendering modes into a single RenderTarget - #2469

Merged
max-sixty merged 2 commits into
mainfrom
list-render-unify
Apr 29, 2026
Merged

max-sixty merged 2 commits into
mainfrom
list-render-unify

Conversation

@max-sixty

@max-sixty max-sixty commented Apr 29, 2026 •

Copy link
Copy Markdown
Owner

The wt list entry point carried three independently-resolved booleans (format, show_progress, render_table) plus a RenderMode enum to encode just three real modes: JSON, buffered table, progressive table. This replaces the boolean soup with a RenderTarget enum (Json | Table { progressive: bool }) resolved once from the CLI flags + stdout TTY at the top of handle_list and threaded through to collect::collect.

TableRenderPlan no longer needs its is_tty fallback — RenderTarget factors TTY detection up front, so a progressive table only exists when it's actually going to be used. The Some(t) → progressive finalize, None → buffered render split is now the only path.

RowCache shrinks to its load-bearing role and is then inlined: the has_data bitmap that tells the reveal-tick renderer which rows still need the skeleton vs the full row. The set_result dedup is gone — ProgressiveTable::update_row already dedups internally and the picker handler writes through an idempotent Mutex<String>, so the second layer was wasted work. The remaining helper inlines as a free render_reveal function.

Follow-up commit (in response to auto-review): PickerProgressHandler::on_reveal tightens from Vec<Option<String>> to Vec<String>. After the RowCache removal, render_reveal always returns a real line for every row, so the Option wrapper was vestigial — the trait now describes the actual post-refactor behavior and the call sites drop their unwrap branches.

Net: −85 lines across the touched list/picker files. No observable behavior change — all wt list snapshots are unchanged.

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

The `wt list` entry point carried three independently-resolved booleans
(`format`, `show_progress`, `render_table`) plus a `RenderMode` enum to
encode just three real modes: JSON, buffered table, progressive table.
Replace the boolean soup with a `RenderTarget` enum (`Json` | `Table {
progressive: bool }`) resolved once from the CLI flags + stdout TTY in
`handle_list`, threaded through to `collect::collect`.

`TableRenderPlan` no longer needs the `is_tty` fallback — `RenderTarget`
factors TTY detection in up front, so the progressive table only exists
when it's actually going to be used. The `Some(t)` → `finalize`,
`None` → buffered split is now the only path.

`RowCache` shrinks to its load-bearing role: the `has_data` bitmap that
tells the reveal-tick renderer which rows still need the skeleton vs the
full row. The `set_result` dedup is gone — `ProgressiveTable::update_row`
already dedups internally and the picker handler writes through an
idempotent `Mutex<String>`. The remaining helper inlines as a free
`render_reveal` function.

Net: −76 lines across the four touched list files; no observable
behavior change (snapshot tests untouched).

Co-Authored-By: Claude <noreply@anthropic.com>

@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.

Nice cleanup — the RenderTarget enum reads much better than the boolean trio, and the RowCache removal is a straightforward consequence of the dedup already happening in update_row and the picker slot.

One follow-up that fell out of the refactor — the PickerProgressHandler::on_reveal trait at src/commands/list/collect/mod.rs:351-354 is now stale:

/// Fired at the 200ms reveal deadline. Entry per row: `Some(line)` for
/// rows still at skeleton state (placeholder needs promoting to `·`),
/// `None` for rows that already received real data via `on_update`.
fn on_reveal(&self, rendered: Vec<Option<String>>);

After this refactor, render_reveal always returns Some(line) for every row (skeleton for rows without data, format_list_item_line for rows with data), so the Some/None semantics described here no longer hold and the Option wrapper carries no information.

Worth tightening to fn on_reveal(&self, rendered: Vec<String>); — the picker handler in progressive_handler.rs drops its if let Some(line) = line and writes every slot (idempotent through Mutex<String>), the test at progressive_handler.rs:242 updates to plain strings, and the doc here describes the actual post-refactor behavior. Otherwise the doc should at least be updated — it now describes a None case that production never produces.

After the `RowCache` removal, `render_reveal` always returns a real line
for every row (skeleton renderer for rows with no data, full renderer
for rows with data) — the `Some`/`None` semantics the trait described
no longer exist in production. Tighten `PickerProgressHandler::on_reveal`
to `Vec<String>` and drop the unwrap branches at the call sites; slot
writes are idempotent through `Mutex<String>`, so unconditionally
rewriting every row at reveal time is safe.

Co-Authored-By: Claude <noreply@anthropic.com>
@max-sixty
max-sixty merged commit b19fc72 into main Apr 29, 2026
26 checks passed
@max-sixty
max-sixty deleted the list-render-unify branch April 29, 2026 17:57
max-sixty added a commit that referenced this pull request Apr 29, 2026
…_tty` (#2473)

Two follow-ups from the `RenderTarget` refactor (#2469), both in
`src/commands/list/`.

**Skip per-result render when no consumer needs it.** In
`drain_results`'s `DrainEvent::Result` arm, `format_list_item_line` ran
on every result and was discarded in `RenderTarget::Json` and the
buffered-table path (both have `progressive_state` and
`progressive_handler` as `None`). Those modes build their final output
later — JSON via `handle_list`, buffered table via
`TableRenderPlan::render`. The arm now sets `has_data[item_idx]` first
(still needed by reveal) and returns early when neither consumer is
present.

**Drop `ProgressiveTable.is_tty`.** After `RenderTarget` resolved
progressive mode only on a TTY, the field was always `true` in
production. Tests drive the silent path by skipping `render_skeleton`,
which keeps `rendered` false — that's the only condition the production
code needs. Field, the `stdout().is_terminal()` call in
`new_with_height`, the `self.is_tty &&` conjuncts in `render_skeleton`
and `flush`'s debug_assert, and the `table.is_tty = true;` setup in
`test_dirty_tracking_after_render` are all gone.

Net diff: +16 / −15. The runtime benefit (no wasted
`format_list_item_line` per result in JSON / buffered modes) doesn't
show up in LOC because the gating block is roughly the same size as the
work it skips.

Verified with `cargo run -- hook pre-merge --yes` (3393 tests pass,
lints clean, no snapshot drift).

> _This was written by Claude Code on behalf of @max-sixty_

Co-authored-by: Claude <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