codex icon indicating copy to clipboard operation
codex copied to clipboard

Fix transcript pager page continuity

Open muyuanjin opened this issue 3 weeks ago • 4 comments

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:

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.

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:

  1. 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 example user_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.
  2. python3 must be callable from PowerShell

    • Some tests (e.g. suite::codex_tool::test_shell_command_approval_triggers_elicitation) expect a python3 command to be available when invoked via pwsh.exe.

    • On many Windows setups only python exists on PATH and python3 is missing, which causes these tests to fail.

    • Mitigation: either install Python such that python3.exe is 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).

muyuanjin avatar Nov 28 '25 06:11 muyuanjin

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

github-actions[bot] avatar Nov 28 '25 06:11 github-actions[bot]

I have read the CLA Document and I hereby sign the CLA

muyuanjin avatar Nov 28 '25 06:11 muyuanjin

@codex review

etraut-openai avatar Nov 28 '25 06:11 etraut-openai

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.

joshka-oai avatar Dec 09 '25 01:12 joshka-oai

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_height for 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.

muyuanjin avatar Dec 09 '25 02:12 muyuanjin