ssg 3

[zackradisic]

What does this PR do?

Fixes a crash related to the dev server overwriting the uws user context
pointer when setting abort callback.

Adds support for return new Response(<jsx />, { ... }) and return Response.render(...) and return Response.redirect(...):

• Created a SSRResponse class to handle this (see JSBakeResponse.{h,cpp})
• SSRResponse is designed to "fake" being a React component
  • This is done in JSBakeResponse::create inside of src/bun.js/bindings/JSBakeResponse.cpp
  • And src/js/builtins/BakeSSRResponse.ts defines a wrapComponent function which wraps
    the passed in component (when doing new Response(<jsx />, ...)). It does
    this to throw an error (in redirect()/render() case) or return the
    component.
  • Created a BakeAdditionsToGlobal struct which contains some properties
    needed for this
  • Added some of the properties we need to fake to BunBuiltinNames.h (e.g.
    $$typeof), the rationale behind this is that we couldn't use
    structure->addPropertyTransition because JSBakeResponse is not a final
    JSObject.
• When bake and server-side, bundler rewrites Response -> Bun.SSRResponse (see src/ast/P.zig and src/ast/visitExpr.zig)
• Created a new WebCore body variant (Render: struct { path: []const u8 })
  • Created when return Response.render(...)
  • When handled, it re-invokes dev server to render the new path

Enables server-side sourcemaps for the dev server:

• New source providers for server-side: (DevServerSourceProvider.{h,cpp})
• IncrementalGraph and SourceMapStore are updated to support this

There are numerous other stuff:

• allow app configuration from Bun.serve(...)
• fix errors stopping dev server
• fix use after free related to in RequestContext.finishRunningErrorHandler
• Request.cookies
• Make "use client"; components work
• Fix some bugs using require(...) in dev server
• Fix catch-all routes not working in the dev server
• Updates findSourceMappingURL(...) to use std.mem.lastIndexOf(...) because
  the sourcemap that should be used is the last one anyway

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (3)

src/bun.js/webcore/Response.zig (1)

444-514: Past review feedback not addressed: redirect implementation still in wrong file.

Maintainer feedback from previous reviews explicitly stated this redirect code should be in BakeResponse.zig, not Response.zig. While the refactoring separates constructRedirectImpl from the wrapper, the core issue remains: this SSR-specific redirect logic doesn't belong in the base Response implementation.

Consider moving constructRedirectImpl and the SSR-aware constructRedirect wrapper to src/bun.js/webcore/BakeResponse.zig as originally requested by the maintainer.

Based on past review comments.

src/bun.js/webcore/BakeResponse.zig (2)

21-29: Critical: Multiple issues from past reviews remain unaddressed.

Two bugs flagged in previous reviews are still present:

1. Wrong parameter type: bake_ssr_has_jsx is declared as *c_int but should be ?*c_int since the constructor (line 31-48) checks for null and the C++ caller may pass null for function-call form.

2. Missing pointer cast: Return type is ?*anyopaque but the code returns @as(*Response, ...) without casting to *anyopaque, which is UB across ABIs.

Apply this fix:

-pub export fn BakeResponseClass__constructForSSR(globalObject: *jsc.JSGlobalObject, callFrame: *jsc.CallFrame, bake_ssr_has_jsx: *c_int) callconv(jsc.conv) ?*anyopaque {
-    return @as(*Response, constructor(globalObject, callFrame, bake_ssr_has_jsx) catch |err| switch (err) {
+pub export fn BakeResponseClass__constructForSSR(globalObject: *jsc.JSGlobalObject, callFrame: *jsc.CallFrame, bake_ssr_has_jsx: ?*c_int) callconv(jsc.conv) ?*anyopaque {
+    const resp = constructor(globalObject, callFrame, bake_ssr_has_jsx) catch |err| switch (err) {
         error.JSError => return null,
         error.OutOfMemory => {
             globalObject.throwOutOfMemory() catch {};
             return null;
         },
-    });
+    };
+    return @ptrCast(resp);
 }

Based on past review comments.

---

31-48: Critical: Null-pointer dereference remains unfixed.

Lines 37 and 43 unconditionally dereference bake_ssr_has_jsx without checking if it's null. The C++ caller passes null for the function-call form (non-constructor invocation), which will cause a crash.

Guard all writes to the optional pointer:

 pub fn constructor(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame, bake_ssr_has_jsx: ?*c_int) bun.JSError!*Response {
     var arguments = callframe.argumentsAsArray(2);
 
+    if (bake_ssr_has_jsx) |out| out.* = 0;
+
     // Allow `return new Response(<jsx> ... </jsx>, { ... }`
     // inside of a react component
     if (!arguments[0].isUndefinedOrNull() and arguments[0].isObject()) {
-        bake_ssr_has_jsx.* = 0;
         if (try arguments[0].isJSXElement(globalThis)) {
             const vm = globalThis.bunVM();
             if (try vm.getDevServerAsyncLocalStorage()) |async_local_storage| {
                 try assertStreamingDisabled(globalThis, async_local_storage, "new Response(<jsx />, { ... })");
             }
-            bake_ssr_has_jsx.* = 1;
+            if (bake_ssr_has_jsx) |out| out.* = 1;
         }
     }
 
     return Response.constructor(globalThis, callframe);
 }

Based on past review comments.

🧹 Nitpick comments (1)

src/bun.js/webcore/BakeResponse.zig (1)

128-136: Consider improving error messages for better debugging.

While the validation logic is correct (including the callable check on line 131), the error messages could be more specific to help developers understand what went wrong. Previous review feedback suggested clearer messages.

Consider applying these message improvements:

 fn assertStreamingDisabled(globalThis: *jsc.JSGlobalObject, async_local_storage: JSValue, display_function: []const u8) bun.JSError!void {
-    if (async_local_storage.isEmptyOrUndefinedOrNull() or !async_local_storage.isObject()) return globalThis.throwInvalidArguments("store value must be an object", .{});
-    const getStoreFn = (try async_local_storage.getPropertyValue(globalThis, "getStore")) orelse return globalThis.throwInvalidArguments("store value must have a \"getStore\" field", .{});
-    if (!getStoreFn.isCallable()) return globalThis.throwInvalidArguments("\"getStore\" must be a function", .{});
+    if (async_local_storage.isEmptyOrUndefinedOrNull() or !async_local_storage.isObject())
+        return globalThis.throwInvalidArguments("AsyncLocalStorage must be an object", .{});
+    const getStoreFn = (try async_local_storage.getPropertyValue(globalThis, "getStore"))
+        orelse return globalThis.throwInvalidArguments("AsyncLocalStorage must have a getStore() function", .{});
+    if (!getStoreFn.isCallable())
+        return globalThis.throwInvalidArguments("AsyncLocalStorage.getStore must be a function", .{});
     const store_value = try getStoreFn.call(globalThis, async_local_storage, &.{});
-    const streaming_val = (try store_value.getPropertyValue(globalThis, "streaming")) orelse return globalThis.throwInvalidArguments("store value must have a \"streaming\" field", .{});
+    if (store_value.isEmptyOrUndefinedOrNull() or !store_value.isObject())
+        return globalThis.throwInvalidArguments("getStore() must return an object", .{});
+    const streaming_val = (try store_value.getPropertyValue(globalThis, "streaming"))
+        orelse return globalThis.throwInvalidArguments("store must have a \"streaming\" field", .{});
     if (!streaming_val.isBoolean()) return globalThis.throwInvalidArguments("\"streaming\" field must be a boolean", .{});
     if (streaming_val.asBoolean()) return globalThis.throwInvalidArguments("\"{s}\" is not available when `export const streaming = true`", .{display_function});
 }

Based on past review comments.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 0c90134 and c5e7973.

📒 Files selected for processing (3)

• src/bun.js/VirtualMachine.zig (2 hunks)
• src/bun.js/webcore/BakeResponse.zig (1 hunks)
• src/bun.js/webcore/Response.zig (5 hunks)

🧰 Additional context used

📓 Path-based instructions (3)

src/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

Implement debug logs in Zig using const log = bun.Output.scoped(.${SCOPE}, false); and invoking log("...", .{})

Files:

• src/bun.js/webcore/BakeResponse.zig
• src/bun.js/VirtualMachine.zig
• src/bun.js/webcore/Response.zig

**/*.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

**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup

Files:

• src/bun.js/webcore/BakeResponse.zig
• src/bun.js/VirtualMachine.zig
• src/bun.js/webcore/Response.zig

src/bun.js/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)

src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy

Files:

• src/bun.js/webcore/BakeResponse.zig
• src/bun.js/VirtualMachine.zig
• src/bun.js/webcore/Response.zig

🧠 Learnings (2)

📚 Learning: 2025-08-30T00:11:00.890Z

Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue

Applied to files:

• src/bun.js/VirtualMachine.zig

📚 Learning: 2025-08-30T00:13:36.815Z

Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)

Applied to files:

• src/bun.js/webcore/Response.zig

🔇 Additional comments (1)

src/bun.js/VirtualMachine.zig (1)

192-198: LGTM! Well-structured ALS integration.

The extern declaration and wrapper follow Zig/JSC interop patterns correctly. The wrapper appropriately uses jsc.fromJSHostCall for error handling and returns null when the result is empty/undefined/null, providing a clean Zig-facing API for dev-server AsyncLocalStorage retrieval.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/bun.js/bindings/bindings.cpp (1)

2386-2398: Use fully qualified JSType enumerator for consistency and to avoid lookup issues.

Other cases in this switch use JSC::JSType::…; switch discriminant is a uint8_t cast of JSType. Prefer qualifying the wrapper case the same way.

Apply this diff:

-    case JSDOMWrapperType: {
+    case JSC::JSType::JSDOMWrapperType: {
         if (jsDynamicCast<WebCore::JSFetchHeaders*>(cell))
             return static_cast<double>(jsCast<WebCore::JSFetchHeaders*>(cell)->wrapped().size());

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between c5e7973 and 8a7deca.

📒 Files selected for processing (6)

• src/ast/visitExpr.zig (1 hunks)
• src/bun.js/VirtualMachine.zig (2 hunks)
• src/bun.js/bindings/JSValue.zig (2 hunks)
• src/bun.js/bindings/ZigGlobalObject.cpp (2 hunks)
• src/bun.js/bindings/bindings.cpp (6 hunks)
• test/no-validate-leaksan.txt (1 hunks)

✅ Files skipped from review due to trivial changes (1)

• test/no-validate-leaksan.txt

🚧 Files skipped from review as they are similar to previous changes (3)

• src/ast/visitExpr.zig
• src/bun.js/bindings/ZigGlobalObject.cpp
• src/bun.js/bindings/JSValue.zig

🧰 Additional context used

📓 Path-based instructions (7)

src/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

Implement debug logs in Zig using const log = bun.Output.scoped(.${SCOPE}, false); and invoking log("...", .{})

Files:

• src/bun.js/VirtualMachine.zig

**/*.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

**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup

Files:

• src/bun.js/VirtualMachine.zig

src/bun.js/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)

src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy

Files:

• src/bun.js/VirtualMachine.zig

**/*.{cpp,h}

📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)

**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible

Files:

• src/bun.js/bindings/bindings.cpp

**/*.cpp

📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)

**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue

Files:

• src/bun.js/bindings/bindings.cpp

**/*.{cpp,cc,cxx,h,hpp,hxx}

📄 CodeRabbit inference engine (CLAUDE.md)

Format C/C++ sources/headers with clang-format (bun run clang-format)

Files:

• src/bun.js/bindings/bindings.cpp

src/bun.js/bindings/**/*.cpp

📄 CodeRabbit inference engine (CLAUDE.md)

src/bun.js/bindings/**/*.cpp: For JS classes with a public constructor, implement Foo, FooPrototype, and FooConstructor (JSC patterns)
Define class properties using HashTableValue arrays in JSC bindings
Add iso subspaces for classes with C++ fields in JSC bindings
Cache structures in ZigGlobalObject for JSC-bound classes

Files:

• src/bun.js/bindings/bindings.cpp

🧠 Learnings (4)

📚 Learning: 2025-09-08T04:44:59.101Z

Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.101Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Cache structures in ZigGlobalObject for JSC-bound classes

Applied to files:

• src/bun.js/bindings/bindings.cpp

📚 Learning: 2025-08-30T00:13:36.815Z

Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes

Applied to files:

• src/bun.js/bindings/bindings.cpp

📚 Learning: 2025-08-30T00:11:00.890Z

Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.cpp : Provide an extern "C" Bun__<Type>__toJS function that creates an instance using the cached Structure and returns an EncodedJSValue

Applied to files:

• src/bun.js/bindings/bindings.cpp

📚 Learning: 2025-09-08T04:44:59.101Z

Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.101Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Define class properties using HashTableValue arrays in JSC bindings

Applied to files:

• src/bun.js/bindings/bindings.cpp

🧬 Code graph analysis (1)

src/bun.js/bindings/bindings.cpp (1)

src/bun.js/bindings/JSDOMExceptionHandling.h (1)

• propagateException (86-90)

⏰ 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 (5)

src/bun.js/VirtualMachine.zig (2)

192-198: LGTM! Clean ALS interop implementation.

The extern declaration and Zig wrapper follow best practices:

• Proper error union return type for JS exception integration
• Correct use of jsc.fromJSHostCall for C++ interop
• Appropriate null-checking for empty/undefined/null values
• Aligns with coding guidelines for Zig bindings

---

845-847: Verify shutdown sequence implications.

The commented-out eventLoop().tick() call prevents processing pending event loop tasks during VM exit. While the comment explains this causes 50+ test failures, this workaround may have implications:

• Pending async operations may not complete
• Resources held by queued tasks may not be properly released
• Deferred cleanup handlers might not execute

Given the PR is already experiencing test failures (per reviewer comments), ensure this change doesn't introduce additional shutdown-related issues in production scenarios, especially for long-running server processes with the new SSR/Bake integration.

Consider opening an issue to track re-enabling this call once the underlying test issues are resolved, as proper event loop draining during shutdown is generally important for cleanup.

src/bun.js/bindings/bindings.cpp (3)

32-32: Includes look good.

root.h is present, and adding WebCoreJSBuiltins.h/Strong.h is fine for builtin names and JSC Strong usage. No issues.

Also applies to: 63-63

---

2005-2025: Native FetchHeaders creation path is correct.

• Returns a native WebCore::FetchHeaders* as expected by Zig callers.
• Handles exceptions via propagateException + deref on failure. LGTM.

---

6044-6045: Style-only change.

Brace repositioning is fine; no behavioral impact.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Bug: returning the wrong value (encodes JSFetchHeaders instead of the JSValue).*

Return should encode the JSValue you created (variable “value”), not the JSFetchHeaders* pointer. Use encode(value) for correctness and consistency with neighboring helpers.

Apply this diff:

-    return JSC::JSValue::encode(fetchHeaders);
+    return JSC::JSValue::encode(value);

🤖 Prompt for AI Agents

In src/bun.js/bindings/bindings.cpp around line 2046, the function currently
returns JSC::JSValue::encode(fetchHeaders) which encodes the JSFetchHeaders*
pointer instead of the JSValue you constructed; change the return to encode the
JSValue variable (value) — i.e., return JSC::JSValue::encode(value) — so the
function returns the actual JSValue as done in neighboring helpers, ensuring
correct encoding and type safety.

[jlucaso1]

We will have something like cache invalidation for static pages?

cc: @Jarred-Sumner @zackradisic

[robobun]

~DevServerSourceProvider (introduced here) held a raw Zig::GlobalObject* and read through it in its destructor, which runs from ~CodeCache after the global object cell has already been swept during BUN_DESTRUCT_VM_ON_EXIT teardown. Fixed in #34035 by storing the Rust VM pointer directly, matching Zig::SourceProvider.