Fix patch application and Vite asset serving#2
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a pluggable Vite client pipeline to the Bun host, a declarative Region-based projection API with automatic patching, React provider/root adapters, demo migrations, ProgramBuilder immutability, and tests. ChangesBun-hosted runtime with Vite client and region-based projection updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 2
🧹 Nitpick comments (2)
src/vite-env.d.ts (1)
1-1: 💤 Low valueConsider providing typed exports for CSS Module imports.
The current declaration allows CSS imports but provides no typing for CSS Module exports. If the project uses CSS Modules (e.g.,
import styles from './App.module.css'), consider providing typed class name exports:declare module "*.css" { const classes: Record<string, string>; export default classes; }If the project only performs side-effect CSS imports (
import './styles.css'), the current declaration is sufficient.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vite-env.d.ts` at line 1, Update the module declaration for "*.css" to provide typed exports for CSS Modules: replace the current bare "declare module \"*.css\";" with a module block that declares and exports a default const (e.g., classes) typed as Record<string, string> so imports like "import styles from './App.module.css'" are typed; if you intentionally only use side-effect imports, leave the existing declaration as-is.tests/bun-host.test.ts (1)
202-291: ⚡ Quick winAdd a negative traversal test for public asset containment.
These new tests cover happy-path asset serving well, but they don’t assert that path traversal attempts are rejected. Adding one request like
/../favicon.svg(or equivalent) with expected 403/404 would lock in the security guarantee introduced in this PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/bun-host.test.ts` around lines 202 - 291, Add a negative traversal request to the existing Vite tests: after fetching the regular favicon in each test (inside the try block of the "Vite client pipeline serves dev modules while Bun owns the stream runtime" and "Vite client pipeline serves production manifest and public assets" tests), issue an extra fetch to "/../favicon.svg" (or similar traversal path) against the same server.port using fetch, and assert the response does not succeed (e.g., expect(response.status).not.toBe(200) or expect(response.status).toBeGreaterThanOrEqual(400) && expect(response.status).toBeLessThan(600)). This uses the same serveBunProgram/TestMessage/TestProjection/TestTrace setup and server variable already present in those tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/adapters/react/root.tsx`:
- Around line 24-26: The hydration gate currently treats options.bootstrap by
truthiness which misroutes valid falsy values; change the condition in the block
that decides between hydrateRoot and createRoot to explicitly test for undefined
(e.g., options.bootstrap !== undefined or typeof options.bootstrap !==
"undefined") together with options.root.hasChildNodes(), so hydrateRoot is used
only when bootstrap was explicitly provided and the root has child nodes; leave
the createRoot fallback for when options.bootstrap is undefined.
In `@src/demo/approvals/client/app.tsx`:
- Around line 20-24: The errorFallback JSX in app.tsx currently renders
error.message directly (in the errorFallback prop), which can leak internals;
update the errorFallback handler (the errorFallback arrow function) to display a
generic user-facing message (e.g., "An unexpected error occurred") unless
running in development, in which case include the actual error.message for
debugging; locate the errorFallback prop in the component and replace the direct
error.message usage with a conditional that checks process.env.NODE_ENV ===
'development' before exposing error.message.
---
Nitpick comments:
In `@src/vite-env.d.ts`:
- Line 1: Update the module declaration for "*.css" to provide typed exports for
CSS Modules: replace the current bare "declare module \"*.css\";" with a module
block that declares and exports a default const (e.g., classes) typed as
Record<string, string> so imports like "import styles from './App.module.css'"
are typed; if you intentionally only use side-effect imports, leave the existing
declaration as-is.
In `@tests/bun-host.test.ts`:
- Around line 202-291: Add a negative traversal request to the existing Vite
tests: after fetching the regular favicon in each test (inside the try block of
the "Vite client pipeline serves dev modules while Bun owns the stream runtime"
and "Vite client pipeline serves production manifest and public assets" tests),
issue an extra fetch to "/../favicon.svg" (or similar traversal path) against
the same server.port using fetch, and assert the response does not succeed
(e.g., expect(response.status).not.toBe(200) or
expect(response.status).toBeGreaterThanOrEqual(400) &&
expect(response.status).toBeLessThan(600)). This uses the same
serveBunProgram/TestMessage/TestProjection/TestTrace setup and server variable
already present in those tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6459ad31-0954-47f0-bbee-50241eda1dac
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
README.mddocs/design/developer-experience.mddocs/design/runtime.mdpackage.jsonsrc/adapters/react/index.tssrc/adapters/react/program-provider.tsxsrc/adapters/react/projection-patch.tssrc/adapters/react/react-adapter.tssrc/adapters/react/root.tsxsrc/demo/approvals/client/app.tsxsrc/demo/approvals/client/approval-app.tsxsrc/demo/approvals/projection-manifest.tssrc/demo/approvals/screen.tsxsrc/framework/bun-host.tssrc/framework/program.tssrc/framework/projection.tssrc/server.tssrc/shell.htmlsrc/vite-env.d.tstests/bun-host.test.tstests/client-patch.test.ts
💤 Files with no reviewable changes (2)
- src/demo/approvals/projection-manifest.ts
- src/shell.html
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/framework/bun-host.ts (1)
122-134:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLet host-managed CSS routes win before the Vite proxy.
client.serve()now runs beforeassets.byRoute, andisViteAssetPath()claims every.cssrequest. In Vite dev mode that means a configured Bun style asset like/assets/demo.cssgets proxied to Vite instead of the host style builder, so enablingclient.kind: "vite"breaksoptions.assets.styles.Suggested fix
- const clientResponse = await client.serve(request); - - if (clientResponse) { - return clientResponse; - } - const asset = assets.byRoute.get(url.pathname); if (asset) { return new Response(Bun.file(asset.output), { headers: { "Content-Type": "text/css; charset=utf-8" }, }); } + + const clientResponse = await client.serve(request); + + if (clientResponse) { + return clientResponse; + }Also applies to: 472-483, 732-750
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/framework/bun-host.ts` around lines 122 - 134, Reorder the host asset check so host-managed CSS routes are served before proxying to Vite: before calling client.serve(), check assets.byRoute.get(url.pathname) (and/or isViteAssetPath()) and if an asset exists return the Bun.file Response for that asset; alternatively, make client.serve() skip proxying when assets.byRoute contains the pathname. Apply this change to the occurrences around client.serve / assets.byRoute (the shown block plus the other spots mentioned at ~472-483 and ~732-750) so host style assets win over the Vite proxy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/framework/bun-host.ts`:
- Around line 574-581: The serveVitePublicAsset function currently restricts
serving to `${root}/public` and only files matching isStaticAssetPath; instead,
change it to use Vite's resolved publicDir and stop filtering by extension:
locate serveVitePublicAsset and replace the hardcoded join(root, "public") +
isStaticAssetPath check with logic that reads Vite's resolved publicDir (from
the Vite config/resolved config passed into your host or obtained via
resolveConfig) and call serveViteFile against that directory for any matching
URL path under publicDir (including extensionless files and .well-known,
.webmanifest, etc.), ensuring you preserve path normalization/security checks
already in serveViteFile.
- Around line 525-533: injectViteProductionAssets currently only uses entry.css
and entry.file so CSS from imported chunks is omitted; update the function
(injectViteProductionAssets) to accept the full Vite manifest (e.g., manifest:
Record<string, ViteManifestEntry>), then recursively traverse entry.imports to
collect css from each referenced manifest entry (follow imports depth-first,
deduplicate files using a Set) and include those CSS links along with entry.css
and the entry.file script when building tags; finally inject the combined,
deduped CSS links + script into the HTML as before.
---
Outside diff comments:
In `@src/framework/bun-host.ts`:
- Around line 122-134: Reorder the host asset check so host-managed CSS routes
are served before proxying to Vite: before calling client.serve(), check
assets.byRoute.get(url.pathname) (and/or isViteAssetPath()) and if an asset
exists return the Bun.file Response for that asset; alternatively, make
client.serve() skip proxying when assets.byRoute contains the pathname. Apply
this change to the occurrences around client.serve / assets.byRoute (the shown
block plus the other spots mentioned at ~472-483 and ~732-750) so host style
assets win over the Vite proxy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 24869886-0096-46d0-9d02-ced759eb0003
📒 Files selected for processing (4)
src/adapters/react/root.tsxsrc/demo/approvals/client/app.tsxsrc/framework/bun-host.tstests/bun-host.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/demo/approvals/client/app.tsx
- src/adapters/react/root.tsx
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.
Summary
Verification
Summary by CodeRabbit
New Features
Documentation
Tests
Chores