fix(tui): keep a transcript selection on the same text across a resize - #1526
Open
zipadoodlez wants to merge 6 commits into
Open
zipadoodlez wants to merge 6 commits into
zipadoodlez wants to merge 6 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-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+268
to
+282
| if let (Some((row, column)), Some(mut point)) = ( | ||
| row_column(&pending.anchor, pending.anchor_column), | ||
| self.copy_selection_anchor, | ||
| ) && point.pane == CopySelectionPane::Chat | ||
| && (point.abs_line, point.column) != (row, column) | ||
| { | ||
| point.abs_line = row; | ||
| point.column = column; | ||
| self.copy_selection_anchor = Some(point); | ||
| changed = true; | ||
| } | ||
| if let (Some((row, column)), Some(mut point)) = ( | ||
| row_column(&pending.cursor, pending.cursor_column), | ||
| self.copy_selection_cursor, | ||
| ) && point.pane == CopySelectionPane::Chat |
There was a problem hiding this comment.
If a user changes a chat selection after resize capture but before rebase, this code applies the old captured endpoints to the new selection. A new drag selected TOKEN029–TOKEN031, but rebase moved it back to TOKEN026–TOKEN028. This timing-dependent issue can cost the user their latest selection; it is a non-blocking concern.
Knowledge Base Used: Restore fixed-rate drag-edge autoscroll
Artifacts
Executed source for the intervening-drag test
- The authored command creates an isolated worktree, renders the TUI, performs the intervening drag, and runs control and rebase cases; it is the source of the executed test.
Control run without selection rebase
- The executed control run records the new drag and unchanged endpoints when rebase is skipped; the run exited 0.
Run with selection rebase applied
- The executed rebase run records endpoints moving to the earlier capture and selected text reverting to TOKEN026–TOKEN028; the run exited 0.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/jcode-tui/src/tui/app/copy_selection.rs
Line: 268-282
Comment:
**New Drag Loses Selection**
If a user changes a chat selection after resize capture but before rebase, this code applies the old captured endpoints to the new selection. A new drag selected `TOKEN029`–`TOKEN031`, but rebase moved it back to `TOKEN026`–`TOKEN028`. This timing-dependent issue can cost the user their latest selection; it is a non-blocking concern.
**Knowledge Base Used:** [Restore fixed-rate drag-edge autoscroll](https://app.greptile.com/solo-systems/-/custom-context/knowledge-base/1jehuang/jcode/-/reverts/revert_1400-20260924-drag-edge-autoscroll-c4c3b62.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+92
to
+93
| // And re-base a transcript selection captured before the rewrap. | ||
| needs_redraw |= app.rebase_selection_after_resize(); |
There was a problem hiding this comment.
During a local agent turn, the turn loop renders a resized transcript without calling this idle-tick selection rebase. The old wrapped-row endpoints remain in use until the turn finishes. A test of that redraw path selected TOKEN026–TOKEN028 before resize but copied a filler fragment and TOKEN018 afterward. Rebase the selection during the turn before merging.
Knowledge Base Used: Terminal user interface
Artifacts
Command used to run the isolated selection test
- This authored script runs the targeted Cargo test in the isolated worktree with the shared build directory.
Executed test source without a turn-loop rebase
- This snapshot contains the TestBackend selection test executed for the failing run.
Selection test output without a turn-loop rebase
- The executed test reports the before and after copied text, a pending rebase, an assertion failure, and exit code 101.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/jcode-tui/src/tui/app/local.rs
Line: 92-93
Comment:
**Active Turns Copy Wrong Text**
During a local agent turn, the turn loop renders a resized transcript without calling this idle-tick selection rebase. The old wrapped-row endpoints remain in use until the turn finishes. A test of that redraw path selected `TOKEN026`–`TOKEN028` before resize but copied a filler fragment and `TOKEN018` afterward. Rebase the selection during the turn before merging.
**Knowledge Base Used:** [Terminal user interface](https://app.greptile.com/solo-systems/-/custom-context/knowledge-base/1jehuang/jcode/-/docs/terminal-user-interface.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comments Outside DiffThese findings sit on lines the diff does not cover, so they could not be posted inline. Each one leaves this list once its file changes.
|
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.
Comment on lines
+186
to
+189
| self.pending_selection_rebase = Some(super::PendingSelectionRebase { | ||
| anchor_raw, | ||
| cursor_raw, | ||
| captured_width: ui::last_layout_snapshot() |
There was a problem hiding this comment.
If older history loads after a resize captures a chat selection but before the next frame, these saved raw-line numbers point to different messages. Rebase moves the selection there, so copying returns text the user did not select. With four older messages prepended, a rendered selection of TOKEN026–TOKEN028 became TOKEN022–TOKEN024. Keep the endpoints tied to their original messages before merging.
Knowledge Base Used: Terminal user interface
Artifacts
Authored Ratatui selection and history-prepend reproduction test
- This executed test drags a selection in a rendered frame, captures resize coordinates, optionally prepends older history, then renders and compares the selected text.
Headless resize control without prepended history
- The control command exited 0 and shows that resize alone preserved TOKEN026–TOKEN028.
Headless resize with older history prepended
- The prepend command exited 0 and shows the selection changing from TOKEN026–TOKEN028 to TOKEN022–TOKEN024.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/jcode-tui/src/tui/app/copy_selection.rs
Line: 186-189
Comment:
**History Prepend Shifts Selection**
If older history loads after a resize captures a chat selection but before the next frame, these saved raw-line numbers point to different messages. Rebase moves the selection there, so copying returns text the user did not select. With four older messages prepended, a rendered selection of TOKEN026–TOKEN028 became TOKEN022–TOKEN024. Keep the endpoints tied to their original messages before merging.
**Knowledge Base Used:** [Terminal user interface](https://app.greptile.com/solo-systems/-/custom-context/knowledge-base/1jehuang/jcode/-/docs/terminal-user-interface.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
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
1 participant
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 5c.
Transcript selection endpoints are wrapped line indices, so a resize rewrap
silently reinterprets them: a drag over
TOKEN026..TOKEN028at 100 columnscopied a stray wrapped fragment plus
TOKEN018after resizing to 60 columns.This captures both 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-bases them onto the
frame laid out at the new width. Only the transcript pane is rebased; the input,
side-panel, diff and pinned panes are not transcript-relative.
It adds
resolve_rangetocrates/jcode-tui-messages/src/anchor.rs, theid-agnostic half of
resolve, so a caller can walk an item's rows instead ofonly resolving a single row.
Stacked on #1477 (phase 6b). The endpoints are anchored by the stable item
ids that #1477 introduces, so the selection never carries the occurrence ordinal
and a prepended or edited message cannot move an endpoint. Please land #1477
first; until then this PR's diff carries #1477's commits, and it rebases to a
single commit on top of master once #1477 merges.
Fails-first:
transcript_selection_covers_the_same_text_after_a_resizefailswithout the capture.
Validation (on this branch, stacked on #1477):
cargo fmt --all --checkclean.cargo clippy --profile selfdev -p jcode-tui-messages -p jcode-tui --all-targets -- -D warningsclean.cargo test -p jcode-tui-messages: 18 passed, 0 failed.cargo test -p jcode-tui --lib selection: 50 passed, 0 failed (includestranscript_selection_covers_the_same_text_after_a_resize).The behavioral copy/scroll/resize suite is unchanged.