Skip to content

refactor(tui): one viewport representation, delete the line-index anchors - #1543

Draft
zipadoodlez wants to merge 8 commits into
1jehuang:masterfrom
zipadoodlez:epic/1411-phase-6a-one-representation
Draft

zipadoodlez wants to merge 8 commits into
1jehuang:masterfrom
zipadoodlez:epic/1411-phase-6a-one-representation

Conversation

@zipadoodlez

@zipadoodlez zipadoodlez commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Epic #1411, phase 6a. Draft: tentatively stacked on #1477, #1526 and #1429.

The chat viewport kept the reader's position in four places at once: a stored
scroll_offset, an auto_scroll_paused flag, a distance-from-bottom
HistoryScrollAnchor for a prepend, and a PendingResizeAnchor for a rewrap.
Every pair of them had to be reconciled whenever the transcript moved, and the
guards that reconciled them were themselves a source of bugs.

This stores one thing instead:

follow: Option<ContentPos>,   // None = live tail, Some = anchored to content

ContentPos names an item id and a row, which survives a rewrap, a prepend and a
compaction because it is content, not a row index. The render path resolves it
against the frame it is drawing and stores nothing back.

Both anchors existed only to compensate for the stored row offset, so they go
with it:

  • a resize needs no capture, because a content position is width-independent;
  • a prepend needs no anchor, because the item id cannot move.

The upward overshoot that fed the prepend anchor is dropped with it (see the
ponytail: note in maybe_queue_compacted_history_load); the content anchor
already keeps the view steady across the load.

The motion telemetry goes too

anchor_stability.rs plus ui_smoothness.rs (~810 production lines) watched the
viewport for jarring motion: content repositioning away from its anchor,
insertions above, single-frame pops, blinks, mass reflows. Three reasons it
belongs in this phase and not a later one:

  • Its input is the state this change deletes: each frame is fed
    app.scroll_offset() and !app.auto_scroll_paused(), i.e. exactly the two
    fields 6a removes.
  • It cannot do its job. Phase 1 established that a rewrap changes every row hash,
    so a correct reflow and a jump are indistinguishable to it — the mass-reflow
    signal fires on precisely the case it was built to catch.
  • Every phase since has been verified by a behavioural regression test instead,
    so what remains is a debug surface measuring a projection of state that no
    longer exists.

The smoothness and smoothness:reset debug commands, and
smoothness_benchmark.rs, go with it.

Supporting changes

  • content_pos_at_row / resolve_content_pos now treat a section prepared
    without a wrap map (the padded header, inline images, batch progress) as one
    raw line per rendered row, so those rows can hold an anchor instead of being
    silently unmappable.
  • message_row_ranges returns an iterator rather than allocating a Vec of
    every message boundary, because the position is now resolved every frame.

Why it is stacked. 6a deletes the state that #1477 (6b), #1526 (5c) and
#1429 (5b) each convert, so its base has to contain all three. Please land those
first. Until they do, this PR's diff carries their commits; it rebases to a
single commit on master once they land.

Size: 2512 deletions against 520 insertions, net -1992 (production -1084,
tests -908) in the single 6a commit.

Tests. resize_anchor.rs and smoothness_benchmark.rs tested the removed
machinery and are deleted. The invariant resize_anchor.rs covered now lives in
scroll_copy_03: a_prepend_keeps_the_reader_on_the_same_content and
a_resize_keeps_the_reader_on_the_same_content assert the anchored content stays
under the reader across a prepend and a rewrap (including a round trip back to
the original width).

The rest of the migration is the encoding change: the old scroll_offset meant
distance from the bottom while following the tail and an absolute row while
parked, so assertions like scroll_offset == 0 for "at the bottom" become
!follow.is_some(), and the up/down comparisons flip. Tests that parked the
viewport before the first render now draw once first, because a content anchor
needs a frame to name a row; those with no terminal set the follow target
directly (park_scroll).

Validation:

  • cargo fmt --all --check clean.
  • cargo clippy --profile selfdev -p jcode-tui-core -p jcode-tui-messages -p jcode-tui --all-targets -- -D warnings clean.
  • cargo test -p jcode-tui --lib -- --test-threads=1: 2421 passed, 0 failed.
  • cargo test -p jcode-tui-core: 48 passed. cargo test -p jcode-tui-messages: 18 passed.

Under the default parallel harness a handful of model-picker, session-restore and
info-widget tests fail. Those are pre-existing: the same tests fail the same way
on this branch's base (5f2e66adb), including a login test that dies on a DNS
error, and all of them pass single-threaded.

Deliberately kept, not oversights: scroll_offset() and auto_scroll_paused()
remain on TuiState as derived reads (34 call sites in metrics/debug/diff would
otherwise move), and last_total_wrapped_lines stays a published per-frame read
rather than being recomputed from the frame.

…rame

Step 2: a Transcript wrapper owns App.display_messages with a parallel id
vector. push/pop/remove/insert/retain/clear/replace keep the two vectors in
lockstep, reads go through Deref<Target = [DisplayMessage]>, and there is no
DerefMut so a desync is a compile error. replace preserves ids across a
prepend or append and mints fresh ones only for the changed middle.

Step 3: MessageBoundary names its message by ItemId instead of the content
hash. The body builder stamps the id at the single segment push site, the
two reuse splices copy it, and matching_prefix_len/matching_suffix_len
compare ids, which removes the stale-splice hazard on duplicate content.

TuiState::display_message_item_id supplies the id; states that do not track
identity (test harnesses, synthetic frames) fall back to the content hash.
…rdinal

Anchor becomes { item_id, row_within_item }. Gone: the occurrence field, the
HashMap counting in anchor_at_row, the seen counter in resolve, the .0 seam
into the boundary, and the module-doc paragraph about the ordinal ceiling.
message_row_ranges now yields ItemId.

The ordinal is unreachable by construction now: transcript ids are unique, so
a position names exactly one message and a prepended duplicate cannot rename
it. The duplicate-occurrence test is deleted with the mechanism it covered.
Strengthen resize_then_prepend_of_a_duplicate_does_not_teleport_the_reader:
it only asserted the reader did not move up. It now also asserts the message
at the top of the viewport is unchanged, which is the outcome that matters.

Add an_identical_prepend_keeps_the_readers_text_and_number: the prepended
message is word-for-word identical to the anchored one, the case where content
cannot say which copy the reader was on. Identity answers it; the reader keeps
the same message. The displayed prompt number is not part of that claim: the
prepend path resolves by distance from the bottom (phase 4), so a revealed
prompt renumbers 1> to 2>. Recorded in the test, not fixed here.
Three review findings on 1jehuang#1477, each with a regression test.

1. Edited messages showed stale text. `MessageBoundary` had dropped its
   content hash for `item_id`, so `matching_prefix_len` matched identity
   alone. Several paths edit content in place and keep the id
   (`replace_display_message_content`, the repeat coalescer, maintenance
   cards, streaming), so the id-only prefix matched fully and
   `build_body_from_base` returned the pre-edit body verbatim. Carry
   `msg_hash` alongside `item_id` again and have both matchers compare the
   pair, so a same-id content change breaks the prefix and re-renders the
   tail.

2. A prepended duplicate stole the original's id. `Transcript::replace`
   matched prefix-first and capped the suffix at `limit - prefix`, so when
   older history began with a message identical to the first visible one,
   the copy took the original's id and the real message was renumbered. A
   pending resize anchor then resolved to the copy above it. When the whole
   old transcript is a suffix of the new one (a prepend, not an append),
   keep the tail's ids and mint only the head.

3. A resize could consume a pending history anchor. `reconcile_history_anchor`
   treated any change in the wrapped-line total as evidence that prepended
   history had rendered, but a resize rewraps and changes that total too. If
   a remote history request was in flight, the resize resolved and dropped
   the anchor, and the later prepend snapped the reader to the top. Require
   the transcript to have actually grown (`base_msg_count`) before resolving.

Tests: matching_prefix_len_stops_at_a_same_id_content_edit,
prepend_of_a_duplicate_keeps_the_originals_id,
a_resize_does_not_resolve_a_pending_history_anchor.
Selection endpoints are wrapped line indices, so a rewrap reinterprets them and
the selection silently starts covering different text: a drag over
TOKEN026..TOKEN028 at 100 columns copied a stray wrapped fragment plus TOKEN018
after resizing to 60.

Capture the endpoints in content coordinates in `commit_resize_redraw` (the
stable transcript item id + row, plus the display column measured from the start
of the message rather than the wrapped row), and re-base them onto the frame
drawn at the new width. Columns measured per row would clamp away the tail of a
message that split in two, which is why the column is kept relative to the
message.

Built on the stable item ids from 1jehuang#1477 (phase 6b), so a prepended or edited
message cannot move a selection endpoint and the occurrence ordinal never enters
the selection code. The PR is stacked on 1jehuang#1477; this rebases to a single commit
on master once that lands.

Only the transcript pane is rebased: the input, side-panel, diff, and pinned
panes are not transcript-relative.

Fails-first: without the capture,
`transcript_selection_covers_the_same_text_after_a_resize` fails on the changed
copy. 212 tests pass across the copy/scroll/selection cluster.

Epic 1jehuang#1411 phase 5c.
Greptile found the resize rebase put the endpoints in two coordinate systems:
capture measured a column by summing the wrapped rows' display widths, which
include a markdown list's continuation indent, while the copy path extracts
through `wrapped_copy_offsets` and `WrappedLineMap`. After a rewrap the two
disagreed and a selection silently slid onto neighbouring characters:
`TARGETALPHA` at 100 columns copied `d TARGETALP` at 60.

Capture and resolve now use the frame's raw (unwrapped) coordinates, the space
extraction already uses, so a rewrap cannot move an endpoint. Raw text is
width-independent, so an endpoint in a section with no message boundary (the
live stream) is captured too, instead of abandoning both endpoints for want of
an anchor.

This also fixes the selection fixture, which still collected directly into
`display_messages` after it became the `Transcript` container and so kept the
library test target from compiling.

Adds a wrapped-list regression test.

Epic 1jehuang#1411 phase 5c.
The bookmark held a wrapped line index, so setting it, resizing, and returning
sent the reader to a different message: the index was captured at the old width
and replayed against the new one. Store an `Anchor` instead and resolve it
against the frame being drawn when returning.

Measured: with the old index behavior, a bookmark set on `TOKEN016` at 100
columns returns to `TOKEN011` at 60 columns (stored index 52). The new test
asserts the same message comes back.

An unresolvable bookmark (message pruned or compacted away) now reports that and
leaves the viewport where it is, instead of replaying a stale index.

Epic 1jehuang#1411 phase 5b.
…hors

Epic 1jehuang#1411 phase 6a.

The chat viewport kept the reader's position in four places at once: a stored
`scroll_offset`, an `auto_scroll_paused` flag, a distance-from-bottom
`HistoryScrollAnchor` for a prepend, and a `PendingResizeAnchor` for a rewrap.
Every pair had to be reconciled whenever the transcript moved, and the guards
that reconciled them were themselves a source of bugs.

Store one thing instead: `App.follow` is `Option<ContentPos>`. `None` follows
the live tail; `Some(pos)` names an item id and a row, which survives a rewrap,
a prepend and a compaction because it is content, not a row index. The render
path resolves it against the frame it is drawing and stores nothing.

Both anchors existed only to compensate for the stored row offset, so they go:

- a resize needs no capture, because a content position is width-independent;
- a prepend needs no anchor, because the item id cannot move.

The motion telemetry goes with them. The anchor-stability recorder
(`anchor_stability.rs` plus `ui_smoothness.rs`, ~810 production lines) watched
the viewport for jarring motion, but its input was the two fields this change
deletes (`scroll_offset`, `following_tail`), and phase 1 established that it
cannot in principle separate a correct reflow from a jump: a rewrap changes
every row hash, so what it flags as a mass reflow is exactly the case it was
built to detect. Every phase since has been verified by a behavioural
regression test instead, so the recorder was a debug surface measuring a
projection of state that no longer exists. The `smoothness` and
`smoothness:reset` debug commands go with it.

The upward overshoot that fed the prepend anchor is dropped too (see the
`ponytail:` note); the content anchor already keeps the view steady.

Supporting changes:

- `content_pos_at_row` and `resolve_content_pos` treat a section prepared
  without a wrap map (the padded header, inline images, batch progress) as one
  raw line per rendered row, so those rows can hold an anchor.
- `message_row_ranges` returns an iterator rather than allocating a Vec, because
  the position is now resolved every frame.

Tests: `resize_anchor.rs` and `smoothness_benchmark.rs` tested the removed
machinery and are deleted. The invariant `resize_anchor.rs` covered now lives in
`scroll_copy_03`'s prepend/resize content tests. The rest of the migration is the
encoding change: `scroll_offset == 0` meant "at the bottom" while tailing, which
is now `!follow.is_some()`. Tests that parked the viewport before the first
render now draw once first, because a content anchor needs a frame to name a
row; those with no terminal set the follow target directly (`park_scroll`). The
trait keeps `scroll_offset()` and `auto_scroll_paused()` as derived reads so the
metrics, debug and diff call sites do not move.

Epic 1jehuang#1411 phase 6a.
@zipadoodlez
zipadoodlez force-pushed the epic/1411-phase-6a-one-representation branch from cd8e895 to b667dfa Compare September 27, 2026 08:58
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.

1 participant