fix(bake): add --no-bundle-page-log to silence "Bundled page in" output#27306
Open
TomasHubelbauer wants to merge 1 commit intooven-sh:mainfrom
Open
fix(bake): add --no-bundle-page-log to silence "Bundled page in" output#27306TomasHubelbauer wants to merge 1 commit intooven-sh:mainfrom
TomasHubelbauer wants to merge 1 commit intooven-sh:mainfrom
Conversation
Contributor
WalkthroughImplements a Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bake/DevServer.zig`:
- Around line 2866-2925: The debug output path can emit text (via
printMemoryLine) even when should_print_bundle_line is false, but the trailing
newline and Output.flush() are only executed inside the should_print_bundle_line
branch; move the flush (and the trailing newline Output.prettyError("\n", .{}))
out of that conditional so they run unconditionally after the if block — update
the code around should_print_bundle_line/Output.prettyError(...) and ensure
Output.flush() is invoked after that whole if so printMemoryLine() output is
always flushed.
In `@src/env_loader.zig`:
- Around line 353-354: Update the doc comments to reflect that the CLI flag
takes precedence over the environment variable: for hasSetNoBundlePageLog and
its twin hasSetNoClearTerminalOnReload, change the description to say they
return whether the option is set (checking the CLI flag
has_no_bundle_page_log_cli_flag / has_no_clear_terminal_on_reload_cli_flag
first, and if not set, falling back to the BUN_CONFIG_* env var), and mention
that the CLI flag wins over the env var to make behavior accurate.
In `@test/js/bun/http/bun-serve-no-bundle-page-log.test.ts`:
- Around line 45-51: Split the existing combined test into two separate tests:
one named like "default logging emits bundle line" that only runs runFixture([])
and asserts the output contains "Bundled page in", and a second named like
"--no-bundle-page-log suppresses bundle line" that runs
runFixture(["--no-bundle-page-log"]) and asserts the output does not contain
"Bundled page in". Additionally add two more tests: one that runs with the flag
and verifies the reload message ("Reloaded in") is still present to cover the
had_reload_event bypass in DevServer.zig, and another that sets the
BUN_CONFIG_NO_BUNDLE_PAGE_LOG environment variable path (instead of the CLI
flag) and asserts suppression behaves the same as the flag; use runFixture
invocations accordingly to exercise these paths.
Comment on lines
2866
to
2925
| const ms_elapsed = @divFloor(current_bundle.timer.read(), std.time.ns_per_ms); | ||
|
|
||
| Output.prettyError("<green>{s} in {d}ms<r>", .{ | ||
| if (current_bundle.had_reload_event) | ||
| "Reloaded" | ||
| else | ||
| "Bundled page", | ||
| ms_elapsed, | ||
| }); | ||
|
|
||
| // Intentionally creating a new scope here so we can limit the lifetime | ||
| // of the `relative_path_buf` | ||
| { | ||
| const relative_path_buf = bun.path_buffer_pool.get(); | ||
| defer bun.path_buffer_pool.put(relative_path_buf); | ||
|
|
||
| // Compute a file name to display | ||
| const file_name: ?[]const u8 = if (current_bundle.had_reload_event) | ||
| if (bv2.graph.entry_points.items.len > 0) | ||
| dev.relativePath( | ||
| relative_path_buf, | ||
| bv2.graph.input_files.items(.source)[bv2.graph.entry_points.items[0].get()].path.text, | ||
| ) | ||
| const should_print_bundle_line = current_bundle.had_reload_event or | ||
| !dev.vm.transpiler.env.hasSetNoBundlePageLog(false); | ||
| if (should_print_bundle_line) { | ||
| Output.prettyError("<green>{s} in {d}ms<r>", .{ | ||
| if (current_bundle.had_reload_event) | ||
| "Reloaded" | ||
| else | ||
| null // TODO: How does this happen | ||
| else brk: { | ||
| const route_bundle_index = route_bundle_index: { | ||
| if (current_bundle.requests.first != null) break :route_bundle_index current_bundle.requests.first.?.data.route_bundle_index; | ||
| const route_bundle_indices = current_bundle.promise.route_bundle_indices.keys(); | ||
| if (route_bundle_indices.len == 0) break :brk null; | ||
| break :route_bundle_index route_bundle_indices[0]; | ||
| }; | ||
| "Bundled page", | ||
| ms_elapsed, | ||
| }); | ||
|
|
||
| break :brk switch (dev.routeBundlePtr(route_bundle_index).data) { | ||
| .html => |html| dev.relativePath(relative_path_buf, html.html_bundle.data.bundle.data.path), | ||
| .framework => |fw| file_name: { | ||
| const route = dev.router.routePtr(fw.route_index); | ||
| const opaque_id = route.file_page.unwrap() orelse | ||
| route.file_layout.unwrap() orelse | ||
| break :file_name null; | ||
| const server_index = fromOpaqueFileId(.server, opaque_id); | ||
| const abs_path = dev.server_graph.bundled_files.keys()[server_index.get()]; | ||
| break :file_name dev.relativePath(relative_path_buf, abs_path); | ||
| }, | ||
| // Intentionally creating a new scope here so we can limit the lifetime | ||
| // of the `relative_path_buf` | ||
| { | ||
| const relative_path_buf = bun.path_buffer_pool.get(); | ||
| defer bun.path_buffer_pool.put(relative_path_buf); | ||
|
|
||
| // Compute a file name to display | ||
| const file_name: ?[]const u8 = if (current_bundle.had_reload_event) | ||
| if (bv2.graph.entry_points.items.len > 0) | ||
| dev.relativePath( | ||
| relative_path_buf, | ||
| bv2.graph.input_files.items(.source)[bv2.graph.entry_points.items[0].get()].path.text, | ||
| ) | ||
| else | ||
| null // TODO: How does this happen | ||
| else brk: { | ||
| const route_bundle_index = route_bundle_index: { | ||
| if (current_bundle.requests.first != null) break :route_bundle_index current_bundle.requests.first.?.data.route_bundle_index; | ||
| const route_bundle_indices = current_bundle.promise.route_bundle_indices.keys(); | ||
| if (route_bundle_indices.len == 0) break :brk null; | ||
| break :route_bundle_index route_bundle_indices[0]; | ||
| }; | ||
|
|
||
| break :brk switch (dev.routeBundlePtr(route_bundle_index).data) { | ||
| .html => |html| dev.relativePath(relative_path_buf, html.html_bundle.data.bundle.data.path), | ||
| .framework => |fw| file_name: { | ||
| const route = dev.router.routePtr(fw.route_index); | ||
| const opaque_id = route.file_page.unwrap() orelse | ||
| route.file_layout.unwrap() orelse | ||
| break :file_name null; | ||
| const server_index = fromOpaqueFileId(.server, opaque_id); | ||
| const abs_path = dev.server_graph.bundled_files.keys()[server_index.get()]; | ||
| break :file_name dev.relativePath(relative_path_buf, abs_path); | ||
| }, | ||
| }; | ||
| }; | ||
| }; | ||
|
|
||
| const total_count = bv2.graph.entry_points.items.len; | ||
| if (file_name) |name| { | ||
| Output.prettyError("<d>:<r> {s}", .{name}); | ||
| if (total_count > 1) { | ||
| Output.prettyError(" <d>+ {d} more<r>", .{total_count - 1}); | ||
| const total_count = bv2.graph.entry_points.items.len; | ||
| if (file_name) |name| { | ||
| Output.prettyError("<d>:<r> {s}", .{name}); | ||
| if (total_count > 1) { | ||
| Output.prettyError(" <d>+ {d} more<r>", .{total_count - 1}); | ||
| } | ||
| } | ||
| } | ||
| Output.prettyError("\n", .{}); | ||
| Output.flush(); | ||
| } |
Contributor
There was a problem hiding this comment.
Output.flush() is elided in the suppression path, leaving printMemoryLine() output unflushed in debug builds.
When should_print_bundle_line is false (initial bundle + flag set), printMemoryLine() at line 2863 may emit output (in builds where AllocationScope.enabled && debug.isVisible()), but neither the trailing newline nor Output.flush() are called. Move the flush outside the conditional to cover this case:
🔧 Proposed fix
if (should_print_bundle_line) {
Output.prettyError("<green>{s} in {d}ms<r>", .{ ... });
// ... path derivation and printing ...
Output.prettyError("\n", .{});
- Output.flush();
}
+ Output.flush();This is harmless in production (where printMemoryLine is a compile-time no-op) and ensures the debug output path is properly flushed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const ms_elapsed = @divFloor(current_bundle.timer.read(), std.time.ns_per_ms); | |
| Output.prettyError("<green>{s} in {d}ms<r>", .{ | |
| if (current_bundle.had_reload_event) | |
| "Reloaded" | |
| else | |
| "Bundled page", | |
| ms_elapsed, | |
| }); | |
| // Intentionally creating a new scope here so we can limit the lifetime | |
| // of the `relative_path_buf` | |
| { | |
| const relative_path_buf = bun.path_buffer_pool.get(); | |
| defer bun.path_buffer_pool.put(relative_path_buf); | |
| // Compute a file name to display | |
| const file_name: ?[]const u8 = if (current_bundle.had_reload_event) | |
| if (bv2.graph.entry_points.items.len > 0) | |
| dev.relativePath( | |
| relative_path_buf, | |
| bv2.graph.input_files.items(.source)[bv2.graph.entry_points.items[0].get()].path.text, | |
| ) | |
| const should_print_bundle_line = current_bundle.had_reload_event or | |
| !dev.vm.transpiler.env.hasSetNoBundlePageLog(false); | |
| if (should_print_bundle_line) { | |
| Output.prettyError("<green>{s} in {d}ms<r>", .{ | |
| if (current_bundle.had_reload_event) | |
| "Reloaded" | |
| else | |
| null // TODO: How does this happen | |
| else brk: { | |
| const route_bundle_index = route_bundle_index: { | |
| if (current_bundle.requests.first != null) break :route_bundle_index current_bundle.requests.first.?.data.route_bundle_index; | |
| const route_bundle_indices = current_bundle.promise.route_bundle_indices.keys(); | |
| if (route_bundle_indices.len == 0) break :brk null; | |
| break :route_bundle_index route_bundle_indices[0]; | |
| }; | |
| "Bundled page", | |
| ms_elapsed, | |
| }); | |
| break :brk switch (dev.routeBundlePtr(route_bundle_index).data) { | |
| .html => |html| dev.relativePath(relative_path_buf, html.html_bundle.data.bundle.data.path), | |
| .framework => |fw| file_name: { | |
| const route = dev.router.routePtr(fw.route_index); | |
| const opaque_id = route.file_page.unwrap() orelse | |
| route.file_layout.unwrap() orelse | |
| break :file_name null; | |
| const server_index = fromOpaqueFileId(.server, opaque_id); | |
| const abs_path = dev.server_graph.bundled_files.keys()[server_index.get()]; | |
| break :file_name dev.relativePath(relative_path_buf, abs_path); | |
| }, | |
| // Intentionally creating a new scope here so we can limit the lifetime | |
| // of the `relative_path_buf` | |
| { | |
| const relative_path_buf = bun.path_buffer_pool.get(); | |
| defer bun.path_buffer_pool.put(relative_path_buf); | |
| // Compute a file name to display | |
| const file_name: ?[]const u8 = if (current_bundle.had_reload_event) | |
| if (bv2.graph.entry_points.items.len > 0) | |
| dev.relativePath( | |
| relative_path_buf, | |
| bv2.graph.input_files.items(.source)[bv2.graph.entry_points.items[0].get()].path.text, | |
| ) | |
| else | |
| null // TODO: How does this happen | |
| else brk: { | |
| const route_bundle_index = route_bundle_index: { | |
| if (current_bundle.requests.first != null) break :route_bundle_index current_bundle.requests.first.?.data.route_bundle_index; | |
| const route_bundle_indices = current_bundle.promise.route_bundle_indices.keys(); | |
| if (route_bundle_indices.len == 0) break :brk null; | |
| break :route_bundle_index route_bundle_indices[0]; | |
| }; | |
| break :brk switch (dev.routeBundlePtr(route_bundle_index).data) { | |
| .html => |html| dev.relativePath(relative_path_buf, html.html_bundle.data.bundle.data.path), | |
| .framework => |fw| file_name: { | |
| const route = dev.router.routePtr(fw.route_index); | |
| const opaque_id = route.file_page.unwrap() orelse | |
| route.file_layout.unwrap() orelse | |
| break :file_name null; | |
| const server_index = fromOpaqueFileId(.server, opaque_id); | |
| const abs_path = dev.server_graph.bundled_files.keys()[server_index.get()]; | |
| break :file_name dev.relativePath(relative_path_buf, abs_path); | |
| }, | |
| }; | |
| }; | |
| }; | |
| const total_count = bv2.graph.entry_points.items.len; | |
| if (file_name) |name| { | |
| Output.prettyError("<d>:<r> {s}", .{name}); | |
| if (total_count > 1) { | |
| Output.prettyError(" <d>+ {d} more<r>", .{total_count - 1}); | |
| const total_count = bv2.graph.entry_points.items.len; | |
| if (file_name) |name| { | |
| Output.prettyError("<d>:<r> {s}", .{name}); | |
| if (total_count > 1) { | |
| Output.prettyError(" <d>+ {d} more<r>", .{total_count - 1}); | |
| } | |
| } | |
| } | |
| Output.prettyError("\n", .{}); | |
| Output.flush(); | |
| } | |
| const ms_elapsed = `@divFloor`(current_bundle.timer.read(), std.time.ns_per_ms); | |
| const should_print_bundle_line = current_bundle.had_reload_event or | |
| !dev.vm.transpiler.env.hasSetNoBundlePageLog(false); | |
| if (should_print_bundle_line) { | |
| Output.prettyError("<green>{s} in {d}ms<r>", .{ | |
| if (current_bundle.had_reload_event) | |
| "Reloaded" | |
| else | |
| "Bundled page", | |
| ms_elapsed, | |
| }); | |
| // Intentionally creating a new scope here so we can limit the lifetime | |
| // of the `relative_path_buf` | |
| { | |
| const relative_path_buf = bun.path_buffer_pool.get(); | |
| defer bun.path_buffer_pool.put(relative_path_buf); | |
| // Compute a file name to display | |
| const file_name: ?[]const u8 = if (current_bundle.had_reload_event) | |
| if (bv2.graph.entry_points.items.len > 0) | |
| dev.relativePath( | |
| relative_path_buf, | |
| bv2.graph.input_files.items(.source)[bv2.graph.entry_points.items[0].get()].path.text, | |
| ) | |
| else | |
| null // TODO: How does this happen | |
| else brk: { | |
| const route_bundle_index = route_bundle_index: { | |
| if (current_bundle.requests.first != null) break :route_bundle_index current_bundle.requests.first.?.data.route_bundle_index; | |
| const route_bundle_indices = current_bundle.promise.route_bundle_indices.keys(); | |
| if (route_bundle_indices.len == 0) break :brk null; | |
| break :route_bundle_index route_bundle_indices[0]; | |
| }; | |
| break :brk switch (dev.routeBundlePtr(route_bundle_index).data) { | |
| .html => |html| dev.relativePath(relative_path_buf, html.html_bundle.data.bundle.data.path), | |
| .framework => |fw| file_name: { | |
| const route = dev.router.routePtr(fw.route_index); | |
| const opaque_id = route.file_page.unwrap() orelse | |
| route.file_layout.unwrap() orelse | |
| break :file_name null; | |
| const server_index = fromOpaqueFileId(.server, opaque_id); | |
| const abs_path = dev.server_graph.bundled_files.keys()[server_index.get()]; | |
| break :file_name dev.relativePath(relative_path_buf, abs_path); | |
| }, | |
| }; | |
| }; | |
| const total_count = bv2.graph.entry_points.items.len; | |
| if (file_name) |name| { | |
| Output.prettyError("<d>:<r> {s}", .{name}); | |
| if (total_count > 1) { | |
| Output.prettyError(" <d>+ {d} more<r>", .{total_count - 1}); | |
| } | |
| } | |
| } | |
| Output.prettyError("\n", .{}); | |
| } | |
| Output.flush(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bake/DevServer.zig` around lines 2866 - 2925, The debug output path can
emit text (via printMemoryLine) even when should_print_bundle_line is false, but
the trailing newline and Output.flush() are only executed inside the
should_print_bundle_line branch; move the flush (and the trailing newline
Output.prettyError("\n", .{})) out of that conditional so they run
unconditionally after the if block — update the code around
should_print_bundle_line/Output.prettyError(...) and ensure Output.flush() is
invoked after that whole if so printMemoryLine() output is always flushed.
Comment on lines
+353
to
+354
| /// Returns whether the `BUN_CONFIG_NO_BUNDLE_PAGE_LOG` env var is set to something truthy | ||
| pub fn hasSetNoBundlePageLog(this: *const Loader, default_value: bool) bool { |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Doc comment omits the CLI-flag check.
The comment says "Returns whether the BUN_CONFIG_NO_BUNDLE_PAGE_LOG env var is set to something truthy", but the function also consults has_no_bundle_page_log_cli_flag first (which wins over the env var). The same inaccuracy exists in the twin hasSetNoClearTerminalOnReload.
📝 Proposed fix
- /// Returns whether the `BUN_CONFIG_NO_BUNDLE_PAGE_LOG` env var is set to something truthy
+ /// Returns true if `--no-bundle-page-log` was passed on the CLI, or if the
+ /// `BUN_CONFIG_NO_BUNDLE_PAGE_LOG` env var is set to a truthy value, or `default_value`.
pub fn hasSetNoBundlePageLog(this: *const Loader, default_value: bool) bool {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Returns whether the `BUN_CONFIG_NO_BUNDLE_PAGE_LOG` env var is set to something truthy | |
| pub fn hasSetNoBundlePageLog(this: *const Loader, default_value: bool) bool { | |
| /// Returns true if `--no-bundle-page-log` was passed on the CLI, or if the | |
| /// `BUN_CONFIG_NO_BUNDLE_PAGE_LOG` env var is set to a truthy value, or `default_value`. | |
| pub fn hasSetNoBundlePageLog(this: *const Loader, default_value: bool) bool { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/env_loader.zig` around lines 353 - 354, Update the doc comments to
reflect that the CLI flag takes precedence over the environment variable: for
hasSetNoBundlePageLog and its twin hasSetNoClearTerminalOnReload, change the
description to say they return whether the option is set (checking the CLI flag
has_no_bundle_page_log_cli_flag / has_no_clear_terminal_on_reload_cli_flag
first, and if not set, falling back to the BUN_CONFIG_* env var), and mention
that the CLI flag wins over the env var to make behavior accurate.
Comment on lines
+45
to
+51
| test("--no-bundle-page-log suppresses only the bundled page log line", async () => { | ||
| const withDefaultLogging = await runFixture([]); | ||
| expect(withDefaultLogging).toContain("Bundled page in"); | ||
|
|
||
| const withFlag = await runFixture(["--no-bundle-page-log"]); | ||
| expect(withFlag).not.toContain("Bundled page in"); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider splitting into two separate test() cases for clearer failure attribution.
The single test runs both the default-logging and flag-suppressed scenarios sequentially. If the first assertion (withDefaultLogging contains "Bundled page in") fails, the flag-behavior scenario never executes. Splitting into two named tests ("default logging emits bundle line" and "--no-bundle-page-log suppresses bundle line") makes CI output unambiguous about which behavior regressed.
Additionally, the test has two coverage gaps worth considering for follow-up:
- No test verifying that reload ("Reloaded in …") is not suppressed when the flag is active (the
had_reload_eventbypass in DevServer.zig). - No test exercising the
BUN_CONFIG_NO_BUNDLE_PAGE_LOGenvironment variable path mentioned in the AI summary.
Would you like me to generate an expanded test file that splits the cases and covers the reload and env-var paths?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/bun/http/bun-serve-no-bundle-page-log.test.ts` around lines 45 - 51,
Split the existing combined test into two separate tests: one named like
"default logging emits bundle line" that only runs runFixture([]) and asserts
the output contains "Bundled page in", and a second named like
"--no-bundle-page-log suppresses bundle line" that runs
runFixture(["--no-bundle-page-log"]) and asserts the output does not contain
"Bundled page in". Additionally add two more tests: one that runs with the flag
and verifies the reload message ("Reloaded in") is still present to cover the
had_reload_event bypass in DevServer.zig, and another that sets the
BUN_CONFIG_NO_BUNDLE_PAGE_LOG environment variable path (instead of the CLI
flag) and asserts suppression behaves the same as the flag; use runFixture
invocations accordingly to exercise these paths.
Contributor
|
✅ d004d — Looks good! Reviewed 4 files across |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds a new runtime CLI flag, --no-bundle-page-log, to suppress the "Bundled page in ..." line printed by the dev server during static page bundling in Bun.serve development mode.
How did you verify your code works?
bun bd test test/js/bun/http/bun-serve-no-bundle-page-log.test.tspasses,USE_SYSTEM_BUN=1 bun test test/js/bun/http/bun-serve-no-bundle-page-log.test.tsfails.Motivation
Adding an extra section here to explain why I propose it: I have a test setup where for each E2E UI test, a new instance of my server and database is spawned on a unique port and it gets torn down with the test fixture as well. This way no server in-memory start or database content can interfere between tests (as it is also per-test and in-memory). However, this results in spam in my Playwright output as the tests are running. I am using the
dotreporter which is compact, but with these lines, I get a line from the reporter and then 150 lines ofBundled x in y ms, one for each E2E test. I've not found a good way to suppress this (I have a wrapper script which streams the Playwright process and ditches this line, but my coding agent likes to run the Playwright CLI directly so it doesn't work well), so I figured I'll take a stab at contributing a fix for this.