fix(tui): preserve URL clickability across all TUI views#12067
fix(tui): preserve URL clickability across all TUI views#12067etraut-openai merged 21 commits intoopenai:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fixes a critical usability issue where URLs containing / and - characters are split across terminal lines by textwrap's default hyphenation rules, breaking terminal link detection and making URLs unclickable. The fix introduces URL-aware wrapping that treats URL-like tokens as atomic units while maintaining backward compatibility for non-URL content.
Changes:
- Adds URL detection heuristics and adaptive wrapping functions to the
wrappingmodule that preserve URL integrity - Migrates all user-facing content rendering (history cells, exec output, markdown, etc.) from
word_wrap_*toadaptive_wrap_*functions - Updates height calculation throughout to use
Paragraph::line_countfor accurate viewport row measurement when lines contain long URLs - Implements two-path terminal scrollback rendering (unwrapped URLs for terminal detection, word-wrapped text) with continuation row clearing
- Adds row-aware truncation in exec cells that accounts for the viewport cost of wrapped lines
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
wrapping.rs |
Core implementation: URL detection heuristics, adaptive wrapping functions, custom WordSplitter, and comprehensive test suite (10 new tests) |
history_cell.rs |
Migrates all cell types to adaptive_wrap_*; establishes default desired_height using Paragraph::line_count; removes redundant custom height implementations; adds 5 new tests |
exec_cell/render.rs |
Implements row-aware truncate_lines_middle that uses Paragraph::line_count to properly budget viewport rows; migrates to adaptive_wrap_* for commands and output; adds 5 new tests |
insert_history.rs |
Two-path scrollback: URL lines kept intact for terminal detection, non-URL lines word-wrapped; pre-clears continuation rows to prevent stale content artifacts; adds 4 new tests |
markdown_render.rs |
Migrates finish_paragraph to adaptive_wrap_line; adds 1 new test |
model_migration.rs |
Adds Wrap { trim: false } to markdown rendering; adds 1 new test |
pager_overlay.rs |
Adds Wrap { trim: false } to transcript and streaming chunk rendering |
queued_user_messages.rs |
Migrates to adaptive_wrap_lines; adds 1 new test |
app_link_view.rs |
Migrates setup URL rendering to adaptive_wrap_lines; updates desired_height to use Paragraph::line_count; adds 2 new tests |
status/card.rs |
Migrates rate limit note rendering to adaptive_wrap_lines |
codex_tui__history_cell__tests__plan_update_with_note_and_wrapping_snapshot.snap |
Expected snapshot update reflecting improved wrapping behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codex review |
|
Security review completed. No security issues were found in this pull request. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24ab72a1b6
ℹ️ 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".
Prevent mid-token URL splitting in exec command rendering by using URL-aware wrap options (AsciiSpace, NoHyphenation, break_words(false)) for command header/continuation lines. Keep wrapped behavior unchanged for non-URL lines. Fix scrollback insertion regression where re-wrapping prefixed URL lines produced orphan `│` lines by preserving URL lines intact while still accounting for visual row usage. Add regression coverage for: URL no-split behavior in exec_cell command display; URL intact behavior with AsciiSpace + NoHyphenation + break_words(false) in wrapping helpers; insert_history prefixed URL rendering so prefix and URL stay on the same row and no orphan prefix row appears. Tests: cargo test -p codex-tui
Centralize URL-like token detection and adaptive wrapping in wrapping.rs, including non-http(s) URL-like forms (bare domains, localhost, IPv4 with path/query/fragment). Apply adaptive wrapping across exec rendering, history cells, markdown wrapping, app link setup view, and insert_history scrollback handling to avoid splitting link tokens or creating orphan prefix lines. Add regression tests for URL-like detection and rendering paths (exec output, history cells, markdown, app-link setup, and insert_history). Validation: just fmt and cargo test -p codex-tui.
Document the intent and contracts behind the adaptive wrapping pipeline, the HistoryCell trait's Paragraph-based height defaults, the row-aware truncation in ExecCell, and the URL-line scrollback strategy in insert_history.
Switch the last two call sites from word_wrap_lines to adaptive_wrap_lines so URL-like tokens in queued user messages and the status card are never split across lines. Adds a test confirming a long URL-like message does not produce extraneous wrapped-ellipsis rows.
…unts URL rows The upstream remote_image_urls change added a desired_height override that counts logical lines instead of viewport rows. This undercounts when a URL overflows its column width and wraps visually. Remove the override so the trait default (Paragraph::line_count) is used.
Lines containing both a URL and substantive non-URL text now flow through adaptive_wrap_line so prose wraps naturally while URL tokens remain unsplit. Pure-URL lines (with only decorative markers like │ or list prefixes) still skip wrapping to preserve terminal link detection.
Long OAuth URLs wrap across multiple terminal rows, and ratatui's cell-based rendering emits MoveTo at each row boundary, breaking normal terminal link detection. OSC 8 hyperlinks explicitly mark text as clickable regardless of row wrapping. After the Paragraph renders, scan the buffer for cyan+underlined cells (the URL's distinctive style) and wrap each cell's symbol with `\x1B]8;;URL\x07char\x1B]8;;\x07`. Add display_width() to custom_terminal.rs that strips OSC escape sequences before width calculation so the layout engine isn't confused by the escape payload characters.
Adjust `ExecCell::truncate_lines_middle` in
`codex-rs/tui/src/exec_cell/render.rs` to avoid relying on
`Paragraph::line_count` for whitespace-only lines.
Whitespace-only prefixed output lines (for example `" "`) could be
counted as two rows with `Wrap { trim: false }`, which caused early
ellipsis insertion and inflated omitted-line counts in exec output.
Add a regression test that reproduces the blank-line case and asserts
no truncation when content fits the row budget.
e9c4b7d to
e646ab1
Compare
|
@codex review |
Problem
Long URLs containing
/and-characters are split across multiple terminal lines bytextwrap's default hyphenation rules. This breaks terminal link detection: emulators can no longer identify the URL as clickable, and copy-paste yields a truncated fragment. The issue affects every view that renders user or agent text — exec output, history cells, markdown, the app-link setup screen, and the VT100 scrollback path.A secondary bug compounds the first:
desired_height()calculations count logical lines rather than viewport rows. When a URL overflows its line and wraps visually, the height budget is too small, causing content to clip or leave gaps.Here is how the complete URL is interpreted by the terminal before (first line only) and after (complete URL):
Mental model
The TUI now treats URL-like tokens as atomic units that must never be split by the wrapping engine. Every call site that previously used
word_wrap_*has been migrated toadaptive_wrap_*, which inspects each line for URL-like tokens and switches wrapping strategy accordingly:textwrappath unchanged (word boundaries, optional indentation, hyphenation).│,-,1.) are emitted unwrapped so terminal link detection works; ratatui'sWrap { trim: false }handles the final character wrap at render time.adaptive_wrap_lineso prose wraps naturally at word boundaries while URL tokens remain unsplit.Height measurement everywhere now delegates to
Paragraph::line_count(width), which accounts for the visual row cost of overflowed lines. This single source of truth replaces ad-hoc line counting in individual cells.For terminal scrollback (the VT100 path that prints history when the TUI exits), URL-only lines are emitted unwrapped so the terminal's own link detector can find them. Mixed URL+prose lines use adaptive wrapping so surrounding text wraps naturally. Continuation rows are pre-cleared to avoid stale content artifacts.
Non-goals
scheme://host, bare domains (example.com/path),localhost:port, and IPv4 hosts. IPv6 ([::1]:8080) and exotic schemes are intentionally excluded from v1.Tradeoffs
src/main.rstruncate_lines_middleis more complex (must compute per-line row cost)desired_heightviaParagraph::line_countArchitecture
flowchart TD A["adaptive_wrap_*()"] --> B{"line_contains_url_like?"} B -- No URL tokens --> C["word_wrap_line<br/>(textwrap default)"] B -- Has URL tokens --> D{"mixed URL + prose?"} D -- "URL-only<br/>(+ decorative markers)" --> E["emit unwrapped<br/>(terminal char-wraps)"] D -- "Mixed<br/>(URL + substantive text)" --> F["adaptive_wrap_line<br/>(AsciiSpace + custom WordSplitter)"] C --> G["Paragraph::line_count(w)<br/>(single height truth)"] E --> G F --> GChanged files:
wrapping.rsadaptive_wrap_*functions, customWordSplitterexec_cell/render.rstruncate_lines_middle, adaptive wrapping for command/output displayhistory_cell.rsadaptive_wrap_*; defaultdesired_heightviaParagraph::line_countinsert_history.rsapp_link_view.rsdesired_heightviaParagraph::line_countmarkdown_render.rsfinish_paragraphmodel_migration.rspager_overlay.rsWrap { trim: false }for transcript and streaming chunksqueued_user_messages.rsadaptive_wrap_linesstatus/card.rsadaptive_wrap_linesObservability
Paragraph::line_countpath is the same code ratatui uses at render time, so measurement and rendering cannot diverge.Tests
26 new unit tests across 7 files, covering:
desired_height()againstParagraph::line_count()for URL-containing content.line_has_mixed_url_and_non_url_tokenscorrectly distinguishes lines with substantive non-URL text from lines with only decorative markers alongside a URL.https://...,example.com/path,localhost:3000/api,192.168.1.1:8080/health) and negative matches (src/main.rs,foo/bar,hello-world).Risks and open items
example.com/apiinside a JSON blob) will trigger URL-preserving wrap on that line. This is acceptable — the worst case is a slightly wider line, not broken output.[::1]:8080/pathwill be treated as a non-URL and may get split. Can be added later without API changes.Fixes #5457