fix(box): use string-width for correct emoji and CJK alignment - #415
fix(box): use string-width for correct emoji and CJK alignment#415mahmoodhamdi wants to merge 2 commits into
Conversation
Replace `stripAnsi(str).length` with `stringWidth()` in box rendering to properly account for the visual width of emoji and CJK characters. This fixes misaligned right edges when box content contains characters that occupy more than one terminal column.
📝 WalkthroughWalkthroughUpdated box sizing to use a display-width-aware Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
🧹 Nitpick comments (3)
test/box.test.ts (1)
5-36: Good test coverage for the core fix.The tests effectively validate that emoji and CJK content produce aligned box borders. Consider adding tests for additional edge cases in a follow-up:
- Title with emoji/CJK (e.g.,
box("content", { title: "🎉 Title" })) since title width calculation was also changed.- ANSI-styled content to verify stripping works correctly with
stringWidth.- Complex emoji like ZWJ sequences (e.g., "👨👩👧") which may behave differently across environments.
💡 Example additional test case
test("aligns box edges with emoji in title", () => { const result = box("Content", { title: "🎉 Title" }); const lines = result.split("\n").filter(Boolean); const widths = lines.map((line) => stringWidth(line)); const uniqueWidths = [...new Set(widths)]; expect(uniqueWidths.length).toBe(1); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/box.test.ts` around lines 5 - 36, Add extra tests to test/box.test.ts to cover title handling and edge cases: create tests that call box("Content", { title: "🎉 Title" }) and box with a CJK title (e.g., "タイトル"), tests with ANSI-styled content (e.g., colored strings) to ensure stringWidth/strip ANSI logic still aligns, and a test using complex ZWJ emoji sequences (e.g., "👨👩👧") and multi-line mixtures; in each test split the result by "\n", filter(Boolean), map with stringWidth and assert a single unique width so box, title handling, and ANSI/ZWJ behavior are validated.src/utils/box.ts (2)
5-11: Consider extractingstringWidthto a shared utility module.This implementation duplicates
src/reporters/fancy.ts:40-47line-for-line. While acceptable for this fix, extracting to a shared module (e.g.,src/utils/string.tsalongsidestripAnsi) would reduce duplication and ensure consistent behavior across the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/box.ts` around lines 5 - 11, Extract the stringWidth implementation into a shared utility (e.g., add a new export in the existing utilities module where stripAnsi lives) and replace the duplicate implementations with imports; specifically move the function stringWidth (which uses stripAnsi and _stringWidth and checks Intl.Segmenter) into a single exported helper and update the callers (including the fancy reporter duplicate) to import and use that shared stringWidth to remove duplication and ensure consistent behavior.
287-289: Minor inconsistency:stripAnsi(left).lengthvsstringWidth(left).For consistency with the rest of the changes, consider using
stringWidth(left)instead ofstripAnsi(left).length. While functionally equivalent here (border characters are single-column), usingstringWidththroughout makes the intent clearer and guards against future changes to border styles.♻️ Proposed change
const right = borderStyle.h.repeat( - width - stringWidth(opts.title) - stripAnsi(left).length + paddingOffset, + width - stringWidth(opts.title) - stringWidth(left) + paddingOffset, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/box.ts` around lines 287 - 289, Replace the use of stripAnsi(left).length with stringWidth(left) in the computation of the right border (the const right = ... expression) so border width calculation consistently uses stringWidth; update the expression that currently uses stripAnsi(left).length to call stringWidth(left) instead (ensure stringWidth is in scope where this expression resides, e.g., same utils that use stringWidth elsewhere).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/utils/box.ts`:
- Around line 5-11: Extract the stringWidth implementation into a shared utility
(e.g., add a new export in the existing utilities module where stripAnsi lives)
and replace the duplicate implementations with imports; specifically move the
function stringWidth (which uses stripAnsi and _stringWidth and checks
Intl.Segmenter) into a single exported helper and update the callers (including
the fancy reporter duplicate) to import and use that shared stringWidth to
remove duplication and ensure consistent behavior.
- Around line 287-289: Replace the use of stripAnsi(left).length with
stringWidth(left) in the computation of the right border (the const right = ...
expression) so border width calculation consistently uses stringWidth; update
the expression that currently uses stripAnsi(left).length to call
stringWidth(left) instead (ensure stringWidth is in scope where this expression
resides, e.g., same utils that use stringWidth elsewhere).
In `@test/box.test.ts`:
- Around line 5-36: Add extra tests to test/box.test.ts to cover title handling
and edge cases: create tests that call box("Content", { title: "🎉 Title" }) and
box with a CJK title (e.g., "タイトル"), tests with ANSI-styled content (e.g.,
colored strings) to ensure stringWidth/strip ANSI logic still aligns, and a test
using complex ZWJ emoji sequences (e.g., "👨👩👧") and multi-line mixtures; in
each test split the result by "\n", filter(Boolean), map with stringWidth and
assert a single unique width so box, title handling, and ANSI/ZWJ behavior are
validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd9931e9-5cc8-4b3e-83b3-de9a8e27b8c2
📒 Files selected for processing (2)
src/utils/box.tstest/box.test.ts
Move the duplicated stringWidth helper from both box.ts and fancy.ts into src/utils/string.ts as a shared export. Use stringWidth consistently for all width calculations in box rendering (including title border). Add tests for emoji titles, CJK titles, and ZWJ sequences.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/utils/string.ts (1)
46-58: Alignment functions usestr.lengthinstead ofstringWidth, limiting support for emoji and CJK content.The
centerAlign,rightAlign,leftAlign, andalignfunctions measure strings usingstr.length, which counts UTF-16 code units rather than visual terminal columns. These functions are part of the public API and would produce incorrect results if used with wide-character content (emoji, CJK).However, note that simply switching to
stringWidth()is insufficient. The character-by-character indexing approach (str[i - offset]) fundamentally cannot handle wide characters correctly since they occupy a single array element but span multiple terminal columns. A complete refactoring of the algorithm would be needed.Worth noting: the library's
box()function does not use these alignment utilities—it handles width calculations directly withstringWidth(), which is why box alignment with emoji and CJK content works correctly despite this limitation in the exported functions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/string.ts` around lines 46 - 58, The alignment helpers (centerAlign, rightAlign, leftAlign, align) currently use str.length and index by code unit which breaks for wide/emoji/CJK; switch to measuring widths with stringWidth() and stop indexing by code unit — instead split the string into visible grapheme clusters (Intl.Segmenter or a grapheme-splitter library), compute each cluster's display width with stringWidth(cluster), then build the output by joining clusters and adding pad characters to the left/right based on the computed display widths (use Math.floor/ceil for centering). Update all four functions to use the cluster array and stringWidth for length/offset calculations so padding aligns correctly for wide characters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/utils/string.ts`:
- Around line 46-58: The alignment helpers (centerAlign, rightAlign, leftAlign,
align) currently use str.length and index by code unit which breaks for
wide/emoji/CJK; switch to measuring widths with stringWidth() and stop indexing
by code unit — instead split the string into visible grapheme clusters
(Intl.Segmenter or a grapheme-splitter library), compute each cluster's display
width with stringWidth(cluster), then build the output by joining clusters and
adding pad characters to the left/right based on the computed display widths
(use Math.floor/ceil for centering). Update all four functions to use the
cluster array and stringWidth for length/offset calculations so padding aligns
correctly for wide characters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e8b2a60-9ef6-445f-b34c-b6a5361e266b
📒 Files selected for processing (5)
src/reporters/fancy.tssrc/utils.tssrc/utils/box.tssrc/utils/string.tstest/box.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/box.test.ts
1 participant
What
Fixed
consola.box()rendering misaligned right edges when content contains emoji or CJK (fullwidth) characters.Why
stripAnsi(str).lengthcounts UTF-16 code units, not visual terminal columns. Emoji like 🌍 and CJK characters like 漢字 occupy 2 terminal columns but.lengthreports them as 1-2 code units, causing the right border to shift.The
string-widthpackage (already a project dependency, used inFancyReporter) correctly calculates visual width usingIntl.Segmenterwhen available.How
Replaced
stripAnsi(str).lengthwith astringWidth()wrapper (same pattern asFancyReporter) insrc/utils/box.tsfor all width calculations:Added tests in
test/box.test.tscovering emoji, CJK, and multiline emoji content.Testing
pnpm vitest run— all tests passpnpm lint— passesFixes #402
Summary by CodeRabbit
Bug Fixes
Tests