fix: calculate box width using display width#416
Conversation
📝 WalkthroughWalkthroughThe changes fix emoji misalignment in the box utility by introducing a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/box.test.ts (1)
5-20: Add a wide-title case to cover the changed title path.The implementation now changes both content and
titlewidth calculations, but the regression test only covers wide content. Adding a title case would protect Lines 263 and 279-282 insrc/utils/box.ts.🧪 Suggested test expansion
describe("box", () => { - test.each(["hello 🚀️ world", "漢字 test"])( - "keeps all rendered lines aligned for %s", - (text) => { + test.each([ + ["hello 🚀️ world", undefined], + ["漢字 test", undefined], + ["hello", "🚀️ title"], + ["hello", "漢字 title"], + ])( + "keeps all rendered lines aligned for text %s and title %s", + (text, title) => { const lines = box(text, { + title, style: { marginTop: 0, marginBottom: 0,🤖 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 - 20, Add a test that exercises the wide-title path by calling the box function with a long/wide title (e.g., a title containing wide Unicode like "漢字 🚀️") in addition to the existing wide-content cases so both content and title width code paths are covered; specifically construct a call to box(...) passing a title option (or the API variant that accepts a title) and assert that stringWidth on each rendered line remains equal (reuse the existing test structure and stringWidth assertions) to protect the logic in the title-width branches touched in src/utils/box.ts (the title handling in box and related width calculations).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/box.ts`:
- Around line 1-7: The stringWidth helper currently calls _stringWidth
unconditionally which will fail on limited-ICU runtimes; update the stringWidth
function to guard with a runtime check (e.g., typeof Intl !== "undefined" &&
typeof Intl.Segmenter !== "undefined") and only call
_stringWidth(stripAnsi(str)) when the guard passes; otherwise return a safe
fallback such as stripAnsi(str).length (or an equivalent simple character-count)
so callers of stringWidth (referencing the stringWidth function and _stringWidth
import) remain robust in environments without Intl.Segmenter.
---
Nitpick comments:
In `@test/box.test.ts`:
- Around line 5-20: Add a test that exercises the wide-title path by calling the
box function with a long/wide title (e.g., a title containing wide Unicode like
"漢字 🚀️") in addition to the existing wide-content cases so both content and
title width code paths are covered; specifically construct a call to box(...)
passing a title option (or the API variant that accepts a title) and assert that
stringWidth on each rendered line remains equal (reuse the existing test
structure and stringWidth assertions) to protect the logic in the title-width
branches touched in src/utils/box.ts (the title handling in box and related
width calculations).
🪄 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
Run ID: 370354a5-c2aa-4e96-9075-1624959b5c15
📒 Files selected for processing (2)
src/utils/box.tstest/box.test.ts
| import _stringWidth from "string-width"; | ||
| import { getColor } from "./color"; | ||
| import { stripAnsi } from "./string"; | ||
|
|
||
| function stringWidth(str: string) { | ||
| return _stringWidth(stripAnsi(str)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
# Verify all string-width call sites and whether they consistently guard Intl.Segmenter.
rg -n -C3 --type=ts '(_stringWidth|stringWidth\s*\(|Intl\.Segmenter)'
echo
# Verify declared string-width version and Node engine constraints.
fd -i '^(package\.json|pnpm-lock\.yaml)$' -d 3 -x sh -c '
echo "--- $1"
rg -n '\''(string-width|"node"|engines)'\'' "$1" || true
' sh {}Repository: unjs/consola
Length of output: 16465
Add Intl.Segmenter guard to the stringWidth helper in src/utils/box.ts.
The new stringWidth helper calls _stringWidth unconditionally, while src/reporters/fancy.ts protects the same dependency with a runtime guard for missing ICU support. The inconsistency risks runtime failures in limited-ICU runtimes that the project already mitigates elsewhere.
🛡️ Proposed fix
import _stringWidth from "string-width";
import { getColor } from "./color";
import { stripAnsi } from "./string";
function stringWidth(str: string) {
+ // https://github.com/unjs/consola/issues/204
+ if (typeof Intl !== "object" || !Intl.Segmenter) {
+ return stripAnsi(str).length;
+ }
return _stringWidth(stripAnsi(str));
}📝 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.
| import _stringWidth from "string-width"; | |
| import { getColor } from "./color"; | |
| import { stripAnsi } from "./string"; | |
| function stringWidth(str: string) { | |
| return _stringWidth(stripAnsi(str)); | |
| } | |
| import _stringWidth from "string-width"; | |
| import { getColor } from "./color"; | |
| import { stripAnsi } from "./string"; | |
| function stringWidth(str: string) { | |
| // https://github.com/unjs/consola/issues/204 | |
| if (typeof Intl !== "object" || !Intl.Segmenter) { | |
| return stripAnsi(str).length; | |
| } | |
| return _stringWidth(stripAnsi(str)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/box.ts` around lines 1 - 7, The stringWidth helper currently calls
_stringWidth unconditionally which will fail on limited-ICU runtimes; update the
stringWidth function to guard with a runtime check (e.g., typeof Intl !==
"undefined" && typeof Intl.Segmenter !== "undefined") and only call
_stringWidth(stripAnsi(str)) when the guard passes; otherwise return a safe
fallback such as stripAnsi(str).length (or an equivalent simple character-count)
so callers of stringWidth (referencing the stringWidth function and _stringWidth
import) remain robust in environments without Intl.Segmenter.
There was a problem hiding this comment.
Pull request overview
Fixes box() rendering misalignment for wide glyphs (emoji/CJK) by switching width and padding calculations from string length to display width, and adds a regression test to ensure all rendered lines have the same visible width.
Changes:
- Use
string-width(with ANSI stripped) to compute content/title display widths inbox(). - Update title centering and per-line right padding to use display width.
- Add a Vitest regression test covering emoji and CJK content alignment.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/utils/box.ts |
Switches box sizing/padding logic from .length to display width via string-width. |
test/box.test.ts |
Adds regression test ensuring all rendered box lines have equal display width for wide characters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { stripAnsi } from "./string"; | ||
|
|
||
| function stringWidth(str: string) { | ||
| return _stringWidth(stripAnsi(str)); |
| const titleWidth = opts.title ? stringWidth(opts.title) : 0; | ||
| const width = | ||
| Math.max( | ||
| ...textLines.map((line) => stripAnsi(line).length), | ||
| opts.title ? stripAnsi(opts.title).length : 0, | ||
| ) + paddingOffset; | ||
| Math.max(...textLines.map((line) => stringWidth(line)), titleWidth) + | ||
| paddingOffset; |
resolves #402
box()currently uses.lengthwhen it calculates content/title width and the trailing padding for each line. That makes the content row narrower than the borders for wide glyphs such as🚀️and漢字.This switches those calculations to display width via
string-widthand adds a regression test that checks all rendered box lines keep the same visible width for wide-character content.Validation:
Summary by CodeRabbit
Bug Fixes
Tests