fix(dev): route specific Nitro paths even when URL has asset extension#4266
Conversation
|
@harshagarwalnyu is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe Vite dev middleware pre-routing computes the request pathname once, matches Nitro routes, sets asset-detection signals and Vary headers, and only suppresses Nitro for root wildcard matches ( ChangesVite dev middleware route matching refinement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 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 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 |
There was a problem hiding this comment.
Pull request overview
This PR fixes Nitro’s Vite dev-server routing precedence so that explicit Nitro routes (e.g. /api/**, /api/photos/**) still reach Nitro even when the request URL ends with an “asset-like” extension (e.g. .jpg), while preserving the original safeguard that prevents a root catch-all (/**) from swallowing Vite-served assets when Sec-Fetch-Dest is absent.
Changes:
- Compute the matched Nitro route once (
nitroRouteMatch) and derive whether the match is the root wildcard route (/**). - Narrow the asset-extension guard so it only blocks Nitro when the matched route is exactly the root wildcard (
/**). - Keep existing
Sec-Fetch-Dest/Acceptbased “treat as asset” behavior, but allow explicit prefixed Nitro routes to win.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/vite/baseurl-dotted-param-fixture/routes/[...path].ts (1)
1-1: ⚡ Quick winConsider using proper event typing and safer parameter access.
The
anytype foreventbypasses type checking, and the non-null assertion onparamscould cause runtime errors if the parameter is unexpectedly undefined. Consider importing and using the proper Nitro event type.♻️ Proposed type-safe alternative
-export default (event: any) => "root-wildcard:" + event.context.params!.path; +import type { EventHandler } from "h3"; + +export default ((event) => { + const path = event.context.params?.path ?? ""; + return `root-wildcard:${path}`; +}) satisfies EventHandler;🤖 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/baseurl-dotted-param-fixture/routes/`[...path].ts at line 1, Replace the untyped handler and unsafe non-null assertion by importing and using the proper Nitro event type (e.g., NitroEvent) for the default export's parameter and accessing params safely; update the default export signature from (event: any) to (event: NitroEvent), add the NitroEvent import, and read the path using optional chaining and a safe fallback (e.g., event.context?.params?.path ?? '') when constructing the "root-wildcard:" return value so you avoid runtime errors if params is undefined.test/vite/baseurl-dotted-param.test.ts (1)
72-81: 💤 Low valueConsider strengthening the negative assertion with additional positive checks.
The test correctly validates that the root wildcard doesn't intercept the request, but the negative assertion doesn't confirm what should happen. While the comment acknowledges Vite's response may vary, you could add a positive check (e.g., status code or content-type) to make the test more robust and self-documenting.
📋 Optional enhancement
const response = await fetch(`${serverURL}/subdir/entry-client.ts`, { headers: { accept: "*/*" }, redirect: "manual", }); + // Vite should handle this request - validate it's not the root wildcard expect(await response.text()).not.toBe("root-wildcard:entry-client.ts"); + // Additionally confirm Vite attempted to serve it (200 or 404 from Vite, not Nitro) + expect(response.status).toBeGreaterThanOrEqual(200); + expect(response.status).toBeLessThan(500); });🤖 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/baseurl-dotted-param.test.ts` around lines 72 - 81, The test "root-level wildcards do not swallow Vite assets when sec-fetch-dest is absent" currently only asserts the body is not "root-wildcard:entry-client.ts"; enhance it by adding a positive assertion about the response shape: after calling fetch(serverURL + '/subdir/entry-client.ts') inspect response.status and response.headers (e.g., assert response.status is one of the expected values such as 200 or 404) and assert response.headers.get('content-type') is present and contains either "text/html" or "javascript" so the test documents what Vite actually returned in addition to the negative check.
🤖 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.
Nitpick comments:
In `@test/vite/baseurl-dotted-param-fixture/routes/`[...path].ts:
- Line 1: Replace the untyped handler and unsafe non-null assertion by importing
and using the proper Nitro event type (e.g., NitroEvent) for the default
export's parameter and accessing params safely; update the default export
signature from (event: any) to (event: NitroEvent), add the NitroEvent import,
and read the path using optional chaining and a safe fallback (e.g.,
event.context?.params?.path ?? '') when constructing the "root-wildcard:" return
value so you avoid runtime errors if params is undefined.
In `@test/vite/baseurl-dotted-param.test.ts`:
- Around line 72-81: The test "root-level wildcards do not swallow Vite assets
when sec-fetch-dest is absent" currently only asserts the body is not
"root-wildcard:entry-client.ts"; enhance it by adding a positive assertion about
the response shape: after calling fetch(serverURL + '/subdir/entry-client.ts')
inspect response.status and response.headers (e.g., assert response.status is
one of the expected values such as 200 or 404) and assert
response.headers.get('content-type') is present and contains either "text/html"
or "javascript" so the test documents what Vite actually returned in addition to
the negative check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5bdc22f-d868-43f4-9dee-5d8087bb4c0a
📒 Files selected for processing (3)
src/build/vite/dev.tstest/vite/baseurl-dotted-param-fixture/routes/[...path].tstest/vite/baseurl-dotted-param.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/build/vite/dev.ts
harshagarwalnyu
force-pushed
the
fix/dev-middleware-asset-ext-swallows-routes
branch
from
789cf7c to
adf0198
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/vite/baseurl-dotted-param.test.ts (1)
60-63: ⚡ Quick winRemove inline explanatory comments and encode intent in test names/assertions.
These comments restate behavior already captured by descriptive test titles and assertions; please drop them to match repo style.
As per coding guidelines, "Do not add comments explaining what the line does unless prompted".
Also applies to: 79-79, 90-90, 97-97
🤖 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/baseurl-dotted-param.test.ts` around lines 60 - 63, Remove the inline explanatory comment block starting with "Browsers omit Sec-Fetch-* on plain-HTTP non-loopback origins..." and similar comment blocks (the ones mentioning "Specific prefixed routes..." and the other occurrences referencing Sec-Fetch and prefixed routes), and instead make the test names/assertions express that behavior directly: rename the affected tests to state the expected behavior (e.g., "prefixed /api/proxy routes should reach Nitro even with asset extension and Sec-Fetch present" or equivalent) and strengthen the assertions in those test cases to validate the exact intent; delete the comment lines and update the surrounding test titles and expect(...) messages so the behavior is encoded in the test names/assertions rather than comments.
🤖 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.
Nitpick comments:
In `@test/vite/baseurl-dotted-param.test.ts`:
- Around line 60-63: Remove the inline explanatory comment block starting with
"Browsers omit Sec-Fetch-* on plain-HTTP non-loopback origins..." and similar
comment blocks (the ones mentioning "Specific prefixed routes..." and the other
occurrences referencing Sec-Fetch and prefixed routes), and instead make the
test names/assertions express that behavior directly: rename the affected tests
to state the expected behavior (e.g., "prefixed /api/proxy routes should reach
Nitro even with asset extension and Sec-Fetch present" or equivalent) and
strengthen the assertions in those test cases to validate the exact intent;
delete the comment lines and update the surrounding test titles and expect(...)
messages so the behavior is encoded in the test names/assertions rather than
comments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac07c5bb-86b0-416e-8009-f928958108c5
📒 Files selected for processing (1)
test/vite/baseurl-dotted-param.test.ts
|
@coderabbitai review:
|
Asset-extensioned URLs (e.g. /api/photos/12345.jpg) were routed to Vite's static middleware instead of the matching Nitro handler. The ASSET_EXT_RE guard in nitroDevMiddlewarePre applied unconditionally to any matched route, including specific prefixed catch-alls like /api/photos/**. The guard was introduced (nitrojs#4234) to prevent a root-level renderer or user catch-all (/**) from swallowing Vite's own <script>/<link> serves on plain-HTTP non-loopback origins where Sec-Fetch-Dest is absent. Fix: narrow the guard so the asset-extension filter only applies when the matched Nitro route pattern is /** or /**:param (named root catch-all). Specific routes (/api/**, /api/photos/**) are never registered on paths Vite owns and should always win — restoring the behaviour from nitro@3.0.1-alpha.2. When the matched route's pattern is unreadable, default to false (let the route win) rather than falling back to the asset-extension guard. Closes nitrojs#4252
harshagarwalnyu
force-pushed
the
fix/dev-middleware-asset-ext-swallows-routes
branch
from
3a48bb9 to
bbea608
Compare
🧩 Analysis chain🏁 Script executed: Length of output: 8441 🏁 Script executed: Length of output: 578 🏁 Script executed: Length of output: 1988 🏁 Script executed: Length of output: 6936
✅ User-stated changes verified
Core logic in
|
| Test | fetchDest values |
Expected |
|---|---|---|
Prefixed splat wins (/subdir/api/proxy/entry-client.ts) |
script, style, image, absent |
200 from Nitro handler |
Root wildcard suppressed (/subdir/entry-client.ts) |
script, style, image, absent |
404 (Vite, no asset file) |
Root wildcard positive (/subdir/some-page) |
absent | 200 root-wildcard:some-page |
The undefined fetchDest case in the prefixed splat test correctly validates the #4252 regression — the exact scenario where Sec-Fetch-Dest is absent on a plain-HTTP non-loopback origin.
The changes are well-scoped, the logic is sound, and the test coverage properly guards both the new fix (#4252) and the preserved behavior (#4234).
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Thanks a lot for this fix and the clear analysis ❤️ I went with an alternative approach in #4272 that builds on the same root-vs-explicit distinction — it also covers the named root catch-all (/**:slug) case you raised here 🙏 |
Asset-extensioned URLs (e.g.
/api/photos/12345.jpg) were routed to Vite's static middlewareinstead of the matching Nitro handler. The
ASSET_EXT_REguard innitroDevMiddlewarePreapplied unconditionally to any matched route, including specific prefixed catch-alls like
/api/photos/**.The guard was introduced (#4234) to prevent a root-level renderer or user catch-all (
/**) fromswallowing Vite's own
<script>/<link>serves on plain-HTTP non-loopback origins whereSec-Fetch-Destis absent.Fix: narrow the guard so the asset-extension filter only applies when the matched Nitro route
pattern is exactly
/**. Specific routes (/api/**,/api/photos/**) are never registered onpaths Vite owns and should always win — restoring the behaviour from
nitro@3.0.1-alpha.2.Test plan
routes/api/photos/[...].ts→curl http://localhost:3000/api/photos/12345.jpgreturns 200 from handler (was 404)<script src="/src/app.ts">still served by Vite (not swallowed by renderer catch-all)Sec-Fetch-Dest: imagefor explicit Nitro routes still reach Nitro (Regression - Sec-fetch-dest: image header causes 404 on API-served images (regression in 3.0.260429-beta) #4241)Fixes #4252