Repository navigation
Ensure we add sourcemappings for S.Comment - #23871
Conversation
|
Updated 12:49 AM PT - Oct 21st, 2025
✅ @Jarred-Sumner, your commit d7de9cb6f34fc06fe8a34e7a7e639783696c672a passed in 🧪 To try this PR locally: bunx bun-pr 23871That installs a local version of the PR into your bun-23871 --bun |
|
Caution Review failedThe pull request is closed. WalkthroughPrints indentation and adds a sourcemap entry before emitting comments; fixes CRLF handling when reindenting multi-line comments and strips trailing Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
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: CodeRabbit UI
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 (1)
src/js_printer.zig(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/js_printer.zig
src/**/js_*.zig
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
src/**/js_*.zig: Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Always implement proper cleanup in deinit() and finalize() for JS-exposed types
Files:
src/js_printer.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/js_printer.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")In Zig code, be careful with allocators and use defer for cleanup
Files:
src/js_printer.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
| p.printIndent(); | ||
| p.addSourceMapping(stmt.loc); | ||
| p.printIndentedComment(s.text); |
There was a problem hiding this comment.
Double indentation bug: remove the explicit printIndent() call.
The printIndentedComment function (lines 5146-5164) already handles indentation internally—it calls p.printIndent() for each line of the comment. Adding an explicit p.printIndent() here causes double indentation on the first line.
The source mapping addition on line 3689 is correct and should be kept.
Apply this diff:
.s_comment => |s| {
- p.printIndent();
p.addSourceMapping(stmt.loc);
p.printIndentedComment(s.text);
},Note: If you intended to change the indentation behavior of printIndentedComment, that function should be modified instead, but based on the PR title focusing on source mappings, this appears to be an unintended side effect.
📝 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.
| p.printIndent(); | |
| p.addSourceMapping(stmt.loc); | |
| p.printIndentedComment(s.text); | |
| p.addSourceMapping(stmt.loc); | |
| p.printIndentedComment(s.text); |
🤖 Prompt for AI Agents
In src/js_printer.zig around lines 3688 to 3690, there's an extra call to
p.printIndent() before adding the source mapping and calling
p.printIndentedComment, causing the first line of the comment to be indented
twice; remove the explicit p.printIndent() call on line 3688 and keep
p.addSourceMapping(stmt.loc) and the p.printIndentedComment(s.text) call
unchanged so indentation is handled only inside printIndentedComment.
pfgithub
left a comment
pfgithub
left a comment
There was a problem hiding this comment.
Confirmed this fixes the issue. I haven't found a minimal test case to reproduce with, but the issue is caused by crlf in a license comment that gets preserved in bundling
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
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 (1)
test/bundler/bundler_comments.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/bundler/bundler_comments.test.ts
test/bundler/**/*
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place bundler/transpiler/CSS/bun build tests under test/bundler/
Files:
test/bundler/bundler_comments.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/bundler/bundler_comments.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:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/bundler/bundler_comments.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/bundler/bundler_comments.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be created under test/ and end with .test.ts or .test.tsx
In tests that open network ports, always use port: 0; do not hardcode port numbers or roll your own random port
Use normalizeBunSnapshot to normalize snapshot output instead of comparing raw stdout
Do not write tests that merely assert absence of strings like "panic" or "uncaught exception"
Use tempDir from "harness" to create temporary directories; do not use fs.mkdtempSync or tmpdirSync
When spawning processes in tests, assert output before asserting the exit code
Avoid shell commands in tests (e.g., find, grep); prefer Bun's Glob and built-in tools
Prefer snapshot tests over direct string equality assertions on stdout
Files:
test/bundler/bundler_comments.test.ts
test/bundler/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Bundler/transpiler tests must use the itBundled helper
Files:
test/bundler/bundler_comments.test.ts
🧠 Learnings (5)
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/sourcemap.test.ts : sourcemap.test.ts should verify source map correctness in dev
Applied to files:
test/bundler/bundler_comments.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/bundle.test.ts : bundle.test.ts should contain DevServer-specific bundling tests
Applied to files:
test/bundler/bundler_comments.test.ts
📚 Learning: 2025-10-20T04:22:55.565Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-20T04:22:55.565Z
Learning: Applies to test/bundler/**/*.test.{ts,tsx} : Bundler/transpiler tests must use the itBundled helper
Applied to files:
test/bundler/bundler_comments.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/bundler/bundler_comments.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Organize tests with `describe` blocks to group related tests
Applied to files:
test/bundler/bundler_comments.test.ts
🧬 Code graph analysis (1)
test/bundler/bundler_comments.test.ts (1)
test/bundler/expectBundled.ts (1)
itBundled(1734-1768)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (3)
test/bundler/bundler_comments.test.ts (3)
1-2: LGTM! Imports support the new sourcemap validation test.The additions of
expectandSourceMapare necessary for the new multi-line comment sourcemap test.
356-393: Good test for sourcemap entries on multi-line comments, but consider additional test coverage.The test correctly verifies that multi-line comments generate sourcemap entries, which aligns with the PR objective. The use of
SourceMapfromnode:modulefor validation is appropriate.However, consider adding test coverage for:
- Multi-line comments with LF-only line endings (not just CRLF)
- Multi-line comments at different positions (beginning, middle, end of file)
- Non-legal multi-line comments (without
!)
369-369: Potential issue: splitting on\nwhen input uses\r\n.The test input uses CRLF line endings (
\r\n) but the output is split on\nonly. This will leave\rcharacters at the end of each line inoutputLines, which may still work for theincludes()check but could cause issues if the bundler normalizes line endings.Consider using a more robust line splitting approach:
- const outputLines = output.split("\n"); + const outputLines = output.split(/\r?\n/);This regex pattern handles both CRLF and LF line endings correctly.
| expect(typeof entry.originalLine).toBe("number"); | ||
| expect(entry.originalLine).toBeGreaterThanOrEqual(0); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Type consistency: use non-null assertion operator.
Line 388 uses entry!.originalSource! with non-null assertions, but line 389 uses entry.originalLine without the assertion operator. While this may work at runtime due to the check on line 384, it's inconsistent.
Apply this diff for consistency:
- expect(typeof entry.originalLine).toBe("number");
- expect(entry.originalLine).toBeGreaterThanOrEqual(0);
+ expect(typeof entry!.originalLine).toBe("number");
+ expect(entry!.originalLine).toBeGreaterThanOrEqual(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.
| expect(typeof entry.originalLine).toBe("number"); | |
| expect(entry.originalLine).toBeGreaterThanOrEqual(0); | |
| expect(typeof entry!.originalLine).toBe("number"); | |
| expect(entry!.originalLine).toBeGreaterThanOrEqual(0); |
🤖 Prompt for AI Agents
In test/bundler/bundler_comments.test.ts around lines 384 to 390, the code uses
non-null assertions for entry!.originalSource! but not for entry.originalLine;
make this consistent by applying non-null assertions to the originalLine checks
(e.g., use entry!.originalLine! in the expect calls) so both type-nullability
assumptions match and TypeScript won't complain.
3 participants
What does this PR do?
How did you verify your code works?