fix: serve dotted paths through the Netlify edge function - #244
Conversation
🦋 Changeset detectedLatest commit: 75f0500 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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:
WalkthroughThe Netlify edge entry now matches all paths, checks static files for 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/run/src/__tests__/fixtures/netlify-adapter-edge-paths/test.config.ts (1)
30-38: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the untested routing branches.
Add a
+404fixture page and assert its rendered content for an unmatched dotted URL. The current assertion only proves that the fallback response has status 404.Add HEAD static-file coverage and POST routing coverage. The current steps only exercise GET, so they do not verify the separate GET/HEAD static branch or router-first behavior for other methods.
🤖 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 `@packages/run/src/__tests__/fixtures/netlify-adapter-edge-paths/test.config.ts` around lines 30 - 38, Add a +404 fixture page and update unmatchedPathIs404 to request an unmatched dotted URL and assert both the 404 status and rendered fallback content. Extend the steps around staticFileWins and dottedPathRoutes with HEAD static-file coverage and POST routing coverage, ensuring the assertions verify the separate GET/HEAD static behavior and router-first handling for non-GET methods.
🤖 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
`@packages/run/src/__tests__/fixtures/netlify-adapter-edge-paths/test.config.ts`:
- Around line 30-38: Add a +404 fixture page and update unmatchedPathIs404 to
request an unmatched dotted URL and assert both the 404 status and rendered
fallback content. Extend the steps around staticFileWins and dottedPathRoutes
with HEAD static-file coverage and POST routing coverage, ensuring the
assertions verify the separate GET/HEAD static behavior and router-first
handling for non-GET methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e33c849-25ba-41eb-97a8-17dabdc54297
⛔ Files ignored due to path filters (2)
packages/run/src/__tests__/fixtures/netlify-adapter-edge-paths/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/netlify-adapter-edge-paths/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (8)
packages/run/src/__tests__/fixtures/netlify-adapter-edge-paths/.marko-run/routes.d.tspackages/run/src/__tests__/fixtures/netlify-adapter-edge-paths/previewAdapter.tspackages/run/src/__tests__/fixtures/netlify-adapter-edge-paths/public/reports/pinned.txtpackages/run/src/__tests__/fixtures/netlify-adapter-edge-paths/src/routes/+page.markopackages/run/src/__tests__/fixtures/netlify-adapter-edge-paths/src/routes/reports/$$rest/+page.markopackages/run/src/__tests__/fixtures/netlify-adapter-edge-paths/test.config.tspackages/run/src/__tests__/fixtures/netlify-adapter-edge-paths/tsconfig.jsonpackages/run/src/__tests__/fixtures/netlify-adapter-edge-paths/vite.config.ts
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 `@packages/adapters/netlify/src/default-edge-entry.ts`:
- Around line 19-23: Update the fallback logic near eagerFallback so
context.next() is used whenever fetch returns no response, regardless of method,
while restricting the response.status === 404 fallback to GET and HEAD requests
through mayBeStatic. Add a regression test covering a declined non-GET/HEAD
request and verify it returns the CDN response rather than the synthetic 404.
🪄 Autofix
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: 1ba986e6-3ae2-4e22-99c9-37db50a6d98f
⛔ Files ignored due to path filters (2)
packages/run/src/__tests__/fixtures/netlify-adapter-edge-paths/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/netlify-adapter-edge-paths/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (8)
.changeset/eight-frogs-report.mdpackages/adapters/netlify/README.mdpackages/adapters/netlify/src/default-edge-entry.tspackages/run/src/__tests__/fixtures/netlify-adapter-edge-paths/.marko-run/routes.d.tspackages/run/src/__tests__/fixtures/netlify-adapter-edge-paths/public/notespackages/run/src/__tests__/fixtures/netlify-adapter-edge-paths/src/routes/+404.markopackages/run/src/__tests__/fixtures/netlify-adapter-edge-paths/src/routes/reports/$$rest/+handler.tspackages/run/src/__tests__/fixtures/netlify-adapter-edge-paths/test.config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/eight-frogs-report.md
- packages/adapters/netlify/README.md
1302f70 to
1dcd70b
Compare
|
It did not reproduce locally: 12 isolated runs of the fixture, 6 isolated runs under CPU saturation, and 2 full-suite runs under saturation were all green. Since the fixture polls for 8s and never recovered — and the cold add step had already passed in the same run — this looks like permanent staleness rather than slowness, most likely a Rather than change Generated by Claude Code |
2f2dbd0 to
3ddb1d7
Compare
The default edge entry matched only dot-free paths (pattern ^[^.]*$), so any route whose URL contained a dot -- a catch-all serving report.2024.pdf, a $handle segment holding jane.doe, a handler emitting .xml -- never reached the router and 404'd from the static CDN. The entry now runs on every path: GET/HEAD requests first let Netlify serve a matching static file (mirroring the functions adapter's preferStatic and the static-first order of the dev and node targets), then fall back to the app's router, which also lets the app's +404 page apply to unmatched paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V888Mxok4otffCprmvyiKU
…er unit test The adapter package had no test setup of its own; the run package's fixture harness is where adapter behavior is exercised. The fixture builds with the netlify edge adapter and previews the built edge function through a local stand-in for the platform: the function runs only for the paths its exported config selects, and its next() answers from the published static dir. That covers the declaration itself, which a stubbed unit test could not -- reverting the entry fails the fixture's preview on the dotted path while dev still passes, which is exactly the divergence the bug described. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V888Mxok4otffCprmvyiKU
Asking Netlify for a static file before routing cost every request an extra pass through the platform. Only GET/HEAD requests can be answered by a file, and a request whose path carries no dot is almost never one, so the lookup now runs up front -- raced against the router, so it adds no latency -- only for dotted paths, and otherwise waits until the router has no answer. A published file still wins wherever one exists, including extensionless files the path gives no hint about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V888Mxok4otffCprmvyiKU
A handler that declines (throw null) a POST or other non-GET/HEAD request dead-ended in a synthetic 404 instead of reaching the rest of the platform chain, which the pre-fix entry forwarded to. The fallback now asks the platform whenever the router has no response, keeping the 404-triggered retry limited to GET/HEAD. Also anchors the fixture's static-body assertions with regexes so Windows CRLF checkouts compare equal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V888Mxok4otffCprmvyiKU
The default export leads the file and helper detail follows, and comments are trimmed to two lines, matching the conventions AGENTS.md now records. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V888Mxok4otffCprmvyiKU
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V888Mxok4otffCprmvyiKU
The edge entry asked the platform for a static file before routing, so every request paid an extra pass through the platform and the handler carried the static-vs-route precedence itself. The adapter now converts the route table into Netlify path declarations, so the function only runs for paths the app actually serves and everything else -- published files included -- stays with the platform's static handling. The handler goes back to routing and falling through on a decline. Route paths reach the entry through a virtual module the adapter's plugin serves. The CLI and the build each evaluate the vite config, so the adapter instance contributing that plugin is not the one receiving routesGenerated; the declaration is passed between them keyed by root. Two behavior notes, both documented in the README: a catch-all route now claims published files under it (a handler can throw null to hand one back), and unmatched paths get Netlify's 404 rather than the app's +404 page. The build's assets directory is excluded so a catch-all cannot claim it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V888Mxok4otffCprmvyiKU
The plugin side is handed the CLI's cwd and the adapter side a path the plugin already normalized. Those are the same string everywhere but windows, where the lookup missed and the entry shipped an empty declaration -- so no path ran the edge function and every route 404'd. Both sides now derive the key the same way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V888Mxok4otffCprmvyiKU
Declaring every route as a Netlify path declaration meant generating them at build time, handing them to the entry through a virtual module, and bridging two adapter instances that the CLI and the build create separately -- which is also where the windows path-separator bug came from. Running on every path and asking the platform only once the router has no answer needs none of that: the entry keeps routing first, and a 404 (or a declined request) is what hands the path to the platform. The router answers unmatched paths with a truthy 404 response, so a plain `||` fallthrough never reaches next() and every published file 404s -- the case the old dot pattern was really guarding. Falling through on the status instead serves those files, and keeping the app's response when the platform also has nothing preserves its +404 page. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V888Mxok4otffCprmvyiKU
Deferred by the maintainer while fixing the Netlify edge adapter. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V888Mxok4otffCprmvyiKU
93337fc to
75f0500
Compare
|
Status after the rebase onto Generated by Claude Code |
PR #244 closed without merging, taking two entries that were recorded on its branch and never rejected: the dev-add-layout CI flake (six observed occurrences with analysis) and the fetch void/404 routing design the maintainer deferred. Restored against main's current state; the netlify dotted-paths entry itself never left main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V888Mxok4otffCprmvyiKU
Description
@marko/run-adapter-netlify: the default edge entry declaredconfig = { pattern: "^[^.]*$" }, so Netlify only invoked the function for URLs with zero dots. Any dotted URL bypassed the app entirely and 404'd from the static CDN — a catch-all servingreport.2024.pdf, a$handlesegment holdingjane.doe, a handler emitting.xml— even though the router matches those paths in dev, on the node adapter, and on the functions adapter.That pattern was a crude "is this a file?" heuristic, standing in for the fact that an edge function runs before Netlify's static handling. Instead of heuristics, the adapter now declares the app's actual routes:
{ "path": ["/", "/reports/*"], "excludedPath": ["/assets/*"] }routesGeneratedconverts the route table into Netlify path declarations ($name→:name,$$rest→*, plus a trailing-slash variant per route so thetrailingSlashesoption still sees those requests). The function then runs only for paths the app serves, and everything else — published files included — stays with Netlify's own static handling. The handler is back to its original one-liner: route, and fall through on a decline.Route paths reach the entry through a virtual module served by the adapter's Vite plugin. The CLI evaluates the Vite config to resolve the adapter and the build evaluates it again, so the instance contributing that plugin is not the one receiving
routesGenerated; the declaration is passed between them keyed by root, and the loader awaits athis.loadof the router so the route build has definitely run.Motivation and Context
Dynamic routes containing dots silently 404 in production on Netlify edge while working on every other target.
Two behavior consequences, both documented in the adapter README:
/reports/*matches/reports/pinned.txt, and the function runs before static handling, so the route wins. A handler canthrow nullto hand such a path back to Netlify — the fixture covers this. The build's assets directory is excluded outright, since a root catch-all would otherwise shadow/assets/*and break hydration; that is the case the old dot heuristic was quietly protecting.+404page, because they never reach the app. Publishing a404.htmlcustomizes that.Testing: the edge preview needs the Netlify CLI and Deno, so the
netlify-adapter-edge-pathsfixture serves the builtdist/index.mjsthrough a local stand-in for the platform (same custom-adapter pattern as theadapter-is-entryfixture) that honorspath/pattern/excludedPaththe way Netlify does. It covers dotted catch-all routing, a published file outside any route, an extensionless published file, the excluded assets path, POST routing, and the decline-to-platform fallthrough. Reverting the entry fails the preview run while dev still passes — the same divergence the bug describes.Screenshots (if appropriate):
Checklist: