Skip to content

fix: clear stale parallel slots on traversal in mergeElements #816

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

Merged
james-elicx merged 5 commits into from
Apr 10, 2026
Merged

fix: clear stale parallel slots on traversal in mergeElements #816

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a7f6b93
fix: clear stale parallel slots on traversal in mergeElements
NathanDrake2406 Apr 10, 2026
b4c2b40
refactor: collapse navigate/traverse reducer cases; add UNMATCHED_SLO…
claude Apr 10, 2026
b26a410
refactor: extract toActionType helper; add reducer-level traverse tests
claude Apr 10, 2026
6d23e51
fix ci
james-elicx Apr 10, 2026
d996df4
fix ci again
james-elicx Apr 10, 2026
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
6 changes: 4 additions & 2 deletions packages/vinext/src/server/app-browser-entry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,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 +484,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 +783,7 @@ async function main(): Promise<void> {
navId,
createNavigationCommitEffect(href, historyUpdateMode, cachedParams),
isSameRoute,
navigationKind === "traverse" ? "traverse" : "navigate",
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Apr 10, 2026

Choose a reason for hiding this comment

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

The ternary navigationKind === "traverse" ? "traverse" : "navigate" appears identically in two places (here and line 875). This correctly maps "refresh" → "navigate", which makes sense since refresh should merge like a forward navigation. Just noting: if a fourth NavigationKind is ever added, both sites would need updating. Not worth extracting now, but worth a comment if the mapping ever becomes non-trivial.

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Apr 10, 2026

Choose a reason for hiding this comment

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

Echoing the previous review's note: the navigationKind === "traverse" ? "traverse" : "navigate" ternary appears identically at lines 786 and 875. This is fine for now (only three NavigationKind values), but if a fourth kind is added, a small helper like toActionType(navigationKind) would prevent the two sites from drifting.

);
} finally {
// Always clear _snapshotPending so the outer catch does not
Expand Down Expand Up @@ -871,6 +872,7 @@ async function main(): Promise<void> {
navId,
createNavigationCommitEffect(href, historyUpdateMode, navParams),
isSameRoute,
navigationKind === "traverse" ? "traverse" : "navigate",
);
} finally {
// Always clear _snapshotPending after renderNavigationPayload returns or
Expand Down
14 changes: 11 additions & 3 deletions packages/vinext/src/server/app-browser-state.ts
View file
Open in desktop
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 @@ -41,6 +41,14 @@ export function routerReducer(state: AppRouterState, action: AppRouterAction): A
rootLayoutTreePath: action.rootLayoutTreePath,
routeId: action.routeId,
};
case "traverse":
return {
elements: mergeElements(state.elements, action.elements, true),
navigationSnapshot: action.navigationSnapshot,
renderId: action.renderId,
rootLayoutTreePath: action.rootLayoutTreePath,
routeId: action.routeId,
};
Comment on lines +36 to +44
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Apr 10, 2026

Choose a reason for hiding this comment

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

Nit: the "traverse" and "navigate" cases are identical except for the clearAbsentSlots boolean. You could collapse them to reduce duplication:

Suggested change
case "traverse":
return {
elements: mergeElements(state.elements, action.elements, true),
navigationSnapshot: action.navigationSnapshot,
renderId: action.renderId,
rootLayoutTreePath: action.rootLayoutTreePath,
routeId: action.routeId,
};
case "traverse":
case "navigate":
return {
elements: mergeElements(state.elements, action.elements, action.type === "traverse"),
navigationSnapshot: action.navigationSnapshot,
renderId: action.renderId,
rootLayoutTreePath: action.rootLayoutTreePath,
routeId: action.routeId,
};

This keeps the exhaustive switch and never guard intact. Not blocking — the current form is also clear.

case "replace":
return {
elements: action.elements,
Expand Down Expand Up @@ -92,7 +100,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 +126,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
View file
Open in desktop
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

@ask-bonk ask-bonk bot Apr 10, 2026

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
37 changes: 37 additions & 0 deletions tests/slot.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,43 @@ 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

@ask-bonk ask-bonk bot Apr 10, 2026

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 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