fix(ui): align split diffs with wide characters#353
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Greptile SummaryThis PR fixes split-diff separator misalignment when code contains CJK characters or emoji by replacing all character-count-based width measurements and string slices with terminal-cell-aware equivalents backed by a new
Confidence Score: 4/5Safe to merge for the stated fix; the separator alignment path is correct and well-covered by both unit and PTY tests. The core alignment fix is correct and thoroughly tested. The one rough edge is in src/ui/diff/renderRows.tsx — specifically the Important Files Changed
Sequence DiagramsequenceDiagram
participant DF as DiffFile
participant CC as codeColumns.ts
participant RR as renderRows.tsx
participant TXT as text.ts
DF->>CC: maxFileCodeLineWidth(file)
CC->>CC: check WeakMap cache (file.metadata)
CC->>TXT: measureTextWidth(expandDiffTabs(line))
TXT->>TXT: ASCII fast path (.length) or stringWidth()
TXT-->>CC: terminal cell count
CC-->>CC: cache result in WeakMap
CC-->>RR: max column width (terminal cells)
RR->>RR: buildPlainSplitCellLines(cell, leftWidth)
RR->>TXT: sliceTextByWidth(joined spans, offset, contentWidth)
TXT->>TXT: grapheme cluster iteration via Intl.Segmenter
TXT-->>RR: "{ text, width } — no wide char split"
RR->>TXT: padText(visible.text, contentWidth)
TXT-->>RR: exactly contentWidth terminal cells
RR->>TXT: padText(leftPrefix + spansText, leftWidth)
TXT-->>RR: normalizedLeft — exactly leftWidth cells
Note over RR: separator always at leftWidth terminal cells
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
src/ui/diff/renderRows.tsx:66-76
**Zero-width spans silently dropped after offset refactor**
`sliceSpansWindow` skips any span where `measureTextWidth(span.text) === 0` unconditionally. Because `remainingOffset` is always `≥ 0`, the guard `remainingOffset >= spanWidth` is always `true` for zero-visual-width spans (e.g., a span whose text is only combining marks such as `\u0301`). Pre-PR the check was against `span.text.length`, so a non-empty span with `length ≥ 1` but zero visual width was only skipped while the initial offset was still being consumed. After this change, such spans are permanently dropped even at `offset = 0`. The same issue appears in `wrapSpans` at the analogous `while (offset < spanWidth)` guard — that loop never runs for a span with `spanWidth === 0`.
In practice, syntax highlighters don't emit spans containing only combining characters, so real-world impact is minimal. But the contract change is worth documenting, or guarding with an explicit `spanWidth === 0` branch that passes the span through unconditionally.
Reviews (1): Last reviewed commit: "fix(ui): align split diffs with wide cha..." | Re-trigger Greptile |
| const spanWidth = measureTextWidth(span.text); | ||
| if (remainingOffset >= spanWidth) { | ||
| remainingOffset -= spanWidth; | ||
| continue; | ||
| } | ||
|
|
||
| const start = remainingOffset; | ||
| const text = span.text.slice(start, start + remaining); | ||
| const visible = sliceTextByWidth(span.text, remainingOffset, remaining); | ||
| remainingOffset = 0; | ||
|
|
||
| if (text.length === 0) { | ||
| if (visible.text.length === 0) { | ||
| continue; |
There was a problem hiding this comment.
sliceSpansWindow skips any span where measureTextWidth(span.text) === 0 unconditionally. Because remainingOffset is always ≥ 0, the guard remainingOffset >= spanWidth is always true for zero-visual-width spans (e.g., a span whose text is only combining marks such as \u0301). Pre-PR the check was against span.text.length, so a non-empty span with length ≥ 1 but zero visual width was only skipped while the initial offset was still being consumed. After this change, such spans are permanently dropped even at offset = 0. The same issue appears in wrapSpans at the analogous while (offset < spanWidth) guard — that loop never runs for a span with spanWidth === 0.
In practice, syntax highlighters don't emit spans containing only combining characters, so real-world impact is minimal. But the contract change is worth documenting, or guarding with an explicit spanWidth === 0 branch that passes the span through unconditionally.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/diff/renderRows.tsx
Line: 66-76
Comment:
**Zero-width spans silently dropped after offset refactor**
`sliceSpansWindow` skips any span where `measureTextWidth(span.text) === 0` unconditionally. Because `remainingOffset` is always `≥ 0`, the guard `remainingOffset >= spanWidth` is always `true` for zero-visual-width spans (e.g., a span whose text is only combining marks such as `\u0301`). Pre-PR the check was against `span.text.length`, so a non-empty span with `length ≥ 1` but zero visual width was only skipped while the initial offset was still being consumed. After this change, such spans are permanently dropped even at `offset = 0`. The same issue appears in `wrapSpans` at the analogous `while (offset < spanWidth)` guard — that loop never runs for a span with `spanWidth === 0`.
In practice, syntax highlighters don't emit spans containing only combining characters, so real-world impact is minimal. But the contract change is worth documenting, or guarding with an explicit `spanWidth === 0` branch that passes the span through unconditionally.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Good catch. I added explicit zero-width span handling so combining-mark-only spans are preserved in both the nowrap slicing path and wrapped row path, and added a DiffRowView regression test that exercises both modes.
This comment was generated by Pi using OpenAI ChatGPT
73dcea1 to
9ecee33
Compare
9ecee33 to
9e55d24
Compare
Summary
Closes #324
Tests
bun run typecheckbun run lintbun testbun test test/pty/ui-integration.test.ts --test-name-pattern "wide characters"This PR description was generated by Pi using OpenAI ChatGPT