Repository navigation
Conversation
write_log_data only treated a negative line as "no location" and wrote the column, length and line text for a location with line == 0, while the client reader (readBundlerMessageLocationOrNull) stops after a zero line. Messages created from plugin exceptions (bun_ast_jsc::msg_from_js) and native plugin logs carry line 0, so the overlay decoded the leftover bytes as an extra empty failure that the server could never remove, which also kept the error page from reloading once the real error was fixed.
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 3:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit a52a40a has 2 failures in
🧪 To try this PR locally: bunx bun-pr 37885That installs a local version of the PR into your bun-37885 --bun |
|
Status: reproduced and fixed, waiting on CI. Reproduced on 1.4.0 by decoding the payload of the dev server's 500 page with the client decoder ( |
There was a problem hiding this comment.
LGTM — one-character fix (< → <=) aligning the writer with the reader and the documented bun_ast::Location contract.
What was reviewed:
- Confirmed
readBundlerMessageLocationOrNullstops after reading a zerolineandbun_ast::Locationdocumentsline <= 0as "no position", so the writer was the mismatched side. - Checked that
line < 0(existing resolution errors) andline > 0paths are unchanged; only the previously-brokenline == 0case is affected. - Tests use the real client decoder via the existing
bake/*path alias and cover onLoad, onResolve, and the end-to-end reload behavior; harness conventions (devTest,await using,expectReload) match neighboring tests.
Extended reasoning...
Overview
The PR changes one comparison in src/runtime/bake/dev_server/serialized_failure.rs (loc.line < 0 → loc.line <= 0) so that a bundler message with line == 0 is serialized as "no location" (a lone zero) rather than as a full location record. It also updates a stale doc comment pointing at the reader file. Three new tests in test/bake/dev/plugins.test.ts cover onLoad throws, onResolve throws, and the end-to-end error-page-reloads-after-fix behavior.
Correctness
I verified the fix against both sides of the contract: readBundlerMessageLocationOrNull in src/runtime/bake/client/error-serialization.ts:69-70 reads a u32 and returns null immediately when it is zero, and bun_ast::Location at src/ast/lib.rs:636-637 documents "Line <= 0 means there is no line and column information". The writer was the only party that disagreed. The change is scoped to exactly the line == 0 case, which was previously producing a desynced byte stream; line < 0 and line > 0 behavior is unchanged. The PR description's rationale for fixing the writer rather than the producers (would change public BuildMessage.position, wouldn't cover Location::default() / native plugins, and the wire format can't express line 0 anyway) is sound.
Security risks
None. This is the dev-server error overlay wire format between Bun's own writer and its own HMR client, in development mode only. No untrusted input, auth, crypto, or external surface is involved.
Level of scrutiny
Low-to-medium. The runtime change is a single comparison operator in a self-contained serialization helper with a matching reader in the same codebase. It is not on a hot path, not memory-sensitive, and the failure mode it fixes was already user-visibly broken. No CODEOWNERS cover src/runtime/bake/ or test/bake/.
Other factors
The tests are well-constructed: they import the real decodeSerializedError / DataViewReader via the existing bake/* tsconfig path alias (already used by test/cli/inspect/BunFrontendDevServer.test.ts), so a future writer/reader mismatch fails here too. The two-failing-files onLoad test and the sort-by-file guard against ordering nondeterminism. The reload test uses await using, expectReload, and expectMessage from the bake harness rather than sleeps. The PR description states all three fail under USE_SYSTEM_BUN=1 and that BunFrontendDevServer.test.ts / bundle.test.ts (real locations, line: -1) still pass. No outstanding reviewer comments on the timeline.
Problem
onLoadoronResolvemakes the error overlay show an extra empty file group per failure; with notes or several messages per file the payload decodes as garbage.0, which the server never removes, so the error page does not reload once the real error is fixed.line == 0was written with column, length and line text, but the client stops reading after a zero line, so the leftover bytes decode as further failures.Location::default()and native plugin logs produce them too.Fix
line <= 0(previously onlyline < 0) as "no location" and writes a lone0.nullforline == 0andbun_ast::Locationdocumentsline <= 0as "no position", so the writer was the one side out of contract.at file.ts:0, and native plugin logs withline > 0butcolumn <= 0still break; tracked separately.USE_SYSTEM_BUN=1and pass with this change. Existing tests for real locations andline: -1still pass.Background
bun_ast::Locationlines are 1-based;line <= 0means no position. Plugin exceptions yieldline: 0, resolution errorsline: -1.Original description
What
In a Bake dev server (
bun ./index.html,Bun.serve({ development: true })), a bundler message whoseLocation.line == 0was serialized with more fields than the error overlay reads, so the overlay decoded the rest of the message as further failures.Plugin exceptions are the common way to get such a message:
bun_ast_jsc::msg_from_js(used foronLoad/onResolvethrows) builds aLocationwithline: 0, as doesLocation::default(), and thebun-native-plugincrate fillsBunLogOptionswithline: 0for every log.Repro: a plugin that throws from
onLoadoronResolvewhile serving an HTML route or a framework route.Decoding the payload embedded in the 500 page (or sent as
BunFrontendDevServer.bundleFailed/ over the HMR socket) with the real client decoder yields, on 1.4.0:User visible effects:
0, which the server never removes, sobuildErrorsnever empties and the error page does not reload after the error is fixed. The new "error page reloads once a plugin exception is fixed" test fails on 1.4.0 withlocation.reload() was not calledeven though the server loggedReloaded in 2ms: index.ts.Why
write_log_datainserialized_failure.rsshort-circuited only online < 0.readBundlerMessageLocationOrNullinclient/error-serialization.tsreturnsnullforline == 0after reading just the line, andbun_ast::Locationdocumentsline <= 0as "no line and column information" (it is 1-based). The writer is the side that disagrees with both the reader and the documented contract, so it now writes a lone0for anyline <= 0. Fixing the producers instead (e.g. makingmsg_from_jsuse-1) would change the publicBuildMessage.positionofBun.buildand would still leaveLocation::default()and native plugin logs broken, and the wire format has no way to express a line of 0 anyway.Left alone on purpose: the terminal printer prints
at file.ts:0for these messages (cosmetic, unrelated to the wire format), and native plugin logs withline > 0butcolumn <= 0still panic the writer / throw in the overlay; that is a different defect and is tracked separately.Tests
test/bake/dev/plugins.test.ts:exceptions thrown from onLoad are reported without a location: two failing files, decodes the error page payload withdecodeSerializedErrorfrom the client and asserts exactly those two failures and nothing else in the payload;exception thrown from onResolve is reported without a location: same for the resolve path (the failure belongs to the importer);error page reloads once a plugin exception is fixed:[serve.static]plugin with an HTML route, loads the error page in the harness client, fixes the import and asserts the page reloads.All three fail with
USE_SYSTEM_BUN=1(the first two decode an extra{ file: null, messages: [] }per failure, the third times out waiting for the reload) and pass withbun bd test.test/cli/inspect/BunFrontendDevServer.test.tsandtest/bake/dev/bundle.test.ts(messages with real locations, and resolution errors withline: -1) still pass.