fix: Fix Text Wrapping & Multi-Byte Character Boundary Calculation on Window Resize (#3339) - #3362
Conversation
github-actions
Bot
added
type:bug
📝 WalkthroughWalkthroughAdds ChangesUnicode text wrapping
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/core/test/textWrap.test.ts (1)
4-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing CJK and 20/40-column regression coverage requested by issue
#3339.The linked issue asks for regression coverage of multibyte text at 20- and 40-column widths, and for handling double-width CJK characters. This file only covers emoji (lines 11-19) at a width of 4. Add tests with CJK text (for example Chinese/Japanese characters, each width 2) wrapped at 20 and 40 columns to close the gap called out in the issue.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/test/textWrap.test.ts` around lines 4 - 31, Add regression tests in the wrapTextWithWidth test suite for double-width CJK text, covering maxWidth values of 20 and 40 columns. Assert that characters are not split, each line stays within the requested visual width, and the expected wrapping differs appropriately between the two widths; keep the existing emoji coverage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/utils/textWrap.ts`:
- Around line 3-6: The WrapOptions.hard setting is ignored by wrapTextWithWidth,
so soft wrapping does not preserve word boundaries. Update wrapTextWithWidth’s
wrap loop to scan backward for the last whitespace cluster before the width
limit when hard is falsy, while retaining strict cluster-width breaks when hard
is true; preserve existing behavior when no suitable boundary exists.
- Around line 19-26: Update the grapheme-cluster construction in the
text-wrapping utility to recognize ANSI escape sequences as zero-width atomic
tokens before or during segmentation. Preserve each complete sequence as one
token, exclude it from visible-width measurement, and ensure wrapping never
splits a sequence across lines while retaining it in the output.
In `@packages/core/test/textWrap.test.ts`:
- Around line 27-30: Update the test “should handle zero or negative maxWidth
gracefully” to add an assertion calling wrapTextWithWidth with a negative
maxWidth such as -5 and expecting an empty array, while preserving the existing
zero-width and empty-input assertions.
---
Nitpick comments:
In `@packages/core/test/textWrap.test.ts`:
- Around line 4-31: Add regression tests in the wrapTextWithWidth test suite for
double-width CJK text, covering maxWidth values of 20 and 40 columns. Assert
that characters are not split, each line stays within the requested visual
width, and the expected wrapping differs appropriately between the two widths;
keep the existing emoji coverage unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b605f44-f545-44e7-ba39-c820000f99cf
📒 Files selected for processing (2)
packages/core/src/utils/textWrap.tspackages/core/test/textWrap.test.ts
| export interface WrapOptions { | ||
| hard?: boolean; | ||
| trim?: boolean; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
hard option has no effect; word-boundary preservation is coincidental.
WrapOptions.hard (line 4) is never read in wrapTextWithWidth. The wrap loop (lines 42-49) always breaks strictly on cumulative cluster width, with no lookback for a word boundary. Passing hard: true or hard: false (or omitting it) produces identical output.
This also means the "without breaking words unnecessarily" behavior exercised by the ASCII test in textWrap.test.ts (lines 5-9) only holds because the wrap point happens to land on a space in that specific input. For example, wrapping 'Hello Worldss Test' at width 12 would split 'Worldss' mid-word, since there is no word-boundary logic. Either implement soft-wrap behavior gated by hard (scan backward for the last whitespace cluster before the width limit when hard is falsy) or remove the unused option from the public interface until it is implemented.
Also applies to: 42-49
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/utils/textWrap.ts` around lines 3 - 6, The WrapOptions.hard
setting is ignored by wrapTextWithWidth, so soft wrapping does not preserve word
boundaries. Update wrapTextWithWidth’s wrap loop to scan backward for the last
whitespace cluster before the width limit when hard is falsy, while retaining
strict cluster-width breaks when hard is true; preserve existing behavior when
no suitable boundary exists.
| // Segment text into grapheme clusters using Intl.Segmenter if available | ||
| let clusters: string[] = []; | ||
| if (typeof Intl !== 'undefined' && Intl.Segmenter) { | ||
| const segmenter = new Intl.Segmenter(undefined, { granularity: 'grapheme' }); | ||
| clusters = Array.from(segmenter.segment(text)).map((s) => s.segment); | ||
| } else { | ||
| clusters = Array.from(text); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg '(^|/)textWrap\.ts$|(^|/)unicode\.ts$' || true
echo "== textWrap outline/contents =="
while IFS= read -r f; do
echo "--- $f"
wc -l "$f"
cat -n "$f"
done < <(git ls-files | rg '(^|/)textWrap\.ts$')
echo "== unicode outline/contents =="
while IFS= read -r f; do
echo "--- $f"
wc -l "$f"
cat -n "$f"
done < <(git ls-files | rg '(^|/)unicode\.ts$')
echo "== usages =="
rg -n "wrapTextWithWidth|stringWidth|Intl\.Segmenter|Segmenter" .Repository: Karanjot786/TermUI
Length of output: 331
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== runtime behavior probe: ANSI clustering and stringWidth state =="
node - <<'JS'
function stringWidth(s) {
let total = 0;
let inEscape = false;
let codepoints = Array.from(s);
for (let c of codepoints) {
if (c === '\x1B') {
inEscape = true;
continue;
}
if (inEscape) {
if (c === '[') continue;
if (/[0-9;]*[A-Za-z]/.test(c)) {
inEscape = false;
continue;
}
}
total += codepointWidth(c);
}
return total;
}
function codepointWidth(c) {
return /\p{Wide}/u.test(c) ? 2 : 1;
}
const esc = '\x1B[31m';
const graphemebroken = `red${esc}text`;
const segmenter = new Intl.Segmenter(undefined, {granularity: 'grapheme'});
const clusters = Array.from(segmenter.segment(graphemebroken), s => s.segment);
console.log(JSON.stringify({ clusters, fullEscStripWidth: graphemebroken.length - esc.length, perClusterWidth: clusters.map(c => ({ c: c.replace(/\x1B/g,'<ESC>'), width: stringWidth(c) })) }));
JSRepository: Karanjot786/TermUI
Length of output: 861
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file sizes =="
wc -l packages/core/src/utils/textWrap.ts packages/core/src/utils/unicode.ts
echo "== textWrap.ts =="
cat -n packages/core/src/utils/textWrap.ts
echo "== unicode.ts =="
cat -n packages/core/src/utils/unicode.ts
echo "== usages =="
rg -n "wrapTextWithWidth|stringWidth|Intl\.Segmenter|Segmenter" packages || true
echo "== runtime behavior probe (fixed regex and fd-style loop) =="
node - <<'JS'
function stringWidth(s, includeEscaped) {
includeEscaped = !!includeEscaped;
let total = 0;
let inEscape = false;
for (const c of [...s]) {
if (c === '\x1B') {
inEscape = true;
if (includeEscaped) total += 1;
continue;
}
if (inEscape) {
if (c === '[') continue;
if (/^[0-9;]*[A-Za-z]$/.test(c)) {
inEscape = false;
continue;
}
}
// No grapheme-cluster expansion here: Node supports \p{GraphemeExtend} only.
total += 1;
}
return total;
}
const esc = '\x1B[31m';
const graphemebroken = `red${esc}text`;
const segmenter = new Intl.Segmenter(undefined, {granularity: 'grapheme'});
const segments = Array.from(segmenter.segment(graphemebroken), s => s.segment);
const tokenWidth = segments.map((c, idx) => ({
idx,
c: c === '\x1B' ? '<ESC>' : c,
width: stringWidth(c),
inEscape: idx < 3 || (idx === 4 && c === 't')
}));
console.log(JSON.stringify({
segmentsLength: segments.length,
sequenceStartsAt: segments.findIndex(s => s === '\x1B'),
wholeEscWidth: stringWidth(esc),
segmentsAfterTokenization: segments.map((c, idx) => ({
char: (c === '\x1B' ? '<ESC>' : c) + (idx === 3 && c === ']' ? '' : ''),
width: stringWidth(c)
})),
totalWidthAsSeparated: segments.reduce((sum, c) => sum + stringWidth(c), 0),
tokenWidth
}));
JSRepository: Karanjot786/TermUI
Length of output: 50374
Keep ANSI escape sequences atomic during text wrapping.
Grapheme segmentation splits escape sequences such as \x1B[31m into separate characters. Each character is then passed to the single-character measurement path, so the trailing part can be counted as visible text and appended as ordinary characters. This miscounts wrap length and can break the preserved ANSI code in output.
Measure escape sequences as zero-width atomic tokens and avoid splitting them across wrapped lines.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/utils/textWrap.ts` around lines 19 - 26, Update the
grapheme-cluster construction in the text-wrapping utility to recognize ANSI
escape sequences as zero-width atomic tokens before or during segmentation.
Preserve each complete sequence as one token, exclude it from visible-width
measurement, and ensure wrapping never splits a sequence across lines while
retaining it in the output.
| it('should handle zero or negative maxWidth gracefully', () => { | ||
| expect(wrapTextWithWidth('Test', 0)).toEqual([]); | ||
| expect(wrapTextWithWidth('', 10)).toEqual([]); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test title promises negative-width coverage; only zero is asserted.
The test name states "should handle zero or negative maxWidth gracefully," but line 28 only exercises maxWidth = 0. There is no assertion for a negative maxWidth (for example -5). Add that case so the test matches its stated intent.
✅ Suggested addition
it('should handle zero or negative maxWidth gracefully', () => {
expect(wrapTextWithWidth('Test', 0)).toEqual([]);
expect(wrapTextWithWidth('', 10)).toEqual([]);
+ expect(wrapTextWithWidth('Test', -5)).toEqual([]);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should handle zero or negative maxWidth gracefully', () => { | |
| expect(wrapTextWithWidth('Test', 0)).toEqual([]); | |
| expect(wrapTextWithWidth('', 10)).toEqual([]); | |
| }); | |
| it('should handle zero or negative maxWidth gracefully', () => { | |
| expect(wrapTextWithWidth('Test', 0)).toEqual([]); | |
| expect(wrapTextWithWidth('', 10)).toEqual([]); | |
| expect(wrapTextWithWidth('Test', -5)).toEqual([]); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/test/textWrap.test.ts` around lines 27 - 30, Update the test
“should handle zero or negative maxWidth gracefully” to add an assertion calling
wrapTextWithWidth with a negative maxWidth such as -5 and expecting an empty
array, while preserving the existing zero-width and empty-input assertions.
Description
Fixes text wrapping boundary calculations for multi-byte Unicode and emoji characters when terminal window dimensions resize.
Related Issue
Closes #3339
Which package(s)?
@termuijs/core
Type of Change
type:bug)type:feature)type:docs)type:testing)type:refactor)type:design)type:accessibility)type:performance)type:devops)type:security)Checklist
needs-starcheck blocks your merge otherwise.bun vitest runbun run buildbun run typecheckCONTRIBUTING.md.type: short description.markDirty()(if your change affects rendering).anytypes without an inline comment explaining why.GSSoC 2026 Participation
https://gssoc.girlscript.org/profile/knoxiboyScreenshots / Recordings (UI changes)
Preserves multi-byte Unicode emoji character boundaries cleanly when wrapped across terminal column bounds.
Notes for the Reviewer
Zero external dependencies; uses native string segmenters and width measurement.
Summary by CodeRabbit
New Features
Tests