fix(console): use single quotes for strings containing double quotes #25259
Conversation
WalkthroughUpdate console string printing to route UTF‑16 strings through the JSON/surrogate-aware path and use a new JSPrinter quoted-string helper for non-UTF16 (UTF‑8 or Latin‑1) strings; add a public Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (6)src/**/*.{cpp,zig}📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Files:
src/**/*.zig📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Files:
**/*.zig📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
Files:
src/**/*.{ts,zig,cpp}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/**/*.{ts,zig}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/js_*.zig📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Files:
🧠 Learnings (21)📓 Common learnings📚 Learning: 2025-09-12T18:16:50.754ZApplied to files:
📚 Learning: 2025-11-24T18:35:39.205ZApplied to files:
📚 Learning: 2025-10-01T21:59:54.571ZApplied to files:
📚 Learning: 2025-11-24T18:36:33.069ZApplied to files:
📚 Learning: 2025-09-25T22:07:13.851ZApplied to files:
📚 Learning: 2025-11-24T18:36:08.558ZApplied to files:
📚 Learning: 2025-10-08T13:56:00.875ZApplied to files:
📚 Learning: 2025-10-18T20:50:47.750ZApplied to files:
📚 Learning: 2025-11-24T18:35:25.883ZApplied to files:
📚 Learning: 2025-11-24T18:36:08.558ZApplied to files:
📚 Learning: 2025-10-24T10:43:09.398ZApplied to files:
📚 Learning: 2025-11-24T18:35:39.205ZApplied to files:
📚 Learning: 2025-11-24T18:35:39.205ZApplied to files:
📚 Learning: 2025-11-10T00:57:09.173ZApplied to files:
📚 Learning: 2025-11-24T18:36:08.558ZApplied to files:
📚 Learning: 2025-11-03T20:40:59.655ZApplied to files:
📚 Learning: 2025-09-05T19:49:26.188ZApplied to files:
📚 Learning: 2025-10-21T20:57:01.892ZApplied to files:
📚 Learning: 2025-11-03T20:43:06.996ZApplied to files:
📚 Learning: 2025-09-02T18:27:23.279ZApplied to files:
🔇 Additional comments (3)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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.
📒 Files selected for processing (3)
src/bun.js/ConsoleObject.zig(2 hunks)src/js_printer.zig(1 hunks)test/regression/issue/25234.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
src/**/*.{cpp,zig}
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
src/**/*.{cpp,zig}: Usebun bdorbun run build:debugto build debug versions for C++ and Zig source files; creates debug build at./build/debug/bun-debug
Run tests usingbun bd test <test-file>with the debug build; never usebun testdirectly as it will not include your changes
Execute files usingbun bd <file> <...args>; never usebun <file>directly as it will not include your changes
Enable debug logs for specific scopes usingBUN_DEBUG_$(SCOPE)=1environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required
Files:
src/js_printer.zigsrc/bun.js/ConsoleObject.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Use
bun.Output.scoped(.${SCOPE}, .hidden)for creating debug logs in Zig codeImplement core functionality in Zig, typically in its own directory in
src/
src/**/*.zig: Zig source code should be organized insrc/*.zigfor core runtime, JavaScript bindings, and package manager
In Zig code, be careful with allocators and use defer for cleanup to manage memory properly
UseBUN_DEBUG_QUIET_LOGS=1to disable debug logging, orBUN_DEBUG_<scopeName>=1to enable specific Output.scoped calls in debug builds
Runbun run zig:check-allto compile the Zig code on all platforms when making platform-specific changes
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@importat the bottom of the file (auto formatter will move them automatically)
Files:
src/js_printer.zigsrc/bun.js/ConsoleObject.zig
**/js_*.zig
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
**/js_*.zig: ImplementJSYourFeaturestruct in a file likejs_your_feature.zigto create JavaScript bindings for Zig functionality
Usebun.JSError!JSValuefor proper error propagation in JavaScript bindings
Implement proper memory management with reference counting usingref()/deref()in JavaScript bindings
Always implement proper cleanup indeinit()andfinalize()methods for JavaScript binding classes
Files:
src/js_printer.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
**/*.zig: Expose generated bindings in Zig structs usingpub const js = JSC.Codegen.JS<ClassName>with trait conversion methods:toJS,fromJS, andfromJSDirect
Use consistent parameter nameglobalObjectinstead ofctxin Zig constructor and method implementations
Usebun.JSError!JSValuereturn type for Zig methods and constructors to enable proper error handling and exception propagation
Implement resource cleanup usingdeinit()method that releases resources, followed byfinalize()called by the GC that invokesdeinit()and frees the pointer
UseJSC.markBinding(@src())in finalize methods for debugging purposes before callingdeinit()
For methods returning cached properties in Zig, declare external C++ functions usingextern fnandcallconv(JSC.conv)calling convention
Implement getter functions with naming patternget<PropertyName>in Zig that acceptthisandglobalObjectparameters and returnJSC.JSValue
Access JavaScript CallFrame arguments usingcallFrame.argument(i), check argument count withcallFrame.argumentCount(), and getthiswithcallFrame.thisValue()
For reference-counted objects, use.deref()in finalize instead ofdestroy()to release references to other JS objects
Files:
src/js_printer.zigsrc/bun.js/ConsoleObject.zig
src/**/*.{ts,zig,cpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use absolute paths in file operations
Files:
src/js_printer.zigsrc/bun.js/ConsoleObject.zig
src/**/*.{ts,zig}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid shell commands in code - don't use
findorgrep; use Bun's Glob and built-in tools instead
Files:
src/js_printer.zigsrc/bun.js/ConsoleObject.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 frombun:test
Usetest.eachand data-driven tests to reduce boilerplate when testing multiple similar cases
Files:
test/regression/issue/25234.test.ts
test/regression/issue/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
When a test is for a specific numbered GitHub Issue, place it in
test/regression/issue/${issueNumber}.test.tswith a REAL issue number, not a placeholder
Files:
test/regression/issue/25234.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: For single-file tests, prefer using-eflag overtempDir
For multi-file tests, prefer usingtempDirfromharnessandBun.spawnover other temporary directory creation methods
Always useport: 0for network tests and do not hardcode ports or use custom random port number functions
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for no 'panic' or 'uncaught exception' or similar in the test output - that is NOT a valid test
UsetempDirfromharnessto create temporary directories, do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, check stdout/stderr expectations BEFORE checking exit code to get more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests, insteadawaitthe condition to be met as you are testing the CONDITION not the TIME PASSING
Verify your test fails withUSE_SYSTEM_BUN=1 bun test <file>and passes withbun bd test <file>- your test is NOT VALID if it passes withUSE_SYSTEM_BUN=1
Files:
test/regression/issue/25234.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testwith 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 useport: 0to get a random port
Prefer concurrent tests over sequential tests usingtest.concurrentordescribe.concurrentwhen multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, usebunExeandbunEnvfromharnessto ensure the same build of Bun is used and debug logging is silenced
Use-eflag for single-file tests when spawning Bun processes
UsetempDir()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, usePromise.withResolversto create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
UseBuffer.alloc(count, fill).toString()instead of'A'.repeat(count)to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Usedescribeblocks for grouping related tests
Always useawait usingorusingto 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
Usedescribe.each()for parameterized tests
UsetoMatchSnapshot()for snapshot testing
UsebeforeAll(),afterEach(),beforeEach()for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup inafterEach()
Files:
test/regression/issue/25234.test.ts
test/regression/issue/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
Regression tests for specific issues go in
/test/regression/issue/${issueNumber}.test.ts. Do not put tests without issue numbers in the regression directory
Files:
test/regression/issue/25234.test.ts
🧠 Learnings (24)
📓 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: 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.
📚 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/regression/issue/25234.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/regression/issue/25234.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/regression/issue/25234.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/regression/issue/25234.test.ts
📚 Learning: 2025-11-24T18:36:33.069Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:33.069Z
Learning: Applies to test/js/bun/**/*.test.{ts,tsx} : For Bun-specific API tests, use the `test/js/bun/` directory (for http, crypto, ffi, shell, etc.)
Applied to files:
test/regression/issue/25234.test.ts
📚 Learning: 2025-11-24T18:36:33.069Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:33.069Z
Learning: Applies to **/*.test.{ts,tsx} : Verify your test fails with `USE_SYSTEM_BUN=1 bun test <file>` and passes with `bun bd test <file>` - your test is NOT VALID if it passes with `USE_SYSTEM_BUN=1`
Applied to files:
test/regression/issue/25234.test.ts
📚 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:
test/regression/issue/25234.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/**/*.test.ts : Assert console messages using `c.expectMessage()` with single or multiple arguments; any unasserted logs fail the test to catch unexpected re-evaluations or reloads
Applied to files:
test/regression/issue/25234.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} : Use `-e` flag for single-file tests when spawning Bun processes
Applied to files:
test/regression/issue/25234.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:
test/regression/issue/25234.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:
test/regression/issue/25234.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} : 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
Applied to files:
test/regression/issue/25234.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/regression/issue/25234.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:
src/bun.js/ConsoleObject.zig
📚 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:
src/bun.js/ConsoleObject.zig
📚 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: Applies to src/bun.js/api/BunObject.zig : Implement getter functions in `src/bun.js/api/BunObject.zig` that return your feature, and export them in the `exportAll()` function
Applied to files:
src/bun.js/ConsoleObject.zig
📚 Learning: 2025-11-24T18:36:08.558Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-11-24T18:36:08.558Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Include new class bindings in `src/bun.js/bindings/generated_classes_list.zig` to register them with the code generator
Applied to files:
src/bun.js/ConsoleObject.zig
📚 Learning: 2025-11-24T18:36:33.069Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:33.069Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : When implementing JavaScript classes in C++, define properties using HashTableValue arrays and add iso subspaces for classes with C++ fields, caching structures in ZigGlobalObject
Applied to files:
src/bun.js/ConsoleObject.zig
📚 Learning: 2025-11-24T18:35:25.883Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-11-24T18:35:25.883Z
Learning: Applies to *.zig : In Zig, declare external C++ functions and wrap them in public methods using the convention `extern fn Bun__ClassName__toJS(...)` and `pub fn toJS(...)`
Applied to files:
src/bun.js/ConsoleObject.zig
📚 Learning: 2025-10-17T20:50:58.644Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23755
File: src/bun.js/api/bun/socket/Handlers.zig:154-159
Timestamp: 2025-10-17T20:50:58.644Z
Learning: In Bun socket configuration error messages (src/bun.js/api/bun/socket/Handlers.zig), use the user-facing JavaScript names "data" and "drain" instead of internal field names "onData" and "onWritable", as these are the names users see in the API according to SocketConfig.bindv2.ts.
Applied to files:
src/bun.js/ConsoleObject.zig
📚 Learning: 2025-11-24T18:36:08.558Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-11-24T18:36:08.558Z
Learning: Applies to **/*.zig : Expose generated bindings in Zig structs using `pub const js = JSC.Codegen.JS<ClassName>` with trait conversion methods: `toJS`, `fromJS`, and `fromJSDirect`
Applied to files:
src/bun.js/ConsoleObject.zig
📚 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: Applies to **/js_*.zig : Use `bun.JSError!JSValue` for proper error propagation in JavaScript bindings
Applied to files:
src/bun.js/ConsoleObject.zig
📚 Learning: 2025-11-11T22:55:04.070Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24571
File: src/css/selectors/parser.zig:908-916
Timestamp: 2025-11-11T22:55:04.070Z
Learning: In oven-sh/bun, CSS serialization uses an arena allocator. In src/css/selectors/parser.zig, functions like PseudoClass.toCss and PseudoElement.toCss intentionally do not call deinit on std.Io.Writer.Allocating, scratch buffers, or css.Printer because dest.allocator is an arena and these temporaries are reclaimed when the CSS pass completes. Only debug-only paths (e.g., DeclarationBlock.DebugFmt in src/css/declaration.zig) may explicitly deinit.
Applied to files:
src/bun.js/ConsoleObject.zig
🧬 Code graph analysis (1)
test/regression/issue/25234.test.ts (1)
test/harness.ts (1)
bunExe(102-105)
🔇 Additional comments (3)
test/regression/issue/25234.test.ts (1)
24-52: LGTM! Well-structured tests with specific assertions.Both tests follow best practices:
- Proper resource cleanup with
await using- Check stdout/stderr before exitCode for better error messages
- Use specific assertions that validate exact expected output
- Correctly use
bunExe()andbunEnvfrom harnesssrc/bun.js/ConsoleObject.zig (1)
2153-2180: LGTM! Correct implementation of intelligent quote selection.The changes properly replace the generic JSON printing path with dedicated quote-selection functions for both UTF-16 and Latin-1 string paths. The implementation:
- Maintains consistency between the two StringObject branches
- Uses
catch unreachableconsistently with the existing code style in this file- Correctly calls the new
JSPrinter.writeQuotedStringUTF16andJSPrinter.writeQuotedStringfunctions- Aligns with the PR objective of using single quotes when strings contain double quotes
src/js_printer.zig (1)
356-404: Console quote selection helpers correctly implement the intended behaviorThe new
bestQuoteCharForConsole/bestQuoteCharForConsoleUTF16andwriteQuotedString/writeQuotedStringUTF16helpers are wired cleanly on top ofwritePreQuotedString, and the rule:
- default to
";- switch to
'only when the string contains"but no'matches the PR objective and avoids unnecessary escaping for the JSON-string case while leaving JSON-printing and bundler string-literal behavior unchanged. UTF‑16 handling mirrors the existing UTF‑16 paths and should be safe as long as callers pass aligned UTF‑16 buffers, consistent with the rest of this file.
When printing strings in console output, use single quotes if the string
contains double quotes but no single quotes. This avoids ugly escaped
output like `"{\"test\":\"value\"}"` and instead shows `'{"test":"value"}'`.
Closes #25234
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
robobun
force-pushed
the
claude/fix-console-string-quoting-25234
branch
from
7c69af4 to
1158d18
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/regression/issue/25234.test.ts (1)
8-22: Consider more specific assertions for better test coverage.The assertions on lines 19-20 only verify the presence of any single quote and absence of escaped double quotes, but don't confirm the exact expected output format. This could lead to false positives if single quotes appear elsewhere in the output.
As noted in the previous review, consider asserting the exact expected output:
expect(stdout.trim()).toBe(`{ test: '{"test":{"pretty":"pretty"}}' }`);
📜 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.
📒 Files selected for processing (3)
src/bun.js/ConsoleObject.zig(2 hunks)src/js_printer.zig(1 hunks)test/regression/issue/25234.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
src/**/*.{cpp,zig}
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
src/**/*.{cpp,zig}: Usebun bdorbun run build:debugto build debug versions for C++ and Zig source files; creates debug build at./build/debug/bun-debug
Run tests usingbun bd test <test-file>with the debug build; never usebun testdirectly as it will not include your changes
Execute files usingbun bd <file> <...args>; never usebun <file>directly as it will not include your changes
Enable debug logs for specific scopes usingBUN_DEBUG_$(SCOPE)=1environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required
Files:
src/bun.js/ConsoleObject.zigsrc/js_printer.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Use
bun.Output.scoped(.${SCOPE}, .hidden)for creating debug logs in Zig codeImplement core functionality in Zig, typically in its own directory in
src/
src/**/*.zig: Zig source code should be organized insrc/*.zigfor core runtime, JavaScript bindings, and package manager
In Zig code, be careful with allocators and use defer for cleanup to manage memory properly
UseBUN_DEBUG_QUIET_LOGS=1to disable debug logging, orBUN_DEBUG_<scopeName>=1to enable specific Output.scoped calls in debug builds
Runbun run zig:check-allto compile the Zig code on all platforms when making platform-specific changes
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@importat the bottom of the file (auto formatter will move them automatically)
Files:
src/bun.js/ConsoleObject.zigsrc/js_printer.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
**/*.zig: Expose generated bindings in Zig structs usingpub const js = JSC.Codegen.JS<ClassName>with trait conversion methods:toJS,fromJS, andfromJSDirect
Use consistent parameter nameglobalObjectinstead ofctxin Zig constructor and method implementations
Usebun.JSError!JSValuereturn type for Zig methods and constructors to enable proper error handling and exception propagation
Implement resource cleanup usingdeinit()method that releases resources, followed byfinalize()called by the GC that invokesdeinit()and frees the pointer
UseJSC.markBinding(@src())in finalize methods for debugging purposes before callingdeinit()
For methods returning cached properties in Zig, declare external C++ functions usingextern fnandcallconv(JSC.conv)calling convention
Implement getter functions with naming patternget<PropertyName>in Zig that acceptthisandglobalObjectparameters and returnJSC.JSValue
Access JavaScript CallFrame arguments usingcallFrame.argument(i), check argument count withcallFrame.argumentCount(), and getthiswithcallFrame.thisValue()
For reference-counted objects, use.deref()in finalize instead ofdestroy()to release references to other JS objects
Files:
src/bun.js/ConsoleObject.zigsrc/js_printer.zig
src/**/*.{ts,zig,cpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use absolute paths in file operations
Files:
src/bun.js/ConsoleObject.zigsrc/js_printer.zig
src/**/*.{ts,zig}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid shell commands in code - don't use
findorgrep; use Bun's Glob and built-in tools instead
Files:
src/bun.js/ConsoleObject.zigsrc/js_printer.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 frombun:test
Usetest.eachand data-driven tests to reduce boilerplate when testing multiple similar cases
Files:
test/regression/issue/25234.test.ts
test/regression/issue/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
When a test is for a specific numbered GitHub Issue, place it in
test/regression/issue/${issueNumber}.test.tswith a REAL issue number, not a placeholder
Files:
test/regression/issue/25234.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: For single-file tests, prefer using-eflag overtempDir
For multi-file tests, prefer usingtempDirfromharnessandBun.spawnover other temporary directory creation methods
Always useport: 0for network tests and do not hardcode ports or use custom random port number functions
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for no 'panic' or 'uncaught exception' or similar in the test output - that is NOT a valid test
UsetempDirfromharnessto create temporary directories, do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, check stdout/stderr expectations BEFORE checking exit code to get more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests, insteadawaitthe condition to be met as you are testing the CONDITION not the TIME PASSING
Verify your test fails withUSE_SYSTEM_BUN=1 bun test <file>and passes withbun bd test <file>- your test is NOT VALID if it passes withUSE_SYSTEM_BUN=1
Files:
test/regression/issue/25234.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testwith 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 useport: 0to get a random port
Prefer concurrent tests over sequential tests usingtest.concurrentordescribe.concurrentwhen multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, usebunExeandbunEnvfromharnessto ensure the same build of Bun is used and debug logging is silenced
Use-eflag for single-file tests when spawning Bun processes
UsetempDir()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, usePromise.withResolversto create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
UseBuffer.alloc(count, fill).toString()instead of'A'.repeat(count)to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Usedescribeblocks for grouping related tests
Always useawait usingorusingto 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
Usedescribe.each()for parameterized tests
UsetoMatchSnapshot()for snapshot testing
UsebeforeAll(),afterEach(),beforeEach()for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup inafterEach()
Files:
test/regression/issue/25234.test.ts
test/regression/issue/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
Regression tests for specific issues go in
/test/regression/issue/${issueNumber}.test.ts. Do not put tests without issue numbers in the regression directory
Files:
test/regression/issue/25234.test.ts
**/js_*.zig
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
**/js_*.zig: ImplementJSYourFeaturestruct in a file likejs_your_feature.zigto create JavaScript bindings for Zig functionality
Usebun.JSError!JSValuefor proper error propagation in JavaScript bindings
Implement proper memory management with reference counting usingref()/deref()in JavaScript bindings
Always implement proper cleanup indeinit()andfinalize()methods for JavaScript binding classes
Files:
src/js_printer.zig
🧠 Learnings (27)
📓 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.
📚 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: Applies to **/js_*.zig : Use `bun.JSError!JSValue` for proper error propagation in JavaScript bindings
Applied to files:
src/bun.js/ConsoleObject.zig
📚 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:
src/bun.js/ConsoleObject.zig
📚 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: Applies to src/bun.js/api/BunObject.zig : Implement getter functions in `src/bun.js/api/BunObject.zig` that return your feature, and export them in the `exportAll()` function
Applied to files:
src/bun.js/ConsoleObject.zig
📚 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:
src/bun.js/ConsoleObject.zig
📚 Learning: 2025-11-24T18:36:08.558Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-11-24T18:36:08.558Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Include new class bindings in `src/bun.js/bindings/generated_classes_list.zig` to register them with the code generator
Applied to files:
src/bun.js/ConsoleObject.zig
📚 Learning: 2025-11-24T18:36:08.558Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-11-24T18:36:08.558Z
Learning: Applies to **/*.zig : Use `bun.JSError!JSValue` return type for Zig methods and constructors to enable proper error handling and exception propagation
Applied to files:
src/bun.js/ConsoleObject.zig
📚 Learning: 2025-11-24T18:36:08.558Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-11-24T18:36:08.558Z
Learning: Applies to **/*.zig : Expose generated bindings in Zig structs using `pub const js = JSC.Codegen.JS<ClassName>` with trait conversion methods: `toJS`, `fromJS`, and `fromJSDirect`
Applied to files:
src/bun.js/ConsoleObject.zigsrc/js_printer.zig
📚 Learning: 2025-11-24T18:35:25.883Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-11-24T18:35:25.883Z
Learning: Applies to *.zig : In Zig, declare external C++ functions and wrap them in public methods using the convention `extern fn Bun__ClassName__toJS(...)` and `pub fn toJS(...)`
Applied to files:
src/bun.js/ConsoleObject.zigsrc/js_printer.zig
📚 Learning: 2025-11-03T20:40:59.655Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/bindings/JSValue.zig:545-586
Timestamp: 2025-11-03T20:40:59.655Z
Learning: In Bun's Zig codebase, JSErrors (returned as `bun.JSError!T`) must always be properly handled. Using `catch continue` or `catch { break; }` to silently suppress JSErrors is a bug. Errors should either be explicitly handled or propagated with `try`. This applies to snapshot serializer error handling where Jest's behavior is to throw when serializers throw.
Applied to files:
src/bun.js/ConsoleObject.zig
📚 Learning: 2025-11-24T18:36:33.069Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:33.069Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : When implementing JavaScript classes in C++, define properties using HashTableValue arrays and add iso subspaces for classes with C++ fields, caching structures in ZigGlobalObject
Applied to files:
src/bun.js/ConsoleObject.zig
📚 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/regression/issue/25234.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/regression/issue/25234.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/regression/issue/25234.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/regression/issue/25234.test.ts
📚 Learning: 2025-11-24T18:36:33.069Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:33.069Z
Learning: Applies to test/js/bun/**/*.test.{ts,tsx} : For Bun-specific API tests, use the `test/js/bun/` directory (for http, crypto, ffi, shell, etc.)
Applied to files:
test/regression/issue/25234.test.ts
📚 Learning: 2025-11-24T18:36:33.069Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:33.069Z
Learning: Applies to **/*.test.{ts,tsx} : Verify your test fails with `USE_SYSTEM_BUN=1 bun test <file>` and passes with `bun bd test <file>` - your test is NOT VALID if it passes with `USE_SYSTEM_BUN=1`
Applied to files:
test/regression/issue/25234.test.ts
📚 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:
test/regression/issue/25234.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/**/*.test.ts : Assert console messages using `c.expectMessage()` with single or multiple arguments; any unasserted logs fail the test to catch unexpected re-evaluations or reloads
Applied to files:
test/regression/issue/25234.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} : Use `-e` flag for single-file tests when spawning Bun processes
Applied to files:
test/regression/issue/25234.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:
test/regression/issue/25234.test.ts
📚 Learning: 2025-11-24T18:36:33.069Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:33.069Z
Learning: Applies to **/*.test.{ts,tsx} : When spawning processes in tests, check stdout/stderr expectations BEFORE checking exit code to get more useful error messages on test failure
Applied to files:
test/regression/issue/25234.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} : Always check exit codes and test error scenarios in error tests
Applied to files:
test/regression/issue/25234.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:
test/regression/issue/25234.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} : 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
Applied to files:
test/regression/issue/25234.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/regression/issue/25234.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/regression/issue/25234.test.ts
🧬 Code graph analysis (1)
test/regression/issue/25234.test.ts (1)
test/harness.ts (1)
bunExe(102-105)
🔇 Additional comments (3)
test/regression/issue/25234.test.ts (2)
24-37: LGTM!This test correctly validates that strings containing single quotes use double quotes, with a precise assertion that checks for the exact expected format.
39-52: LGTM!This test appropriately validates the default quoting behavior with a specific assertion for the expected output format.
src/js_printer.zig (1)
356-367: Console quote selection logic matches the regression goalThe logic cleanly encodes the intended behavior: default to double quotes, and switch to single quotes only when the string contains at least one
"and no', with a dedicated UTF‑16 path delegating tobestQuoteCharForConsoleUTF16. This is minimal, easy to reason about, and directly addresses the{ test: "{\"...\"}" }→{ test: '{"..."}' }regression case without affecting JSON printing paths.
…ring-quoting-25234
robobun
force-pushed
the
claude/fix-console-string-quoting-25234
branch
from
5af331d to
149ace4
Compare
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
It should do a single loop over all the input using @vector to count the number of single quotes and double quotes and then it should choose the cheapest one. There is already similar logic in js_printer and ideally it would reuse that.
it should not be testing with snapshots because snapshots do not go thorugh ConsoleObject.zig they go through the Jest pretty printer.
Address review feedback: - Remove custom bestQuoteCharForConsole function and reuse existing bestQuoteCharForString which counts quotes in a single SIMD pass - Update tests to use toContain assertions instead of snapshots (snapshots go through Jest pretty printer, not ConsoleObject.zig) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
Thanks for the review @Jarred-Sumner! I've addressed your feedback:
|
|
I thin kwe have to preserve the encoding argument instead of hardcoding it to latin1? |
Address review feedback: - Add encoding parameter to writeQuotedString instead of hardcoding latin1 - Detect UTF-8 vs Latin-1 strings in ConsoleObject and pass correct encoding - Supports proper handling of non-ASCII characters in both encodings 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
Thanks @Jarred-Sumner! Fixed in d859f23 - |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/bun.js/ConsoleObject.zig (1)
2177-2188: Same encoding detection pattern - consider the same clarity improvement.This code segment has the same correct encoding detection logic as lines 2159-2163, just for the
StringObjectcase. The same optional refactor suggestion applies here for consistency.Apply the same pattern:
if (str.isUTF16()) { - // Use JSON path for UTF-16 to properly handle surrogate pairs (emojis) try this.printAs(.JSON, Writer, writer_, value, .StringObject, enable_ansi_colors); -} else if (str.isUTF8()) { - JSPrinter.writeQuotedString(str.latin1(), Writer, writer_, .utf8) catch unreachable; +} else if (str.isUTF8()) { + JSPrinter.writeQuotedString(str.asUTF8() orelse unreachable, Writer, writer_, .utf8) catch unreachable; } else { JSPrinter.writeQuotedString(str.latin1(), Writer, writer_, .latin1) catch unreachable; }
📜 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.
📒 Files selected for processing (2)
src/bun.js/ConsoleObject.zig(2 hunks)src/js_printer.zig(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.{cpp,zig}
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
src/**/*.{cpp,zig}: Usebun bdorbun run build:debugto build debug versions for C++ and Zig source files; creates debug build at./build/debug/bun-debug
Run tests usingbun bd test <test-file>with the debug build; never usebun testdirectly as it will not include your changes
Execute files usingbun bd <file> <...args>; never usebun <file>directly as it will not include your changes
Enable debug logs for specific scopes usingBUN_DEBUG_$(SCOPE)=1environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required
Files:
src/js_printer.zigsrc/bun.js/ConsoleObject.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Use
bun.Output.scoped(.${SCOPE}, .hidden)for creating debug logs in Zig codeImplement core functionality in Zig, typically in its own directory in
src/
src/**/*.zig: Zig source code should be organized insrc/*.zigfor core runtime, JavaScript bindings, and package manager
In Zig code, be careful with allocators and use defer for cleanup to manage memory properly
UseBUN_DEBUG_QUIET_LOGS=1to disable debug logging, orBUN_DEBUG_<scopeName>=1to enable specific Output.scoped calls in debug builds
Runbun run zig:check-allto compile the Zig code on all platforms when making platform-specific changes
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@importat the bottom of the file (auto formatter will move them automatically)
Files:
src/js_printer.zigsrc/bun.js/ConsoleObject.zig
**/js_*.zig
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
**/js_*.zig: ImplementJSYourFeaturestruct in a file likejs_your_feature.zigto create JavaScript bindings for Zig functionality
Usebun.JSError!JSValuefor proper error propagation in JavaScript bindings
Implement proper memory management with reference counting usingref()/deref()in JavaScript bindings
Always implement proper cleanup indeinit()andfinalize()methods for JavaScript binding classes
Files:
src/js_printer.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
**/*.zig: Expose generated bindings in Zig structs usingpub const js = JSC.Codegen.JS<ClassName>with trait conversion methods:toJS,fromJS, andfromJSDirect
Use consistent parameter nameglobalObjectinstead ofctxin Zig constructor and method implementations
Usebun.JSError!JSValuereturn type for Zig methods and constructors to enable proper error handling and exception propagation
Implement resource cleanup usingdeinit()method that releases resources, followed byfinalize()called by the GC that invokesdeinit()and frees the pointer
UseJSC.markBinding(@src())in finalize methods for debugging purposes before callingdeinit()
For methods returning cached properties in Zig, declare external C++ functions usingextern fnandcallconv(JSC.conv)calling convention
Implement getter functions with naming patternget<PropertyName>in Zig that acceptthisandglobalObjectparameters and returnJSC.JSValue
Access JavaScript CallFrame arguments usingcallFrame.argument(i), check argument count withcallFrame.argumentCount(), and getthiswithcallFrame.thisValue()
For reference-counted objects, use.deref()in finalize instead ofdestroy()to release references to other JS objects
Files:
src/js_printer.zigsrc/bun.js/ConsoleObject.zig
src/**/*.{ts,zig,cpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use absolute paths in file operations
Files:
src/js_printer.zigsrc/bun.js/ConsoleObject.zig
src/**/*.{ts,zig}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid shell commands in code - don't use
findorgrep; use Bun's Glob and built-in tools instead
Files:
src/js_printer.zigsrc/bun.js/ConsoleObject.zig
🧠 Learnings (19)
📓 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: 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
📚 Learning: 2025-11-24T18:36:08.558Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-11-24T18:36:08.558Z
Learning: Applies to **/*.zig : Expose generated bindings in Zig structs using `pub const js = JSC.Codegen.JS<ClassName>` with trait conversion methods: `toJS`, `fromJS`, and `fromJSDirect`
Applied to files:
src/js_printer.zigsrc/bun.js/ConsoleObject.zig
📚 Learning: 2025-11-24T18:35:25.883Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-11-24T18:35:25.883Z
Learning: Applies to *.zig : In Zig, declare external C++ functions and wrap them in public methods using the convention `extern fn Bun__ClassName__toJS(...)` and `pub fn toJS(...)`
Applied to files:
src/js_printer.zigsrc/bun.js/ConsoleObject.zig
📚 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: Applies to src/bun.js/api/BunObject.zig : Implement getter functions in `src/bun.js/api/BunObject.zig` that return your feature, and export them in the `exportAll()` function
Applied to files:
src/bun.js/ConsoleObject.zig
📚 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:
src/bun.js/ConsoleObject.zig
📚 Learning: 2025-11-24T18:36:33.069Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:33.069Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : When implementing JavaScript classes in C++, define properties using HashTableValue arrays and add iso subspaces for classes with C++ fields, caching structures in ZigGlobalObject
Applied to files:
src/bun.js/ConsoleObject.zig
📚 Learning: 2025-11-24T18:36:08.558Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-11-24T18:36:08.558Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Include new class bindings in `src/bun.js/bindings/generated_classes_list.zig` to register them with the code generator
Applied to files:
src/bun.js/ConsoleObject.zig
📚 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:
src/bun.js/ConsoleObject.zig
📚 Learning: 2025-09-25T22:07:13.851Z
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.
Applied to files:
src/bun.js/ConsoleObject.zig
📚 Learning: 2025-10-18T20:50:47.750Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:366-373
Timestamp: 2025-10-18T20:50:47.750Z
Learning: In Bun's Zig codebase (src/bun.js/bindings/JSValue.zig), the JSValue enum uses `.null` (not `.js_null`) for JavaScript's null value. Only `js_undefined` has the `js_` prefix to avoid collision with Zig's built-in `undefined` keyword. The correct enum fields are: `js_undefined`, `null`, `true`, `false`, and `zero`.
Applied to files:
src/bun.js/ConsoleObject.zig
📚 Learning: 2025-10-11T15:19:30.301Z
Learnt from: mastermakrela
Repo: oven-sh/bun PR: 19167
File: src/bun.js/api.zig:31-31
Timestamp: 2025-10-11T15:19:30.301Z
Learning: The learnings about `pub const js = JSC.Codegen.JS<ClassName>` and re-exporting toJS/fromJS/fromJSDirect apply to class-based Zig bindings with constructors and prototype methods (e.g., those with `new` constructors). They do NOT apply to simple namespace-style API objects like TOMLObject, YAMLObject, and CSVObject which expose static functions via a `create()` method that returns a plain JS object.
Applied to files:
src/bun.js/ConsoleObject.zig
📚 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/bun.js/ConsoleObject.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/bun.js/ConsoleObject.zig
📚 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: Applies to **/js_*.zig : Use `bun.JSError!JSValue` for proper error propagation in JavaScript bindings
Applied to files:
src/bun.js/ConsoleObject.zig
📚 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: Applies to **/*js_bindings.classes.ts : Use `JSC.Codegen` correctly to generate necessary binding code for JavaScript-Zig integration
Applied to files:
src/bun.js/ConsoleObject.zig
📚 Learning: 2025-11-24T18:36:08.558Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-11-24T18:36:08.558Z
Learning: Applies to **/*.zig : Use `bun.JSError!JSValue` return type for Zig methods and constructors to enable proper error handling and exception propagation
Applied to files:
src/bun.js/ConsoleObject.zig
📚 Learning: 2025-09-05T19:49:26.188Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun’s Zig code, `.js_undefined` is a valid and preferred JSValue literal for “undefined” (e.g., resolving JSPromise). Do not refactor usages to `jsc.JSValue.jsUndefined()`, especially in src/valkey/js_valkey_functions.zig unsubscribe().
Applied to files:
src/bun.js/ConsoleObject.zig
📚 Learning: 2025-11-03T20:40:59.655Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/bindings/JSValue.zig:545-586
Timestamp: 2025-11-03T20:40:59.655Z
Learning: In Bun's Zig codebase, JSErrors (returned as `bun.JSError!T`) must always be properly handled. Using `catch continue` or `catch { break; }` to silently suppress JSErrors is a bug. Errors should either be explicitly handled or propagated with `try`. This applies to snapshot serializer error handling where Jest's behavior is to throw when serializers throw.
Applied to files:
src/bun.js/ConsoleObject.zig
📚 Learning: 2025-10-21T20:57:01.892Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23889
File: src/string.zig:751-752
Timestamp: 2025-10-21T20:57:01.892Z
Learning: In Zig code handling bun.String: calling deref() on an empty bun.String (String.empty or after being set to .empty) is well-defined behavior and safely does nothing (no-op). While such calls may be redundant and could be removed for code clarity, their presence is not a correctness issue.
Applied to files:
src/bun.js/ConsoleObject.zig
Address CodeRabbit review feedback: - Use str.asUTF8() instead of str.latin1() for UTF-8 strings to use the semantically correct accessor - Add comptime guard to writeQuotedString to reject unsupported encodings (only latin1 and utf8 are supported; use JSON path for UTF-16) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Summary
Before:
After:
Closes #25234
Test plan
test/regression/issue/25234.test.tsbun bd testand fails withUSE_SYSTEM_BUN=1🤖 Generated with Claude Code