Fix crash when resizing a terminal containing wide (CJK) characters#13041
Fix crash when resizing a terminal containing wide (CJK) characters#13041Im-Tae wants to merge 4 commits into
Conversation
resize_storage converted each cursor to a flat-storage content offset and unwrapped the result with `.expect()`. content_offset_at_point returns Err for a cursor whose column doesn't map to tracked content — reachable with wide (full-width / CJK) characters, where the grid's per-column accounting diverges from flat storage's grapheme-run accounting — so resizing a terminal showing CJK text (e.g. a Korean directory name in the prompt) aborted the app. Clamp the cursor to the nearest preceding cell that has a content offset instead of unwrapping, mirroring the defensive clamping already in InitialCursorState::new. Add a regression guard exercising a wide-char reflow.
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @Im-Tae on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
github-actions
Bot
added
the
external-contributor
|
Every PR must be linked to a same-repo issue before Oz can review it. This PR is linked to #13040, but no linked issue is marked See the contribution guidelines for the full readiness model. Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Every PR must be linked to a same-repo issue before Oz can review it.
This PR is linked to #13040, but no linked issue is marked ready-to-implement yet. Only repository maintainers apply that label, so please wait for a maintainer to mark the issue. Once it is marked, push a new commit or comment /oz-review to re-trigger review.
See the contribution guidelines for the full readiness model.
Powered by Oz
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR hardens terminal grid resize cursor remapping so wide/CJK terminal content no longer aborts when content_offset_at_point cannot resolve the exact cursor cell, and it adds a regression test that resizes mixed-width content across several column counts.
Concerns
- For this user-facing terminal behavior change, please include screenshots or a short screen recording demonstrating resize with wide/CJK characters working end to end. The PR description currently leaves manual testing unchecked and has an empty Screenshots / Videos section.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR hardens terminal grid resizing when flat-storage cursor offset lookup fails for wide/CJK content, and adds a smoke-style resize test covering mixed-width text. Visual evidence is included in the PR description.
Concerns
- The new fallback correctly searches backward for column-local lookup failures, but if every lookup in the row fails it returns
ByteOffset::zero(). That offset can be invalid after scrollback truncation and can still panic during the reversecontent_offset_to_pointconversion.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @Im-Tae on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
content_offset_for_point fell back to ByteOffset::zero() when no cell in the cursor's row resolved to a content offset. Content offsets count every byte the grid has ever seen, so the earliest tracked offset is generally greater than zero (and grows as scrollback is truncated from the front); a zero offset sits before the first stored row, which content_offset_to_point rejects, re-introducing the very panic this function exists to prevent. Walk backward over earlier rows as well as columns, snapping to the nearest cell flat storage can actually resolve, so the fallback yields a valid offset from a real row instead of a fabricated zero.
Im-Tae
force-pushed
the
imtae/fix-cjk-resize-crash
branch
from
92925a6 to
8e6ed2e
Compare
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @Im-Tae on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
@cla-bot check |
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @Im-Tae on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR hardens terminal grid resize cursor remapping for wide/CJK content by replacing the direct content_offset_at_point(...).expect(...) with a backward search for a resolvable content offset, and adds a regression test that resizes mixed-width terminal content across several widths. The PR description includes before/after recordings, and there is no approved repository spec context to compare against.
Concerns
- The new no-resolvable-cell fallback returns
ByteOffset::zero()even though the helper documents that zero can be before the first stored row and will be rejected by the latercontent_offset_to_point(...).expect(...)conversion, preserving a panic path in the defensive branch.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| // Flat storage tracks no resolvable cell at or before the cursor (e.g. it is | ||
| // empty). There is nothing to anchor the cursor to, so fall back to the | ||
| // start of content. | ||
| ByteOffset::zero() |
There was a problem hiding this comment.
into_cursor_point will call content_offset_to_point(ByteOffset::zero()).expect(...) and can panic again. Return a real stored offset or make the no-anchor case non-panicking before converting back.
The earlier fallback returned ByteOffset::zero() when no cell at or before the cursor resolved to a content offset, contradicting the function's own invariant. Flat storage that is empty, or whose first stored row begins after zero (scrollback truncated from the front), has no valid offset of zero, so content_offset_to_point would reject it and the .expect() in into_cursor_point would panic again on resize. Have content_offset_for_point return Option<ByteOffset> (None when nothing resolves) and convert offsets back through resolved_point_or_origin, which homes the cursor to the grid origin on a missing or unmappable offset instead of unwrapping. This removes the last panicking path through the resize cursor remap.
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR replaces the panic-prone cursor content-offset conversion during terminal grid resize with defensive clamping/fallback logic and adds a wide-character resize regression test. The PR description includes before/after recordings, and there is no approved spec context to compare against.
Concerns
- The new no-anchor/unmapped-offset fallback can still flow through the
AtCellAfterPointadjustment, so it may advance from origin instead of homing the cursor as documented.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| FinalCursorState::AtPoint(resolved_point_or_origin(grid, offset)) | ||
| } | ||
| Self::AtCellAfterPoint(offset) => { | ||
| let mut point = resolved_point_or_origin(grid, offset); |
There was a problem hiding this comment.
AtCellAfterPoint to the origin: when resolved_point_or_origin returns { row: 0, col: 0 } for None or an unmapped offset, the unchanged cell-after logic below can advance it to col 1 or set input_needs_wrap. Return an AtPoint(origin) or otherwise distinguish fallback from a real anchor before applying cell-after semantics.
resolve_content_offset now returns Option<Point>: None when flat storage holds no anchor, or when a concrete offset falls before the first stored row and cannot be mapped. The AtPoint arm falls back to the origin, and the AtCellAfterPoint arm returns AtPoint(origin) directly instead of feeding the fabricated origin through the cell-after logic, which would otherwise advance it to column 1 or set input_needs_wrap. This keeps the no-anchor case a true cursor home and the real-anchor case unchanged.
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: Comment Powered by Oz |
oz-for-oss
Bot
dismissed stale reviews from themself
Oz no longer requests changes for this pull request after the latest automated review.
There was a problem hiding this comment.
Overview
This PR hardens terminal grid resize cursor remapping for wide/CJK character content by replacing panic-on-missing-content-offset paths with defensive fallback resolution, and adds a regression test that resizes mixed ASCII/CJK terminal content across narrow widths.
Concerns
- No blocking correctness, security, or spec-alignment concerns found in the attached diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
1 participant
Description
Fixes a crash (SIGABRT /
abort()) that quits Warp when a terminal containingwide (full-width / CJK) characters — e.g. a Korean directory name shown in the
shell prompt — is resized (window/pane layout change, prompt redraw, etc.).
From the attached macOS crash report, the panicking frame is unambiguous:
Call chain:
TerminalView::resize_internal → TerminalModel::resize → BlockList::resize → GridHandler::resize → resize_storage → InitialCursorState::into_content_offset → panicRoot cause
During
resize_storage, each cursor (current / saved / max) is converted from alive-grid
Pointto a flat-storage content offset viaFlatStorage::content_offset_at_point. That function returnsErrfor a pointwhose column/row does not map to tracked content (
ColumnExceedsContent,NonZeroColumnInEmptyRow,RowOutOfBounds,MissingGraphemeSizing). Theconversion unwrapped the result with
.expect(...), so any such case aborted thewhole process instead of degrading.
Wide characters make this reachable: a wide char spans two columns but is a
single grapheme, so the grid's per-column accounting (which also counts wide-char
spacer cells) can diverge from flat storage's grapheme-run accounting.
content_offset_at_point's own doc comment flagsWIDE_CHAR_SPACER/LEADING_WIDE_CHAR_SPACERcells as cases needing coverage, andresize_storagealready carries a maintainer TODO noting that
max_cursor_pointcan end up with"content after it" — i.e. in a position that does not line up with content.
Fix
into_content_offsetno longer unwraps. The newcontent_offset_for_pointhelper clamps the column back toward the start of the row until it finds a cell
that does have a content offset (column 0 of a non-empty row always resolves, so
this terminates), logging a warning. This mirrors the defensive cursor clamping
already present in
InitialCursorState::new, and turns an app-killing abort intoa one-cell cursor placement difference in an already-anomalous state.
The sibling
.expect("content offset should be valid")inCursorContentOffset::into_cursor_point(the reverse conversion) is intentionallyleft untouched to keep this change focused on the confirmed crash site; happy to
harden it in a follow-up if maintainers prefer.
Linked Issue
Fixes #9013
Fixes #8881
These are the same
InitialCursorState::into_content_offsetunwrap panic duringterminal resize, both already labeled
ready-to-implement. #13040 (my report)was triaged as a duplicate of these.
ready-to-implement.Testing
test_resize_with_wide_chars_does_not_panicingrid_handler_tests.rs, which drives mixed narrow/wide (CJK) prompt-and-outputcontent through a reflow across a range of widths (including widths too narrow
to hold a wide char in the trailing column) and asserts the resizes complete.
cargo test -p warp -- terminal::model::gridpasses (full grid module).Transparency on reproduction: I was not able to reproduce the exact
production
content_offset_at_pointErrin a unit test, because the in-memorygrid pads rows to full width with blank cells, which makes the column/empty-row
error paths unreachable from the public
GridHandlerAPI (verified byinstrumenting the conversion). The crash report nonetheless identifies the
.expect()site unambiguously, and the fix removes that abort. I'd appreciate amaintainer sanity-check against Sentry telemetry for the exact triggering state.
./script/runScreenshots / Videos
before (crash)
https://github.com/user-attachments/assets/52b53e3c-81a6-44e0-b192-990206f50454
after (no crash)
https://github.com/user-attachments/assets/65a59338-de95-48e6-832f-f5c43cd2d8e4
Agent Mode