Skip to content

Improve Bun.stringWidth accuracy and robustness - #25447

Merged
Jarred-Sumner merged 3 commits into
mainfrom
jarred/better-string-width
Dec 11, 2025
Merged

Jarred-Sumner merged 3 commits into
mainfrom
jarred/better-string-width

Conversation

@Jarred-Sumner

Copy link
Copy Markdown
Collaborator

This PR significantly improves Bun.stringWidth to handle a wider variety of Unicode characters and escape sequences correctly.

Zero-width character handling

Added support for many previously unhandled zero-width characters:

  • Soft hyphen (U+00AD)
  • Word joiner and invisible operators (U+2060-U+2064)
  • Lone surrogates (U+D800-U+DFFF)
  • Arabic formatting characters (U+0600-U+0605, U+06DD, U+070F, U+08E2)
  • Indic script combining marks (Devanagari through Malayalam)
  • Thai and Lao combining marks
  • Combining Diacritical Marks Extended and Supplement
  • Tag characters (U+E0000-U+E007F)

ANSI escape sequence handling

CSI sequences

  • Now properly handles ALL CSI final bytes (0x40-0x7E), not just m
  • This means cursor movement (A/B/C/D), erase (J/K), scroll (S/T), and other CSI commands are now correctly excluded from width calculation

OSC sequences

  • Added support for OSC sequences (ESC ] ... BEL/ST)
  • OSC 8 hyperlinks are now properly handled
  • Supports both BEL (0x07) and ST (ESC ) terminators

ESC ESC fix

  • Fixed state machine bug where ESC ESC would incorrectly reset state
  • Now correctly handles consecutive ESC characters

Emoji handling

Added proper grapheme-aware emoji width calculation:

  • Flag emoji (regional indicator pairs) → width 2
  • Skin tone modifiers → width 2
  • ZWJ sequences (family, professions, etc.) → width 2
  • Keycap sequences → width 2
  • Variation selectors (VS15 for text, VS16 for emoji presentation)
  • Uses ICU's UCHAR_EMOJI property for accurate emoji detection

Test coverage

Added comprehensive test suite with 94 tests covering:

  • All zero-width character categories
  • All CSI final bytes
  • OSC sequences with various terminators
  • Emoji edge cases (flags, skin tones, ZWJ, keycaps, variation selectors)
  • East Asian width (CJK, fullwidth, halfwidth katakana)
  • Indic and Thai script combining marks
  • Fuzzer-like stress tests for robustness

Breaking changes

This is a behavior change - stringWidth will return different values for some inputs. However, the new values are more accurate representations of terminal display width:

Input Old New Why
Flag emoji 🇺🇸 1 2 Flags display as 2 cells
Skin tone 👋🏽 4 2 Emoji + modifier = 1 grapheme
ZWJ family 👨‍👩‍👧 8 2 ZWJ sequence = 1 grapheme
Word joiner U+2060 1 0 Invisible character
OSC 8 hyperlinks counted URL just visible text URLs are invisible
Cursor movement ESC[5A counted 0 Control sequence

🤖 Generated with Claude Code

@robobun

robobun commented Dec 10, 2025 •

Copy link
Copy Markdown
Collaborator
Updated 3:11 PM PT - Dec 10th, 2025

❌ Your commit bbbcb157 has 5 failures in Build #33132 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 25447

That installs a local version of the PR into your bun-25447 executable, so you can run:

bun-25447 --bun

@coderabbitai

coderabbitai Bot commented Dec 10, 2025 •

Copy link
Copy Markdown
Contributor

Walkthrough

Adds a public GraphemeState for grapheme tracking, refactors UTF‑16 width computation to use it, expands zero‑width codepoint detection (including U+00AD and multiple combining/formatting ranges), improves CSI/OSC escape stripping and bulk ASCII/SIMD paths, and adds extensive string‑width tests for many Unicode/escape edge cases.

Changes

Cohort / File(s) Summary
Core visible string width implementation
src/string/immutable/visible.zig
Introduces public GraphemeState with reset, add, width, and predicates (isEmojiBase, isRegionalIndicator, isSkinToneModifier); refactors visibleUTF16WidthFn to use GraphemeState and maintain state across codepoints/surrogates; expands zero‑width detection (soft hyphen U+00AD, surrogates, Arabic formatting, Indic/Thai/Lao combining marks, Combining Diacritical Marks Extended/Supplement, Tag chars, Variation Selectors Supplement); treats 0xAD as zero‑width; enhances CSI/OSC termination and stripping; adds SIMD/ASCII bulk path and improved escape handling.
String width tests
test/js/bun/util/stringWidth.test.ts
Adds a large "stringWidth extended" test suite covering zero‑width characters, CSI/OSC sequences (well‑formed, malformed, unterminated), emoji variants (flags, skin tones, ZWJ, keycaps, VS), East Asian width cases, Indic and Thai scripts, lone/invalid surrogates, controls, long/mixed strings, and fuzzer‑style stress tests.

Suggested reviewers

  • dylan-conway

Pre-merge checks

✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: improving Bun.stringWidth accuracy and robustness, which aligns with the PR's core objective of expanding Unicode and escape sequence handling.
Description check ✅ Passed The description is comprehensive and well-organized, covering what the PR does (zero-width chars, ANSI sequences, emoji handling, tests) and verification approach (94 new tests with specific categories). Both required template sections are adequately addressed.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 258095c and bbbcb15.

📒 Files selected for processing (2)
  • src/string/immutable/visible.zig (6 hunks)
  • test/js/bun/util/stringWidth.test.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
test/**/*.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

test/**/*.{js,ts,jsx,tsx}: Write tests as JavaScript and TypeScript files using Jest-style APIs (test, describe, expect) and import from bun:test
Use test.each and data-driven tests to reduce boilerplate when testing multiple similar cases

Files:

  • test/js/bun/util/stringWidth.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}

📄 CodeRabbit inference engine (test/CLAUDE.md)

test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Use bun:test with files that end in *.test.{ts,js,jsx,tsx,mjs,cjs}
Do not write flaky tests. Never wait for time to pass in tests; always wait for the condition to be met instead of using an arbitrary amount of time
Never use hardcoded port numbers in tests. Always use port: 0 to get a random port
Prefer concurrent tests over sequential tests using test.concurrent or describe.concurrent when multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, use bunExe and bunEnv from harness to ensure the same build of Bun is used and debug logging is silenced
Use -e flag for single-file tests when spawning Bun processes
Use tempDir() from harness to create temporary directories with files for multi-file tests instead of creating files manually
Prefer async/await over callbacks in tests
When callbacks must be used and it's just a single callback, use Promise.withResolvers to create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
Use Buffer.alloc(count, fill).toString() instead of 'A'.repeat(count) to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Use describe blocks for grouping related tests
Always use await using or using to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Always check exit codes and test error scenarios in error tests
Use describe.each() for parameterized tests
Use toMatchSnapshot() for snapshot testing
Use beforeAll(), afterEach(), beforeEach() for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup in afterEach()

Files:

  • test/js/bun/util/stringWidth.test.ts
test/**/*.test.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

test/**/*.test.{ts,tsx}: For single-file tests in Bun test suite, prefer using -e flag over tempDir
For multi-file tests in Bun test suite, prefer using tempDir and Bun.spawn
Always use port: 0 when spawning servers in tests - do not hardcode ports or use custom random port functions
Use normalizeBunSnapshot to normalize snapshot output in tests instead of manual output comparison
Never write tests that check for no 'panic', 'uncaught exception', or similar strings in test output - that is not a valid test
Use tempDir from harness to create temporary directories in tests - do not use tmpdirSync or fs.mkdtempSync
In tests, call expect(stdout).toBe(...) before expect(exitCode).toBe(0) when spawning processes for more useful error messages on failure
Do not write flaky tests - do not use setTimeout in tests; instead await the condition to be met since you're testing the CONDITION, not TIME PASSING
Verify your test fails with USE_SYSTEM_BUN=1 bun test <file> and passes with bun bd test <file> - tests are not valid if they pass with USE_SYSTEM_BUN=1
Avoid shell commands in tests - do not use find or grep; use Bun's Glob and built-in tools instead
Test files must end in .test.ts or .test.tsx and be created in the appropriate test folder structure

Files:

  • test/js/bun/util/stringWidth.test.ts
src/**/*.{cpp,zig}

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

src/**/*.{cpp,zig}: Use bun bd or bun run build:debug to build debug versions for C++ and Zig source files; creates debug build at ./build/debug/bun-debug
Run tests using bun bd test <test-file> with the debug build; never use bun test directly as it will not include your changes
Execute files using bun bd <file> <...args>; never use bun <file> directly as it will not include your changes
Enable debug logs for specific scopes using BUN_DEBUG_$(SCOPE)=1 environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required

Files:

  • src/string/immutable/visible.zig
src/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

Use bun.Output.scoped(.${SCOPE}, .hidden) for creating debug logs in Zig code

Implement core functionality in Zig, typically in its own directory in src/

src/**/*.zig: Private fields in Zig are fully supported using the # prefix: struct { #foo: u32 };
Use decl literals in Zig for declaration initialization: const decl: Decl = .{ .binding = 0, .value = 0 };
Prefer @import at the bottom of the file (auto formatter will move them automatically)

Be careful with memory management in Zig code - use defer for cleanup with allocators

Files:

  • src/string/immutable/visible.zig
**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)

**/*.zig: Expose generated bindings in Zig structs using pub const js = JSC.Codegen.JS<ClassName> with trait conversion methods: toJS, fromJS, and fromJSDirect
Use consistent parameter name globalObject instead of ctx in Zig constructor and method implementations
Use bun.JSError!JSValue return type for Zig methods and constructors to enable proper error handling and exception propagation
Implement resource cleanup using deinit() method that releases resources, followed by finalize() called by the GC that invokes deinit() and frees the pointer
Use JSC.markBinding(@src()) in finalize methods for debugging purposes before calling deinit()
For methods returning cached properties in Zig, declare external C++ functions using extern fn and callconv(JSC.conv) calling convention
Implement getter functions with naming pattern get<PropertyName> in Zig that accept this and globalObject parameters and return JSC.JSValue
Access JavaScript CallFrame arguments using callFrame.argument(i), check argument count with callFrame.argumentCount(), and get this with callFrame.thisValue()
For reference-counted objects, use .deref() in finalize instead of destroy() to release references to other JS objects

Files:

  • src/string/immutable/visible.zig
🧠 Learnings (20)
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `Buffer.alloc(count, fill).toString()` instead of `'A'.repeat(count)` to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/html.test.ts : Organize HTML tests in html.test.ts for tests relating to HTML files themselves

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/css.test.ts : Organize CSS tests in css.test.ts for tests concerning bundling bugs with CSS files

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: See `test/harness.ts` for common test utilities and helpers

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use JSC intrinsics (prefixed with `$`) such as `$Array.from()`, `$isCallable()`, and `$newArrayWithSize()` for performance-critical operations

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-09-12T18:16:50.754Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22606
File: src/glob/GlobWalker.zig:449-452
Timestamp: 2025-09-12T18:16:50.754Z
Learning: For Bun codebase: prefer using `std.fs.path.sep` over manual platform separator detection, and use `bun.strings.lastIndexOfChar` instead of `std.mem.lastIndexOfScalar` for string operations.

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:35:39.205Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-11-24T18:35:39.205Z
Learning: Add tests for new Bun runtime functionality

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-10-25T17:20:19.041Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 24063
File: test/js/bun/telemetry/server-header-injection.test.ts:5-20
Timestamp: 2025-10-25T17:20:19.041Z
Learning: In the Bun telemetry codebase, tests are organized into two distinct layers: (1) Internal API tests in test/js/bun/telemetry/ use numeric InstrumentKind enum values to test Zig↔JS injection points and low-level integration; (2) Public API tests in packages/bun-otel/test/ use string InstrumentKind values ("http", "fetch", etc.) to test the public-facing BunSDK and instrumentation APIs. This separation allows internal tests to use efficient numeric enums for refactoring flexibility while the public API maintains a developer-friendly string-based interface.

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-10-18T05:23:24.403Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/telemetry-server.test.ts:91-100
Timestamp: 2025-10-18T05:23:24.403Z
Learning: In the Bun codebase, telemetry tests (test/js/bun/telemetry-*.test.ts) should focus on telemetry API behavior: configure/disable/isEnabled, callback signatures and invocation, request ID correlation, and error handling. HTTP protocol behaviors like status code normalization (e.g., 200 with empty body → 204) should be tested in HTTP server tests (test/js/bun/http/), not in telemetry tests. Keep separation of concerns: telemetry tests verify the telemetry API contract; HTTP tests verify HTTP semantics.

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Ensure V8 API tests compare identical C++ code output between Node.js and Bun through the test suite validation process

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-10-24T10:43:09.398Z
Learnt from: fmguerreiro
Repo: oven-sh/bun PR: 23774
File: src/install/PackageManager/updatePackageJSONAndInstall.zig:548-548
Timestamp: 2025-10-24T10:43:09.398Z
Learning: In Bun's Zig codebase, the `as(usize, intCast(...))` cast pattern triggers a Zig compiler bug that causes compilation to hang indefinitely when used in complex control flow contexts (loops + short-circuit operators + optional unwrapping). Avoid this pattern and use simpler alternatives like just `intCast(...)` if type casting is necessary.

Applied to files:

  • src/string/immutable/visible.zig
📚 Learning: 2025-09-04T02:04:43.094Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22278
File: src/ast/E.zig:980-1003
Timestamp: 2025-09-04T02:04:43.094Z
Learning: In Bun's Zig codebase, `as(i32, u8_or_u16_value)` is sufficient for casting u8/u16 to i32 in comparison operations. `intCast` is not required in this context, and the current casting approach compiles successfully.

Applied to files:

  • src/string/immutable/visible.zig
📚 Learning: 2025-11-10T00:57:09.173Z
Learnt from: franciscop
Repo: oven-sh/bun PR: 24514
File: src/bun.js/api/crypto/PasswordObject.zig:86-101
Timestamp: 2025-11-10T00:57:09.173Z
Learning: In Bun's Zig codebase (PasswordObject.zig), when validating the parallelism parameter for Argon2, the upper limit is set to 65535 (2^16 - 1) rather than using `std.math.maxInt(u24)` because the latter triggers Zig's truncation limit checks. The value 65535 is a practical upper bound that avoids compiler issues while being sufficient for thread parallelism use cases.

Applied to files:

  • src/string/immutable/visible.zig
📚 Learning: 2025-10-15T22:03:50.832Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23710
File: src/envvars.zig:135-144
Timestamp: 2025-10-15T22:03:50.832Z
Learning: In src/envvars.zig, the boolean feature flag cache uses a single atomic enum and should remain monotonic. Only the string cache (which uses two atomics: ptr and len) requires acquire/release ordering to prevent torn reads.

Applied to files:

  • src/string/immutable/visible.zig
📚 Learning: 2025-10-16T21:24:52.779Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23710
File: src/crash_handler.zig:1415-1423
Timestamp: 2025-10-16T21:24:52.779Z
Learning: When a boolean EnvVar in src/envvars.zig is defined with a default value (e.g., `.default = false`), the `get()` method returns `bool` instead of `?bool`. This means you cannot distinguish between "environment variable not set" and "environment variable explicitly set to the default value". For opt-out scenarios where detection of explicit setting is needed (like `BUN_ENABLE_CRASH_REPORTING` on platforms where crash reporting defaults to enabled), either: (1) don't provide a default value so `get()` returns `?bool`, or (2) use the returned boolean directly instead of only checking if it's true.

Applied to files:

  • src/string/immutable/visible.zig
📚 Learning: 2025-09-02T19:17:26.376Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 0
File: :0-0
Timestamp: 2025-09-02T19:17:26.376Z
Learning: In Bun's Zig codebase, when handling error unions where the same cleanup operation (like `rawFree`) needs to be performed regardless of success or failure, prefer using boolean folding with `else |err| switch (err)` over duplicating the cleanup call in multiple switch branches. This approach avoids code duplication while maintaining compile-time error checking.

Applied to files:

  • src/string/immutable/visible.zig
📚 Learning: 2025-09-02T17:14:01.470Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/ptr/shared.zig:470-470
Timestamp: 2025-09-02T17:14:01.470Z
Learning: In Zig, regular arithmetic operations (like +=) have built-in overflow detection in debug/safe builds and will panic automatically. Atomic operations (like fetchAdd) do not have this automatic safety checking, so explicit overflow checks must be added manually. This is why NonAtomicCount doesn't need explicit overflow checks while AtomicCount does.

Applied to files:

  • src/string/immutable/visible.zig
📚 Learning: 2025-09-07T14:00:36.526Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22258
File: src/bun.js/test/jest.zig:63-86
Timestamp: 2025-09-07T14:00:36.526Z
Learning: In Bun's Zig codebase, the established pattern for HashMap hash functions is to truncate hash values to u32, even when the underlying hash computation produces u64. This is seen throughout the codebase, such as in src/css/rules/rules.zig where u64 hashes are stored but truncated to u32 in HashMap contexts. When implementing HashContext for HashMapUnmanaged, the hash function should return u32 to match this pattern.

Applied to files:

  • src/string/immutable/visible.zig
📚 Learning: 2025-11-12T04:11:52.293Z
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 24622
File: src/deps/uws/us_socket_t.zig:112-113
Timestamp: 2025-11-12T04:11:52.293Z
Learning: In Bun's Zig codebase, when passing u32 values to C FFI functions that expect c_uint parameters, no explicit intCast is needed because c_uint is equivalent to u32 on Bun's target platforms and Zig allows implicit coercion between equivalent types. This pattern is used consistently throughout src/deps/uws/us_socket_t.zig in functions like setTimeout, setLongTimeout, and setKeepalive.

Applied to files:

  • src/string/immutable/visible.zig
🔇 Additional comments (2)
src/string/immutable/visible.zig (2)

1071-1094: OSC non-ASCII character handling appears correct and is tested.

The code at lines 1071–1080 correctly skips non-ASCII characters in OSC sequences using continue, preventing them from contributing to width. The test suite includes explicit test cases at lines 491–502 verifying this behavior:

  • "a\x1b]8;;https://🎉\x07b" expects width 2
  • "a\x1b]8;;https://中.com\x07b" expects width 2

A recent commit (bbbcb15) specifically addressed "Fix stringWidth for non-ASCII in OSC sequences." The implementation correctly handles ASCII OSC content (added to stretch_len) while skipping non-ASCII characters, and only counts the content if the OSC sequence terminates properly (line 1056).


57-70: The Indic script heuristic has been correctly fixed in this commit. All four characters mentioned are now properly classified as visible:

  • U+093D (Devanagari Avagraha) and U+09BD (Bengali Avagraha) at offset 0x3D are explicitly excluded from zero-width ranges
  • U+0B83 (Tamil Visarga) at offset 0x03 is correctly not caught by the offset <= 0x02 condition
  • U+0D4F (Malayalam Sign Para) at offset 0x4F is not in any zero-width range

The code is correct.

Likely an incorrect or invalid review comment.


Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a2d8b75 and d2af4bdb06e1cec466b1357332c511cb801ae5f0.

📒 Files selected for processing (2)
  • src/string/immutable/visible.zig (7 hunks)
  • test/js/bun/util/stringWidth.test.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
test/**/*.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

test/**/*.{js,ts,jsx,tsx}: Write tests as JavaScript and TypeScript files using Jest-style APIs (test, describe, expect) and import from bun:test
Use test.each and data-driven tests to reduce boilerplate when testing multiple similar cases

Files:

  • test/js/bun/util/stringWidth.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}

📄 CodeRabbit inference engine (test/CLAUDE.md)

test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Use bun:test with files that end in *.test.{ts,js,jsx,tsx,mjs,cjs}
Do not write flaky tests. Never wait for time to pass in tests; always wait for the condition to be met instead of using an arbitrary amount of time
Never use hardcoded port numbers in tests. Always use port: 0 to get a random port
Prefer concurrent tests over sequential tests using test.concurrent or describe.concurrent when multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, use bunExe and bunEnv from harness to ensure the same build of Bun is used and debug logging is silenced
Use -e flag for single-file tests when spawning Bun processes
Use tempDir() from harness to create temporary directories with files for multi-file tests instead of creating files manually
Prefer async/await over callbacks in tests
When callbacks must be used and it's just a single callback, use Promise.withResolvers to create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
Use Buffer.alloc(count, fill).toString() instead of 'A'.repeat(count) to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Use describe blocks for grouping related tests
Always use await using or using to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Always check exit codes and test error scenarios in error tests
Use describe.each() for parameterized tests
Use toMatchSnapshot() for snapshot testing
Use beforeAll(), afterEach(), beforeEach() for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup in afterEach()

Files:

  • test/js/bun/util/stringWidth.test.ts
test/**/*.test.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

test/**/*.test.{ts,tsx}: For single-file tests in Bun test suite, prefer using -e flag over tempDir
For multi-file tests in Bun test suite, prefer using tempDir and Bun.spawn
Always use port: 0 when spawning servers in tests - do not hardcode ports or use custom random port functions
Use normalizeBunSnapshot to normalize snapshot output in tests instead of manual output comparison
Never write tests that check for no 'panic', 'uncaught exception', or similar strings in test output - that is not a valid test
Use tempDir from harness to create temporary directories in tests - do not use tmpdirSync or fs.mkdtempSync
In tests, call expect(stdout).toBe(...) before expect(exitCode).toBe(0) when spawning processes for more useful error messages on failure
Do not write flaky tests - do not use setTimeout in tests; instead await the condition to be met since you're testing the CONDITION, not TIME PASSING
Verify your test fails with USE_SYSTEM_BUN=1 bun test <file> and passes with bun bd test <file> - tests are not valid if they pass with USE_SYSTEM_BUN=1
Avoid shell commands in tests - do not use find or grep; use Bun's Glob and built-in tools instead
Test files must end in .test.ts or .test.tsx and be created in the appropriate test folder structure

Files:

  • test/js/bun/util/stringWidth.test.ts
src/**/*.{cpp,zig}

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

src/**/*.{cpp,zig}: Use bun bd or bun run build:debug to build debug versions for C++ and Zig source files; creates debug build at ./build/debug/bun-debug
Run tests using bun bd test <test-file> with the debug build; never use bun test directly as it will not include your changes
Execute files using bun bd <file> <...args>; never use bun <file> directly as it will not include your changes
Enable debug logs for specific scopes using BUN_DEBUG_$(SCOPE)=1 environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required

Files:

  • src/string/immutable/visible.zig
src/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

Use bun.Output.scoped(.${SCOPE}, .hidden) for creating debug logs in Zig code

Implement core functionality in Zig, typically in its own directory in src/

src/**/*.zig: Private fields in Zig are fully supported using the # prefix: struct { #foo: u32 };
Use decl literals in Zig for declaration initialization: const decl: Decl = .{ .binding = 0, .value = 0 };
Prefer @import at the bottom of the file (auto formatter will move them automatically)

Be careful with memory management in Zig code - use defer for cleanup with allocators

Files:

  • src/string/immutable/visible.zig
**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)

**/*.zig: Expose generated bindings in Zig structs using pub const js = JSC.Codegen.JS<ClassName> with trait conversion methods: toJS, fromJS, and fromJSDirect
Use consistent parameter name globalObject instead of ctx in Zig constructor and method implementations
Use bun.JSError!JSValue return type for Zig methods and constructors to enable proper error handling and exception propagation
Implement resource cleanup using deinit() method that releases resources, followed by finalize() called by the GC that invokes deinit() and frees the pointer
Use JSC.markBinding(@src()) in finalize methods for debugging purposes before calling deinit()
For methods returning cached properties in Zig, declare external C++ functions using extern fn and callconv(JSC.conv) calling convention
Implement getter functions with naming pattern get<PropertyName> in Zig that accept this and globalObject parameters and return JSC.JSValue
Access JavaScript CallFrame arguments using callFrame.argument(i), check argument count with callFrame.argumentCount(), and get this with callFrame.thisValue()
For reference-counted objects, use .deref() in finalize instead of destroy() to release references to other JS objects

Files:

  • src/string/immutable/visible.zig
🧠 Learnings (12)
📓 Common learnings
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24212
File: src/cli/publish_command.zig:782-788
Timestamp: 2025-10-30T21:52:04.707Z
Learning: In the Bun codebase (oven-sh/bun), `enable_ansi_colors` flags are used to gate both ANSI color codes and Unicode box-drawing characters/emoji. This is the established pattern across the codebase.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: test/js/sql/sql.test.ts:195-202
Timestamp: 2025-09-25T22:07:13.851Z
Learning: PR oven-sh/bun#22946: JSON/JSONB result parsing updates (e.g., returning parsed arrays instead of legacy strings) are out of scope for this PR; tests keep current expectations with a TODO. Handle parsing fixes in a separate PR.
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22606
File: src/glob/GlobWalker.zig:449-452
Timestamp: 2025-09-12T18:16:50.754Z
Learning: For Bun codebase: prefer using `std.fs.path.sep` over manual platform separator detection, and use `bun.strings.lastIndexOfChar` instead of `std.mem.lastIndexOfScalar` for string operations.
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `Buffer.alloc(count, fill).toString()` instead of `'A'.repeat(count)` to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/css.test.ts : Organize CSS tests in css.test.ts for tests concerning bundling bugs with CSS files

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/html.test.ts : Organize HTML tests in html.test.ts for tests relating to HTML files themselves

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: See `test/harness.ts` for common test utilities and helpers

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Applies to test/**/*.{js,ts,jsx,tsx} : Write tests as JavaScript and TypeScript files using Jest-style APIs (`test`, `describe`, `expect`) and import from `bun:test`

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-12-02T05:59:51.485Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-02T05:59:51.485Z
Learning: Applies to test/**/*.test.{ts,tsx} : Verify your test fails with `USE_SYSTEM_BUN=1 bun test <file>` and passes with `bun bd test <file>` - tests are not valid if they pass with `USE_SYSTEM_BUN=1`

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Ensure V8 API tests compare identical C++ code output between Node.js and Bun through the test suite validation process

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:35:39.205Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-11-24T18:35:39.205Z
Learning: Add tests for new Bun runtime functionality

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-10-08T13:48:02.430Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23373
File: test/js/bun/tarball/extract.test.ts:107-111
Timestamp: 2025-10-08T13:48:02.430Z
Learning: In Bun's test runner, use `expect(async () => { await ... }).toThrow()` to assert async rejections. Unlike Jest/Vitest, Bun does not require `await expect(...).rejects.toThrow()` - the async function wrapper with `.toThrow()` is the correct pattern for async error assertions in Bun tests.

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-09-07T22:26:50.213Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22478
File: test/regression/issue/22475.test.ts:0-0
Timestamp: 2025-09-07T22:26:50.213Z
Learning: The `toBeDate()` matcher is acceptable and supported in Bun's test suite - don't suggest replacing it with `toBeInstanceOf(Date)`.

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
🔇 Additional comments (10)
test/js/bun/util/stringWidth.test.ts (4)

158-259: Comprehensive zero-width character test coverage.

This test suite thoroughly covers the expanded zero-width character handling including soft hyphen, word joiner, Arabic formatting characters, Indic script combining marks, and tag characters. The tests align well with the implementation changes in visible.zig.


360-436: Excellent emoji handling test coverage.

The emoji tests comprehensively cover:

  • Basic emoji (width 2)
  • Flag emoji (regional indicator pairs → width 2, single → width 1)
  • Skin tone modifiers
  • ZWJ sequences (family, professions)
  • Keycap sequences
  • Variation selectors (VS15 for text, VS16 for emoji presentation)

These tests align with the GraphemeState implementation.


340-358: Verify OSC sequence handling with ST terminator.

The tests for OSC sequences with ST terminator (ESC ) look correct. However, ensure the implementation correctly handles the backslash character after ESC in all cases—specifically when ESC and \ are processed in different iterations of the UTF-16 width loop.

#!/bin/bash
# Verify the OSC test expectations match what Bun actually produces
# This tests the ST terminator handling

cat << 'EOF' > /tmp/osc_test.ts
// Test OSC with ST terminator
const str1 = "\x1b]8;;https://example.com\x1b\\link\x1b]8;;\x1b\\";
console.log("OSC with ST:", Bun.stringWidth(str1), "expected: 4");

const str2 = "\x1b]8;;url\x07link\x1b]8;;\x07";
console.log("OSC with BEL:", Bun.stringWidth(str2), "expected: 4");

// Mixed OSC and CSI
const str3 = "\x1b[31m\x1b]8;;url\x07red link\x1b]8;;\x07\x1b[0m";
console.log("Mixed:", Bun.stringWidth(str3), "expected: 8");
EOF

echo "Running OSC tests..."
bun /tmp/osc_test.ts

642-653: The expected width of 2997 is correct for the mixed escape sequences test.

The calculation accurately reflects how the stringWidth function handles pattern boundaries. When the incomplete CSI (\x1b[) at the end of each pattern meets the CSI (\x1b[31m) at the start of the next pattern, the second [ terminates the first CSI, leaving the subsequent "31m" (3 characters) visible. With 1000 pattern repetitions, there are 999 boundaries, each contributing 3 visible characters: 999 × 3 = 2997.

src/string/immutable/visible.zig (6)

11-13: Correct soft hyphen handling.

Adding U+00AD (soft hyphen) as zero-width is correct—it's an invisible formatting character used for optional line breaks.


49-84: Comprehensive zero-width character coverage.

The expanded ranges correctly cover:

  • Lone surrogates (U+D800–U+DFFF) as zero-width
  • Arabic formatting characters
  • Indic script combining marks (with efficient offset calculation)
  • Thai/Lao combining marks
  • Combining Diacritical Marks Extended/Supplement
  • Tag characters (U+E0000–U+E007F)

776-903: Well-designed GraphemeState for complex emoji width calculation.

The GraphemeState struct correctly handles:

  • Regional indicator pairs (flags) → width 2
  • Keycap sequences → width 2
  • Skin tone modifiers with emoji base → width 2
  • ZWJ sequences → width 2
  • Variation selector logic (VS15 for text, VS16 for emoji presentation)
  • Proper handling of digits/ASCII with VS16 (remain width 1)

The use of saturating addition (+|=) at line 813 prevents overflow. The logic flow in width() correctly prioritizes special cases.


1018-1019: Correct final grapheme width finalization.

Adding the width of the final grapheme after the loop ensures the last grapheme cluster is properly counted. This is necessary because width is accumulated on grapheme breaks, which wouldn't include the trailing grapheme.


691-694: Latin1 width scalar correctly handles soft hyphen.

The updated condition correctly treats soft hyphen (0xAD) as zero-width alongside control characters.


1001-1002: Lone surrogate handling is correct.

The is_lead flag properly identifies unpaired high surrogates. It's set to true in two cases: (1) a high surrogate (0xD800–0xDBFF) with no following code unit, and (2) a high surrogate followed by a non-low-surrogate. The condition on line 1002 correctly skips these cases along with other invalid sequences (replacement.fail), treating lone surrogates as zero-width, which is consistent with the display width calculation.

Comment thread src/string/immutable/visible.zig Outdated
Comment on lines +494 to +500
test("very long strings", () => {
const long = "a".repeat(10000);
expect(Bun.stringWidth(long)).toBe(10000);

const longEmoji = "😀".repeat(1000);
expect(Bun.stringWidth(longEmoji)).toBe(2000);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Use Buffer.alloc for single-byte character repetition in stress tests.

Per coding guidelines, ''.repeat() is slow in debug JavaScriptCore builds. For single-byte ASCII characters, use Buffer.alloc instead.

 test("very long strings", () => {
-  const long = "a".repeat(10000);
+  const long = Buffer.alloc(10000, "a").toString();
   expect(Bun.stringWidth(long)).toBe(10000);

   const longEmoji = "😀".repeat(1000);
   expect(Bun.stringWidth(longEmoji)).toBe(2000);
 });

Note: The emoji .repeat() is fine since Buffer.alloc cannot handle multi-byte characters.

🤖 Prompt for AI Agents
In test/js/bun/util/stringWidth.test.ts around lines 494 to 500, the test uses
"a".repeat(10000) which is slow in debug JSC builds; replace the single-byte
ASCII repetition with a Buffer allocation filled with 'a' of length 10000 and
convert it to a string for the test, leaving the emoji.repeat() case unchanged
since Buffer.alloc can't handle multi-byte characters.

Comment on lines +512 to +517
test("many ESC characters without valid sequences", () => {
// Many bare ESC characters - should not hang
const input = "\x1b".repeat(10000);
// Each ESC is a control character with width 0
expect(Bun.stringWidth(input)).toBe(0);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Use Buffer.alloc for ESC character repetition.

Similar to above, this single-byte repetition can use Buffer.alloc for better debug build performance.

 test("many ESC characters without valid sequences", () => {
   // Many bare ESC characters - should not hang
-  const input = "\x1b".repeat(10000);
+  const input = Buffer.alloc(10000, 0x1b).toString();
   // Each ESC is a control character with width 0
   expect(Bun.stringWidth(input)).toBe(0);
 });
📝 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.

Suggested change
test("many ESC characters without valid sequences", () => {
// Many bare ESC characters - should not hang
const input = "\x1b".repeat(10000);
// Each ESC is a control character with width 0
expect(Bun.stringWidth(input)).toBe(0);
});
test("many ESC characters without valid sequences", () => {
// Many bare ESC characters - should not hang
const input = Buffer.alloc(10000, 0x1b).toString();
// Each ESC is a control character with width 0
expect(Bun.stringWidth(input)).toBe(0);
});
🤖 Prompt for AI Agents
In test/js/bun/util/stringWidth.test.ts around lines 512 to 517, replace the
string repetition of ESC ("\x1b".repeat(10000)) with a Buffer allocated and
filled with the ESC byte for better debug-build performance; create the input
using Buffer.alloc(10000, 0x1b) and keep the same assertion that
Bun.stringWidth(input) returns 0.

@Jarred-Sumner
Jarred-Sumner force-pushed the jarred/better-string-width branch 3 times, most recently from d7c5d30 to 0f104d0 Compare December 10, 2025 05:21

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

♻️ Duplicate comments (2)
test/js/bun/util/stringWidth.test.ts (2)

497-503: Use Buffer.alloc for single-byte repetition in stress tests.

Per coding guidelines, ''.repeat() is slow in debug JavaScriptCore builds. For single-byte ASCII characters, use Buffer.alloc instead.

Apply this diff:

 test("very long strings", () => {
-  const long = "a".repeat(10000);
+  const long = Buffer.alloc(10000, "a").toString();
   expect(Bun.stringWidth(long)).toBe(10000);

   const longEmoji = "😀".repeat(1000);
   expect(Bun.stringWidth(longEmoji)).toBe(2000);
 });

Note: The emoji .repeat() is fine since Buffer.alloc cannot handle multi-byte characters.

Based on coding guidelines for test files.


515-520: Use Buffer.alloc for ESC character repetition.

Similar to above, this single-byte repetition can use Buffer.alloc for better debug build performance.

Apply this diff:

 test("many ESC characters without valid sequences", () => {
   // Many bare ESC characters - should not hang
-  const input = "\x1b".repeat(10000);
+  const input = Buffer.alloc(10000, 0x1b).toString();
   // Each ESC is a control character with width 0
   expect(Bun.stringWidth(input)).toBe(0);
 });

Based on coding guidelines for test files.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between d7c5d3083c39c89fc3f8ffd78ec62585d0f35d66 and 0f104d0e606567e7d0aaab1e9e1505c4f3cb5c21.

⛔ Files ignored due to path filters (1)
  • bench/bun.lock is excluded by !**/*.lock
📒 Files selected for processing (5)
  • DESIGN.md (1 hunks)
  • a.js (1 hunks)
  • src/string/immutable/visible.zig (7 hunks)
  • test/js/bun/util/stringWidth.test.ts (1 hunks)
  • undici (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
test/**/*.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

test/**/*.{js,ts,jsx,tsx}: Write tests as JavaScript and TypeScript files using Jest-style APIs (test, describe, expect) and import from bun:test
Use test.each and data-driven tests to reduce boilerplate when testing multiple similar cases

Files:

  • test/js/bun/util/stringWidth.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}

📄 CodeRabbit inference engine (test/CLAUDE.md)

test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Use bun:test with files that end in *.test.{ts,js,jsx,tsx,mjs,cjs}
Do not write flaky tests. Never wait for time to pass in tests; always wait for the condition to be met instead of using an arbitrary amount of time
Never use hardcoded port numbers in tests. Always use port: 0 to get a random port
Prefer concurrent tests over sequential tests using test.concurrent or describe.concurrent when multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, use bunExe and bunEnv from harness to ensure the same build of Bun is used and debug logging is silenced
Use -e flag for single-file tests when spawning Bun processes
Use tempDir() from harness to create temporary directories with files for multi-file tests instead of creating files manually
Prefer async/await over callbacks in tests
When callbacks must be used and it's just a single callback, use Promise.withResolvers to create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
Use Buffer.alloc(count, fill).toString() instead of 'A'.repeat(count) to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Use describe blocks for grouping related tests
Always use await using or using to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Always check exit codes and test error scenarios in error tests
Use describe.each() for parameterized tests
Use toMatchSnapshot() for snapshot testing
Use beforeAll(), afterEach(), beforeEach() for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup in afterEach()

Files:

  • test/js/bun/util/stringWidth.test.ts
test/**/*.test.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

test/**/*.test.{ts,tsx}: For single-file tests in Bun test suite, prefer using -e flag over tempDir
For multi-file tests in Bun test suite, prefer using tempDir and Bun.spawn
Always use port: 0 when spawning servers in tests - do not hardcode ports or use custom random port functions
Use normalizeBunSnapshot to normalize snapshot output in tests instead of manual output comparison
Never write tests that check for no 'panic', 'uncaught exception', or similar strings in test output - that is not a valid test
Use tempDir from harness to create temporary directories in tests - do not use tmpdirSync or fs.mkdtempSync
In tests, call expect(stdout).toBe(...) before expect(exitCode).toBe(0) when spawning processes for more useful error messages on failure
Do not write flaky tests - do not use setTimeout in tests; instead await the condition to be met since you're testing the CONDITION, not TIME PASSING
Verify your test fails with USE_SYSTEM_BUN=1 bun test <file> and passes with bun bd test <file> - tests are not valid if they pass with USE_SYSTEM_BUN=1
Avoid shell commands in tests - do not use find or grep; use Bun's Glob and built-in tools instead
Test files must end in .test.ts or .test.tsx and be created in the appropriate test folder structure

Files:

  • test/js/bun/util/stringWidth.test.ts
src/**/*.{cpp,zig}

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

src/**/*.{cpp,zig}: Use bun bd or bun run build:debug to build debug versions for C++ and Zig source files; creates debug build at ./build/debug/bun-debug
Run tests using bun bd test <test-file> with the debug build; never use bun test directly as it will not include your changes
Execute files using bun bd <file> <...args>; never use bun <file> directly as it will not include your changes
Enable debug logs for specific scopes using BUN_DEBUG_$(SCOPE)=1 environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required

Files:

  • src/string/immutable/visible.zig
src/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

Use bun.Output.scoped(.${SCOPE}, .hidden) for creating debug logs in Zig code

Implement core functionality in Zig, typically in its own directory in src/

src/**/*.zig: Private fields in Zig are fully supported using the # prefix: struct { #foo: u32 };
Use decl literals in Zig for declaration initialization: const decl: Decl = .{ .binding = 0, .value = 0 };
Prefer @import at the bottom of the file (auto formatter will move them automatically)

Be careful with memory management in Zig code - use defer for cleanup with allocators

Files:

  • src/string/immutable/visible.zig
**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)

**/*.zig: Expose generated bindings in Zig structs using pub const js = JSC.Codegen.JS<ClassName> with trait conversion methods: toJS, fromJS, and fromJSDirect
Use consistent parameter name globalObject instead of ctx in Zig constructor and method implementations
Use bun.JSError!JSValue return type for Zig methods and constructors to enable proper error handling and exception propagation
Implement resource cleanup using deinit() method that releases resources, followed by finalize() called by the GC that invokes deinit() and frees the pointer
Use JSC.markBinding(@src()) in finalize methods for debugging purposes before calling deinit()
For methods returning cached properties in Zig, declare external C++ functions using extern fn and callconv(JSC.conv) calling convention
Implement getter functions with naming pattern get<PropertyName> in Zig that accept this and globalObject parameters and return JSC.JSValue
Access JavaScript CallFrame arguments using callFrame.argument(i), check argument count with callFrame.argumentCount(), and get this with callFrame.thisValue()
For reference-counted objects, use .deref() in finalize instead of destroy() to release references to other JS objects

Files:

  • src/string/immutable/visible.zig
🧠 Learnings (30)
📓 Common learnings
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: test/js/sql/sql.test.ts:195-202
Timestamp: 2025-09-25T22:07:13.851Z
Learning: PR oven-sh/bun#22946: JSON/JSONB result parsing updates (e.g., returning parsed arrays instead of legacy strings) are out of scope for this PR; tests keep current expectations with a TODO. Handle parsing fixes in a separate PR.
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24212
File: src/cli/publish_command.zig:782-788
Timestamp: 2025-10-30T21:52:04.707Z
Learning: In the Bun codebase (oven-sh/bun), `enable_ansi_colors` flags are used to gate both ANSI color codes and Unicode box-drawing characters/emoji. This is the established pattern across the codebase.
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Applies to test/cli/**/*.{js,ts,jsx,tsx} : When testing Bun as a CLI, use the `spawn` API from `bun` with the `bunExe()` and `bunEnv` from `harness` to execute Bun commands and validate exit codes, stdout, and stderr

Applied to files:

  • a.js
  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-06T00:58:23.965Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.

Applied to files:

  • a.js
📚 Learning: 2025-11-08T04:06:33.198Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24491
File: test/js/bun/transpiler/declare-global.test.ts:17-17
Timestamp: 2025-11-08T04:06:33.198Z
Learning: In Bun test files, `await using` with Bun.spawn() is the preferred pattern for spawned processes regardless of whether they are short-lived or long-running. Do not suggest replacing `await using proc = Bun.spawn(...)` with `const proc = Bun.spawn(...); await proc.exited;`.

Applied to files:

  • a.js
📚 Learning: 2025-10-26T01:32:04.844Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.

Applied to files:

  • a.js
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced

Applied to files:

  • a.js
  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Write JS builtins for Bun's Node.js compatibility and APIs, and run `bun bd` after changes

Applied to files:

  • a.js
📚 Learning: 2025-12-02T05:59:51.485Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-02T05:59:51.485Z
Learning: Applies to test/**/*.test.{ts,tsx} : For multi-file tests in Bun test suite, prefer using `tempDir` and `Bun.spawn`

Applied to files:

  • a.js
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `-e` flag for single-file tests when spawning Bun processes

Applied to files:

  • a.js
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output

Applied to files:

  • a.js
  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/js/node/test/{parallel,sequential}/*.js : For test/js/node/test/{parallel,sequential}/*.js files without a .test extension, use `bun bd <file>` instead of `bun bd test <file>` since these expect exit code 0 and don't use bun's test runner

Applied to files:

  • a.js
📚 Learning: 2025-10-04T21:17:53.040Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23253
File: test/js/valkey/valkey.failing-subscriber-no-ipc.ts:40-59
Timestamp: 2025-10-04T21:17:53.040Z
Learning: In Bun runtime, the global `console` object is an AsyncIterable that yields lines from stdin. The pattern `for await (const line of console)` is valid and documented in Bun for reading input line-by-line from the child process stdin.

Applied to files:

  • a.js
📚 Learning: 2025-09-02T05:33:37.517Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22323
File: test/js/web/websocket/websocket-subprotocol.test.ts:74-75
Timestamp: 2025-09-02T05:33:37.517Z
Learning: In Bun's runtime, `await using` with Node.js APIs like `net.createServer()` is properly supported and should not be replaced with explicit cleanup. Bun has extended Node.js APIs with proper async dispose support.

Applied to files:

  • a.js
📚 Learning: 2025-09-02T06:10:17.252Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22323
File: test/js/web/websocket/websocket-subprotocol-strict.test.ts:80-91
Timestamp: 2025-09-02T06:10:17.252Z
Learning: Bun's WebSocket implementation includes a terminate() method, which is available even though it's not part of the standard WebSocket API. This method can be used in Bun test files and applications for immediate WebSocket closure.

Applied to files:

  • a.js
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `Buffer.alloc(count, fill).toString()` instead of `'A'.repeat(count)` to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/css.test.ts : Organize CSS tests in css.test.ts for tests concerning bundling bugs with CSS files

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/html.test.ts : Organize HTML tests in html.test.ts for tests relating to HTML files themselves

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: See `test/harness.ts` for common test utilities and helpers

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Applies to test/**/*.{js,ts,jsx,tsx} : Write tests as JavaScript and TypeScript files using Jest-style APIs (`test`, `describe`, `expect`) and import from `bun:test`

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-12-02T05:59:51.485Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-02T05:59:51.485Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use `normalizeBunSnapshot` to normalize snapshot output in tests instead of manual output comparison

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/sourcemap.test.ts : Organize source-map tests in sourcemap.test.ts for tests verifying source-maps are correct

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use JSC intrinsics (prefixed with `$`) such as `$Array.from()`, `$isCallable()`, and `$newArrayWithSize()` for performance-critical operations

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-09-12T18:16:50.754Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22606
File: src/glob/GlobWalker.zig:449-452
Timestamp: 2025-09-12T18:16:50.754Z
Learning: For Bun codebase: prefer using `std.fs.path.sep` over manual platform separator detection, and use `bun.strings.lastIndexOfChar` instead of `std.mem.lastIndexOfScalar` for string operations.

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-12-02T05:59:51.485Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-02T05:59:51.485Z
Learning: Applies to test/**/*.test.{ts,tsx} : Verify your test fails with `USE_SYSTEM_BUN=1 bun test <file>` and passes with `bun bd test <file>` - tests are not valid if they pass with `USE_SYSTEM_BUN=1`

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:35:39.205Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-11-24T18:35:39.205Z
Learning: Add tests for new Bun runtime functionality

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-09-30T22:53:19.887Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 23117
File: src/bun.js/test/snapshot.zig:265-276
Timestamp: 2025-09-30T22:53:19.887Z
Learning: In Bun's snapshot testing (src/bun.js/test/snapshot.zig), multiple inline snapshots at the same line and column (same call position) must have identical values. However, multiple inline snapshots on the same line at different columns are allowed to have different values. The check is position-specific (line+col), not line-wide.

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Unit tests for specific features are organized by module (e.g., `/test/js/bun/`, `/test/js/node/`)

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:34:55.173Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/building-bun.mdc:0-0
Timestamp: 2025-11-24T18:34:55.173Z
Learning: Applies to src/**/*.{cpp,zig} : Use `bun bd` or `bun run build:debug` to build debug versions for C++ and Zig source files; creates debug build at `./build/debug/bun-debug`

Applied to files:

  • DESIGN.md
📚 Learning: 2025-11-24T18:34:55.173Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/building-bun.mdc:0-0
Timestamp: 2025-11-24T18:34:55.173Z
Learning: Applies to src/**/*.{cpp,zig} : Execute files using `bun bd <file> <...args>`; never use `bun <file>` directly as it will not include your changes

Applied to files:

  • DESIGN.md
📚 Learning: 2025-11-14T16:07:01.064Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 24719
File: docs/bundler/executables.mdx:527-560
Timestamp: 2025-11-14T16:07:01.064Z
Learning: In the Bun repository, certain bundler features like compile with code splitting (--compile --splitting) are CLI-only and not supported in the Bun.build() JavaScript API. Tests for CLI-only features use backend: "cli" flag (e.g., test/bundler/bundler_compile_splitting.test.ts). The CompileBuildConfig interface correctly restricts these with splitting?: never;. When documenting CLI-only bundler features, add a note clarifying they're not available via the programmatic API.

Applied to files:

  • DESIGN.md
🪛 markdownlint-cli2 (0.18.1)
DESIGN.md

15-15: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


34-34: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


73-73: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


119-119: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


158-158: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


223-223: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


223-223: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


232-232: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


245-245: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


264-264: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

🔇 Additional comments (7)
undici (1)

1-1: Clarify the necessity and scope of the submodule update.

The PR objectives focus on improving Bun.stringWidth (zero-width characters, ANSI/OSC sequences, emoji, tests), but the rationale for updating the undici submodule is not mentioned. Undici is an HTTP client library and appears unrelated to string width improvements.

Please confirm:

  1. Is this submodule update necessary for the stringWidth changes to function correctly?
  2. Was this update intentional or incidental?
  3. If intentional, what specific change in the commit (e.g., e3fedb0...) supports the stringWidth improvements?

If this update is unrelated to the PR objectives, consider removing it or addressing it in a separate PR to keep this change focused.

src/string/immutable/visible.zig (5)

11-88: LGTM! Comprehensive zero-width character handling.

The extended zero-width detection properly covers all the categories mentioned in the PR objectives:

  • Soft hyphen, word joiner, invisible operators
  • Lone surrogates
  • Arabic formatting characters
  • Indic script combining marks (clever bit-level logic for U+0900-0D4F blocks)
  • Thai and Lao combining marks
  • Extended and Supplement diacritical marks
  • Tag characters

The Indic script logic using cp & 0x7f is an efficient way to handle multiple Unicode blocks with similar structure.


776-927: Well-structured GraphemeState implementation.

The new GraphemeState struct properly encapsulates complex grapheme-cluster width logic for emoji and combining sequences. Key strengths:

  • Fast path optimization for ASCII in reset() (lines 798-809)
  • Comprehensive flag tracking for emoji components (keycaps, skin tones, ZWJ, variation selectors, regional indicators)
  • Correct priority order in width() method: regional indicator pairs → keycaps → single RI → skin tone modifiers → ZWJ sequences → variation selectors → fallback
  • Efficient use of ICU's UCHAR_EMOJI property for accurate emoji detection

The public API exposure as visible.GraphemeState allows reuse in other parts of the codebase.


696-744: Improved ANSI escape sequence handling.

The enhanced CSI and OSC sequence stripping properly handles:

CSI sequences (lines 709-719):

  • All final bytes in range 0x40-0x7E per ECMA-48, not just 'm' for colors
  • Now strips cursor movement, erase, scroll, and other CSI commands

OSC sequences (lines 720-735):

  • Both BEL (0x07) and ST (ESC ) terminators
  • Correctly advances by 2 bytes for ST terminator (line 731)
  • Handles OSC 8 hyperlinks and other OSC codes

The past review comment about ST terminator handling has been properly addressed.


929-1069: Excellent GraphemeState-based refactoring.

The refactored UTF-16 width calculation properly:

Fast path optimization (lines 944-970):

  • Bulk processes ASCII when not in escape sequences
  • Correctly handles the last character separately in case combining marks follow
  • Significant performance improvement for ASCII-heavy strings

State machine improvements:

  • Separate flags for CSI (saw_csi) and OSC (saw_osc) sequences
  • Proper handling of ESC ESC pattern (line 1017-1020)
  • All CSI final bytes (0x40-0x7E) correctly terminate sequences

Grapheme clustering:

  • Uses GraphemeState to accumulate codepoints within graphemes
  • Calls grapheme.graphemeBreak() to determine boundaries
  • Properly skips invalid UTF-16 and lone surrogates (lines 1049-1051)

Final grapheme handling (lines 1066-1067):

  • Correctly adds the last grapheme's width after the loop
  • Essential to avoid losing the final grapheme

The refactoring maintains correctness while adding support for complex emoji sequences.


691-694: Correct soft hyphen handling in Latin1 path.

The addition of soft hyphen (0xAD) to the zero-width check properly aligns the Latin1 fast path with the general zero-width detection in isZeroWidthCodepointType. This ensures consistent behavior across all code paths.

a.js (1)

1-24: Question: Is this file relevant to the PR objectives?

This example file demonstrates Bun.Terminal API usage but appears unrelated to the stringWidth improvements described in the PR objectives (zero-width characters, ANSI sequences, emoji handling). Was this file accidentally included?

⛔ Skipped due to learnings
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24212
File: src/cli/publish_command.zig:782-788
Timestamp: 2025-10-30T21:52:04.707Z
Learning: In the Bun codebase (oven-sh/bun), `enable_ansi_colors` flags are used to gate both ANSI color codes and Unicode box-drawing characters/emoji. This is the established pattern across the codebase.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24491
File: test/js/bun/transpiler/declare-global.test.ts:17-17
Timestamp: 2025-11-08T04:06:33.198Z
Learning: In Bun test files, `await using` with Bun.spawn() is the preferred pattern for spawned processes regardless of whether they are short-lived or long-running. Do not suggest replacing `await using proc = Bun.spawn(...)` with `const proc = Bun.spawn(...); await proc.exited;`.
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Applies to test/cli/**/*.{js,ts,jsx,tsx} : When testing Bun as a CLI, use the `spawn` API from `bun` with the `bunExe()` and `bunEnv` from `harness` to execute Bun commands and validate exit codes, stdout, and stderr
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 0f104d0e606567e7d0aaab1e9e1505c4f3cb5c21 and 2dfa1459b30cb8d027973b96d42370a020e4080d.

📒 Files selected for processing (1)
  • src/string/immutable/visible.zig (7 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,zig}

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

src/**/*.{cpp,zig}: Use bun bd or bun run build:debug to build debug versions for C++ and Zig source files; creates debug build at ./build/debug/bun-debug
Run tests using bun bd test <test-file> with the debug build; never use bun test directly as it will not include your changes
Execute files using bun bd <file> <...args>; never use bun <file> directly as it will not include your changes
Enable debug logs for specific scopes using BUN_DEBUG_$(SCOPE)=1 environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required

Files:

  • src/string/immutable/visible.zig
src/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

Use bun.Output.scoped(.${SCOPE}, .hidden) for creating debug logs in Zig code

Implement core functionality in Zig, typically in its own directory in src/

src/**/*.zig: Private fields in Zig are fully supported using the # prefix: struct { #foo: u32 };
Use decl literals in Zig for declaration initialization: const decl: Decl = .{ .binding = 0, .value = 0 };
Prefer @import at the bottom of the file (auto formatter will move them automatically)

Be careful with memory management in Zig code - use defer for cleanup with allocators

Files:

  • src/string/immutable/visible.zig
**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)

**/*.zig: Expose generated bindings in Zig structs using pub const js = JSC.Codegen.JS<ClassName> with trait conversion methods: toJS, fromJS, and fromJSDirect
Use consistent parameter name globalObject instead of ctx in Zig constructor and method implementations
Use bun.JSError!JSValue return type for Zig methods and constructors to enable proper error handling and exception propagation
Implement resource cleanup using deinit() method that releases resources, followed by finalize() called by the GC that invokes deinit() and frees the pointer
Use JSC.markBinding(@src()) in finalize methods for debugging purposes before calling deinit()
For methods returning cached properties in Zig, declare external C++ functions using extern fn and callconv(JSC.conv) calling convention
Implement getter functions with naming pattern get<PropertyName> in Zig that accept this and globalObject parameters and return JSC.JSValue
Access JavaScript CallFrame arguments using callFrame.argument(i), check argument count with callFrame.argumentCount(), and get this with callFrame.thisValue()
For reference-counted objects, use .deref() in finalize instead of destroy() to release references to other JS objects

Files:

  • src/string/immutable/visible.zig
🧠 Learnings (1)
📓 Common learnings
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: test/js/sql/sql.test.ts:195-202
Timestamp: 2025-09-25T22:07:13.851Z
Learning: PR oven-sh/bun#22946: JSON/JSONB result parsing updates (e.g., returning parsed arrays instead of legacy strings) are out of scope for this PR; tests keep current expectations with a TODO. Handle parsing fixes in a separate PR.
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22606
File: src/glob/GlobWalker.zig:449-452
Timestamp: 2025-09-12T18:16:50.754Z
Learning: For Bun codebase: prefer using `std.fs.path.sep` over manual platform separator detection, and use `bun.strings.lastIndexOfChar` instead of `std.mem.lastIndexOfScalar` for string operations.
🔇 Additional comments (5)
src/string/immutable/visible.zig (5)

11-88: Comprehensive zero-width character handling improvements look good.

The expanded coverage now includes soft hyphen, word joiner, Arabic formatting characters, Indic/Thai/Lao combining marks, extended diacritical marks, tag characters, and surrogates. The efficient bitmask logic for Indic scripts (lines 58-64) is well-designed.

Note: Treating lone surrogates (lines 49-51) as zero-width is a breaking change documented in the PR summary—typical behavior would render them as U+FFFD with width 1.


691-744: ANSI escape sequence handling significantly improved.

The CSI enhancement (lines 709-719) now correctly strips all CSI sequences (cursor movement, erase, scroll, etc.) by recognizing any final byte in range 0x40-0x7E, not just SGR. The OSC sequence support (lines 720-735) properly handles both BEL and ST terminators, enabling correct width calculation for OSC 8 hyperlinks.

The soft hyphen update (line 693) maintains consistency with the expanded zero-width classification.


776-892: Clarify intended visibility of GraphemeState.

The AI summary indicates "GraphemeState made publicly accessible via the visible module," but line 792 declares it as const GraphemeState without the pub keyword. This makes it inaccessible outside the visible struct.

If GraphemeState is intended as a public API (e.g., for external grapheme-aware width calculations), add pub to line 792:

-    const GraphemeState = struct {
+    pub const GraphemeState = struct {

If it's intentionally private for internal use only, the implementation is correct but the AI summary should be updated.

The grapheme width calculation logic (lines 840-866) comprehensively handles regional indicator pairs, keycaps, emoji with skin tones/ZWJ, and variation selectors—well done!


920-957: Excellent fast-path optimization for bulk ASCII processing.

The optimization (lines 936-957) cleverly handles common-case ASCII strings by:

  1. Flushing any pending grapheme from previous non-ASCII processing
  2. Counting all but the last ASCII character in bulk using SIMD
  3. Holding the last character in grapheme_state in case a combining mark follows
  4. Adding the final character's width at function end (line 1054)

This avoids double-counting and correctly handles combining marks that might follow ASCII characters. The condition !exclude_ansi_colors (line 938) is correct—when excluding ANSI codes, per-character ESC detection is required, preventing the fast path.


959-1056: Main loop correctly handles escape sequences and grapheme boundaries.

The OSC handling (lines 963-983) properly detects both BEL and ST terminators, and correctly waits for '' after ESC before exiting OSC mode (lines 976-979). The CSI handling (lines 984-994) recognizes all final bytes in range 0x40-0x7E. Consecutive ESC sequences (lines 1004-1007) are handled correctly by keeping saw_1b = true.

The grapheme processing (lines 1014-1024, 1041-1051) properly maintains state across codepoints and adds the final grapheme width at line 1054.

Surrogate handling note: Lines 1036-1037 skip invalid sequences and lone surrogates, treating them as zero-width. This aligns with the expanded isZeroWidthCodepointType (lines 49-51) and is documented as a breaking change in the PR summary—most terminals would render lone surrogates as U+FFFD with width 1.

Comment on lines +894 to +918
/// Count printable ASCII characters (0x20-0x7E) in a UTF-16 slice using SIMD
fn countPrintableAscii16(input: []const u16) usize {
var total: usize = 0;
var remaining = input;

// Process 8 u16 values at a time using SIMD
const vec_len = 8;
while (remaining.len >= vec_len) {
const chunk: @Vector(vec_len, u16) = remaining[0..vec_len].*;
const low: @Vector(vec_len, u16) = @splat(0x20);
const high: @Vector(vec_len, u16) = @splat(0x7F);
const ge_low = chunk >= low;
const lt_high = chunk < high;
const printable = @select(bool, ge_low, lt_high, @as(@Vector(vec_len, bool), @splat(false)));
total += @popCount(@as(u8, @bitCast(printable)));
remaining = remaining[vec_len..];
}

// Handle remaining elements
for (remaining) |c| {
total += @intFromBool(c >= 0x20 and c < 0x7F);
}

return total;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Potential issue with boolean vector bitcast.

Line 908 bitcasts @Vector(8, bool) to u8 and uses @popCount to count true values. This assumes each bool occupies exactly 1 bit, but Zig's boolean vector representation is implementation-defined and may use 1 byte per element, causing the bitcast to truncate and produce incorrect counts.

Use @reduce for a portable solution:

-            const printable = @select(bool, ge_low, lt_high, @as(@Vector(vec_len, bool), @splat(false)));
-            total += @popCount(@as(u8, @bitCast(printable)));
+            const printable_int = @select(u16, ge_low and lt_high, 
+                @as(@Vector(vec_len, u16), @splat(1)), 
+                @as(@Vector(vec_len, u16), @splat(0)));
+            total += @reduce(.Add, printable_int);

This counts without relying on specific boolean vector layout.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 0f104d0e606567e7d0aaab1e9e1505c4f3cb5c21 and 8b88fec9ea022c52a640e7b7b3832e69debdcdea.

📒 Files selected for processing (1)
  • src/string/immutable/visible.zig (7 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,zig}

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

src/**/*.{cpp,zig}: Use bun bd or bun run build:debug to build debug versions for C++ and Zig source files; creates debug build at ./build/debug/bun-debug
Run tests using bun bd test <test-file> with the debug build; never use bun test directly as it will not include your changes
Execute files using bun bd <file> <...args>; never use bun <file> directly as it will not include your changes
Enable debug logs for specific scopes using BUN_DEBUG_$(SCOPE)=1 environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required

Files:

  • src/string/immutable/visible.zig
src/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

Use bun.Output.scoped(.${SCOPE}, .hidden) for creating debug logs in Zig code

Implement core functionality in Zig, typically in its own directory in src/

src/**/*.zig: Private fields in Zig are fully supported using the # prefix: struct { #foo: u32 };
Use decl literals in Zig for declaration initialization: const decl: Decl = .{ .binding = 0, .value = 0 };
Prefer @import at the bottom of the file (auto formatter will move them automatically)

Be careful with memory management in Zig code - use defer for cleanup with allocators

Files:

  • src/string/immutable/visible.zig
**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)

**/*.zig: Expose generated bindings in Zig structs using pub const js = JSC.Codegen.JS<ClassName> with trait conversion methods: toJS, fromJS, and fromJSDirect
Use consistent parameter name globalObject instead of ctx in Zig constructor and method implementations
Use bun.JSError!JSValue return type for Zig methods and constructors to enable proper error handling and exception propagation
Implement resource cleanup using deinit() method that releases resources, followed by finalize() called by the GC that invokes deinit() and frees the pointer
Use JSC.markBinding(@src()) in finalize methods for debugging purposes before calling deinit()
For methods returning cached properties in Zig, declare external C++ functions using extern fn and callconv(JSC.conv) calling convention
Implement getter functions with naming pattern get<PropertyName> in Zig that accept this and globalObject parameters and return JSC.JSValue
Access JavaScript CallFrame arguments using callFrame.argument(i), check argument count with callFrame.argumentCount(), and get this with callFrame.thisValue()
For reference-counted objects, use .deref() in finalize instead of destroy() to release references to other JS objects

Files:

  • src/string/immutable/visible.zig
🧠 Learnings (1)
📓 Common learnings
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: test/js/sql/sql.test.ts:195-202
Timestamp: 2025-09-25T22:07:13.851Z
Learning: PR oven-sh/bun#22946: JSON/JSONB result parsing updates (e.g., returning parsed arrays instead of legacy strings) are out of scope for this PR; tests keep current expectations with a TODO. Handle parsing fixes in a separate PR.
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22606
File: src/glob/GlobWalker.zig:449-452
Timestamp: 2025-09-12T18:16:50.754Z
Learning: For Bun codebase: prefer using `std.fs.path.sep` over manual platform separator detection, and use `bun.strings.lastIndexOfChar` instead of `std.mem.lastIndexOfScalar` for string operations.
🔇 Additional comments (6)
src/string/immutable/visible.zig (6)

11-88: LGTM - Comprehensive zero-width codepoint coverage.

The expanded zero-width handling correctly covers soft hyphen, word joiner, surrogates, Arabic formatting characters, Indic combining marks, Thai/Lao combining marks, extended diacritical marks, and tag characters. These additions align with the PR objectives and Unicode standards for non-displaying characters.


692-693: LGTM - Consistent soft hyphen handling.

The soft hyphen (0xAD) is correctly treated as zero-width, consistent with the addition in isZeroWidthCodepointType.


707-735: LGTM - Robust ANSI escape sequence handling.

The enhanced CSI handling correctly excludes all CSI final bytes (0x40-0x7E), and the new OSC sequence support properly handles both BEL and ST terminators. The Latin-1 path correctly advances past both characters of the ST terminator (ESC ).


776-892: Excellent GraphemeState design for emoji-aware width calculation.

The GraphemeState structure is well-designed with efficient bit packing in PackedState and comprehensive emoji handling. The width calculation correctly handles regional indicator pairs (flags), keycap sequences, skin tone modifiers, ZWJ sequences, and variation selectors. Using ICU's UCHAR_EMOJI property ensures accurate emoji detection.

The public API exposure aligns with the PR objectives and provides a solid foundation for grapheme-aware width calculations.


894-918: LGTM - Efficient SIMD optimization for ASCII counting.

The countPrintableAscii16 function uses SIMD effectively to count printable ASCII characters (0x20-0x7E) in UTF-16 strings, processing 8 characters at a time with vector comparisons and popcount. This is a solid performance optimization for the bulk ASCII path.


1071-1072: LGTM - Essential final grapheme width accounting.

Correctly adds the width of the final grapheme cluster after the main loop completes, ensuring complex emoji sequences at the end of the string are properly counted.

@Jarred-Sumner
Jarred-Sumner force-pushed the jarred/better-string-width branch from 8b88fec to c9ea71a Compare December 10, 2025 05:59
This PR significantly improves Bun.stringWidth to handle a wider variety of
Unicode characters and escape sequences correctly.

## Zero-width character handling

Added support for many previously unhandled zero-width characters:
- Soft hyphen (U+00AD)
- Word joiner and invisible operators (U+2060-U+2064)
- Lone surrogates (U+D800-U+DFFF)
- Arabic formatting characters (U+0600-U+0605, U+06DD, U+070F, U+08E2)
- Indic script combining marks (Devanagari through Malayalam)
- Thai and Lao combining marks
- Combining Diacritical Marks Extended and Supplement
- Tag characters (U+E0000-U+E007F)

## ANSI escape sequence handling

### CSI sequences
- Now properly handles ALL CSI final bytes (0x40-0x7E), not just 'm'
- This means cursor movement (A/B/C/D), erase (J/K), scroll (S/T), and
  other CSI commands are now correctly excluded from width calculation

### OSC sequences
- Added support for OSC sequences (ESC ] ... BEL/ST)
- OSC 8 hyperlinks are now properly handled
- Supports both BEL (0x07) and ST (ESC \) terminators

### ESC ESC fix
- Fixed state machine bug where ESC ESC would incorrectly reset state
- Now correctly handles consecutive ESC characters

## Emoji handling

Added proper grapheme-aware emoji width calculation:
- Flag emoji (regional indicator pairs) → width 2
- Skin tone modifiers → width 2
- ZWJ sequences (family, professions, etc.) → width 2
- Keycap sequences → width 2
- Variation selectors (VS15 for text, VS16 for emoji presentation)
- Uses ICU's UCHAR_EMOJI property for accurate emoji detection

## Test coverage

Added comprehensive test suite with 94 tests covering:
- All zero-width character categories
- All CSI final bytes
- OSC sequences with various terminators
- Emoji edge cases (flags, skin tones, ZWJ, keycaps, variation selectors)
- East Asian width (CJK, fullwidth, halfwidth katakana)
- Indic and Thai script combining marks
- Fuzzer-like stress tests for robustness

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@Jarred-Sumner
Jarred-Sumner force-pushed the jarred/better-string-width branch from c9ea71a to fbdbe74 Compare December 10, 2025 06:00

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (4)
src/string/immutable/visible.zig (1)

894-918: Boolean vector bitcast may have implementation-defined behavior.

Line 908 bitcasts @Vector(8, bool) to u8 for @popCount. While this likely works in practice (LLVM typically packs bool vectors as i1 per element), the Zig language specification doesn't guarantee this representation.

Consider using @reduce for a more portable implementation:

-            const printable = @select(bool, ge_low, lt_high, @as(@Vector(vec_len, bool), @splat(false)));
-            total += @popCount(@as(u8, @bitCast(printable)));
+            const printable = ge_low and lt_high;
+            const ones: @Vector(vec_len, u16) = @splat(1);
+            const zeros: @Vector(vec_len, u16) = @splat(0);
+            total += @reduce(.Add, @select(u16, printable, ones, zeros));

Alternatively, verify this works correctly by running the test suite on different architectures.

test/js/bun/util/stringWidth.test.ts (3)

497-503: Use Buffer.alloc for single-byte character repetition.

Per coding guidelines, ''.repeat() is slow in debug JavaScriptCore builds. Use Buffer.alloc for the single-byte ASCII string.

 test("very long strings", () => {
-  const long = "a".repeat(10000);
+  const long = Buffer.alloc(10000, "a").toString();
   expect(Bun.stringWidth(long)).toBe(10000);

   const longEmoji = "😀".repeat(1000);
   expect(Bun.stringWidth(longEmoji)).toBe(2000);
 });

Note: The emoji .repeat() is acceptable since Buffer.alloc cannot handle multi-byte characters.


515-520: Use Buffer.alloc for ESC character repetition.

Per coding guidelines, single-byte repetition should use Buffer.alloc for better debug build performance.

 test("many ESC characters without valid sequences", () => {
-  const input = "\x1b".repeat(10000);
+  const input = Buffer.alloc(10000, 0x1b).toString();
   expect(Bun.stringWidth(input)).toBe(0);
 });

522-535: Additional single-byte repetitions could use Buffer.alloc.

Lines 524 and 532 use single-byte character repetition that would benefit from Buffer.alloc in debug builds.

 test("CSI without final byte (unterminated)", () => {
-  const input = "a\x1b[" + "9".repeat(10000) + "b";
+  const input = "a\x1b[" + Buffer.alloc(10000, "9").toString() + "b";
   expect(Bun.stringWidth(input)).toBeGreaterThanOrEqual(1);
 });

 test("OSC without terminator (unterminated)", () => {
-  const input = "a\x1b]8;;" + "x".repeat(10000);
+  const input = "a\x1b]8;;" + Buffer.alloc(10000, "x").toString();
   expect(Bun.stringWidth(input)).toBe(1);
 });
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 8b88fec9ea022c52a640e7b7b3832e69debdcdea and fbdbe74.

⛔ Files ignored due to path filters (1)
  • bench/bun.lock is excluded by !**/*.lock
📒 Files selected for processing (2)
  • src/string/immutable/visible.zig (7 hunks)
  • test/js/bun/util/stringWidth.test.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.{cpp,zig}

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

src/**/*.{cpp,zig}: Use bun bd or bun run build:debug to build debug versions for C++ and Zig source files; creates debug build at ./build/debug/bun-debug
Run tests using bun bd test <test-file> with the debug build; never use bun test directly as it will not include your changes
Execute files using bun bd <file> <...args>; never use bun <file> directly as it will not include your changes
Enable debug logs for specific scopes using BUN_DEBUG_$(SCOPE)=1 environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required

Files:

  • src/string/immutable/visible.zig
src/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

Use bun.Output.scoped(.${SCOPE}, .hidden) for creating debug logs in Zig code

Implement core functionality in Zig, typically in its own directory in src/

src/**/*.zig: Private fields in Zig are fully supported using the # prefix: struct { #foo: u32 };
Use decl literals in Zig for declaration initialization: const decl: Decl = .{ .binding = 0, .value = 0 };
Prefer @import at the bottom of the file (auto formatter will move them automatically)

Be careful with memory management in Zig code - use defer for cleanup with allocators

Files:

  • src/string/immutable/visible.zig
**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)

**/*.zig: Expose generated bindings in Zig structs using pub const js = JSC.Codegen.JS<ClassName> with trait conversion methods: toJS, fromJS, and fromJSDirect
Use consistent parameter name globalObject instead of ctx in Zig constructor and method implementations
Use bun.JSError!JSValue return type for Zig methods and constructors to enable proper error handling and exception propagation
Implement resource cleanup using deinit() method that releases resources, followed by finalize() called by the GC that invokes deinit() and frees the pointer
Use JSC.markBinding(@src()) in finalize methods for debugging purposes before calling deinit()
For methods returning cached properties in Zig, declare external C++ functions using extern fn and callconv(JSC.conv) calling convention
Implement getter functions with naming pattern get<PropertyName> in Zig that accept this and globalObject parameters and return JSC.JSValue
Access JavaScript CallFrame arguments using callFrame.argument(i), check argument count with callFrame.argumentCount(), and get this with callFrame.thisValue()
For reference-counted objects, use .deref() in finalize instead of destroy() to release references to other JS objects

Files:

  • src/string/immutable/visible.zig
test/**/*.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

test/**/*.{js,ts,jsx,tsx}: Write tests as JavaScript and TypeScript files using Jest-style APIs (test, describe, expect) and import from bun:test
Use test.each and data-driven tests to reduce boilerplate when testing multiple similar cases

Files:

  • test/js/bun/util/stringWidth.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}

📄 CodeRabbit inference engine (test/CLAUDE.md)

test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Use bun:test with files that end in *.test.{ts,js,jsx,tsx,mjs,cjs}
Do not write flaky tests. Never wait for time to pass in tests; always wait for the condition to be met instead of using an arbitrary amount of time
Never use hardcoded port numbers in tests. Always use port: 0 to get a random port
Prefer concurrent tests over sequential tests using test.concurrent or describe.concurrent when multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, use bunExe and bunEnv from harness to ensure the same build of Bun is used and debug logging is silenced
Use -e flag for single-file tests when spawning Bun processes
Use tempDir() from harness to create temporary directories with files for multi-file tests instead of creating files manually
Prefer async/await over callbacks in tests
When callbacks must be used and it's just a single callback, use Promise.withResolvers to create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
Use Buffer.alloc(count, fill).toString() instead of 'A'.repeat(count) to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Use describe blocks for grouping related tests
Always use await using or using to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Always check exit codes and test error scenarios in error tests
Use describe.each() for parameterized tests
Use toMatchSnapshot() for snapshot testing
Use beforeAll(), afterEach(), beforeEach() for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup in afterEach()

Files:

  • test/js/bun/util/stringWidth.test.ts
test/**/*.test.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

test/**/*.test.{ts,tsx}: For single-file tests in Bun test suite, prefer using -e flag over tempDir
For multi-file tests in Bun test suite, prefer using tempDir and Bun.spawn
Always use port: 0 when spawning servers in tests - do not hardcode ports or use custom random port functions
Use normalizeBunSnapshot to normalize snapshot output in tests instead of manual output comparison
Never write tests that check for no 'panic', 'uncaught exception', or similar strings in test output - that is not a valid test
Use tempDir from harness to create temporary directories in tests - do not use tmpdirSync or fs.mkdtempSync
In tests, call expect(stdout).toBe(...) before expect(exitCode).toBe(0) when spawning processes for more useful error messages on failure
Do not write flaky tests - do not use setTimeout in tests; instead await the condition to be met since you're testing the CONDITION, not TIME PASSING
Verify your test fails with USE_SYSTEM_BUN=1 bun test <file> and passes with bun bd test <file> - tests are not valid if they pass with USE_SYSTEM_BUN=1
Avoid shell commands in tests - do not use find or grep; use Bun's Glob and built-in tools instead
Test files must end in .test.ts or .test.tsx and be created in the appropriate test folder structure

Files:

  • test/js/bun/util/stringWidth.test.ts
🧠 Learnings (26)
📚 Learning: 2025-10-24T10:43:09.398Z
Learnt from: fmguerreiro
Repo: oven-sh/bun PR: 23774
File: src/install/PackageManager/updatePackageJSONAndInstall.zig:548-548
Timestamp: 2025-10-24T10:43:09.398Z
Learning: In Bun's Zig codebase, the `as(usize, intCast(...))` cast pattern triggers a Zig compiler bug that causes compilation to hang indefinitely when used in complex control flow contexts (loops + short-circuit operators + optional unwrapping). Avoid this pattern and use simpler alternatives like just `intCast(...)` if type casting is necessary.

Applied to files:

  • src/string/immutable/visible.zig
📚 Learning: 2025-09-04T02:04:43.094Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22278
File: src/ast/E.zig:980-1003
Timestamp: 2025-09-04T02:04:43.094Z
Learning: In Bun's Zig codebase, `as(i32, u8_or_u16_value)` is sufficient for casting u8/u16 to i32 in comparison operations. `intCast` is not required in this context, and the current casting approach compiles successfully.

Applied to files:

  • src/string/immutable/visible.zig
📚 Learning: 2025-11-10T00:57:09.173Z
Learnt from: franciscop
Repo: oven-sh/bun PR: 24514
File: src/bun.js/api/crypto/PasswordObject.zig:86-101
Timestamp: 2025-11-10T00:57:09.173Z
Learning: In Bun's Zig codebase (PasswordObject.zig), when validating the parallelism parameter for Argon2, the upper limit is set to 65535 (2^16 - 1) rather than using `std.math.maxInt(u24)` because the latter triggers Zig's truncation limit checks. The value 65535 is a practical upper bound that avoids compiler issues while being sufficient for thread parallelism use cases.

Applied to files:

  • src/string/immutable/visible.zig
📚 Learning: 2025-10-15T22:03:50.832Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23710
File: src/envvars.zig:135-144
Timestamp: 2025-10-15T22:03:50.832Z
Learning: In src/envvars.zig, the boolean feature flag cache uses a single atomic enum and should remain monotonic. Only the string cache (which uses two atomics: ptr and len) requires acquire/release ordering to prevent torn reads.

Applied to files:

  • src/string/immutable/visible.zig
📚 Learning: 2025-10-16T21:24:52.779Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23710
File: src/crash_handler.zig:1415-1423
Timestamp: 2025-10-16T21:24:52.779Z
Learning: When a boolean EnvVar in src/envvars.zig is defined with a default value (e.g., `.default = false`), the `get()` method returns `bool` instead of `?bool`. This means you cannot distinguish between "environment variable not set" and "environment variable explicitly set to the default value". For opt-out scenarios where detection of explicit setting is needed (like `BUN_ENABLE_CRASH_REPORTING` on platforms where crash reporting defaults to enabled), either: (1) don't provide a default value so `get()` returns `?bool`, or (2) use the returned boolean directly instead of only checking if it's true.

Applied to files:

  • src/string/immutable/visible.zig
📚 Learning: 2025-09-02T19:17:26.376Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 0
File: :0-0
Timestamp: 2025-09-02T19:17:26.376Z
Learning: In Bun's Zig codebase, when handling error unions where the same cleanup operation (like `rawFree`) needs to be performed regardless of success or failure, prefer using boolean folding with `else |err| switch (err)` over duplicating the cleanup call in multiple switch branches. This approach avoids code duplication while maintaining compile-time error checking.

Applied to files:

  • src/string/immutable/visible.zig
📚 Learning: 2025-09-02T17:14:01.470Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/ptr/shared.zig:470-470
Timestamp: 2025-09-02T17:14:01.470Z
Learning: In Zig, regular arithmetic operations (like +=) have built-in overflow detection in debug/safe builds and will panic automatically. Atomic operations (like fetchAdd) do not have this automatic safety checking, so explicit overflow checks must be added manually. This is why NonAtomicCount doesn't need explicit overflow checks while AtomicCount does.

Applied to files:

  • src/string/immutable/visible.zig
📚 Learning: 2025-09-07T14:00:36.526Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22258
File: src/bun.js/test/jest.zig:63-86
Timestamp: 2025-09-07T14:00:36.526Z
Learning: In Bun's Zig codebase, the established pattern for HashMap hash functions is to truncate hash values to u32, even when the underlying hash computation produces u64. This is seen throughout the codebase, such as in src/css/rules/rules.zig where u64 hashes are stored but truncated to u32 in HashMap contexts. When implementing HashContext for HashMapUnmanaged, the hash function should return u32 to match this pattern.

Applied to files:

  • src/string/immutable/visible.zig
📚 Learning: 2025-11-12T04:11:52.293Z
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 24622
File: src/deps/uws/us_socket_t.zig:112-113
Timestamp: 2025-11-12T04:11:52.293Z
Learning: In Bun's Zig codebase, when passing u32 values to C FFI functions that expect c_uint parameters, no explicit intCast is needed because c_uint is equivalent to u32 on Bun's target platforms and Zig allows implicit coercion between equivalent types. This pattern is used consistently throughout src/deps/uws/us_socket_t.zig in functions like setTimeout, setLongTimeout, and setKeepalive.

Applied to files:

  • src/string/immutable/visible.zig
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `Buffer.alloc(count, fill).toString()` instead of `'A'.repeat(count)` to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/css.test.ts : Organize CSS tests in css.test.ts for tests concerning bundling bugs with CSS files

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/html.test.ts : Organize HTML tests in html.test.ts for tests relating to HTML files themselves

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: See `test/harness.ts` for common test utilities and helpers

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Applies to test/**/*.{js,ts,jsx,tsx} : Write tests as JavaScript and TypeScript files using Jest-style APIs (`test`, `describe`, `expect`) and import from `bun:test`

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-12-02T05:59:51.485Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-02T05:59:51.485Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use `normalizeBunSnapshot` to normalize snapshot output in tests instead of manual output comparison

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/sourcemap.test.ts : Organize source-map tests in sourcemap.test.ts for tests verifying source-maps are correct

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use JSC intrinsics (prefixed with `$`) such as `$Array.from()`, `$isCallable()`, and `$newArrayWithSize()` for performance-critical operations

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-09-12T18:16:50.754Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22606
File: src/glob/GlobWalker.zig:449-452
Timestamp: 2025-09-12T18:16:50.754Z
Learning: For Bun codebase: prefer using `std.fs.path.sep` over manual platform separator detection, and use `bun.strings.lastIndexOfChar` instead of `std.mem.lastIndexOfScalar` for string operations.

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-12-02T05:59:51.485Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-02T05:59:51.485Z
Learning: Applies to test/**/*.test.{ts,tsx} : Verify your test fails with `USE_SYSTEM_BUN=1 bun test <file>` and passes with `bun bd test <file>` - tests are not valid if they pass with `USE_SYSTEM_BUN=1`

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Applies to test/cli/**/*.{js,ts,jsx,tsx} : When testing Bun as a CLI, use the `spawn` API from `bun` with the `bunExe()` and `bunEnv` from `harness` to execute Bun commands and validate exit codes, stdout, and stderr

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:35:39.205Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-11-24T18:35:39.205Z
Learning: Add tests for new Bun runtime functionality

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-09-30T22:53:19.887Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 23117
File: src/bun.js/test/snapshot.zig:265-276
Timestamp: 2025-09-30T22:53:19.887Z
Learning: In Bun's snapshot testing (src/bun.js/test/snapshot.zig), multiple inline snapshots at the same line and column (same call position) must have identical values. However, multiple inline snapshots on the same line at different columns are allowed to have different values. The check is position-specific (line+col), not line-wide.

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Unit tests for specific features are organized by module (e.g., `/test/js/bun/`, `/test/js/node/`)

Applied to files:

  • test/js/bun/util/stringWidth.test.ts
🔇 Additional comments (6)
src/string/immutable/visible.zig (4)

11-88: LGTM - Comprehensive zero-width character coverage.

The expanded zero-width handling correctly covers soft hyphen, word joiner/invisible operators, surrogates, Arabic formatting, Indic combining marks, Thai/Lao marks, and tag characters. The Indic mask logic efficiently identifies combining positions within each script block.


691-694: LGTM - Soft hyphen handling added consistently.

The scalar function now correctly treats soft hyphen (0xAD) as zero-width, matching the isZeroWidthCodepointType implementation.


696-744: LGTM - Robust CSI/OSC handling.

The ANSI sequence handling correctly:

  • Recognizes all CSI final bytes (0x40-0x7E) per ECMA-48
  • Handles both OSC terminators (BEL and ST)
  • Advances correctly past the ST terminator (ESC )

868-883: The fast-path range logic is correct and does not incorrectly exclude emoji.

U+2600–U+26FF (Miscellaneous Symbols) are not caught by the line 875 fast-path exclusion (cp >= 0x2C00), as 0x2600 < 0x2C00. These emoji correctly proceed to the ICU check. Test coverage confirms this: stringWidth.test.ts (lines 402, 406) actively test ☀ (U+2600) with variation selectors and expect proper width calculations.

Likely an incorrect or invalid review comment.

test/js/bun/util/stringWidth.test.ts (2)

158-259: LGTM - Comprehensive zero-width character tests.

The tests thoroughly cover:

  • Soft hyphen, word joiner, invisible operators
  • ZWS/ZWNJ/ZWJ, directional marks, BOM
  • Arabic formatting characters
  • Variation selectors, tag characters, surrogates
  • All combining diacritical mark ranges
  • Control characters (C0, DEL, C1)

261-439: LGTM - Thorough ANSI and emoji sequence coverage.

Excellent test coverage for:

  • All major CSI command types and final bytes
  • OSC sequences with both BEL and ST terminators
  • Emoji with regional indicators, skin tones, ZWJ sequences
  • Variation selector behavior (VS15 for text, VS16 for emoji presentation)

Comment thread src/string/immutable/visible.zig
@oven-sh oven-sh deleted a comment from coderabbitai Bot Dec 10, 2025
@oven-sh oven-sh deleted a comment from coderabbitai Bot Dec 10, 2025
@oven-sh oven-sh deleted a comment from coderabbitai Bot Dec 10, 2025
@oven-sh oven-sh deleted a comment from coderabbitai Bot Dec 10, 2025
@dylan-conway

Copy link
Copy Markdown
Member

This seems to regress string width calculation of these strings:

describe("non-ASCII in escape sequences and Indic script handling", () => {
    test("OSC with non-ASCII (emoji) in URL should be invisible", () => {
      // Non-ASCII characters inside OSC sequence should NOT be counted
      // The emoji is part of the invisible hyperlink URL
      const result = Bun.stringWidth("a\x1b]8;;https://🎉\x07b");
      expect(result).toBe(2); // just "ab"
    });

    test("OSC with CJK in URL should be invisible", () => {
      // CJK character inside OSC sequence should NOT be counted
      const result = Bun.stringWidth("a\x1b]8;;https://中.com\x07b");
      expect(result).toBe(2); // just "ab"
    });

    test("Indic Avagraha (U+093D) should have width 1", () => {
      // U+093D (ऽ) is Devanagari Avagraha - a visible letter (category Lo)
      // The Indic heuristic incorrectly marks it as zero-width
      expect(Bun.stringWidth("\u093D")).toBe(1);
      expect(Bun.stringWidth("a\u093Db")).toBe(3);
    });

    test("Malayalam Sign Para (U+0D4F) should have width 1", () => {
      // U+0D4F (൏) is Malayalam Sign Para - a visible symbol (category So)
      // The Indic heuristic incorrectly marks it as zero-width
      expect(Bun.stringWidth("\u0D4F")).toBe(1);
    });

    test("Bengali Avagraha (U+09BD) should have width 1", () => {
      // U+09BD (ঽ) is Bengali Avagraha - a visible letter (category Lo)
      expect(Bun.stringWidth("\u09BD")).toBe(1);
    });

    test("Tamil Visarga (U+0B83) should have width 1", () => {
      // U+0B83 (ஃ) is Tamil Sign Visarga - a visible letter (category Lo)
      expect(Bun.stringWidth("\u0B83")).toBe(1);
    });
  });

Two fixes:
1. OSC sequence handling: Non-ASCII characters (emoji, CJK) inside OSC
   sequences were incorrectly counted towards visible width. Added
   escape sequence state checks in the non-ASCII character handling
   path to properly skip these characters.

2. Indic script heuristic: The zero-width detection for Indic scripts
   was too broad, incorrectly marking visible characters as zero-width:
   - Avagraha (position 0x3D in each block) is a visible letter (Lo)
   - Signs at position 0x03 (like Tamil Visarga) are visible
   - Malayalam Sign Para (0x0D4F) is a visible symbol (So)
   Refined the heuristic to exclude these positions.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants