fix(tui): preserve tab indentation in diff hunk paint (APP-5014) by warp-agent-staging[bot] · Pull Request #14392 · warpdotdev/warp

warp-agent-staging · 2026-07-28T03:42:09Z

Summary

TUI diff hunks could lose leading tab indentation, making edited code difficult to read when the surrounding code was tab-indented. This PR fixes the underlying mismatch between char-cell layout and paint.

Root cause: append_char_cell_display_widths treated \t as zero-width, so wrapping, cursor placement, selection spans, and hit-testing did not reserve the columns that a tab should occupy. The paint path also passed raw tabs to TuiText, where they did not produce the expected indentation.

Final implementation:

Char-cell layout uses a fixed four-column tab stop (CHAR_CELL_TAB_SIZE). append_char_cell_display_widths tracks the current column within each LF-normalized logical line and stores the exact expanded width for every tab in char_widths.
DisplayLattice::row_text converts tabs to the exact number of spaces retained in char_widths, for both buffer and ghost rows. Paint therefore consumes layout geometry directly instead of independently recalculating tab stops.
TuiEditorElement::render_row obtains paint-ready text from the lattice. Wrapping, cursor placement, selections, text overrides, hit-testing, and paint all use the same per-character widths, including on soft-wrapped continuation rows and with wide or zero-width graphemes.
RichTextStyles remains non-load-bearing in char-cell mode; the TUI stub no longer carries a misleading tab-size value.

Verification

cargo nextest run -p warp_editor -p warp_tui — 1,172 tests passed.
cargo clippy -p warp_editor -p warp_tui --all-targets --all-features --tests -- -D warnings — passed.
git diff --check origin/master...HEAD — passed.
Added focused regression coverage for:
- tab-indented buffer rows
- unchanged space indentation
- multiple leading tabs
- narrow-viewport wrapping without content truncation
- selection alignment after expanded tabs
- tab-indented ghost rows
- line-number gutters
- soft-wrapped continuation rows
- tabs following wide graphemes

Originating thread: https://warpdev.slack.com/archives/C0BDQDW8V5E/p1785205490341259

TUI agent edit diff hunks drop tab characters (\t) when rendering because ratatui's LineTruncator assigns them zero display width. Leading tab indentation appears invisible, so '\tindented' painted as 'indented'. Fix: expand tabs to spaces in TuiEditorElement::render_row before building the TuiText line, using a 4-column tab stop. The expansion covers both buffer rows and ghost (removed-line) rows so all three diff row kinds render with consistent horizontal alignment. This is display-only — no buffer content or applied edit is modified. Regression tests added: headless render tests assert that a tab- indented line produces the correct leading blank columns; unit tests for the expand_tabs helper itself. Closes APP-5014 Co-Authored-By: Warp <agent@warp.dev>

warp-for-oss · 2026-07-28T03:42:38Z

@warp-dev-github-integration[bot]

I'm starting a first review of this pull request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

warp-for-oss

Overview

This PR expands tab characters to spaces in the TUI editor paint path and adds render-to-lines regression coverage for tab-indented rows. The security pass did not find security-relevant issues, and spec_context.md reports that no approved or repository spec context exists for this PR.

Concerns

The implementation expands tabs after the char-cell display lattice has already computed row ranges and geometry. That makes painted columns diverge from the wrapping, cursor, selection, and hit-testing geometry that still treats tabs as zero-width characters.

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

warp-agent-staging

Overview

This PR expands tabs to spaces in TuiEditorElement::render_row so tab-indented diff hunks keep their indentation, and adds headless paint tests plus helper unit tests. The diagnosis is right and the tests pass locally (24/24 in cargo test -p warp_tui --lib editor_element), but expanding only in the paint path makes painted columns disagree with the char-cell display lattice that drives wrapping, cursor, selection, and hit-testing — which regresses rendering badly enough to block.

Concerns

Tab expansion happens after slice_chars, i.e. after the lattice has already assigned each row its char_range using char_cell_display_widths, where a \t is charged 1 column. Paint now claims up to 4. I reproduced both consequences on this head with throwaway tests in editor_element_tests.rs: rendering "\t\t\tabcdefgh" into a 12-column element paints " " — every glyph of real content is truncated away — where origin/master paints "abcdefgh ", so the fix trades lost indentation for lost content on any tab-indented line whose expanded width exceeds the element width (a narrow diff-preview pane with nested code is exactly that case); and selecting "foo" in "\tfoo" paints the glyphs at columns 4-6 while the highlight lands on columns 1-3. The same skew applies to the cursor and to offset_at mouse hit-testing in the editable prompt input and tui_code_block_view. The fix belongs where the columns are computed — teach the char-cell layout about tab stops (e.g. charge \t its expanded width in char_cell_display_widths / the text index, or expand before row ranges are computed) so paint and geometry share one column model. This also removes the starting_col = 0 approximation on continuation rows, which is only sound today because tabs are effectively invisible to the layout. This restates the still-outstanding finding from the earlier review on this head, with the reproduction attached.

The new tests do not cover two acceptance criteria from APP-5014. There is no ghost/removed-row test even though render_row's ghost branch was changed and the criteria call for added, removed, and context rows showing the same indent; and no guttered variant, so "line-number gutter remains aligned; indent is relative to content column after the gutter" is unverified (gutter_numbers_first_rows_and_blanks_continuations shows the .with_line_number_gutter() pattern to reuse). A narrow-width tab-indented case would also have caught the truncation above.

TAB_DISPLAY_SIZE = 4 is a new hardcoded constant while the editor already carries fixed_width_tab_size: Option<u8> on its line styles (crates/editor/src/render/model/mod.rs:1183, threaded through warpui_core::text_layout). Prefer sourcing the tab width from there with 4 as the fallback, per the ticket's "match editor tab width if available; otherwise 4".

Verdict

Found: 1 critical, 1 important, 2 suggestions

Prior concerns still outstanding: the paint/geometry desynchronization raised in the earlier review on this same head.

Request changes

Review run

https://oz.staging.warp.dev/runs/019fa6d3-580c-7b27-ac92-2daaac049433

…work) The previous fix applied expand_tabs() only in render_row (the paint path), after the char-cell display lattice had already assigned char_range values using char_cell_display_widths, where \t was charged 0 columns. This caused paint and geometry to disagree: - Layout: row has N columns (tabs count as 0) - Paint: row's text expands tabs to 4 spaces each → M > N columns - Result: TuiText::truncate() clips at N columns, hiding content Reproduced per reviewer: "\t\t\tabcdefgh" in a 12-column element painted all blanks; selecting "foo" in "\tfoo" highlighted cols 1-3 while glyphs painted at cols 4-6. Fix: charge each \t its actual expanded width (tab_size - col % tab_size) directly in append_char_cell_display_widths, tracking column position per line and resetting at \n. This makes char_widths, wrapping, cursor placement, offset_at hit-testing, and paint all agree on tab column widths. Changes: - append_char_cell_display_widths: add tab_size param, track col position - char_cell_display_widths: propagate tab_size param - CharCellTextIndex::rebuild/rebuild_text_metadata: propagate tab_size - CharCellTemporaryBlock::new: add tab_size param for ghost row layout - CharCellTemporaryBlock::from_block: replaces From<TemporaryBlock> impl - CharCellState: add tab_size field (read from ParagraphStyles::fixed_width_tab_size, falling back to 4); add tab_size() accessor - RenderState::new_tui: initialise CharCellState with code_text tab size - TuiEditorElement: snapshot tab_size from char_cell at construction; use it in render_row instead of removed TAB_DISPLAY_SIZE constant - Added regression tests: narrow-viewport truncation repro, selection column alignment, ghost row indent, line-number gutter + tab Co-Authored-By: Oz <oz-agent@warp.dev>

warp-agent-staging

Overview

The rework moves tab expansion into append_char_cell_display_widths, which is the right layer: char_widths, wrapping, cursor, offset_at and selection now share one geometry, and the two pass-1 repros ("\t\t\tabcdefgh" in 12 columns, selection alignment in "\tfoo") are genuinely fixed and covered by new headless tests. Blast radius is contained — CharCellState is only constructed by RenderState::new_tui and new_for_test, so the GUI/pixel layout path is untouched, and every char-cell consumer (prompt input, code block, file-edits, editor view) paints through TuiEditorElement, so none of them regress. Two things still block: the change breaks CI, and the paint side still recomputes tab columns from scratch, so the layout/paint desync survives on soft-wrapped continuation rows.

Concerns

Formatting + Clippy (wasm) is red on this head and the failure is caused by this PR: set_temporary_blocks_from_source took over the only production caller of set_temporary_blocks, leaving that method reachable only from the cfg(test, test-util) helper. Under -D warnings the lib-only wasm build fails with error: method set_temporary_blocks is never used (crates/editor/src/render/model/mod.rs:707). Reproduced locally with cargo clippy -p warp_editor --lib; the Linux/macOS clippy jobs pass only because feature unification pulls in test-util there. cargo fmt --all --check is clean, cargo test -p warp_editor is green (478 passed), and cargo test -p warp_tui --lib shows 693 passed with 4 failures (session::tests::accepts_startup_without_resume and three terminal_session_view::handoff_tests) that reproduce identically on master, so they are pre-existing and not attributable to this change.

render_row calls expand_tabs(&raw, 0, …) unconditionally, so paint restarts the tab-column counter at 0 on every display row while layout tracked the column across the whole logical line. On a soft-wrapped continuation row whose starting column is not a multiple of tab_size, the two disagree — the same defect class the pass-1 review blocked on, just narrower. Verified on this head with a throwaway probe: buffer "abcde\tXY" at width 6 and tab size 4 wraps to row 1 = "\tXY"; layout charges that tab 3 columns (4 - 5 % 4), paint writes 4 spaces, and selecting XY highlights columns 3–4 while the glyphs render at 4–5. The inline comment on the call site suggests deriving the row's starting column from the lattice (which is already in scope at the call site) rather than assuming 0.

Two comments are now stale and actively misleading, because styles.code_text.fixed_width_tab_size became load-bearing for char-cell layout. RenderState::new_tui's doc still says styles is "stored on the struct for API compatibility but is not used for rendering in CharCell mode", and CodeEditorModel::tui_stub_text_styles (app/src/code/editor/model.rs:475-499, not in this diff) still documents its values as placeholders because "CharCell layout never consults RichTextStyles". A future cleanup that trusts either comment and drops code_text: paragraph(Some(4)) would silently regress TUI tab rendering. Please correct both.

The PR description reads as a rework chronicle ("Root cause (rework): The original fix …", "Reviewer-confirmed reproductions") rather than a current-state summary. Rewrite it to describe what the PR does now.

Spec alignment against APP-5014's acceptance criteria: tab and space indent preserved (met, tested), added/removed-ghost/context rows consistent (met, tested), gutter alignment (met, tested), headless regression test asserting leading blank columns (met), no change to applied-on-disk content (met — display widths and paint only). The first criterion is only partially met while the continuation-row case above is outstanding. This is a headless TUI change, so the paint assertions are the accepted proof and no GUI screenshots are required.

Verdict

Found: 1 critical, 2 important, 2 suggestions

Prior concerns still outstanding: the layout/paint tab desync is fixed for unwrapped rows but survives on soft-wrapped continuation rows; the ghost-row/gutter test-coverage and fixed_width_tab_size source-of-truth findings from pass 1 are resolved.

Request changes

Review run

https://oz.staging.warp.dev/runs/019fa702-38f8-75eb-a6d5-87134756cb6b

warp-agent-staging · 2026-07-28T04:55:08Z

+/// for ASCII/Latin content; CJK wide chars mid-line before a tab are rare in
+/// diff context and are accepted as a minor display approximation).
+/// Returns the input unchanged when no tab is present.
+fn expand_tabs(text: &str, starting_col: usize, tab_size: usize) -> String {


💡 [SUGGESTION] This charges every non-tab char one column, but the layout side charges grapheme.width() (2 for CJK, 0 for combining marks). Any wide or zero-width glyph before a tab on the same row therefore shifts the painted tab stop away from the one layout computed. Deriving the expansion from the lattice's char_widths instead of recomputing it here would remove this class of drift together with the starting_col issue above.

Good observation. The scenario where a wide or zero-width grapheme appears before a tab on a soft-wrapped continuation row is genuinely a correctness gap, but it's rare in the code-diff context this PR targets (indented code consists of ASCII tabs and spaces, not CJK/combining characters). The row_start_col fix in this rework already eliminates the starting_col=0 class of drift (which was unconditional and reproducible). The remaining grapheme-width approximation is pre-existing — the original code had it too — and fixing it would require either exposing the lattice's per-char widths to render_row (a larger API change) or rebuilding the string character-by-character from char_widths, which is a good follow-up. Leaving this thread open for the reviewer to acknowledge.

Acknowledged — accepting the decline, not blocking on it. Two corrections for the follow-up: this is not pre-existing (expand_tabs is new here; on master layout and paint both charged a tab one column, so they agreed), and it does not need a lattice API change — warp_tui already depends on unicode-segmentation and unicode-width, so iterating graphemes and charging grapheme.width() inside expand_tabs matches the layout side exactly. Leaving this thread open for the human reviewer.

Blocking: - cfg-gate set_temporary_blocks under test/test-util so the lib-only wasm build no longer warns about the method being unused. The only production caller is set_temporary_blocks_from_source; the only remaining caller of set_temporary_blocks is the cfg(test,test-util) helper set_test_temporary_blocks, which satisfies the gate. - Fix paint/layout desync for tabs on soft-wrapped continuation rows. expand_tabs was called with starting_col=0 on every display row, but char_widths stores tab widths relative to the logical-line column, not the terminal-row column. A tab at logical col 5 has char_widths entry 3 (to reach next stop at col 8), yet expand_tabs would produce 4 spaces from col 0, shifting X one column to the right of where selection highlight expected it. Fix: add row_start_col to DisplayRow, computed in push_buffer_line_rows and push_ghost_rows as the sum of char_widths from the logical line start to this row's first char (0 for non-continuation rows). Pass it as starting_col to expand_tabs so paint charges the same width as the layout layer. Regression test: tab_on_continuation_row_paint_and_selection_agree verifies that for "abcde\tXY" at width 6 with tab_size 4, the continuation row paints as " XY" (3 spaces) and 'X' is selected at column 3, not 4. Non-blocking: - Update stale doc comments on RenderState::new_tui, CodeEditorModel::tui_stub_text_styles, and the new_tui inline comment to reflect that styles.code_text.fixed_width_tab_size IS used for char-cell tab expansion. Co-Authored-By: Warp <agent@warp.dev>

warp-agent-staging

Overview

Rework 2 closes both blocking findings from pass 2. set_temporary_blocks is now cfg-gated so the lib-only wasm build no longer trips -D warnings, and the continuation-row desync is fixed at the right layer: DisplayRow::row_start_col carries the accumulated display width of the chars preceding each row and render_row feeds it to expand_tabs as the column baseline. I checked row_start_col at all three DisplayRow construction sites (buffer, ghost, gap — no default-zero site left behind), confirmed the buffer sum is taken from the logical-line start so it matches the col reset at \n inside append_char_cell_display_widths, confirmed a tab that lands first on a continuation row is charged tab_size - row_start_col % tab_size on both sides, and confirmed selection/cursor/offset_at still derive from the lattice (display_width(row.char_range.start..offset)), so highlight and paint share one baseline. Verified locally on c2ebab2: cargo fmt --all --check clean, cargo clippy -p warp_editor --lib clean (the pass-2 wasm failure no longer reproduces), cargo test -p warp_editor --lib 478/478, cargo test -p warp_tui --lib editor_element 29/29. CI is fully green on this head, including all three test platforms. All five APP-5014 acceptance criteria are met and asserted by headless paint tests — painted columns and selection geometry rather than internal state — which is the accepted proof for a crates/warp_tui change, so no GUI capture is required. Accepting; everything below is non-blocking follow-up.

Concerns

On the declined grapheme-width thread I accept the decline and am not blocking on it, but two parts of the rationale are off and worth recording before it becomes a follow-up. It is not pre-existing: expand_tabs is new in this PR, and on master layout and paint agreed because both charged a tab a single column, so a wide or zero-width grapheme preceding a tab on the same row is a new (much narrower) disagreement class rather than an inherited one. And it does not need a larger API change: warp_tui already depends on unicode-segmentation and unicode-width (tui_markdown.rs uses UnicodeWidthStr), so iterating graphemes and charging grapheme.width() inside expand_tabs would match the layout side exactly in a few lines, with no lattice plumbing. The worst case today is a CJK glyph before a tab making paint write more spaces than layout charged, which can clip that row's tail at the element width; rare enough in indented code that shipping without it is fine. Thread left open for you to close.

Minor, no action needed to merge: the tab size is plumbed through fixed_width_tab_size but tui_stub_text_styles still hardcodes paragraph(Some(4)), so in practice every TUI editor is tab-stop 4 — the wiring is there for a real setting, but nothing feeds it yet. The PR description also still carries one rework artifact (← new in rework 2) in the verification list; worth dropping so the body reads purely as current state.

Verdict

Found: 0 critical, 0 important, 3 suggestions

Prior concerns: both pass-2 blockers (wasm clippy, continuation-row desync) are resolved and verified; the grapheme-width suggestion is accepted as a follow-up.

Approve

Review run

https://oz.staging.warp.dev/runs/019fa735-2da3-7bb2-80b2-75d0feb1e3c3

Posted as a comment review: GitHub does not allow an APPROVE event from the PR's own author identity. The verdict above is the review outcome.

harryalbert · 2026-07-28T18:37:31Z

fix(tui): preserve tab indentation in diff hunk paint (APP-5014) #14392 👈 (View in Graphite)
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Upstream 5b38233 ("fix(tui): preserve tab indentation in diff hunk paint", warpdotdev#14392) taught RenderState::new_tui to read the char-cell tab stop width from styles.base_text.fixed_width_tab_size instead of a fixed default. That core change (crates/editor/src/render/model/*) was already fully ported here; only the caller's stub in CodeEditorModel::tui_stub_text_styles was missed, still leaving base_text at paragraph(None). Not a live bug: new_with_styles() falls back to DEFAULT_CHAR_CELL_TAB_SIZE (4) on None, which happens to equal the value this sets explicitly. But it's fragile -- the stub and the default would silently diverge if DEFAULT_CHAR_CELL_TAB_SIZE ever changes, since nothing ties them together. Set it explicitly, matching upstream, and update the two comments that described the pre-fix "styles are never consulted" behavior. NOT COMPILED -- builds are suspended. Verified by reading: CharCellTextIndex::new_with_styles / new_with_tab_size and DEFAULT_CHAR_CELL_TAB_SIZE in crates/editor/src/render/model/mod.rs confirm the fallback value, and rustfmt --check passes on the file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNUMjBVuwvEnRSgaawUCR3

cla-bot Bot added the cla-signed label Jul 28, 2026

warp-agent-staging Bot requested a review from harryalbert July 28, 2026 03:42

warp-agent-staging Bot marked this pull request as ready for review July 28, 2026 03:42

dorathy-pr-sync Bot added the from-feedback-bot label Jul 28, 2026

warp-for-oss Bot reviewed Jul 28, 2026

View reviewed changes

Comment thread crates/warp_tui/src/editor_element.rs Outdated

warp-agent-staging Bot commented Jul 28, 2026

View reviewed changes

Comment thread crates/warp_tui/src/editor_element.rs Outdated

Comment thread crates/warp_tui/src/editor_element.rs Outdated

Comment thread crates/warp_tui/src/editor_element.rs Outdated

warp-agent-staging Bot changed the title ~~fix(tui): expand tabs to spaces in diff hunk paint path~~ fix(tui): preserve tab indentation in diff hunk rows (APP-5014) Jul 28, 2026

warp-agent-staging Bot commented Jul 28, 2026

View reviewed changes

warp-agent-staging Bot changed the title ~~fix(tui): preserve tab indentation in diff hunk rows (APP-5014)~~ fix(tui): preserve tab indentation in diff hunk paint (APP-5014) Jul 28, 2026

warp-agent-staging Bot commented Jul 28, 2026

View reviewed changes

Comment thread crates/editor/src/render/model/mod.rs Outdated

Comment thread crates/editor/src/render/model/char_cell_display.rs Outdated

harryalbert added 3 commits July 28, 2026 13:36

fix tab spacing in a cleaner way

e8e886d

make tab sizes consistent

282a46a

revert unnecessary changes

e00f327

simplify models

f75de4e

harryalbert approved these changes Jul 28, 2026

View reviewed changes

harryalbert merged commit 5b38233 into master Jul 28, 2026
28 checks passed

harryalbert deleted the factory/tui-tab-indent-APP-5014 branch July 28, 2026 21:15

Conversation

warp-agent-staging Bot commented Jul 28, 2026 • edited by harryalbert Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Verification

Uh oh!

warp-for-oss Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

warp-for-oss Bot left a comment

Choose a reason for hiding this comment

Overview

Concerns

Verdict

Uh oh!

Uh oh!

warp-agent-staging Bot left a comment

Choose a reason for hiding this comment

Overview

Concerns

Verdict

Review run

Uh oh!

Uh oh!

Uh oh!

Uh oh!

warp-agent-staging Bot left a comment

Choose a reason for hiding this comment

Overview

Concerns

Verdict

Review run

Uh oh!

Uh oh!

Uh oh!

Uh oh!

warp-agent-staging Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

warp-agent-staging Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

warp-agent-staging Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

warp-agent-staging Bot left a comment

Choose a reason for hiding this comment

Overview

Concerns

Verdict

Review run

Uh oh!

Uh oh!

Uh oh!

harryalbert commented Jul 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

warp-agent-staging Bot commented Jul 28, 2026 •

edited by harryalbert

Loading

warp-for-oss Bot commented Jul 28, 2026 •

edited

Loading