Skip to content

fix(tui): recover on owned wrap mapping mismatch#12

Open
fcoury wants to merge 3 commits intomainfrom
fix-wrapping-panic
Open

fix(tui): recover on owned wrap mapping mismatch#12
fcoury wants to merge 3 commits intomainfrom
fix-wrapping-panic

Conversation

@fcoury
Copy link
Owner

@fcoury fcoury commented Feb 23, 2026

Summary

  • Replace the panic! in map_owned_wrapped_line_to_range with a recoverable flow that skips synthetic leading characters, logs a warning on mid-line mismatch, and returns the mapped prefix range instead of crashing
  • Fixes a crash when textwrap produces owned lines with synthetic indent prefixes (e.g. non-space indents via initial_indent/subsequent_indent)

Test plan

  • Added unit test for direct mismatch recovery (map_owned_wrapped_line_to_range_recovers_on_non_prefix_mismatch)
  • Added end-to-end wrap_ranges test with non-space indents that forces owned wrapped lines and validates full source reconstruction
  • Verify no regressions in existing wrapping.rs tests (cargo test -p codex-tui)

Summary by CodeRabbit

  • Bug Fixes

    • Line wrapping is more resilient to indentation-induced mismatches: it recovers partial source mappings instead of failing, logs warnings for partial recoveries, and better preserves formatting across varied indent styles and non-space indents.
  • Tests

    • Added comprehensive tests for wrapping recovery, indentation alignment/divergence, non-prefix edge cases, and end-to-end reconstruction.

Replace the `panic!` path in `tui/src/wrapping.rs` with a recoverable
flow. The mapper now skips synthetic leading characters, logs a warning
on mid-line mismatch, and returns the mapped prefix range so wrapping
continues instead of aborting.

Add focused regression coverage for the recovery behavior. Tests now
validate direct mismatch recovery and an end-to-end `wrap_ranges` case
with non-space indents that forces owned wrapped lines and preserves
round-trip source reconstruction.
Copilot AI review requested due to automatic review settings February 23, 2026 19:04
@coderabbitai
Copy link

coderabbitai bot commented Feb 23, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between c79eefc and f25afcb.

📒 Files selected for processing (1)
  • codex-rs/tui/src/wrapping.rs

📝 Walkthrough

Walkthrough

Updated wrapping mapping to accept a synthetic prefix, strip it when mapping, and attempt partial recoverable mappings instead of panicking; mapping now tracks whether any source characters were seen and emits a tracing warning on partial recovery. Added tests for indentation and corner cases; function signature changed.

Changes

Cohort / File(s) Summary
Wrapping logic & tests
codex-rs/tui/src/wrapping.rs
Added synthetic_prefix: &str parameter to map_owned_wrapped_line_to_range, updated wrap_ranges/wrap_ranges_trim to pass synthetic prefixes, changed mapping to strip prefixes, track saw_source_char, and return partial ranges with tracing::warn! on mismatches instead of panicking. Added extensive tests for partial recovery, indent alignment, non-space indents, overconsumption prevention, and URL-wrap behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I nibbled prefixes, peeled them with care,
Matched bits of the source when full matches were rare,
If crumbs stayed behind, I whispered, not cried —
A gentle warn, a recovered stride,
Hopping wrapped lines with joy in the air.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: fixing a panic in wrap mapping by implementing recovery logic for owned wrap mapping mismatches.
Description check ✅ Passed The description includes a clear summary of changes, specific fixes (replacing panic with recoverable flow), and a comprehensive test plan with implementation status, but lacks a link to an associated bug report or enhancement request as suggested by the template.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-wrapping-panic

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps
Copy link

greptile-apps bot commented Feb 23, 2026

Greptile Summary

Replaces a panic-on-mismatch in map_owned_wrapped_line_to_range with a recovery algorithm that tries all possible starting offsets in the wrapped line to find the longest valid source mapping. The fix prevents crashes when textwrap produces owned lines with synthetic indent prefixes (e.g., initial_indent/subsequent_indent options).

Key changes:

  • Iterates through all character positions in the wrapped line as potential starting points
  • Tracks the best (longest) mapping found and whether a mismatch occurred after matching source characters
  • Logs a warning when partial mapping occurs instead of crashing
  • Adds three new unit tests covering recovery scenarios and indent-prefix edge cases

Confidence Score: 4/5

  • Safe to merge with minimal risk - replaces crash with graceful recovery
  • The change is well-tested with three new unit tests covering the edge cases. The recovery algorithm is sound and degrades gracefully by logging warnings instead of panicking. The logic iterates through all possible alignments to find the best match, which is a robust approach for handling synthetic characters from textwrap
  • No files require special attention

Important Files Changed

Filename Overview
codex-rs/tui/src/wrapping.rs Replaces panic with recovery logic for owned wrapped lines; adds comprehensive test coverage including edge cases with synthetic indent prefixes

Last reviewed commit: c79eefc

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR replaces a panic in map_owned_wrapped_line_to_range with a graceful recovery mechanism that handles textwrap's synthetic indent prefixes. When textwrap produces owned wrapped lines with synthetic characters (e.g., from initial_indent/subsequent_indent options), the function now skips leading synthetic characters and returns partial source ranges with a warning instead of crashing the application.

Changes:

  • Added saw_source_char flag to track whether any source characters have been matched
  • Implemented logic to skip synthetic leading characters (e.g., indent prefixes) before matching source content
  • Replaced panic with warning log and partial range return on mid-line mismatches
  • Added two comprehensive tests: unit test for direct mismatch recovery and end-to-end test with non-space indents

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

continue;
}

tracing::warn!(
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

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

The tracing module is used but not imported. Add use tracing; or call the macro with the fully qualified path. Looking at the imports at the top of the file, there is no tracing import, which will cause a compilation error.

Copilot uses AI. Check for mistakes.
Update `map_owned_wrapped_line_to_range` to evaluate candidate starts in
owned wrapped output and keep the longest source mapping. This avoids
truncating ranges when synthetic indent prefixes share leading
characters with source text.

Add regression tests for the ambiguous prefix case and an end-to-end
`wrap_ranges` reconstruction scenario so this edge case stays covered.
@fcoury fcoury reopened this Feb 23, 2026
@fcoury
Copy link
Owner Author

fcoury commented Feb 23, 2026

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Feb 23, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Pass the expected `initial_indent` or `subsequent_indent` into
`map_owned_wrapped_line_to_range` and strip only that validated leading
prefix before matching against source bytes. This avoids over-consuming
source text when repeated prefix patterns appear in both indent and
content.

Add a focused regression test that reproduces the repeated `- ` pattern
case on narrow widths and asserts the mapper does not over-advance.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants