Skip to content
Merged
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
269 changes: 266 additions & 3 deletions packages/vinext/src/routing/app-route-graph.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/
import path from "node:path";
import fs from "node:fs";
import { createHash } from "node:crypto";
import { compareRoutes, decodeRouteSegment } from "./utils.js";
import { scanWithExtensions, type ValidFileMatcher } from "./file-matcher.js";
import { validateRoutePatterns } from "./route-validation.js";
Expand Down Expand Up @@ -151,6 +152,7 @@ export type AppRouteSemanticIds = {
route: string;
page: string | null;
routeHandler: string | null;
rootBoundary: RootBoundaryId | null;
layouts: readonly string[];
templates: readonly string[];
/**
Expand All @@ -164,9 +166,81 @@ export type AppRouteGraphParallelSlot = ParallelSlot & {
id: string;
};

export type AppRouteGraphRoute = Omit<AppRoute, "ids" | "parallelSlots"> & {
export type AppRouteGraphRoute = Omit<AppRoute, "ids" | "parallelSlots" | "rootParamNames"> & {
ids: AppRouteSemanticIds;
parallelSlots: AppRouteGraphParallelSlot[];
rootParamNames: string[];
};

type Flavor<T, Brand extends string> = T & { readonly __flavor?: Brand };

export type GraphVersion = Flavor<string, "GraphVersion">;
export type RootBoundaryId = Flavor<string, "RootBoundaryId">;

export type RouteManifestRoute = {
id: string;
pattern: string;
patternParts: readonly string[];
isDynamic: boolean;
paramNames: readonly string[];
rootParamNames: readonly string[];
rootBoundaryId: RootBoundaryId | null;
pageId: string | null;
routeHandlerId: string | null;
layoutIds: readonly string[];
templateIds: readonly string[];
slotIds: readonly string[];
};

export type RouteManifestPage = {
id: string;
routeId: string;
pattern: string;
};

export type RouteManifestRouteHandler = {
id: string;
routeId: string;
pattern: string;
};

export type RouteManifestLayout = {
id: string;
treePath: string;
rootBoundaryId: RootBoundaryId | null;
};

export type RouteManifestTemplate = {
id: string;
treePath: string;
rootBoundaryId: RootBoundaryId | null;
};

export type RouteManifestSlot = {
id: string;
key: string;
name: string;
};

export type RouteManifestRootBoundary = {
id: RootBoundaryId;
layoutId: string;
treePath: string;
};

export type StaticSegmentGraph = {
routes: ReadonlyMap<string, RouteManifestRoute>;
pages: ReadonlyMap<string, RouteManifestPage>;
routeHandlers: ReadonlyMap<string, RouteManifestRouteHandler>;
layouts: ReadonlyMap<string, RouteManifestLayout>;
templates: ReadonlyMap<string, RouteManifestTemplate>;
slots: ReadonlyMap<string, RouteManifestSlot>;
rootBoundaries: ReadonlyMap<RootBoundaryId, RouteManifestRootBoundary>;
};

export type RouteManifest = {
graphVersion: GraphVersion;
segmentGraph: StaticSegmentGraph;
};

function createAppRouteGraphRouteId(pattern: string): string {
Expand All @@ -193,10 +267,184 @@ function createAppRouteGraphSlotId(slotName: string, ownerTreePath: string): str
return `slot:${slotName}:${ownerTreePath}`;
}

function createAppRouteGraphRootBoundaryId(treePath: string): RootBoundaryId {
return `root-boundary:${treePath}`;
}

function compareStableStrings(left: string, right: string): number {
if (left < right) return -1;
if (left > right) return 1;
return 0;
}

function sortedMapValues<T>(map: ReadonlyMap<string, T>): T[] {
return Array.from(map.entries())
.sort(([left], [right]) => compareStableStrings(left, right))
.map(([, value]) => value);
}

function createRouteManifest(routes: readonly AppRouteGraphRoute[]): RouteManifest {
const segmentGraph = createStaticSegmentGraph(routes);

return {
graphVersion: createRouteManifestGraphVersion(segmentGraph),
segmentGraph,
};
}

function createStaticSegmentGraph(routes: readonly AppRouteGraphRoute[]): StaticSegmentGraph {
const routeEntries = new Map<string, RouteManifestRoute>();
const pages = new Map<string, RouteManifestPage>();
const routeHandlers = new Map<string, RouteManifestRouteHandler>();
const layouts = new Map<string, RouteManifestLayout>();
const templates = new Map<string, RouteManifestTemplate>();
const slots = new Map<string, RouteManifestSlot>();
const rootBoundaries = new Map<RootBoundaryId, RouteManifestRootBoundary>();

for (const route of routes) {
routeEntries.set(route.ids.route, {
id: route.ids.route,
pattern: route.pattern,
patternParts: [...route.patternParts],
isDynamic: route.isDynamic,
paramNames: [...route.params],
rootParamNames: [...route.rootParamNames],
rootBoundaryId: route.ids.rootBoundary,
pageId: route.ids.page,
routeHandlerId: route.ids.routeHandler,
layoutIds: [...route.ids.layouts],
templateIds: [...route.ids.templates],
slotIds: route.parallelSlots.map((slot) => slot.id).sort(compareStableStrings),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

slot.id here is typed as string | undefined on the base ParallelSlot type (the id field is id?: string), but on AppRouteGraphParallelSlot it's required (id: string). The route.parallelSlots array is typed as AppRouteGraphParallelSlot[] so this is safe — but worth noting that if a code path ever passes a bare ParallelSlot with a missing id, the .sort() would silently sort "undefined" strings.

The type system prevents this today. Just flagging the narrowing dependency.

});

if (route.ids.page) {
pages.set(route.ids.page, {
id: route.ids.page,
routeId: route.ids.route,
pattern: route.pattern,
});
}

if (route.ids.routeHandler) {
routeHandlers.set(route.ids.routeHandler, {
id: route.ids.routeHandler,
routeId: route.ids.route,
pattern: route.pattern,
});
}

for (const [index, layoutId] of route.ids.layouts.entries()) {
const treePosition = route.layoutTreePositions[index];
assertRouteManifestTreePosition("layout", route, layoutId, treePosition);

const treePath = createAppRouteGraphTreePath(route.routeSegments, treePosition);
const existingLayout = layouts.get(layoutId);
if (existingLayout) {
assertRouteManifestRootBoundary("layout", route, layoutId, existingLayout.rootBoundaryId);
}
layouts.set(layoutId, {
id: layoutId,
treePath,
rootBoundaryId: route.ids.rootBoundary,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shared intermediate layouts get last-writer-wins rootBoundaryId.

When two routes share a layout (e.g. layout:/dashboard used by both /dashboard/a under root boundary A and /dashboard/b under root boundary B, if such a topology were possible through route groups), the second route to iterate will silently overwrite the first's rootBoundaryId on the shared layout entry.

In practice today this is likely fine because layouts within the same tree path share the same root boundary. But it's an implicit invariant the code doesn't assert. Consider either:

  • Adding an assertion that if layouts.has(layoutId), the existing rootBoundaryId matches the new one, or
  • Adding a comment explaining why last-writer-wins is safe here.

This would make the invariant explicit and catch future regressions if the route graph shape changes.

});

if (index === 0 && route.ids.rootBoundary) {
rootBoundaries.set(route.ids.rootBoundary, {
id: route.ids.rootBoundary,
layoutId,
treePath,
});
}
}

for (const [index, templateId] of route.ids.templates.entries()) {
const treePosition = route.templateTreePositions?.[index];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: route.templateTreePositions is accessed with optional chaining (?.) here, but route.layoutTreePositions at line 336 is not. Looking at the AppRoute type, templateTreePositions is typed as number[] | undefined (optional), while layoutTreePositions is number[] (required). The optional chaining is correct, but this asymmetry between the two loops is easy to miss — a reader might wonder why one needs ?. and the other doesn't.

The type system enforces this correctly so it's not a bug, just noting the visual inconsistency.

assertRouteManifestTreePosition("template", route, templateId, treePosition);

const existingTemplate = templates.get(templateId);
if (existingTemplate) {
assertRouteManifestRootBoundary(
"template",
route,
templateId,
existingTemplate.rootBoundaryId,
);
}
templates.set(templateId, {
id: templateId,
treePath: createAppRouteGraphTreePath(route.routeSegments, treePosition),
rootBoundaryId: route.ids.rootBoundary,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same last-writer-wins observation applies to templates -- if two routes share a template id but somehow have different rootBoundaryId, the second route silently wins. Same mitigation applies as for layouts.

});
}

// Slots are boundary-agnostic in this minimal read model; unlike layouts
// and templates, they do not carry rootBoundaryId facts to guard.
for (const slot of route.parallelSlots) {
slots.set(slot.id, {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Slots don't get the same assertRouteManifestRootBoundary dedup check that layouts and templates get. A slot can appear on multiple routes (inherited parallel slots), but the set here silently last-writer-wins on the slot entry.

This is safe because RouteManifestSlot doesn't carry rootBoundaryId — it only has id, key, name, all of which are the same regardless of which route contributes the slot. But it's worth noting the asymmetry: layouts/templates get an assertion guard because they carry rootBoundaryId, while slots don't need one because they're boundary-agnostic. A brief comment would help readers understand why slots are treated differently.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in the latest push. The slot loop now documents the asymmetry: slots are boundary-agnostic in this minimal manifest, while layouts/templates carry rootBoundaryId facts and need the invariant guard.

id: slot.id,
key: slot.key,
name: slot.name,
});
}
}

return {
routes: routeEntries,
pages,
routeHandlers,
layouts,
templates,
slots,
rootBoundaries,
};
}

function assertRouteManifestTreePosition(
kind: "layout" | "template",
route: AppRouteGraphRoute,
id: string,
treePosition: number | undefined,
): asserts treePosition is number {
if (treePosition !== undefined) return;

throw new Error(
`[vinext] App route graph invariant violated: missing ${kind} tree position for ${id} on ${route.pattern}`,
);
}

function assertRouteManifestRootBoundary(
kind: "layout" | "template",
route: AppRouteGraphRoute,
id: string,
existingRootBoundaryId: RootBoundaryId | null,
): void {
if (existingRootBoundaryId === route.ids.rootBoundary) return;

throw new Error(
`[vinext] App route graph invariant violated: ${kind} ${id} is shared across root boundaries (${existingRootBoundaryId ?? "none"} and ${route.ids.rootBoundary ?? "none"}) on ${route.pattern}`,
);
}

function createRouteManifestGraphVersion(segmentGraph: StaticSegmentGraph): GraphVersion {
// The manifest hash is canonical only if top-level map keys are sorted and
// inner route arrays keep their own semantic order: layoutIds/templateIds in
// tree-position order, and slotIds in compareStableStrings order.
const stableShape = {
routes: sortedMapValues(segmentGraph.routes),
pages: sortedMapValues(segmentGraph.pages),
routeHandlers: sortedMapValues(segmentGraph.routeHandlers),
layouts: sortedMapValues(segmentGraph.layouts),
templates: sortedMapValues(segmentGraph.templates),
slots: sortedMapValues(segmentGraph.slots),
rootBoundaries: sortedMapValues(segmentGraph.rootBoundaries),
};
return `graph:${createHash("sha256").update(JSON.stringify(stableShape)).digest("hex")}`;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The graph: prefix means graphVersion values are 71 characters ("graph:" + 64 hex chars). This is fine, but the prefix creates a slight oddity: the Flavor-branded GraphVersion type suggests it's an opaque token, yet the format is publicly observable and tested against (/^graph:[a-f0-9]{64}$/). If Layer 4 ever needs to compare graph versions for ordering (not just equality), the graph: prefix would need to be stripped first.

Not blocking — just noting that the prefix is a minor commitment to a format that tests now assert on. If the version is meant to be opaque, consider whether the test should assert typeof manifest.graphVersion === 'string' && manifest.graphVersion.length > 0 instead of matching the internal format.

}

export async function buildAppRouteGraph(
appDir: string,
matcher: ValidFileMatcher,
): Promise<{ routes: AppRouteGraphRoute[] }> {
): Promise<{ routes: AppRouteGraphRoute[]; routeManifest: RouteManifest }> {
// Find all page.tsx and route.ts files, excluding @slot directories
// (slot pages are not standalone routes — they're rendered as props of their parent layout)
// and _private folders (Next.js convention for colocated non-route files).
Expand Down Expand Up @@ -262,7 +510,7 @@ export async function buildAppRouteGraph(
// Sort: static routes first, then dynamic, then catch-all
routes.sort(compareRoutes);

return { routes };
return { routes, routeManifest: createRouteManifest(routes) };
}

function hasParallelSlotDirectory(dir: string): boolean {
Expand Down Expand Up @@ -705,6 +953,20 @@ export function computeRootParamNames(
return names;
}

function resolveRootBoundaryId(
routeSegments: readonly string[],
layoutTreePositions: readonly number[],
): RootBoundaryId | null {
const rootLayoutPosition = layoutTreePositions[0];
if (rootLayoutPosition === undefined) return null;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The resolveRootBoundaryId function returns null when layoutTreePositions[0] is undefined, which handles layoutless routes correctly. However, computeRootParamNames at line 944 also guards on layoutTreePositions[0] but uses rootLayoutPosition == null || rootLayoutPosition <= 0 (loose equality, catching both null and undefined, plus a <= 0 guard). The difference in guard styles between these two sibling functions is slightly surprising — resolveRootBoundaryId doesn't guard against rootLayoutPosition <= 0.

Is position 0 a valid root layout position (root layout in the app directory itself)? Looking at computeLayoutTreePositions, position 0 means the layout is at appDir itself — the true root. So computeRootParamNames returns [] for position 0 (no segments before the root), which is correct. resolveRootBoundaryId would create root-boundary:/ for position 0, which is also correct. The asymmetry is fine on closer reading, but a brief comment on why <= 0 isn't needed here would help future readers.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor asymmetry with computeRootParamNames at line 946, which guards rootLayoutPosition == null || rootLayoutPosition <= 0. Here you only guard === undefined.

Both are correct: computeRootParamNames returns [] for position 0 because there are no segments before the root layout, while resolveRootBoundaryId correctly returns a root-boundary:/ for position 0 (the root layout at the app directory itself). The <= 0 guard in computeRootParamNames is specifically about "no segments to slice", not about validity.

A one-line comment here explaining why position 0 is valid (unlike in computeRootParamNames) would help readers who notice the asymmetry:

Suggested change
if (rootLayoutPosition === undefined) return null;
const rootLayoutPosition = layoutTreePositions[0];
if (rootLayoutPosition === undefined) return null;
// Position 0 means the root layout is at appDir itself — it still owns a
// real root boundary. (computeRootParamNames treats 0 differently because
// there are no segments before the root layout to extract param names from.)


// Position 0 is the app root layout and still owns a real root boundary.
// Only a missing layout position means the route is layoutless.
return createAppRouteGraphRootBoundaryId(
createAppRouteGraphTreePath(routeSegments, rootLayoutPosition),
);
}

function createAppRouteSemanticIds(input: {
pattern: string;
pagePath: string | null;
Expand All @@ -723,6 +985,7 @@ function createAppRouteSemanticIds(input: {
route: createAppRouteGraphRouteId(input.pattern),
page: input.pagePath ? createAppRouteGraphPageId(input.pattern) : null,
routeHandler: input.routePath ? createAppRouteGraphRouteHandlerId(input.pattern) : null,
rootBoundary: resolveRootBoundaryId(input.routeSegments, input.layoutTreePositions),
layouts: input.layoutTreePositions.map((treePosition) =>
createAppRouteGraphLayoutId(createAppRouteGraphTreePath(input.routeSegments, treePosition)),
),
Expand Down
44 changes: 35 additions & 9 deletions packages/vinext/src/routing/app-router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,39 +15,65 @@
*/
import { createValidFileMatcher, type ValidFileMatcher } from "./file-matcher.js";
import { createRouteTrieCache, matchRouteWithTrie } from "./route-matching.js";
import { buildAppRouteGraph, type AppRoute, type AppRouteGraphRoute } from "./app-route-graph.js";
import {
buildAppRouteGraph,
type AppRoute,
type AppRouteGraphRoute,
type RouteManifest,
} from "./app-route-graph.js";
export type { AppRoute } from "./app-route-graph.js";
export { computeRootParamNames } from "./app-route-graph.js";

type AppRouteGraph = {
routes: AppRouteGraphRoute[];
routeManifest: RouteManifest;
};

// Cache for app routes
let cachedRoutes: AppRouteGraphRoute[] | null = null;
let cachedGraph: AppRouteGraph | null = null;
let cachedAppDir: string | null = null;
let cachedPageExtensionsKey: string | null = null;

export function invalidateAppRouteCache(): void {
cachedRoutes = null;
cachedGraph = null;
cachedAppDir = null;
cachedPageExtensionsKey = null;
}

/**
* Scan the app/ directory and return a list of routes.
* Scan the app/ directory and return the route graph.
* TODO(#726): Layer 4 should consume this read model directly once the
* navigation planner owns route graph facts.
*
* @internal
*/
export async function appRouter(
export async function appRouteGraph(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

appRouteGraph is exported but has no production callers — only the test in routing.test.ts imports it. All production code still goes through appRouter(), which discards the manifest.

This is fine if it's intentional scaffolding for Layer 4. But since this is a public API surface (any downstream code can import it), I'd add a brief doc comment noting its purpose:

Suggested change
export async function appRouteGraph(
/**
* Scan the app/ directory and return the route graph.
* TODO(#726): Currently unused in production exposed for Layer 4 NavigationPlanner.
*/
export async function appRouteGraph(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in the latest push. appRouteGraph() now has a TODO(#726) doc comment making the Layer 4 planner handoff explicit while preserving appRouter() for current callers.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The TODO(#726) comment addresses the prior review feedback well. One remaining observation: appRouteGraph is exported from app-router.ts, but app-router.ts's re-exports in index.ts (line 11) only import appRouter and invalidateAppRouteCache. So appRouteGraph is reachable by deep-importing vinext/routing/app-router, but not from the package's public surface. This is probably intentional (internal API for Layer 4), but worth confirming — if it's meant to stay internal, a @internal JSDoc tag would signal that.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The @internal JSDoc tag and TODO(#726) comment are good. One thing worth confirming: since app-router.ts re-exports are pulled into index.ts (line 11), and index.ts only imports appRouter and invalidateAppRouteCache, the appRouteGraph function is reachable only via deep import (vinext/routing/app-router). The @internal tag signals this correctly.

If the package's exports map in package.json restricts deep imports, appRouteGraph would be truly unreachable from external consumers — which is the right posture for scaffolding. Worth verifying the exports map doesn't inadvertently expose ./routing/*.

appDir: string,
pageExtensions?: readonly string[],
matcher?: ValidFileMatcher,
): Promise<AppRouteGraphRoute[]> {
): Promise<AppRouteGraph> {
matcher ??= createValidFileMatcher(pageExtensions);
const pageExtensionsKey = JSON.stringify(matcher.extensions);
if (cachedRoutes && cachedAppDir === appDir && cachedPageExtensionsKey === pageExtensionsKey) {
return cachedRoutes;
if (cachedGraph && cachedAppDir === appDir && cachedPageExtensionsKey === pageExtensionsKey) {
return cachedGraph;
}

const graph = await buildAppRouteGraph(appDir, matcher);
cachedRoutes = graph.routes;
cachedGraph = graph;
cachedAppDir = appDir;
cachedPageExtensionsKey = pageExtensionsKey;
return graph;
}

/**
* Scan the app/ directory and return a list of routes.
*/
export async function appRouter(
appDir: string,
pageExtensions?: readonly string[],
matcher?: ValidFileMatcher,
): Promise<AppRouteGraphRoute[]> {
const graph = await appRouteGraph(appDir, pageExtensions, matcher);
return graph.routes;
}

Expand Down
Loading
Loading