Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions website/flags/audience.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export interface Route {
* @description higher priority means that this route will be used in favor of other routes with less or none priority
*/
highPriority?: boolean;
supportedExtensions?: string[];
}
/**
* @title Routes
Expand Down
1 change: 1 addition & 0 deletions website/handlers/fresh.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ export default function Fresh(
const timing = appContext?.monitoring?.timings?.start?.("load-data");
let didFinish = false;
const url = new URL(req.url);

const startedAt = Date.now();
const asJson = url.searchParams.get("asJson");
const delayFromProps = appContext.firstByteThresholdMS ? 1 : 0;
Expand Down
28 changes: 24 additions & 4 deletions website/handlers/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
RouteContext,
} from "@deco/deco";
import { isAwaitable } from "@deco/deco/utils";
import { extname } from "@std/path/extname";
import { weakcache } from "../../utils/weakcache.ts";
import { Route, Routes } from "../flags/audience.ts";
import { isFreshCtx } from "../handlers/fresh.ts";
Expand All @@ -19,6 +20,7 @@ export interface SelectionConfig {
}
interface MaybePriorityHandler {
func: Resolvable<Handler>;
supportedExtensions?: string[];
highPriority?: boolean;
}
const HIGH_PRIORITY_ROUTE_RANK_BASE_VALUE = 1000;
Expand All @@ -38,6 +40,7 @@ const rankRoute = (pattern: string): number =>
return acc + 3;
}, 0);
const urlPatternCache: Record<string, URLPattern> = {};

export const router = (
routes: Route[],
hrefRoutes: Record<string, Resolvable<Handler>> = {},
Expand Down Expand Up @@ -77,7 +80,14 @@ export const router = (
if (handler) {
return route(handler, `${url.pathname}${url.search || ""}`);
}
for (const { pathTemplate: routePath, handler } of routes) {
for (
const { pathTemplate: routePath, handler, supportedExtensions } of routes
) {
// Skip catch-all routes for paths with file extensions
const ext = extname(url.pathname).slice(1);
if (ext && !supportedExtensions?.includes(ext)) {
continue;
}
const pattern = urlPatternCache[routePath] ??= (() => {
let url;
if (URL.canParse(routePath)) {
Expand Down Expand Up @@ -110,12 +120,21 @@ export const buildRoutes = (audiences: Routes[]): [
// We should tackle this problem elsewhere
// check if the audience matches with the given context considering the `isMatch` provided by the cookies.
for (const audience of audiences.filter(Boolean).flat()) {
const { pathTemplate, isHref, highPriority, handler: { value: handler } } =
audience;
const {
pathTemplate,
isHref,
highPriority,
handler: { value: handler },
supportedExtensions,
} = audience;
if (isHref) {
hrefRoutes[pathTemplate] = handler;
} else {
routeMap[pathTemplate] = { func: handler, highPriority };
routeMap[pathTemplate] = {
func: handler,
highPriority,
supportedExtensions,
};
}
}
return [routeMap, hrefRoutes];
Expand Down Expand Up @@ -155,6 +174,7 @@ const prepareRoutes = (audiences: Routes[], ctx: AppContext) => {
routes: builtRoutes.map((route) => ({
pathTemplate: route[0],
handler: { value: route[1].func },
supportedExtensions: route[1].supportedExtensions,
})),
hrefRoutes,
};
Expand Down
11 changes: 9 additions & 2 deletions website/loaders/pages.ts
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 @@ -21,6 +21,7 @@ async function getAllPages(ctx: AppContext): Promise<Route[]> {
}
routes.push({
pathTemplate,
supportedExtensions: props.supportedExtensions,
handler: {
value: {
__resolveType: "website/handlers/fresh.ts",
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
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

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

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