From 933e247e9f68f11ec4d89a39654b381c778b8f2e Mon Sep 17 00:00:00 2001 From: muyuanjin Date: Tue, 9 Dec 2025 10:45:20 +0800 Subject: [PATCH] Fix transcript pager page continuity (#7363) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- codex-rs/tui/src/pager_overlay.rs | 112 ++++++++++++++++++++++++++++-- 1 file changed, 108 insertions(+), 4 deletions(-) diff --git a/codex-rs/tui/src/pager_overlay.rs b/codex-rs/tui/src/pager_overlay.rs index 3b47e9a70..f5854d554 100644 --- a/codex-rs/tui/src/pager_overlay.rs +++ b/codex-rs/tui/src/pager_overlay.rs @@ -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 { + 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 + }) + .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 = (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(