Skip to content

feat: enhance router to skip catch-all routes for paths with file ext… #1487

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
guitavano wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat: enhance router to skip catch-all routes for paths with file ext… #1487

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
509dc73
feat: enhance router to skip catch-all routes for paths with file ext…
guitavano Nov 26, 2025
1312d98
add supportedExtensions
guitavano Nov 26, 2025
92a62bf
change tto router.ts
guitavano Nov 26, 2025
a5ef906
fmt
guitavano Nov 26, 2025
a77479e
use extname from std
guitavano Nov 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions website/handlers/fresh.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ type ConnInfo = Deno.ServeHandlerInfo;
*/
export interface FreshConfig {
page: Page;
/**
* @title Supported extensions
* @description Extensions that will be supported by the page.
*/
supportedExtensions?: string[];
}
export const isFreshCtx = <TState>(
ctx: ConnInfo | HandlerContext<unknown, TState>,
Expand Down Expand Up @@ -109,6 +114,21 @@ export default function Fresh(
const timing = appContext?.monitoring?.timings?.start?.("load-data");
let didFinish = false;
const url = new URL(req.url);

const lastPathname = url.pathname.split("/").pop();
const extension = lastPathname?.includes(".") ? lastPathname?.split(".").pop() : undefined;

// Only check if there's an extension
if (extension) {
const supportedExtensions = freshConfig.supportedExtensions ?? [];

// If no extensions are configured, block all extensions (default behavior)
// If extensions are configured, only allow those
if (!supportedExtensions.includes(extension)) {
return new Response(null, { status: 404 });
}
}

const startedAt = Date.now();
const asJson = url.searchParams.get("asJson");
const delayFromProps = appContext.firstByteThresholdMS ? 1 : 0;
Expand Down
11 changes: 9 additions & 2 deletions website/loaders/pages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { Route } from "../flags/audience.ts";
import { AppContext } from "../mod.ts";
import Page from "../pages/Page.tsx";

async function getAllPages(ctx: AppContext): Promise<Route[]> {
async function getAllPages(ctx: AppContext, props: Props): Promise<Route[]> {
const allPages = await ctx.get<
Record<string, Parameters<typeof Page>[0]>
>({
Expand All @@ -27,6 +27,7 @@ async function getAllPages(ctx: AppContext): Promise<Route[]> {
page: {
__resolveType: pageId,
},
supportedExtensions: props.supportedExtensions,
},
},
});
Expand All @@ -45,6 +46,12 @@ export interface Props {
* @description Deco routes that will ignore the previous rule. If the same route exists on other routes loader, the deco page will be used.
*/
alwaysVisiblePages?: SiteRoute[];

/**
* @title Supported extensions
* @description Extensions that will be supported by pages routes.
*/
supportedExtensions?: string[];
}

/**
Expand All @@ -58,7 +65,7 @@ export default async function Pages(
const allPages = await ctx.get<
Route[]
>({
func: () => getAllPages(ctx),
func: () => getAllPages(ctx, props),
__resolveType: "once",
});

Expand Down
6 changes: 4 additions & 2 deletions website/mod.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import {
type FnContext,
type Site,
} from "@deco/deco";
import { VTEX_PATHS_THAT_REQUIRES_SAME_REFERER } from "../vtex/loaders/proxy.ts";

declare global {
interface Window {
Expand Down Expand Up @@ -77,6 +76,7 @@ export interface AbTesting {
includeScriptsToBody?: {
includes?: Script[];
};
pathsThatRequireSameReferer?: string[];
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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.

}
/** @titleBy framework */
interface Fresh {
Expand Down Expand Up @@ -209,7 +209,9 @@ const getAbTestAudience = (abTesting: AbTesting) => {
includeScriptsToHead: abTesting.includeScriptsToHead,
includeScriptsToBody: abTesting.includeScriptsToBody,
replaces: abTesting.replaces,
pathsThatRequireSameReferer: [...VTEX_PATHS_THAT_REQUIRES_SAME_REFERER],
pathsThatRequireSameReferer: [
...(abTesting.pathsThatRequireSameReferer ?? []),
],
Comment on lines +212 to +214
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

},
};
if (abTesting.enabled) {
Expand Down
Loading