Skip to content

Conversation

@muyuanjin
Copy link
Contributor

What

  • Fix the Ctrl+T transcript overlay so that very long exec output lines are soft‑wrapped to the viewport width instead of being rendered as a single truncated row.
  • Add a regression test to TranscriptOverlay to ensure long exec outputs are rendered on multiple lines in the overlay.

Why

How

  • Update ExecCell::transcript_lines to wrap exec output lines using the existing RtOptions/word_wrap_line helpers so that transcript rendering is width‑aware.
  • Reuse the existing line utilities to expand the wrapped Line values into the transcript overlay, preserving styling while respecting the current viewport width.
  • Add transcript_overlay_wraps_long_exec_output_lines test in pager_overlay.rs that constructs a long single‑line exec output, renders the transcript overlay into a small buffer, and asserts that the long marker string spans multiple rendered lines.

@etraut-openai
Copy link
Collaborator

Thanks for the contribution. Before I assign it to someone on the codex team for review, please fix the CI failure. Looks like a simple formatting issue (cargo fmt).

@etraut-openai etraut-openai added the needs-response Additional information is requested label Dec 3, 2025
@muyuanjin muyuanjin force-pushed the bug/transcript-wrap-long-lines-ctrl-t branch from 3423fe1 to 2cefa37 Compare December 3, 2025 22:58
@muyuanjin
Copy link
Contributor Author

Thanks for the contribution. Before I assign it to someone on the codex team for review, please fix the CI failure. Looks like a simple formatting issue (cargo fmt).

Thanks for pointing that out. I’ve updated the formatting in the PR, and the CI is now passing.

Copy link
Collaborator

@joshka-oai joshka-oai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Impl LGTM. nit about the test. I think it could be improved for maintainability. Not a blocker however. I'd suggest throw codex at the test and see if it does better.

Comment on lines +751 to +752
#[test]
fn transcript_overlay_wraps_long_exec_output_lines() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test seems a bit more complex than I'd like to see in a test, but it's sort of in pattern for the existing code. I wonder if it could be simplified to be more hard coded (e.g. check a snapshot). The perspective is that I have to really read every line of the test carefully to understand what property of the code this is testing.

Some small things that reduce line count are intermediate vars (e.g.

let command_output = CommandOutput {...};
exec_cell.complete_call("exec-long", command_output, Duration::from_millis(10));

But also perhaps a snapshot instead of counting lines etc.

I'm very much of the opinion that generally tests should generally be simpler than the behavior they're testing. Here that seems inverted.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not convinced a test is really needed in this case.

I'm going to merge and then address the test as a follow-up PR.

@etraut-openai etraut-openai merged commit 70b9779 into openai:main Dec 4, 2025
26 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

needs-response Additional information is requested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ctrl+T transcript overlay truncates very long single lines instead of wrapping to viewport width

3 participants