feat: enhance router to skip catch-all routes for paths with file ext… #1487
Conversation
…ensions * Added a utility function to check for file extensions in the pathname. * Updated the router logic to skip catch-all routes when a file extension is detected.
Tagging OptionsShould a new tag be published when this PR is merged?
|
WalkthroughPropagates a new optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant RouteHandler
Client->>Router: HTTP request (path)
Router->>Router: hasFileExtension(path)?
alt path has extension
Router->>Router: extract extension (e.g. ".png")
Router->>Router: for each candidate route: check candidate.supportedExtensions
alt candidate supports extension
Router->>RouteHandler: dispatch to matched route
RouteHandler-->>Client: response
else candidate does NOT support extension
Router-->>Router: skip candidate / continue search
Router-->>Client: 404 if no match
end
else no extension
Router->>RouteHandler: dispatch to matched route
RouteHandler-->>Client: response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
website/handlers/router.ts (2)
41-44: ClarifyhasFileExtensionsemantics and consider reusing for route patternsThe helper is concise and does what you need (looks only at the final segment, ignores leading-dot names like
.well-known). Two small points you may want to make explicit in code or docs:
- Any last segment containing a dot (e.g.
/health.check) is treated as “file-like”, which changes routing behavior for such URLs; if you ever rely on dots in slugs that are not file extensions, this will now bypass catch-all routes.- If you later refine the catch‑all skipping logic to depend on whether a route pattern itself represents a “file-like” path (e.g.
/sitemap.xml*), you can factor this helper to operate on arbitrary path strings (e.g. take apathnameargument derived fromnew URL(...).pathname) and reuse it instead of open-coding similar logic elsewhere.
85-88: Narrow the catch‑all skip condition to avoid hiding intentional “file-like” wildcard routesThe new condition:
if (routePath.endsWith("*") && hasFileExtension(url.pathname)) { continue; }does skip generic catch‑alls like
"/*"for asset‑looking URLs (e.g./image.png), which matches the PR goal. However, it also skips all*-terminated patterns when the request has an extension, including intentional file‑style routes such as:
/sitemap.xml*or/assets/:fileName*meant to handle/sitemap.xml,/sitemap.xml.gz, etc.Those routes will now be silently ignored for
*.xml,*.json, etc., even though they might be the most specific match.To reduce the risk of surprising behavior, consider tightening the condition along these lines:
- Only skip catch‑alls whose own pathname does not contain a dot in the last segment (i.e. patterns like
"/*"or"/:slug*"), so that patterns explicitly targeting file‑like paths (e.g./sitemap.xml*) continue to match.- Optionally precompute
const isFileLikeRequest = hasFileExtension(url.pathname);once before the loop and reuse it inside, to avoid recomputing per iteration.Conceptually:
const isFileLikeRequest = hasFileExtension(url.pathname); for (const { pathTemplate: routePath, handler } of routes) { const isCatchAll = routePath.endsWith("*"); const routeUrl = URL.canParse(routePath) ? new URL(routePath) : new URL(routePath, "http://localhost:8000"); const routeIsFileLike = hasFileExtension(routeUrl.pathname.replace(/\*$/, "")); if (isCatchAll && isFileLikeRequest && !routeIsFileLike) { continue; } // existing pattern logic... }Even if you don’t implement exactly this, some distinction between “generic catch‑alls” and “file‑specific catch‑alls” would make the behavior more predictable.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
website/handlers/fresh.ts(2 hunks)website/loaders/pages.ts(4 hunks)website/mod.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
website/loaders/pages.ts (2)
website/mod.ts (2)
AppContext(29-29)Props(91-162)website/flags/audience.ts (1)
Route(10-25)
🪛 GitHub Actions: ci
website/handlers/fresh.ts
[error] 117-133: Deno fmt --check failed: Found 1 not formatted file in 2081 files. Run 'deno fmt' to fix code style issues.
🔇 Additional comments (5)
website/handlers/fresh.ts (1)
23-27: LGTM!The
supportedExtensionsproperty is well-documented and appropriately optional for backward compatibility.website/loaders/pages.ts (4)
6-6: LGTM!The signature update correctly adds
propsparameter to enablesupportedExtensionspropagation.
30-30: Verify the propagation works correctly once fresh.ts is fixed.The propagation of
supportedExtensionsto the Fresh handler is correct. However, note that the default behavior infresh.tscurrently blocks all extensions when this isundefined, which needs to be fixed.
50-54: LGTM!The
supportedExtensionsproperty is well-documented and appropriately optional.
68-68: LGTM!The call site correctly passes
propstogetAllPagesto enable configuration propagation.
| includeScriptsToBody?: { | ||
| includes?: Script[]; | ||
| }; | ||
| pathsThatRequireSameReferer?: string[]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add documentation for the new property.
The pathsThatRequireSameReferer property lacks a description, making its purpose and usage unclear. Other properties in this interface include @description tags.
Apply this diff to add documentation:
+ /**
+ * @description Paths that require the same referer for A/B test routing
+ */
pathsThatRequireSameReferer?: string[];📝 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.
| pathsThatRequireSameReferer?: string[]; | |
| /** | |
| * @description Paths that require the same referer for A/B test routing | |
| */ | |
| pathsThatRequireSameReferer?: string[]; |
🤖 Prompt for AI Agents
In website/mod.ts around line 79, the interface property
pathsThatRequireSameReferer?: string[]; is missing a JSDoc/@description tag; add
a descriptive comment above this property explaining that it is an optional
array of path prefixes or exact paths which must be accessed with the same
Referer header as the origin (e.g., for CSRF or hotlink protection), describe
expected format (array of strings), behavior when omitted (no referer checks),
and any matching rules (prefix vs exact) consistent with the other documented
properties in the interface.
| pathsThatRequireSameReferer: [ | ||
| ...(abTesting.pathsThatRequireSameReferer ?? []), | ||
| ], |
There was a problem hiding this comment.
Verify this change aligns with PR objectives.
The PR title indicates it's about "skip catch-all routes for paths with file ext…" but this change relates to referer-based path matching for A/B testing. This appears unrelated to file extension handling.
🤖 Prompt for AI Agents
In website/mod.ts around lines 212 to 214, the change to include
abTesting.pathsThatRequireSameReferer in pathsThatRequireSameReferer is
unrelated to the PR title about skipping catch-all routes for file extensions;
verify intent and either revert this change or move it to a dedicated PR:
confirm with the author whether referer-based A/B testing behavior was
intentionally modified for this branch, if not revert these lines to their
original state, if yes extract the change into a separate commit/PR with an
updated title/description and add/update tests and release notes to cover
referer matching behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
website/flags/audience.ts (1)
20-26: Document semantics ofsupportedExtensionsonRouteThe new
supportedExtensions?: string[]field is type-safe and fits the routing changes, but its expected format isn’t obvious (e.g.,"js"vs".js", case sensitivity, whether it only applies to non‑isHrefroutes). A brief JSDoc here would help avoid config mistakes and keep behavior consistent across loaders and manual route definitions.website/handlers/router.ts (1)
20-24: Propagation ofsupportedExtensionsthrough router pipeline looks correct
MaybePriorityHandlernow carriessupportedExtensions, andbuildRoutes/prepareRoutescorrectly thread it through from audience routes back into theroutesarray used byrouter(). The types line up withRoute.supportedExtensions, and href routes remain unaffected.One small clean-up you might consider is computing the extension once per request (outside the loop) and passing both
hasExtandextinto the loop, instead of recomputingurl.pathname.split(".")for each candidate, but that’s a micro‑optimization and fully optional.Also applies to: 127-133, 137-141, 178-182
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
website/flags/audience.ts(1 hunks)website/handlers/fresh.ts(1 hunks)website/handlers/router.ts(5 hunks)website/loaders/pages.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- website/handlers/fresh.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- website/loaders/pages.ts
website/handlers/router.ts
Outdated
| const hasFileExtension = (pathname: string): boolean => { | ||
| const lastSegment = pathname.split("/").pop() || ""; | ||
| return lastSegment.includes(".") && !lastSegment.startsWith("."); | ||
| }; |
There was a problem hiding this comment.
File-extension skip applies to all pattern routes, not just catch‑alls
The comment says “Skip catch-all routes for paths with file extensions”, but the condition:
if (
hasFileExtension(url.pathname) &&
!supportedExtensions?.includes(url.pathname.split(".").pop() || "")
) {
continue;
}is evaluated for every non‑isHref route, regardless of whether pathTemplate is catch‑all or a concrete path.
This means:
- Any pattern-based route (e.g.
/sitemap.xml,/feed.rss) that relied on URLPattern matching and does not definesupportedExtensionswill now be skipped for those extension paths, changing behavior from matching → 404. - Only routes that explicitly set
supportedExtensionscontaining the extension will be considered, which is stricter than “just skip catch‑alls”.
If the intent is truly to avoid only wildcard/catch‑all taking over asset-like URLs, consider tightening the condition to check for catch‑all patterns (e.g., routePath.includes("*") or similar), or introducing an explicit opt‑in flag rather than applying this to all routes.
For example:
- for (
- const { pathTemplate: routePath, handler, supportedExtensions } of routes
- ) {
+ for (
+ const { pathTemplate: routePath, handler, supportedExtensions } of routes
+ ) {
// Skip catch-all routes for paths with file extensions
- if (
- hasFileExtension(url.pathname) &&
- !supportedExtensions?.includes(url.pathname.split(".").pop() || "")
- ) {
+ const isCatchAll = routePath.includes("*");
+ const lastSegment = url.pathname.split("/").pop() || "";
+ const ext = lastSegment.includes(".")
+ ? lastSegment.split(".").pop() || ""
+ : "";
+ if (
+ isCatchAll &&
+ ext &&
+ !supportedExtensions?.includes(ext)
+ ) {
continue;
}This preserves existing concrete extension routes while still protecting catch‑alls from grabbing asset-like URLs.
Also applies to: 85-94
🤖 Prompt for AI Agents
In website/handlers/router.ts around lines 42-45 (also apply same change at
85-94): the current file-extension check runs for every pattern route and
wrongly skips concrete extension routes; restrict this guard so it only applies
to catch-all/wildcard pattern routes (e.g., detect patterns with "*" or whatever
identifies a catch-all in this codebase) or add an explicit route opt-in flag,
then only continue when hasFileExtension(url.pathname) && routeIsCatchAll &&
!supportedExtensions?.includes(ext) — leaving concrete extension routes
unaffected.
Summary by CodeRabbit
New Features
Chore
✏️ Tip: You can customize this high-level summary in your review settings.