Fix transcript pager page continuity
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_stephelper that computes the page size from the last rendered content height and falls back tocontent_area(viewport_area).heightwhen 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_tripsthat:- Renders a synthetic transcript with numbered
line-NNrows. - 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.
- Renders a synthetic transcript with numbered
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:
cargo test
cargo clippy --tests
cargo fmt -- --config imports_granularity=Item
- All tests passed.
cargo clippy --testsreported some pre-existing warnings that are unrelated to this change; no new lints were introduced in the modified code.
Environment notes (Windows / PowerShell)
While running the tests on Windows / PowerShell, I ran into a couple of environment-specific issues that may be helpful to document:
-
PowerShell profile startup noise can break shell history tests
- If the PowerShell profile (e.g.
$PROFILE) prints anything to stdout on startup (banners, logging,Write-Host, etc.), tests that assert on exact shell output (for exampleuser_shell_command_history_is_persisted_and_shared_with_model) can fail because the extra lines get captured as part of the shell output. - Mitigation: ensure profiles used during Codex tests do not write to stdout. If logging is needed, send it to a log file or stderr, or disable the profile for the test session.
- If the PowerShell profile (e.g.
-
python3must be callable from PowerShell-
Some tests (e.g.
suite::codex_tool::test_shell_command_approval_triggers_elicitation) expect apython3command to be available when invoked viapwsh.exe. -
On many Windows setups only
pythonexists on PATH andpython3is missing, which causes these tests to fail. -
Mitigation: either install Python such that
python3.exeis on PATH, or add an alias in PowerShell for the test environment, for example:Set-Alias python3 python -
Make sure this alias is visible to the PowerShell instance that Codex uses, and that it does not introduce extra stdout output (to avoid the issue in point 1).
-
All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.
I have read the CLA Document and I hereby sign the CLA
@codex review
Codex Review: Didn't find any major issues. Can't wait for the next one!
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Just a heads up. I also have https://github.com/openai/codex/pull/7550 which touches the same code paths as this. I'd like to get that PR merged rather than having it blocked on this. This PR will require a small amount of rework - likely as simple as adding the appropriate half page items. I hope this is ok with you.
Just a heads up. I also have #7550 which touches the same code paths as this. I'd like to get that PR merged rather than having it blocked on this. This PR will require a small amount of rework - likely as simple as adding the appropriate half page items. I hope this is ok with you.
Thanks a lot for the detailed review and for calling out the test readability issues.
I’ve addressed your feedback in the latest commit:
- Renamed the paging helper and locals to
page_heightfor clearer intent. - Added a short doc comment that explains how the page height is derived.
- Reworked the transcript paging test into concrete, scenario-based checks that assert on the full set of visible lines instead of using boolean assertions.
Once #7550 is merged, I will rebase this branch.