-
Notifications
You must be signed in to change notification settings - Fork 230
fix: Fix Text Wrapping & Multi-Byte Character Boundary Calculation on Window Resize (#3339) #3362
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| import { stringWidth } from './unicode'; | ||
|
|
||
| export interface WrapOptions { | ||
| hard?: boolean; | ||
| trim?: boolean; | ||
| } | ||
|
|
||
| /** | ||
| * Wrap text into lines of specified maximum visual column width. | ||
| * Preserves multi-byte Unicode emoji and CJK character boundaries without splitting (#3339). | ||
| */ | ||
| export function wrapTextWithWidth( | ||
| text: string, | ||
| maxWidth: number, | ||
| options: WrapOptions = {} | ||
| ): string[] { | ||
| if (!text || maxWidth <= 0) return []; | ||
|
|
||
| // 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); | ||
| } | ||
|
Comment on lines
+19
to
+26
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 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 Measure escape sequences as zero-width atomic tokens and avoid splitting them across wrapped lines. 🤖 Prompt for AI Agents |
||
|
|
||
| const lines: string[] = []; | ||
| let currentLine = ''; | ||
| let currentLineWidth = 0; | ||
|
|
||
| for (const cluster of clusters) { | ||
| if (cluster === '\n') { | ||
| lines.push(options.trim ? currentLine.trimEnd() : currentLine); | ||
| currentLine = ''; | ||
| currentLineWidth = 0; | ||
| continue; | ||
| } | ||
|
|
||
| const clusterWidth = stringWidth(cluster); | ||
|
|
||
| if (currentLineWidth + clusterWidth > maxWidth && currentLine.length > 0) { | ||
| lines.push(options.trim ? currentLine.trimEnd() : currentLine); | ||
| currentLine = cluster; | ||
| currentLineWidth = clusterWidth; | ||
| } else { | ||
| currentLine += cluster; | ||
| currentLineWidth += clusterWidth; | ||
| } | ||
| } | ||
|
|
||
| if (currentLine.length > 0 || text.endsWith('\n')) { | ||
| lines.push(options.trim ? currentLine.trimEnd() : currentLine); | ||
| } | ||
|
|
||
| return lines; | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,31 @@ | ||||||||||||||||||||
| import { describe, it, expect } from 'bun:test'; | ||||||||||||||||||||
| import { wrapTextWithWidth } from '../src/utils/textWrap'; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| describe('wrapTextWithWidth Unit Tests', () => { | ||||||||||||||||||||
| it('should wrap basic ASCII text without breaking words unnecessarily', () => { | ||||||||||||||||||||
| const lines = wrapTextWithWidth('Hello World Test', 12, { trim: true }); | ||||||||||||||||||||
| expect(lines.length).toBeGreaterThan(1); | ||||||||||||||||||||
| expect(lines[0]).toBe('Hello World'); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('should preserve multi-byte emoji boundaries when wrapping', () => { | ||||||||||||||||||||
| const text = '🚀🎉⭐🔥💡'; | ||||||||||||||||||||
| const lines = wrapTextWithWidth(text, 4); | ||||||||||||||||||||
| // Each emoji has a visual width of 2 columns. Max width 4 holds 2 emojis per line. | ||||||||||||||||||||
| expect(lines.length).toBe(3); | ||||||||||||||||||||
| expect(lines[0]).toBe('🚀🎉'); | ||||||||||||||||||||
| expect(lines[1]).toBe('⭐🔥'); | ||||||||||||||||||||
| expect(lines[2]).toBe('💡'); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('should handle newline characters properly', () => { | ||||||||||||||||||||
| const text = 'Line 1\nLine 2'; | ||||||||||||||||||||
| const lines = wrapTextWithWidth(text, 20); | ||||||||||||||||||||
| expect(lines).toEqual(['Line 1', 'Line 2']); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('should handle zero or negative maxWidth gracefully', () => { | ||||||||||||||||||||
| expect(wrapTextWithWidth('Test', 0)).toEqual([]); | ||||||||||||||||||||
| expect(wrapTextWithWidth('', 10)).toEqual([]); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
Comment on lines
+27
to
+30
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 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 ✅ 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||
| }); | ||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
hardoption has no effect; word-boundary preservation is coincidental.WrapOptions.hard(line 4) is never read inwrapTextWithWidth. The wrap loop (lines 42-49) always breaks strictly on cumulative cluster width, with no lookback for a word boundary. Passinghard: trueorhard: 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 byhard(scan backward for the last whitespace cluster before the width limit whenhardis falsy) or remove the unused option from the public interface until it is implemented.Also applies to: 42-49
🤖 Prompt for AI Agents