fix(vite): route asset-tagged requests to opaque catch-alls in dev - #4467
Merged
Conversation
Routes owned by a custom server entry or the SSR renderer are invisible to `nitro.routing.routes`, so the dev middleware pre-empted asset-tagged requests to them with `_nitroHandled` and they could never run (#4252). Dispatch such requests to nitro after Vite declines and decide from the response instead: a 2xx page/data content-type means the catch-all swallowed a missing asset (#4234) and falls through to the 404; anything else passes through verbatim. User-file root catch-alls keep the existing divert. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesVite dev routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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. Comment |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/vite/server-entry.test.ts`:
- Around line 74-85: Update the “missing assets still 404” test to assert that
each request returns status 404, replacing the weaker non-200 assertion while
preserving the existing headers, URL, and manual redirect behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d16b69ff-e79c-4822-8166-22baf1db7219
📒 Files selected for processing (7)
.agents/vite-dev.mdsrc/build/vite/dev.tstest/vite/app-fixture/app/entry-server.tstest/vite/app.test.tstest/vite/server-entry-fixture/server.tstest/vite/server-entry-fixture/vite.config.tstest/vite/server-entry.test.ts
Comment on lines
+74
to
+85
| // #4234 contract: a genuinely missing asset must still not be answered with 200. | ||
| test("missing assets still 404", async () => { | ||
| for (const headers of [{ "sec-fetch-dest": "style" }, { accept: "*/*" }] as Record< | ||
| string, | ||
| string | ||
| >[]) { | ||
| const res = await fetch(`${serverURL}/missing-asset.css`, { | ||
| headers, | ||
| redirect: "manual", | ||
| }); | ||
| expect(res.status, JSON.stringify(headers)).not.toBe(200); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the promised 404 status.
Lines 74-85 only reject 200, so incorrect non-200 responses would pass. The test and routing contract both specify a 404 fallback.
Proposed fix
- expect(res.status, JSON.stringify(headers)).not.toBe(200);
+ expect(res.status, JSON.stringify(headers)).toBe(404);📝 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
| // #4234 contract: a genuinely missing asset must still not be answered with 200. | |
| test("missing assets still 404", async () => { | |
| for (const headers of [{ "sec-fetch-dest": "style" }, { accept: "*/*" }] as Record< | |
| string, | |
| string | |
| >[]) { | |
| const res = await fetch(`${serverURL}/missing-asset.css`, { | |
| headers, | |
| redirect: "manual", | |
| }); | |
| expect(res.status, JSON.stringify(headers)).not.toBe(200); | |
| } | |
| // `#4234` contract: a genuinely missing asset must still not be answered with 200. | |
| test("missing assets still 404", async () => { | |
| for (const headers of [{ "sec-fetch-dest": "style" }, { accept: "*/*" }] as Record< | |
| string, | |
| string | |
| >[]) { | |
| const res = await fetch(`${serverURL}/missing-asset.css`, { | |
| headers, | |
| redirect: "manual", | |
| }); | |
| expect(res.status, JSON.stringify(headers)).toBe(404); | |
| } |
🤖 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 `@test/vite/server-entry.test.ts` around lines 74 - 85, Update the “missing
assets still 404” test to assert that each request returns status 404, replacing
the weaker non-200 assertion while preserving the existing headers, URL, and
manual redirect behavior.
Hoist opaque catch-all detection into an isOpaqueHandler helper (dropping the per-request Set), share a NitroDevRequest type between both middlewares (removing the inline cast), and drop the intermediate array in the mounted-path skip. No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cemaps through Non-GET/HEAD requests are never browser asset loads, so a POST to an asset-extensioned URL now bypasses the asset classification. For `.map` URLs only text/html counts as a catch-all swallow, since sourcemaps are legitimately application/json. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Opaque frameworks answer API routes tagged as asset loads with JSON on purpose (TanStack/router#7403), and sourcemaps are legitimately JSON, so narrow the response inspection deny to text/html. A naive SSR entry swallowing missing assets renders an HTML page, which is still coerced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/vite/app-fixture/app/entry-server.ts`:
- Around line 17-22: Align the fallback condition in the entry-server handler
with its documented SSR behavior: either broaden the predicate so extensionless
unmatched pathnames also return the HTML Response, or revise the comment to
state that only extensioned paths are handled. Preserve the existing JSON
fallback for paths excluded by the chosen behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fea9441c-ee24-4ec9-83af-68cca69c7d3e
📒 Files selected for processing (4)
.agents/vite-dev.mdsrc/build/vite/dev.tstest/vite/app-fixture/app/entry-server.tstest/vite/app.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .agents/vite-dev.md
- src/build/vite/dev.ts
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pi0
approved these changes
Jul 22, 2026
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
2 participants
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.
🔗 Linked issue
resolves #4252, resolves TanStack/router#7403 (related: #4234, #4241, #4266, #4270, #4272, #4274)
📚 Description
Routes owned by an opaque catch-all — a custom
server.tsserver entry (an H3 app with its own routes) or the SSR renderer (framework routers like TanStack Start) — are invisible tonitro.routing.routes. The dev middleware classifier therefore treated asset-tagged requests to them (sec-fetch-dest: image, asset extensions, …) as Vite assets and marked them_nitroHandled, so the user's handler could never run:<img src="/image.png">served by aserver.tsroute, or a TanStack Start API route serving a thumbnail, died in a connectfinalhandler404.The two cases (
/image.pngis a real opaque route vs. a genuinely missing asset a naive SSR/**would render as a 200 page) cannot be told apart before dispatching — but they can from the response:routes/[...path].ts— Nitro sees everything they can handle, so the existing_nitroHandleddivert stays and they still never swallow Vite asset misses) and opaque ones (renderer /serverEntry/**, identified by their registered handler paths — or no match at all)._nitroAssetCheckinstead: Vite keeps first shot, and on a Vite miss the post catch-all dispatches to nitro and inspects the response. A 2xxtext/htmlresponse means the catch-all rendered a page for a missing asset (Accessing via a non-localhost URL breaks vite dev server #4234) → discarded vianext()(samefinalhandler404 as before). Anything else — real asset types, JSON (deliberate API responses, File extension/Image src does not end to server route but 404 TanStack/router#7403; sourcemaps),text/plain(bridge default for bare string returns), no content-type, non-2xx framework 404 pages, redirects — passes through verbatim.GET/HEADrequests can be classified as asset loads at all (aPOST /upload.pngis never a browser asset load).Zero config, host-side only: no runtime/bundle changes and no production behavior change. All #4234/#4241/#4266/#4270/#4272 regression contracts stay covered by the existing tests. Verified against a real
@tanstack/react-startapp: API routes undersec-fetch-dest: image(extensionless,.png-param, and?filename=x.pngshapes from #7403 / #4274) now return their JSON 200s, while genuinely missing assets and unmatched routes still 404.New regression tests (fail before, pass after, both builders):
test/vite/server-entry.test.ts+ fixture — customserver.tsH3 app: asset-extensioned routes reachable underimage/absent/scriptfetch dests (including a no-content-type string return and a JSON sourcemap),POSTto an asset-extensioned route, missing assets still 404, navigations work.test/vite/app.test.ts— the SSR catch-all can deliberately serve/dynamic-asset.png(image/png) and JSON API routes undersec-fetch-dest: image(#7403 shapes), while the missing-asset swallow contracts still hold against an HTML-rendering fixture.Known limitation (documented): opaque semantics require registration via
renderer/serverEntry— a/**catch-all added throughroutes:/handlers:config or a module is still classified transparent and pre-empted for asset-tagged requests.Also includes
.agents/vite-dev.mddocumenting the routing design, its issue lineage, and the invariants to preserve.🤖 Generated with Claude Code