fix(vite): route browser asset loads to vite when sec-fetch-dest is absent#4238
fix(vite): route browser asset loads to vite when sec-fetch-dest is absent#4238
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR enhances the pre-Vite Nitro middleware to better distinguish between asset and document requests by examining the Accept header and file extensions when Sec-Fetch-Dest is unavailable, ensuring proper routing between Vite and Nitro in non-loopback scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/build/vite/dev.ts (1)
20-24: Move internal helper and trim explanatory comments to match repo conventions.Line 23 introduces a non-exported helper constant near the top, and Lines 20-22, 254-255, and 273-275 add explanatory comments that are broader than needed. Please move
ASSET_EXT_REto the internal-helper section at file end and keep comments minimal.As per coding guidelines "Place non-exported/internal helpers at the end of the file" and "Do not add comments explaining what the line does unless prompted."
Also applies to: 254-255, 273-275
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/build/vite/dev.ts` around lines 20 - 24, Move the non-exported constant ASSET_EXT_RE out of the top of the file into the internal-helper section at the end of the file, and remove or trim the surrounding explanatory comments (the multi-line comments currently around ASSET_EXT_RE and at the other noted spots) to minimal one-line notes per repo convention; update any internal references to ASSET_EXT_RE if needed but do not change its value or export status.test/vite/baseurl-dotted-param.test.ts (1)
51-57: Strengthen the asset test to avoid body-only false positives.Line 56 validates only response text. Consider asserting against the splat signature as a tuple (status + body), so this test fails only when behavior regresses to Nitro splat handling.
Suggested assertion refinement
test("does not misroute asset loads to splat Nitro routes when sec-fetch-dest is absent", async () => { const response = await fetch(`${serverURL}/subdir/api/proxy/entry-client.ts`, { headers: { accept: "*/*" }, redirect: "manual", }); - expect(await response.text()).not.toBe("entry-client.ts"); + const body = await response.text(); + expect({ status: response.status, body }).not.toEqual({ + status: 200, + body: "entry-client.ts", + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/vite/baseurl-dotted-param.test.ts` around lines 51 - 57, The test "does not misroute asset loads to splat Nitro routes when sec-fetch-dest is absent" currently only checks response.text() which can yield false positives; update the assertion to assert both response.status and response.text together (e.g., read response.status and await response.text() and assert the tuple is not equal to the splat signature such as [200, "entry-client.ts"]) for the fetch to `${serverURL}/subdir/api/proxy/entry-client.ts`, using response.status and response.text() in a combined expect to ensure the test only fails when the Nitro splat route is actually returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/build/vite/dev.ts`:
- Around line 20-24: Move the non-exported constant ASSET_EXT_RE out of the top
of the file into the internal-helper section at the end of the file, and remove
or trim the surrounding explanatory comments (the multi-line comments currently
around ASSET_EXT_RE and at the other noted spots) to minimal one-line notes per
repo convention; update any internal references to ASSET_EXT_RE if needed but do
not change its value or export status.
In `@test/vite/baseurl-dotted-param.test.ts`:
- Around line 51-57: The test "does not misroute asset loads to splat Nitro
routes when sec-fetch-dest is absent" currently only checks response.text()
which can yield false positives; update the assertion to assert both
response.status and response.text together (e.g., read response.status and await
response.text() and assert the tuple is not equal to the splat signature such as
[200, "entry-client.ts"]) for the fetch to
`${serverURL}/subdir/api/proxy/entry-client.ts`, using response.status and
response.text() in a combined expect to ensure the test only fails when the
Nitro splat route is actually returned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 94020b4f-85fa-4ef8-9a7f-44650daef640
📒 Files selected for processing (2)
src/build/vite/dev.tstest/vite/baseurl-dotted-param.test.ts
commit: |
fix(vite): route browser asset loads to vite when sec-fetch-dest is absent
browsers only send sec-fetch-dest on "potentially trustworthy" origins, so on
plain-http non-loopback urls (e.g. http://10.0.0.x:3000) the header is missing
and a splat nitro route would swallow
<script src=".../entry-client.ts">requests. fall back to accept + a narrow asset-extension list to detect asset
loads, and mark the request as handled so the catch-all middleware after vite
doesn't re-route a vite 404 into the splat.
closes #4234