fix: clear stale parallel slots on traversal in mergeElements#816
Conversation
Add "traverse" as a first-class action type in the router reducer. When the browser navigates back/forward, the action type is now "traverse" instead of "navigate", which tells mergeElements to clear slot keys absent from the server response. On soft forward navigation, absent slots are still preserved (the server may omit parent layout slots that haven't changed). Only traversals clear them, because the server renders the full destination route tree on popstate. Closes cloudflare#814
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-scoped fix. The approach of threading "traverse" as a distinct action type to differentiate browser back/forward from soft forward navigation is the right call — it avoids overloading the existing navigate path and makes the intent explicit at every layer.
The mergeElements logic is correct: the clearAbsentSlots loop runs after the UNMATCHED_SLOT preservation loop, so a slot explicitly sent as UNMATCHED_SLOT by the server (present in next) won't be cleared, while a slot completely absent from the traversal response will be. The slot: prefix filter correctly protects layout/page/metadata keys from deletion.
A few observations below — one minor suggestion and a couple of notes on test coverage. Nothing blocking.
| case "traverse": | ||
| return { | ||
| elements: mergeElements(state.elements, action.elements, true), | ||
| navigationSnapshot: action.navigationSnapshot, | ||
| renderId: action.renderId, | ||
| rootLayoutTreePath: action.rootLayoutTreePath, | ||
| routeId: action.routeId, | ||
| }; |
There was a problem hiding this comment.
Nit: the "traverse" and "navigate" cases are identical except for the clearAbsentSlots boolean. You could collapse them to reduce duplication:
| 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.
| ); | ||
|
|
||
| expect(Object.hasOwn(merged, "slot:modal:/feed")).toBe(false); | ||
| }); |
There was a problem hiding this comment.
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.
| navId, | ||
| createNavigationCommitEffect(href, historyUpdateMode, cachedParams), | ||
| isSameRoute, | ||
| navigationKind === "traverse" ? "traverse" : "navigate", |
There was a problem hiding this comment.
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.
|
Review posted and approved. Here's a summary of the feedback: Approved — the fix is correct and well-designed. Three inline comments:
|
…T traversal test Address review feedback on cloudflare#816: - Collapse the identical navigate/traverse switch cases in routerReducer, passing `action.type === "traverse"` as the clearAbsentSlots boolean. Keeps exhaustive switch and never guard intact. - Add a test pinning the subtle interaction between clearAbsentSlots=true and the UNMATCHED_SLOT preservation loop: a slot sent as UNMATCHED_SLOT during traversal (key present in next) is restored from prev real content and is not cleared by the absent-slot loop. https://claude.ai/code/session_01RhqyMQWoHBr8qrLZoUjoZj
|
/bigbonk review |
There was a problem hiding this comment.
Clean, well-scoped fix. The approach is sound: threading "traverse" as a distinct action type through the reducer pipeline gives mergeElements the context it needs to differentiate browser back/forward (clear stale slots) from soft forward navigation (preserve absent slots).
The loop ordering in mergeElements is correct — UNMATCHED_SLOT restoration runs before clearAbsentSlots deletion, and since UNMATCHED_SLOT slots are present in next (they pass Object.hasOwn(next, key)), they survive the clear loop. The tests cover the three key cases well.
I see the previous review's suggestion to collapse the switch cases has been adopted. Two minor observations below — neither blocking.
Suggestion (not blocking): Consider adding a routerReducer test for type: "traverse" in tests/app-browser-entry.test.ts. The mergeElements unit tests in slot.test.ts cover the core logic, but a reducer-level test would pin the wiring — confirming the reducer actually passes clearAbsentSlots=true when the action type is "traverse". This would catch a regression if someone changes the action.type === "traverse" condition in the reducer.
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);
});| navId, | ||
| createNavigationCommitEffect(href, historyUpdateMode, cachedParams), | ||
| isSameRoute, | ||
| navigationKind === "traverse" ? "traverse" : "navigate", |
There was a problem hiding this comment.
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.
|
Review submitted successfully. Here's a summary of my review: Approved — the fix is correct and well-designed. Key observations:
Two non-blocking suggestions:
|
Address second-round review feedback on cloudflare#816: - Extract toActionType(kind: NavigationKind) helper to consolidate the duplicated `navigationKind === "traverse" ? "traverse" : "navigate"` ternary that appeared at two call sites in renderNavigationPayload. A comment documents the "refresh" → "navigate" mapping intent and flags both sites as needing updates if NavigationKind gains new values. - Add routerReducer integration tests in app-browser-entry.test.ts: "traverse" clears stale parallel slots; "navigate" preserves them. These pin the wiring between the reducer and mergeElements. https://claude.ai/code/session_01RhqyMQWoHBr8qrLZoUjoZj
|
/bigbonk review |
There was a problem hiding this comment.
Clean, well-scoped fix. The approach is correct: threading "traverse" as a distinct action type from popstate through the full pipeline (navigateRsc → renderNavigationPayload → createPendingNavigationCommit → dispatchBrowserTree → routerReducer → mergeElements) gives the merge logic the context it needs to differentiate browser back/forward from soft forward navigation.
The implementation details are solid:
-
Loop ordering in
mergeElementsis correct — UNMATCHED_SLOT restoration (line 34-37) runs beforeclearAbsentSlotsdeletion (line 44-49). Since UNMATCHED_SLOT entries are present innext(they passObject.hasOwn(next, key)), they survive the clear loop. This correctly handles the "traversal to a route that declares but doesn't match a slot" case. -
The
slot:prefix guard (line 46) correctly protects layout/page/metadata keys from deletion during traversal. -
toActionTypehelper (line 82-84) addresses the previous review's note about duplicated ternaries, centralizing theNavigationKind→ action type mapping with a clear comment about"refresh"mapping to"navigate". -
Switch case collapse in the reducer (line 36-37 of
app-browser-state.ts) is clean — theaction.type === "traverse"expression inside the shared case body keeps behavior distinct without duplicating the return shape. -
Test coverage is thorough: unit tests for all three
mergeElementscases (clear absent, preserve UNMATCHED_SLOT on traversal, preserve absent on soft nav), plus reducer-level integration tests that pin the wiring betweenrouterReducerandmergeElements.
No blocking issues.
| if (clearAbsentSlots) { | ||
| for (const key of Object.keys(merged)) { | ||
| if (key.startsWith("slot:") && !Object.hasOwn(next, key)) { | ||
| delete merged[key]; |
There was a problem hiding this comment.
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.
| // 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" { |
There was a problem hiding this comment.
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.
|
Review submitted and approved. Here's a summary: Approved — the fix is correct and well-designed. The PR cleanly threads Key things I verified:
Two non-blocking inline comments noting the |
3 participants
Summary
"traverse"as a first-class action type in the router reducer, distinct from"navigate"and"replace"mergeElementsgains aclearAbsentSlotsflag (default false) that removes slot keys present inprevbut absent fromnextContext
When navigating back from an intercepted route (e.g.,
/feedwith photo modal -> browser back), the RSC response for the destination omits the@modalslot. Previously,mergeElementscarried the stale modal forward fromprevvia{ ...prev, ...next }.The fix cannot simply clear all absent slots because soft forward navigation (e.g.,
/dashboard->/dashboard/settings) intentionally omits parent layout slots that haven't changed. Those must be preserved.The solution: thread the navigation kind through the action type. Browser back/forward dispatches
"traverse"instead of"navigate", which tellsmergeElementsto clear absent slots.Closes #814
Test plan
mergeElements clears stale slots absent from next when clearAbsentSlots is setmergeElements preserves absent slots when clearAbsentSlots is not setmergeElementstests pass (UNMATCHED_SLOT preservation, shallow merge)parallel slot content persists on soft navigation to child route(existing, should still pass in CI)