Fix static plugin serving nothing when urlPath is configured#1584
Conversation
Two stacked defects: the Scope server proxy passed the raw config value
('assets') as the route mount, which can never match leading-slash
pathnames; and the routing chain strips the mount prefix from
req.pathname while the static maps were keyed by the full entry URL
path. normalizeUrlPath now ensures a leading slash, the Scope proxy
resolves urlPath via resolveBaseURLPath (same normalization as the
entry pipeline), and static.ts keys its maps relative to the mount
base captured at registration. urlPath runtime changes now request a
restart (the route mount cannot be re-registered live), and the mount
root disambiguates the no-slash form via the unstripped request so
the root index 301s to the trailing-slash path.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request resolves issue #1583 where the static plugin failed to serve files when a custom urlPath was configured. It normalizes slash-less paths by ensuring a leading slash, keys static file maps relative to the base URL path, and requests a server restart if urlPath changes. It also handles redirects to trailing-slash paths and adds comprehensive tests. The review feedback correctly identifies that query parameters are lost during both the mount root and directory redirects, and provides actionable suggestions to preserve them in the Location header.
|
Reviewed; no blockers found. |
…oot disambiguation Codex review: BunRequest sets _nodeRequest to null, so the mount-root redirect silently never fired on Bun. stripPrefix now exposes the unstripped pathname as an explicit property instead of handlers reaching into runtime internals. Verified on both runtimes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Gemini review: both 301 Location headers dropped the original query string. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kriszyp
left a comment
There was a problem hiding this comment.
Reviewed via Claude review-queue. Real fix for the completely-broken urlPath static-serving feature — the three-part fix (mount normalization, resolveBaseURLPath resolution, and mount-relative map keys) is coherent and well-tested end to end, including the mount-root-redirect edge case. (Gemini flagged four "blockers" here that don't hold up on closer read — TypeStrip .ts imports are this repo's existing convention, requestRestart() is a real instance method, and resolveBaseURLPath already guards its inputs.)
One perf nit worth a quick follow-up: query-string parsing in server/static.ts (~line 121) runs unconditionally on the index-serving path, even in the common case where indexEntries resolves immediately to a 200 with no redirect. Since static serving is squarely in the request hot path, worth hoisting that queryIndex/query computation inside the two redirect branches so the default-index case stays allocation-free.
Approving — the core fix is solid, this is a follow-up not a blocker.
Keep the common index-serve path (200, no redirect) allocation-free by computing queryIndex/query lazily inside each 301 branch instead of unconditionally on every index request. Static serving is in the request hot path. Follow-up to review on #1584. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
no-restricted-imports bans the node:assert/strict subpath; node:assert exports the same strictEqual/ok. Unblocks runLinter on #1584. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Fix static plugin serving nothing when urlPath is configured (#1583) Two stacked defects: the Scope server proxy passed the raw config value ('assets') as the route mount, which can never match leading-slash pathnames; and the routing chain strips the mount prefix from req.pathname while the static maps were keyed by the full entry URL path. normalizeUrlPath now ensures a leading slash, the Scope proxy resolves urlPath via resolveBaseURLPath (same normalization as the entry pipeline), and static.ts keys its maps relative to the mount base captured at registration. urlPath runtime changes now request a restart (the route mount cannot be re-registered live), and the mount root disambiguates the no-slash form via the unstripped request so the root index 301s to the trailing-slash path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Expose originalPathname from stripPrefix for runtime-agnostic mount-root disambiguation Codex review: BunRequest sets _nodeRequest to null, so the mount-root redirect silently never fired on Bun. stripPrefix now exposes the unstripped pathname as an explicit property instead of handlers reaching into runtime internals. Verified on both runtimes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Preserve query strings on static trailing-slash redirects Gemini review: both 301 Location headers dropped the original query string. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * perf(static): hoist query-string parse into redirect branches Keep the common index-serve path (200, no redirect) allocation-free by computing queryIndex/query lazily inside each 301 branch instead of unconditionally on every index request. Static serving is in the request hot path. Follow-up to review on #1584. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(lint): import from node:assert, not node:assert/strict no-restricted-imports bans the node:assert/strict subpath; node:assert exports the same strictEqual/ok. Unblocks runLinter on #1584. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Kyle Bernhardy <kyle.bernhardy@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Kris Zyp <kris@harperdb.io> Co-authored-by: Kris Zyp <kriszyp@gmail.com> (cherry picked from commit 5e6d9f1)
Patch cherry-pick: conflictCherry-pick onto The conflict markers are committed on branch |
Fixes #1583.
Summary
With
static: { files: 'web/**', urlPath: 'assets' }, no file was served at any path. Two stacked defects:assets) as the route mount, andmatchesRoutecompares mounts against leading-slash pathnames — the handler was never invoked for any request.stripPrefixhands handlers a pathname with the mount prefix removed, whileserver/static.tskeyed its file maps by the fullentry.urlPath(which includes the base) — every lookup missed.Fix, in three parts:
normalizeUrlPathnow ensures a leading slash (closing the dead-mount trap for any registrant); the Scope proxy resolvesurlPaththroughresolveBaseURLPath— the same normalization the entry pipeline uses, including'.'/'./x'semantics (verified both sides use the same plugin name); andstatic.tskeys its maps relative to the mount base. Two review-driven behavior decisions on top:urlPathruntime changes now request a restart instead of pretending to hot-apply: the HTTP route mount is registered once at load and cannot be re-registered live, so recomputing keys against a changed option would drift from the actual mount./assetsand/assets/to/, so the handler consults the unstripped pathname (exposed bystripPrefixasoriginalPathname, runtime-agnostic — Bun sets_nodeRequestto null) and 301s/assets→/assets/(relative links on a root index page would otherwise break). Falls back to serving when the property is unavailable.Where to look
scope.server.httpwith a configurlPathnow mounts at the resolved base. Previously slash-less values produced dead mounts (nothing matched — the reported bug), so this activates broken configs rather than changing working ones; expliciturlPathin registration options still overrides via...options(MCP'smountPathis unaffected).originalPathnameproperty onstripPrefix-wrapped requests is a small contract addition — it is the only way the original pathname survives the strip, and static's mount-root redirect (and any future consumer) depends on it.Testing
normalizeUrlPathleading-slash cases and slash-lessmatchesRoute(unitTests/server/middlewareChain.test.js, 59 passing).integrationTests/components/static-urlpath.test.ts, on the issue's exact fixture): file under mount, no root leak, directory index, directory 301 with prefixed Location, root index at/assets/, and/assets→/assets/301. Verified 3 of the core 4 fail with the fix stashed.Reported on 5.1.15 while dogfooding the docs-site replatform — good v5.1 patch candidate once it bakes on main. No docs change needed: this restores the documented
urlPathbehavior (the restart-on-change nuance may deserve a line in the static-files docs if reviewers agree).Generated by an LLM (Claude Fable 5), cross-reviewed by Codex (hot-reload drift + mount-root redirect findings addressed) and Gemini (approved; flagged the mount-root case as worth noting, now fixed).