Print the separator between JSX props from what was printed, not the property index

[robobun]

Problem

• toMatchSnapshot / toMatchInlineSnapshot and the Expected/Received lines of toEqual, toStrictEqual, toMatchObject, toHaveBeenCalledWith, ... print every JSX element that has two or more props without the space before the last prop: <x a="1"b="2" />. With six or more props the line break also lands one prop early. Bun.inspect prints the same element correctly, so console output and test output disagree on the same value.
• Cause (bun:test formatter only): src/runtime/test_runner/pretty_format.rs, Tag::JSX arm. The Zig version compared the iterator's 0-based index; the Rust port (Rewrite Bun in Rust #30412) replaced it with a local counter that is incremented before use (so it is 1-based) and kept the 0-based comparisons (iter_i + 1 < count_without_children, iter_i > 3), so the separator after the second to last prop is never written.
• Both formatters (Bun.inspect / console.log in src/jsc/ConsoleObject.rs print_jsx, and the arm above) derive the separator from the property slot index and props.length - 1. Any slot the loop skips throws that arithmetic off:
  • children listed before other props (a spread such as <x {...props} a="1" b="2" />, or jsx("x", { children, ...rest })): <x a="1"b="2">c</x>
  • children equal to undefined (<x a="1" b="2">{cond && ...}</x> with cond undefined): <x a="1" b="2" /> (two spaces)
  • children first plus six props: the line break moves one prop earlier and the last space is still dropped
  • the same happens in compact mode (Bun.inspect(el, { compact: true }), console.table / Bun.inspect.table cells), which shares the loop: a table cell for the children-first element reads <x a="1"b="2" />

Fix

• Both loops now write the separator immediately before each prop they are about to print, and pick newline vs space from the number of props printed so far. A separator is only ever written between two props that were actually printed, so nothing the loop skips (children in any position, children: undefined, hidden values, empty names) can desynchronize it.
• The layout is unchanged on purpose: for an element whose children is last (what createElement and the JSX runtime produce), the old "newline after slot index > 3" and the new "newline before the sixth printed prop" are the same rule, so Bun.inspect output is byte for byte identical for every element it printed correctly before, and bun:test goes back to what it printed before the port. The existing JSX tests in inspect.test.js pass unchanged. Compact mode (single_line) keeps its rule too: always a space, never a line break; it only gains the missing separators.
• The always-true needs_space flag and JSPropertyIterator::i are removed: the separator logic was their only reader (i is pub(crate) and dead_code is denied in the workspace).
• Verified:
  • test/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts ("jsx element props are separated in snapshots and diffs"): 1 to 7 props, key, children last / first / undefined, a nested element, and a toEqual failure message. Fails on the unfixed build at the 2-prop case; all 11 assertions pass with the fix (per-assertion list below).
  • test/js/bun/util/inspect.test.js ("jsx props are separated no matter where children sits in props"): children first via spread, with key, {undefined} child, the 6-prop layout with children last and first, and compact mode (6 props stay on one line; {undefined} child prints <x a="1" b="2" />, two spaces before the fix). Fails on the unfixed build at the first assertion, passes with the fix.
  • bun bd test on both files plus test/js/bun/test/snapshot-tests, test/js/bun/test/printing, test/js/web/console/console-log.test.ts, test/cli/run/run-eval.test.ts: only failures are pre-existing and unrelated (error snapshots needs colors enabled, diffexample prints a seconds suffix on the slow debug build, inspect-error gets an extra require frame on debug builds).
• Note: bun:test: wrap multi-line JSX snapshots in line breaks like every other value #38947 (open) wraps multi-line top-level JSX snapshots in line breaks. Whichever of the two lands second needs to adjust the three multi-line inline snapshots in the new test; the source changes do not overlap.

Background

• Bun has two native value printers. src/jsc/ConsoleObject.rs backs Bun.inspect / console.log (and matchers such as toBe); src/runtime/test_runner/pretty_format.rs is a jest-pretty-format flavoured copy used for snapshots and for the Expected/Received side of the diffing matchers. Both have a near identical arm that prints a React element ($$typeof === Symbol.for("react.element")) as JSX: <type key=... prop=value ...> followed by either children or />.
• Props are walked with JSPropertyIterator, which yields the object's own property names in order. children is not printed as a prop (it becomes the element body), and values whose JSC cell type is internal (is_hidden()) are skipped too, so the number of props printed is not props.length - 1, and the slot index of a printed prop is not its position among the printed props. The old code assumed both.

Per-assertion status of the new bun:test cases (unfixed canary vs this branch)

=== unfixed ===
PASS a
FAIL a,b                       Received: <x a="1"b="2" />
FAIL a,b,c                     Received: <x a="1" b="2"c="3" />
FAIL key,a,b                   Received: <x key="k" a="1"b="2" />
FAIL a,b + child               Received: <x a="1"b="2">c</x>
FAIL children,a,b              Received: <x a="1"b="2">c</x>
PASS a,b,children:undefined    (the two off-by-ones cancel out here; Bun.inspect prints two spaces, covered in inspect.test.js)
FAIL 7 props                   Received: <x a="1" b="2" c="3" d="4"\n  e="5"\n  f="6"g="7" />
FAIL children + 6 props        Received: <x a="1" b="2" c="3"\n  d="4"e="5"f="6">c</x>
FAIL nested                    Received: <x a="1">\n  <y p="1"q="2" />\n</x>
FAIL toEqual diff              Expected: <x a="1"b="3" /> / Received: <x a="1"b="2" />
=== fixed ===
all 11 PASS

Bun.inspect on the unfixed build, elements built with real React 18 JSX:

<x {...{ children: "c" }} a="1" b="2" />                 -> <x a="1"b="2">c</x>
<x a="1" b="2">{undefined}</x>                            -> <x a="1" b="2"  />
<x a="1" b="2" {...{ "": "z" }} />                        -> <x a="1" b="2"  />
<x {...{ children: "c" }} a="1" ... f="6" />              -> <x a="1" b="2" c="3" d="4"\n  e="5"f="6">c</x>
<x a="1" ... f="6" />                                     -> <x a="1" b="2" c="3" d="4" e="5"\n  f="6" />   (already correct, unchanged)
{ compact: true }:
<x {...{ children: "c" }} a="1" ... f="6" />              -> <x a="1" b="2" c="3" d="4" e="5"f="6" />
<x a="1" b="2">{undefined}</x>                            -> <x a="1" b="2"  />
Bun.inspect.table([{ el: <x {...{ children: "c" }} a="1" b="2" /> }])  cell: <x a="1"b="2" />

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 2 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e87f7e35-d320-49d4-a715-d3f0fe442b0b

📥 Commits

Reviewing files that changed from the base of the PR and between 9fb606f and 903e93b.

📒 Files selected for processing (7)

• src/jsc/ConsoleObject.rs
• src/jsc/JSPropertyIterator.rs
• src/runtime/api/JSBundler.rs
• src/runtime/api/JSTranspiler.rs
• src/runtime/test_runner/pretty_format.rs
• test/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts
• test/js/bun/util/inspect.test.js

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Status: fix is in this PR, ready for review.

Reproduced on the current canary (bun v1.4.0-canary.1) with expect(createElement("x", { a: "1", b: "2" })).toMatchInlineSnapshot(), which receives <x a="1"b="2" />, and with Bun.inspect(<x {...{ children: "c" }} a="1" b="2" />), which prints <x a="1"b="2">c</x>. Both new tests fail on the unfixed build and pass with this branch (bun bd test test/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts test/js/bun/util/inspect.test.js).

CI: every lane that ran is green for the files this PR touches (the red entries on the latest build are unrelated tests that passed alone or on retry). The macOS 14 aarch64 test lane has not run because no agent picked the job up (it expired once and the retry is queued); the change is platform independent and the same tests passed on the Linux and Windows lanes.

[robobun]

^{Updated 12:05 PM PT - Aug 15th, 2026}

❌ @robobun, your commit 903e93b has some failures in Build #98143 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 38957

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

bun-38957 --bun

[claude]

LGTM — the separator-before-prop rewrite is a clean simplification of both JSX printers, and the earlier ANSI-color issue in the new snapshot test has been addressed with Bun.stripANSI. The one inline nit (two stale comments referencing the removed .i field) is cosmetic.

Extended reasoning...

Overview

This PR fixes JSX prop separator printing in both native value formatters — src/jsc/ConsoleObject.rs (Bun.inspect/console.log) and src/runtime/test_runner/pretty_format.rs (snapshots and diff-matcher output). The old code derived the separator from the property-slot index and props.length - 1, which broke when the loop skipped a slot (children first, children: undefined, hidden values) and, in the pretty-format copy, was additionally off-by-one from the Zig→Rust port. The new code writes the separator immediately before each prop it is about to print and picks newline vs space from a printed_props counter, so skipped slots cannot desynchronize it. The always-true needs_space flag and the now-unread JSPropertyIterator::i field are removed. Two new tests cover 1–7 props, key, children in every position, nested elements, and the toEqual diff message.

Security risks

None. This is purely output-formatting logic for developer-facing inspect/snapshot text; no parsing of untrusted input, no allocation-size arithmetic, no auth/crypto/permissions surface.

Level of scrutiny

Low-to-moderate. The change is localised to two near-identical print loops plus a dead-field removal on a pub(crate) struct. I verified: (1) the iter.i reads at ConsoleObject.rs:5440 and pretty_format.rs:2321 are on the unrelated PropertyIteratorCtx local struct, not JSPropertyIterator, so the field removal compiles cleanly (and dead_code is denied, so any missed reader would fail the build); (2) needs_space was assigned true on both branches of the type if/else, so collapsing if needs_space { " key=" } else { "key=" } to the first arm is behaviour-preserving; (3) the pretty-format copy correctly omits the !self.single_line guard because that formatter has no single_line field — matching the code it replaces.

Other factors

• My previous review flagged the toEqual diff assertion failing under FORCE_COLOR=1; commit 9365f4d addressed it by wrapping the caught message in Bun.stripANSI and the author verified with FORCE_COLOR=1 locally. The comment-cop bot's paragraph-comment warnings were also addressed (shortened to one line each).
• The one remaining finding is a nit: two comments in JSTranspiler.rs and JSBundler.rs still reference define_iter.i / loader_iter.i. Pure documentation staleness with no runtime effect — fine to fold into this PR or clean up separately.
• The PR description flags a snapshot-format interaction with open PR #38947; whichever lands second adjusts three inline snapshots. Not a blocker here.

[claude]

LGTM — all prior feedback addressed (ANSI-stripping in the diff assertion, the two .i comment rewrites, and the comment-cop one-liners in ce831e5).

What was reviewed:

• Traced the new printed_props-before-prop separator against the old props_i/count_without_children post-prop logic in both print_jsx and the Tag::JSX arm — byte-identical for the previously-correct children-last case, and now robust to skipped slots.
• Confirmed needs_space was unconditionally true on every path, so its removal and the " key=" collapse are dead-branch elimination.
• JSPropertyIterator::i was pub(crate) with the two formatters as its only code readers; the JSBundler/JSTranspiler comment references are now reworded to len-as-capacity-hint.

Extended reasoning...

Overview

The PR fixes an off-by-one in the bun:test pretty-formatter's JSX arm (introduced in the Rust port #30412) that dropped the separator before the last prop, and a related brittleness in both formatters (ConsoleObject.rs print_jsx and pretty_format.rs) where the separator was derived from the property slot index rather than the count of props actually printed — so any skipped slot (children first, children: undefined, hidden values) desynchronized it. The fix moves the separator to be emitted before each printed prop, keyed on a printed_props counter, which makes skipped slots irrelevant. Collateral cleanup: the always-true needs_space flag is removed, JSPropertyIterator::i (whose only code readers were these two loops) is removed, and the two comments in JSBundler.rs/JSTranspiler.rs that named .i are reworded. Two new tests cover 1–7 props, key, children in every position, undefined children, nested elements, and the toEqual diff path.

Security risks

None. This is output-formatting logic for debug/test output; no parsing of untrusted input, no auth/crypto/permissions surface.

Level of scrutiny

Medium. The change touches two near-identical hot-ish formatting loops in native code, but the transformation is small (~15 lines each), the layout rule is preserved unchanged (5 props on the tag's line, verified by walking both index arithmetics for the children-last case), and the test matrix is thorough. The JSPropertyIterator::i removal is pub(crate)-scoped with dead_code denied in the workspace, so a missed reader would fail to compile.

Other factors

All three rounds of prior feedback have been addressed in follow-up commits: my ANSI-color CI-failure finding (fixed via Bun.stripANSI in 9365f4d), my stale-.i-comment finding (reworded in 5b7355c), and the comment-cop length flags on the reworded comments (collapsed to one line in ce831e5 — the current diff shows single-line comments, so the two unresolved comment-cop threads at 09:13 are stale). The PR description is unusually thorough, including a per-assertion pass/fail table on the unfixed build and a heads-up about a test-fixture-only interaction with open PR #38947. The bug-hunting system found nothing on this revision.

[claude]

LGTM — both prior findings (ANSI stripping in the toEqual assertion, stale .i comments in JSBundler/JSTranspiler) are addressed, and the comment-cop notes on those two files fired on the intermediate commit before ce831e5 collapsed them to one line.

Extended reasoning...

Overview

Fixes JSX prop separator placement in both value formatters (Bun.inspect in src/jsc/ConsoleObject.rs and the bun:test pretty-format in src/runtime/test_runner/pretty_format.rs). The old code wrote the separator after each prop using the property-slot index and len - 1, which desynchronizes when the loop skips a slot (children first, children: undefined, hidden values) and was also off-by-one in pretty_format.rs since the Rust port. The new code writes the separator before each prop from a local printed_props counter, so skipped slots cannot affect it. The always-true needs_space flag and the now-unread JSPropertyIterator::i field are removed; the two comments that named .i in JSBundler.rs/JSTranspiler.rs are collapsed to a one-line capacity-hint note.

Security risks

None. This is display-only formatting of already-held JS values; no new user-input parsing, no allocation-size arithmetic, no FFI surface changes.

Level of scrutiny

Medium — a hot, twice-duplicated formatter arm, but the change is a strict simplification (leading separator vs. trailing lookahead). I hand-traced the 6- and 7-prop cases against the old props_i + 1 < count_without_children && props_i > 3 rule and the new printed_props >= 5 rule; they produce identical output when children is last (the only case the old code handled correctly), matching the PR description's byte-for-byte claim. The !self.single_line guard is preserved in ConsoleObject.rs (compact mode covered by the new test) and correctly absent from pretty_format.rs, which has no such field. needs_space was assigned true on both branches with no path to false before first read, so its removal is dead-code cleanup. JSPropertyIterator::i was pub(crate) and its only reader is deleted here; iter_i still drives the loop.

Other factors

Both prior review findings are resolved: the toEqual message assertion now goes through Bun.stripANSI (verified Bun.stripANSI exists), and the .i-referencing comments are reworded. The two unresolved comment-cop inline notes on JSBundler.rs/JSTranspiler.rs fired at 09:13 on the intermediate 5b7355c state; ce831e5 subsequently collapsed both to a single line, so they are stale rather than outstanding. Test coverage is thorough — both formatters, 1–7 props, key, children last/first/undefined, nested elements, compact mode, and the diff-message path — and the PR body documents per-assertion fail/pass on the unfixed build.