Repository navigation
refactor(tui): one viewport representation, delete the line-index anchors - #1543
Draft
zipadoodlez wants to merge 8 commits into
Draft
zipadoodlez wants to merge 8 commits into
zipadoodlez wants to merge 8 commits into
Conversation
…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
force-pushed
the
epic/1411-phase-6a-one-representation
branch
from
September 27, 2026 08:58
cd8e895 to
b667dfa
Compare
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
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.
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, anauto_scroll_pausedflag, a distance-from-bottomHistoryScrollAnchorfor a prepend, and aPendingResizeAnchorfor 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:
ContentPosnames an item id and a row, which survives a rewrap, a prepend and acompaction 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:
The upward overshoot that fed the prepend anchor is dropped with it (see the
ponytail:note inmaybe_queue_compacted_history_load); the content anchoralready keeps the view steady across the load.
The motion telemetry goes too
anchor_stability.rsplusui_smoothness.rs(~810 production lines) watched theviewport 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:
app.scroll_offset()and!app.auto_scroll_paused(), i.e. exactly the twofields 6a removes.
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.
so what remains is a debug surface measuring a projection of state that no
longer exists.
The
smoothnessandsmoothness:resetdebug commands, andsmoothness_benchmark.rs, go with it.Supporting changes
content_pos_at_row/resolve_content_posnow treat a section preparedwithout 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_rangesreturns an iterator rather than allocating aVecofevery 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.rsandsmoothness_benchmark.rstested the removedmachinery and are deleted. The invariant
resize_anchor.rscovered now lives inscroll_copy_03:a_prepend_keeps_the_reader_on_the_same_contentanda_resize_keeps_the_reader_on_the_same_contentassert the anchored content staysunder 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_offsetmeantdistance from the bottom while following the tail and an absolute row while
parked, so assertions like
scroll_offset == 0for "at the bottom" become!follow.is_some(), and the up/down comparisons flip. Tests that parked theviewport 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 --checkclean.cargo clippy --profile selfdev -p jcode-tui-core -p jcode-tui-messages -p jcode-tui --all-targets -- -D warningsclean.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 DNSerror, and all of them pass single-threaded.
Deliberately kept, not oversights:
scroll_offset()andauto_scroll_paused()remain on
TuiStateas derived reads (34 call sites in metrics/debug/diff wouldotherwise move), and
last_total_wrapped_linesstays a published per-frame readrather than being recomputed from the frame.