Skip to content

Commit 279283f

Browse files
authored
fix(tui2): avoid scroll stickiness at cell boundaries (#8695)
Mouse/trackpad scrolling in tui2 applies deltas in visual lines, but the transcript scroll state was anchored only to CellLine entries. When a 1-line scroll landed on the synthetic inter-cell Spacer row (inserted between non-continuation cells), `TranscriptScroll::anchor_for` would skip that row and snap back to the adjacent cell line. That makes the resolved top offset unchanged for small/coalesced scroll deltas, so scrolling appears to get stuck right before certain cells (commonly user prompts and command output cells). Fix this by making spacer rows a first-class scroll anchor: - Add `TranscriptScroll::ScrolledSpacerBeforeCell` and resolve it back to the spacer row index when present. - Update `anchor_for`/`scrolled_by` to preserve spacers instead of skipping them. - Treat the new variant as "already anchored" in `lock_transcript_scroll_to_current_view`. Tests: - cargo test -p codex-tui2
1 parent 0c1658d commit 279283f

File tree

2 files changed

+99
-18
lines changed

2 files changed

+99
-18
lines changed

codex-rs/tui2/src/app.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1135,7 +1135,8 @@ impl App {
11351135
let max_start = total_lines.saturating_sub(max_visible);
11361136
let top_offset = match self.transcript_scroll {
11371137
TranscriptScroll::ToBottom => max_start,
1138-
TranscriptScroll::Scrolled { .. } => {
1138+
TranscriptScroll::Scrolled { .. }
1139+
| TranscriptScroll::ScrolledSpacerBeforeCell { .. } => {
11391140
// Already anchored; nothing to lock.
11401141
return;
11411142
}

codex-rs/tui2/src/tui/scrolling.rs

Lines changed: 97 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
//! the newly flattened line list on the next frame.
1818
//!
1919
//! Spacer rows between non-continuation cells are represented as `TranscriptLineMeta::Spacer`.
20-
//! They are not valid anchors; `anchor_for` will pick the nearest non-spacer line when needed.
20+
//! They are valid scroll anchors so 1-line scrolling does not "stick" at cell boundaries.
2121
2222
pub(crate) mod mouse;
2323
pub(crate) use mouse::MouseScrollState;
@@ -78,6 +78,13 @@ pub(crate) enum TranscriptScroll {
7878
cell_index: usize,
7979
line_in_cell: usize,
8080
},
81+
/// Anchor the viewport to the spacer row immediately before a cell.
82+
///
83+
/// This exists because spacer rows are real, visible transcript rows, and users may scroll
84+
/// through them one line at a time (especially with trackpads). Without a dedicated spacer
85+
/// anchor, a 1-line scroll that lands on a spacer would snap back to the adjacent cell line
86+
/// and appear to "stick" at boundaries.
87+
ScrolledSpacerBeforeCell { cell_index: usize },
8188
}
8289

8390
impl TranscriptScroll {
@@ -108,6 +115,13 @@ impl TranscriptScroll {
108115
None => (Self::ToBottom, max_start),
109116
}
110117
}
118+
Self::ScrolledSpacerBeforeCell { cell_index } => {
119+
let anchor = spacer_before_cell_index(line_meta, cell_index);
120+
match anchor {
121+
Some(idx) => (self, idx.min(max_start)),
122+
None => (Self::ToBottom, max_start),
123+
}
124+
}
111125
}
112126
}
113127

@@ -142,6 +156,11 @@ impl TranscriptScroll {
142156
} => anchor_index(line_meta, cell_index, line_in_cell)
143157
.unwrap_or(max_start)
144158
.min(max_start),
159+
Self::ScrolledSpacerBeforeCell { cell_index } => {
160+
spacer_before_cell_index(line_meta, cell_index)
161+
.unwrap_or(max_start)
162+
.min(max_start)
163+
}
145164
};
146165

147166
let new_top = if delta_lines < 0 {
@@ -164,15 +183,35 @@ impl TranscriptScroll {
164183
/// This is the inverse of "resolving a scroll state to a top-row offset":
165184
/// given a concrete flattened line index, pick a stable `(cell_index, line_in_cell)` anchor.
166185
///
167-
/// See `resolve_top` for `line_meta` semantics. This prefers the nearest line at or after `start`
168-
/// (skipping spacer rows), falling back to the nearest line before it when needed.
186+
/// See `resolve_top` for `line_meta` semantics. This prefers the line at `start` (including
187+
/// spacer rows), falling back to the nearest non-spacer line after or before it when needed.
169188
pub(crate) fn anchor_for(line_meta: &[TranscriptLineMeta], start: usize) -> Option<Self> {
170-
let anchor =
171-
anchor_at_or_after(line_meta, start).or_else(|| anchor_at_or_before(line_meta, start));
172-
anchor.map(|(cell_index, line_in_cell)| Self::Scrolled {
173-
cell_index,
174-
line_in_cell,
175-
})
189+
if line_meta.is_empty() {
190+
return None;
191+
}
192+
193+
let start = start.min(line_meta.len().saturating_sub(1));
194+
match line_meta[start] {
195+
TranscriptLineMeta::CellLine {
196+
cell_index,
197+
line_in_cell,
198+
} => Some(Self::Scrolled {
199+
cell_index,
200+
line_in_cell,
201+
}),
202+
TranscriptLineMeta::Spacer => {
203+
if let Some((cell_index, _)) = anchor_at_or_after(line_meta, start) {
204+
Some(Self::ScrolledSpacerBeforeCell { cell_index })
205+
} else {
206+
anchor_at_or_before(line_meta, start).map(|(cell_index, line_in_cell)| {
207+
Self::Scrolled {
208+
cell_index,
209+
line_in_cell,
210+
}
211+
})
212+
}
213+
}
214+
}
176215
}
177216
}
178217

@@ -198,6 +237,26 @@ fn anchor_index(
198237
})
199238
}
200239

240+
/// Locate the flattened line index for the spacer row immediately before `cell_index`.
241+
///
242+
/// The spacer itself is not uniquely tagged in `TranscriptLineMeta`, so we locate the first
243+
/// visual line of the cell (`line_in_cell == 0`) and, if it is preceded by a spacer row, return
244+
/// that spacer's index. If the spacer is missing (for example when the cell is a stream
245+
/// continuation), we fall back to the cell's first line index so scrolling remains usable.
246+
fn spacer_before_cell_index(line_meta: &[TranscriptLineMeta], cell_index: usize) -> Option<usize> {
247+
let cell_first = anchor_index(line_meta, cell_index, 0)?;
248+
if cell_first > 0
249+
&& matches!(
250+
line_meta.get(cell_first.saturating_sub(1)),
251+
Some(TranscriptLineMeta::Spacer)
252+
)
253+
{
254+
Some(cell_first.saturating_sub(1))
255+
} else {
256+
Some(cell_first)
257+
}
258+
}
259+
201260
/// Find the first transcript line at or after the given flattened index.
202261
fn anchor_at_or_after(line_meta: &[TranscriptLineMeta], start: usize) -> Option<(usize, usize)> {
203262
if line_meta.is_empty() {
@@ -272,6 +331,33 @@ mod tests {
272331
assert_eq!(top, 2);
273332
}
274333

334+
#[test]
335+
fn scrolled_by_can_land_on_spacer_rows() {
336+
let meta = meta(&[
337+
cell_line(0, 0),
338+
TranscriptLineMeta::Spacer,
339+
cell_line(1, 0),
340+
cell_line(1, 1),
341+
]);
342+
343+
let scroll = TranscriptScroll::Scrolled {
344+
cell_index: 1,
345+
line_in_cell: 0,
346+
};
347+
348+
assert_eq!(
349+
scroll.scrolled_by(-1, &meta, 2),
350+
TranscriptScroll::ScrolledSpacerBeforeCell { cell_index: 1 }
351+
);
352+
assert_eq!(
353+
TranscriptScroll::ScrolledSpacerBeforeCell { cell_index: 1 }.scrolled_by(-1, &meta, 2),
354+
TranscriptScroll::Scrolled {
355+
cell_index: 0,
356+
line_in_cell: 0
357+
}
358+
);
359+
}
360+
275361
#[test]
276362
fn resolve_top_scrolled_falls_back_when_anchor_missing() {
277363
let meta = meta(&[cell_line(0, 0), TranscriptLineMeta::Spacer, cell_line(1, 0)]);
@@ -350,17 +436,11 @@ mod tests {
350436

351437
assert_eq!(
352438
TranscriptScroll::anchor_for(&meta, 0),
353-
Some(TranscriptScroll::Scrolled {
354-
cell_index: 0,
355-
line_in_cell: 0
356-
})
439+
Some(TranscriptScroll::ScrolledSpacerBeforeCell { cell_index: 0 })
357440
);
358441
assert_eq!(
359442
TranscriptScroll::anchor_for(&meta, 2),
360-
Some(TranscriptScroll::Scrolled {
361-
cell_index: 1,
362-
line_in_cell: 0
363-
})
443+
Some(TranscriptScroll::ScrolledSpacerBeforeCell { cell_index: 1 })
364444
);
365445
assert_eq!(
366446
TranscriptScroll::anchor_for(&meta, 3),

0 commit comments

Comments
 (0)