Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
14 changes: 12 additions & 2 deletions packages/vinext/src/server/app-browser-entry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,14 @@ type ServerActionResult = {
};

type NavigationKind = "navigate" | "traverse" | "refresh";

// Maps NavigationKind to the AppRouterAction type used by the reducer.
// "refresh" is intentionally treated as "navigate" (merge, preserve absent slots).
// Both call sites must stay in sync — update here if NavigationKind gains new values.
function toActionType(kind: NavigationKind): "navigate" | "traverse" {
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.

Good call extracting this into a named helper with the doc comment. This directly addresses the prior review feedback about the duplicated ternary and makes the "refresh""navigate" mapping explicit and centralized.

return kind === "traverse" ? "traverse" : "navigate";
}

type HistoryUpdateMode = "push" | "replace";
type VisitedResponseCacheEntry = {
params: Record<string, string | string[]>;
Expand Down Expand Up @@ -453,7 +461,7 @@ function dispatchBrowserTree(
elements: AppElements,
navigationSnapshot: ClientNavigationRenderSnapshot,
renderId: number,
actionType: "navigate" | "replace",
actionType: "navigate" | "replace" | "traverse",
routeId: string,
rootLayoutTreePath: string | null,
useTransitionMode: boolean,
Expand Down Expand Up @@ -484,7 +492,7 @@ async function renderNavigationPayload(
navId: number,
prePaintEffect: (() => void) | null = null,
useTransition = true,
actionType: "navigate" | "replace" = "navigate",
actionType: "navigate" | "replace" | "traverse" = "navigate",
): Promise<void> {
const renderId = ++nextNavigationRenderId;
const committed = new Promise<void>((resolve) => {
Expand Down Expand Up @@ -783,6 +791,7 @@ async function main(): Promise<void> {
navId,
createNavigationCommitEffect(href, historyUpdateMode, cachedParams),
isSameRoute,
toActionType(navigationKind),
);
} finally {
// Always clear _snapshotPending so the outer catch does not
Expand Down Expand Up @@ -871,6 +880,7 @@ async function main(): Promise<void> {
navId,
createNavigationCommitEffect(href, historyUpdateMode, navParams),
isSameRoute,
toActionType(navigationKind),
);
} finally {
// Always clear _snapshotPending after renderNavigationPayload returns or
Expand Down
9 changes: 5 additions & 4 deletions packages/vinext/src/server/app-browser-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export type AppRouterAction = {
renderId: number;
rootLayoutTreePath: string | null;
routeId: string;
type: "navigate" | "replace";
type: "navigate" | "replace" | "traverse";
};

export type PendingNavigationCommit = {
Expand All @@ -33,9 +33,10 @@ export type ClassifiedPendingNavigationCommit = {

export function routerReducer(state: AppRouterState, action: AppRouterAction): AppRouterState {
switch (action.type) {
case "traverse":
case "navigate":
return {
elements: mergeElements(state.elements, action.elements),
elements: mergeElements(state.elements, action.elements, action.type === "traverse"),
navigationSnapshot: action.navigationSnapshot,
renderId: action.renderId,
rootLayoutTreePath: action.rootLayoutTreePath,
Expand Down Expand Up @@ -92,7 +93,7 @@ export async function createPendingNavigationCommit(options: {
nextElements: Promise<AppElements>;
navigationSnapshot: ClientNavigationRenderSnapshot;
renderId: number;
type: "navigate" | "replace";
type: "navigate" | "replace" | "traverse";
}): Promise<PendingNavigationCommit> {
const elements = await options.nextElements;
const metadata = readAppElementsMetadata(elements);
Expand All @@ -118,7 +119,7 @@ export async function resolveAndClassifyNavigationCommit(options: {
nextElements: Promise<AppElements>;
renderId: number;
startedNavigationId: number;
type: "navigate" | "replace";
type: "navigate" | "replace" | "traverse";
}): Promise<ClassifiedPendingNavigationCommit> {
const pending = await createPendingNavigationCommit({
currentState: options.currentState,
Expand Down
18 changes: 17 additions & 1 deletion packages/vinext/src/shims/slot.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,11 @@ export const ParallelSlotsContext = React.createContext<Readonly<
Record<string, React.ReactNode>
> | null>(null);

export function mergeElements(prev: AppElements, next: AppElements): AppElements {
export function mergeElements(
prev: AppElements,
next: AppElements,
clearAbsentSlots = false,
): AppElements {
const merged: Record<string, AppElementValue> = { ...prev, ...next };
// On soft navigation, unmatched parallel slots preserve their previous subtree
// instead of firing notFound(). Only hard navigation (full page load) should 404.
Expand All @@ -32,6 +36,18 @@ export function mergeElements(prev: AppElements, next: AppElements): AppElements
merged[key] = prev[key];
}
}
// On traversal (browser back/forward), the server renders the full destination
// route tree. A slot absent from next means the destination route tree does not
// include it, so clear it rather than keeping the stale prev value. This only
// runs for traversals because soft forward navigations may omit parent layout
// slots that should be preserved.
if (clearAbsentSlots) {
for (const key of Object.keys(merged)) {
if (key.startsWith("slot:") && !Object.hasOwn(next, key)) {
delete merged[key];
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 (non-blocking): Iterating Object.keys(merged) and deleting in-place is fine here since the delete doesn't affect the already-captured keys array, but it's worth noting this is safe specifically because Object.keys() returns a snapshot. If this ever moved to a for...in loop it would be subtly broken. The current form is correct.

}
}
}
return merged;
}

Expand Down
40 changes: 40 additions & 0 deletions tests/app-browser-entry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,46 @@ describe("app browser entry state helpers", () => {
expect(shouldHardNavigate(null, "/")).toBe(false);
expect(shouldHardNavigate("/", null)).toBe(false);
});

it("clears stale parallel slots on traverse", () => {
const state = createState({
elements: createResolvedElements("route:/feed", "/", {
"slot:modal:/feed": React.createElement("div", null, "modal"),
}),
});
const nextElements = createResolvedElements("route:/feed", "/");

const nextState = routerReducer(state, {
elements: nextElements,
navigationSnapshot: createState().navigationSnapshot,
renderId: 1,
rootLayoutTreePath: "/",
routeId: "route:/feed",
type: "traverse",
});

expect(Object.hasOwn(nextState.elements, "slot:modal:/feed")).toBe(false);
});

it("preserves absent parallel slots on navigate", () => {
const state = createState({
elements: createResolvedElements("route:/feed", "/", {
"slot:modal:/feed": React.createElement("div", null, "modal"),
}),
});
const nextElements = createResolvedElements("route:/feed/comments", "/");

const nextState = routerReducer(state, {
elements: nextElements,
navigationSnapshot: createState().navigationSnapshot,
renderId: 1,
rootLayoutTreePath: "/",
routeId: "route:/feed/comments",
type: "navigate",
});

expect(Object.hasOwn(nextState.elements, "slot:modal:/feed")).toBe(true);
});
});

describe("mounted slot helpers", () => {
Expand Down
62 changes: 62 additions & 0 deletions tests/slot.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,68 @@ describe("slot primitives", () => {
expect(merged["slot:modal:/"]).toBe(UNMATCHED_SLOT);
});

it("mergeElements clears stale slots absent from next when clearAbsentSlots is set", async () => {
const { mergeElements } = await import("../packages/vinext/src/shims/slot.js");

const merged = mergeElements(
{
"layout:/": React.createElement("div", null, "layout"),
"page:/feed": React.createElement("div", null, "feed"),
"slot:modal:/feed": React.createElement("div", null, "intercepted modal"),
},
{
"layout:/": React.createElement("div", null, "layout"),
"page:/feed": React.createElement("div", null, "feed"),
},
true,
);

expect(Object.hasOwn(merged, "slot:modal:/feed")).toBe(false);
});
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.

Consider adding a test for the interaction between clearAbsentSlots and the UNMATCHED_SLOT preservation logic. Specifically: when clearAbsentSlots=true and next explicitly contains a slot as UNMATCHED_SLOT that prev has real content for, the slot should survive (restored to prev content by the UNMATCHED_SLOT loop, and not cleared because the key is present in next).

This is the "traversal to a route that declares the slot but doesn't match it" case. The current code handles it correctly, but it's a subtle interaction worth pinning with a test.


it("mergeElements on traversal: UNMATCHED_SLOT in next is restored from prev and not cleared", async () => {
const { mergeElements, UNMATCHED_SLOT } = await import("../packages/vinext/src/shims/slot.js");

const realContent = React.createElement("div", null, "modal content");
const merged = mergeElements(
{
"layout:/": React.createElement("div", null, "layout"),
"page:/feed": React.createElement("div", null, "feed"),
"slot:modal:/feed": realContent,
},
{
"layout:/": React.createElement("div", null, "layout"),
"page:/feed": React.createElement("div", null, "feed"),
"slot:modal:/feed": UNMATCHED_SLOT,
},
true,
);

// The slot IS present in next (as UNMATCHED_SLOT), so clearAbsentSlots does not
// delete it. The UNMATCHED_SLOT preservation loop then restores the real prev
// content because prev had a non-sentinel value.
expect(Object.hasOwn(merged, "slot:modal:/feed")).toBe(true);
expect(merged["slot:modal:/feed"]).toBe(realContent);
});

it("mergeElements preserves absent slots when clearAbsentSlots is not set", async () => {
const { mergeElements } = await import("../packages/vinext/src/shims/slot.js");

const merged = mergeElements(
{
"layout:/": React.createElement("div", null, "layout"),
"page:/dashboard": React.createElement("div", null, "dashboard"),
"slot:team:/dashboard": React.createElement("div", null, "team panel"),
},
{
"page:/dashboard/settings": React.createElement("div", null, "settings"),
},
);

// Without clearAbsentSlots, absent slots survive (soft nav to child route)
expect(Object.hasOwn(merged, "slot:team:/dashboard")).toBe(true);
});

it("Slot renders element from resolved context", async () => {
const mod = await import("../packages/vinext/src/shims/slot.js");

Expand Down
Loading