Fix transcript pager page continuity (#7363)

## What

Fix PageUp/PageDown behaviour in the Ctrl+T transcript overlay so that
paging is continuous and reversible, and add tests to lock in the
expected behaviour.

## Why

Today, paging in the transcript overlay uses the raw viewport height
instead of the effective content height after layout. Because the
overlay reserves some rows for chrome (header/footer), this can cause:

- PageDown to skip transcript lines between pages.
- PageUp/PageDown not to “round-trip” cleanly (PageDown then PageUp does
not always return to the same set of visible lines).

This shows up when inspecting longer transcripts via Ctrl+T; see #7356
for context.

## How

- Add a dedicated `PagerView::page_step` helper that computes the page
size from the last rendered content height and falls back to
`content_area(viewport_area).height` when that is not yet available.
- Use `page_step(...)` for both PageUp and PageDown (including SPACE) so
the scroll step always matches the actual content area height, not the
full viewport height.
- Add a focused test
`transcript_overlay_paging_is_continuous_and_round_trips` that:
  - Renders a synthetic transcript with numbered `line-NN` rows.
- Asserts that successive PageDown operations show continuous line
numbers (no gaps).
- Asserts that PageDown+PageUp and PageUp+PageDown round-trip correctly
from non-edge offsets.

The change is limited to `codex-rs/tui/src/pager_overlay.rs` and only
affects the transcript overlay paging semantics.

## Related issue

- #7356

## Testing

On Windows 11, using PowerShell 7 in the repo root:

```powershell
cargo test
cargo clippy --tests
cargo fmt -- --config imports_granularity=Item
```

- All tests passed.
- `cargo clippy --tests` reported some pre-existing warnings that are
unrelated to this change; no new lints were introduced in the modified
code.

---------

Signed-off-by: muyuanjin <24222808+muyuanjin@users.noreply.github.com>
Co-authored-by: Eric Traut <etraut@openai.com>
This commit is contained in:
muyuanjin 2025-12-09 10:45:20 +08:00 committed by GitHub
parent 68505abf0f
commit 933e247e9f
No known key found for this signature in database
GPG key ID: B5690EEEBB952194

View file

@ -241,12 +241,12 @@ impl PagerView {
self.scroll_offset = self.scroll_offset.saturating_add(1);
}
e if KEY_PAGE_UP.is_press(e) => {
let area = self.content_area(tui.terminal.viewport_area);
self.scroll_offset = self.scroll_offset.saturating_sub(area.height as usize);
let page_height = self.page_height(tui.terminal.viewport_area);
self.scroll_offset = self.scroll_offset.saturating_sub(page_height);
}
e if KEY_PAGE_DOWN.is_press(e) || KEY_SPACE.is_press(e) => {
let area = self.content_area(tui.terminal.viewport_area);
self.scroll_offset = self.scroll_offset.saturating_add(area.height as usize);
let page_height = self.page_height(tui.terminal.viewport_area);
self.scroll_offset = self.scroll_offset.saturating_add(page_height);
}
e if KEY_HOME.is_press(e) => {
self.scroll_offset = 0;
@ -263,6 +263,16 @@ impl PagerView {
Ok(())
}
/// Returns the height of one page in content rows.
///
/// Prefers the last rendered content height (excluding header/footer chrome);
/// if no render has occurred yet, falls back to the content area height
/// computed from the given viewport.
fn page_height(&self, viewport_area: Rect) -> usize {
self.last_content_height
.unwrap_or_else(|| self.content_area(viewport_area).height as usize)
}
fn update_last_content_height(&mut self, height: u16) {
self.last_content_height = Some(height as usize);
}
@ -812,6 +822,100 @@ mod tests {
assert_snapshot!(term.backend());
}
/// Render transcript overlay and return visible line numbers (`line-NN`) in order.
fn transcript_line_numbers(overlay: &mut TranscriptOverlay, area: Rect) -> Vec<usize> {
let mut buf = Buffer::empty(area);
overlay.render(area, &mut buf);
let top_h = area.height.saturating_sub(3);
let top = Rect::new(area.x, area.y, area.width, top_h);
let content_area = overlay.view.content_area(top);
let mut nums = Vec::new();
for y in content_area.y..content_area.bottom() {
let mut line = String::new();
for x in content_area.x..content_area.right() {
line.push(buf[(x, y)].symbol().chars().next().unwrap_or(' '));
}
if let Some(n) = line
.split_whitespace()
.find_map(|w| w.strip_prefix("line-"))
.and_then(|s| s.parse().ok())
{
nums.push(n);
}
}
nums
}
#[test]
fn transcript_overlay_paging_is_continuous_and_round_trips() {
let mut overlay = TranscriptOverlay::new(
(0..50)
.map(|i| {
Arc::new(TestCell {
lines: vec![Line::from(format!("line-{i:02}"))],
}) as Arc<dyn HistoryCell>
})
.collect(),
);
let area = Rect::new(0, 0, 40, 15);
// Prime layout so last_content_height is populated and paging uses the real content height.
let mut buf = Buffer::empty(area);
overlay.view.scroll_offset = 0;
overlay.render(area, &mut buf);
let page_height = overlay.view.page_height(area);
// Scenario 1: starting from the top, PageDown should show the next page of content.
overlay.view.scroll_offset = 0;
let page1 = transcript_line_numbers(&mut overlay, area);
let page1_len = page1.len();
let expected_page1: Vec<usize> = (0..page1_len).collect();
assert_eq!(
page1, expected_page1,
"first page should start at line-00 and show a full page of content"
);
overlay.view.scroll_offset = overlay.view.scroll_offset.saturating_add(page_height);
let page2 = transcript_line_numbers(&mut overlay, area);
assert_eq!(
page2.len(),
page1_len,
"second page should have the same number of visible lines as the first page"
);
let expected_page2_first = *page1.last().unwrap() + 1;
assert_eq!(
page2[0], expected_page2_first,
"second page after PageDown should immediately follow the first page"
);
// Scenario 2: from an interior offset (start=3), PageDown then PageUp should round-trip.
let interior_offset = 3usize;
overlay.view.scroll_offset = interior_offset;
let before = transcript_line_numbers(&mut overlay, area);
overlay.view.scroll_offset = overlay.view.scroll_offset.saturating_add(page_height);
let _ = transcript_line_numbers(&mut overlay, area);
overlay.view.scroll_offset = overlay.view.scroll_offset.saturating_sub(page_height);
let after = transcript_line_numbers(&mut overlay, area);
assert_eq!(
before, after,
"PageDown+PageUp from interior offset ({interior_offset}) should round-trip"
);
// Scenario 3: from the top of the second page, PageUp then PageDown should round-trip.
overlay.view.scroll_offset = page_height;
let before2 = transcript_line_numbers(&mut overlay, area);
overlay.view.scroll_offset = overlay.view.scroll_offset.saturating_sub(page_height);
let _ = transcript_line_numbers(&mut overlay, area);
overlay.view.scroll_offset = overlay.view.scroll_offset.saturating_add(page_height);
let after2 = transcript_line_numbers(&mut overlay, area);
assert_eq!(
before2, after2,
"PageUp+PageDown from the top of the second page should round-trip"
);
}
#[test]
fn static_overlay_wraps_long_lines() {
let mut overlay = StaticOverlay::with_title(