From 20c30e3f9e173fdc26fc8e3f37c2bc94dab18861 Mon Sep 17 00:00:00 2001 From: k8o Date: Mon, 18 May 2026 13:18:31 +0900 Subject: [PATCH 1/2] test(router): add intercepted-navigation simulation helper to mock Adds __simulateInterceptedNavigation(url, { privateBrowsing }) that drives the full navigate -> commit -> handler -> navigatesuccess lifecycle in unit tests, with an opt-in flag to simulate the WebKit Private Browsing bug where currentEntry stays stale and currententrychange does not fire. --- packages/router/src/__tests__/setup.ts | 51 ++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/packages/router/src/__tests__/setup.ts b/packages/router/src/__tests__/setup.ts index 44b99f3..5569568 100644 --- a/packages/router/src/__tests__/setup.ts +++ b/packages/router/src/__tests__/setup.ts @@ -256,6 +256,57 @@ export function createMockNavigation(initialUrl = "http://localhost/") { }; }, + // Test helper to run the full intercepted-navigation lifecycle: + // navigate event → commit → currententrychange → handler → navigatesuccess. + // + // When `privateBrowsing: true`, simulates the WebKit Private Browsing bug + // where currentEntry does NOT update and currententrychange does NOT fire + // after intercept — only the handler runs and navigatesuccess fires. + // + // Awaits the intercept handler before resolving. + async __simulateInterceptedNavigation( + url: string, + options?: { privateBrowsing?: boolean }, + ): Promise { + const newUrl = new URL(url, currentEntry.url).href; + const event = createMockNavigateEvent(newUrl); + + dispatchEvent("navigate", event); + + if (!event.defaultPrevented && !options?.privateBrowsing) { + const previousEntry = currentEntry; + const currentIndex = entries.indexOf(currentEntry); + while (entries.length > currentIndex + 1) { + const disposedEntry = entries.pop()!; + disposedEntry.__dispose(); + } + const newEntry = new MockNavigationHistoryEntry(newUrl, entries.length); + entries.push(newEntry); + currentEntry = newEntry; + mockNavigation.currentEntry = currentEntry; + const changeEvent = Object.assign(new Event("currententrychange"), { + navigationType: "push" as const, + from: previousEntry, + }); + dispatchEvent("currententrychange", changeEvent); + } + + // Run the intercept handler the browser would have called. + const interceptMock = event.intercept as ReturnType; + const interceptArg = interceptMock.mock.calls[0]?.[0] as + | { handler?: () => Promise } + | undefined; + if (interceptArg?.handler) { + await interceptArg.handler(); + } + + if (!event.defaultPrevented) { + dispatchEvent("navigatesuccess", new Event("navigatesuccess")); + } + + return event; + }, + // Test helper to simulate reload navigation // Dispatches navigate event with navigationType: "reload", then // dispatches currententrychange without changing the entry. From 8cae4b5049485c22bcb47b3e29281906f01f0c04 Mon Sep 17 00:00:00 2001 From: k8o Date: Mon, 18 May 2026 13:18:42 +0900 Subject: [PATCH 2/2] fix(router): handle stale Navigation API entries in Safari Private Browsing Works around a WebKit bug where intercepted navigations commit but navigation.currentEntry.url/.id stay stale and currententrychange does not fire. Captures event.destination.url scoped to entry.id, prefers it in getSnapshot, and listens to navigatesuccess as a fallback notifier. Composite loader cache keys (entry.id + url) keep loader behavior consistent across both modes. Upstream bug: https://bugs.webkit.org/show_bug.cgi?id=314976 --- ...vigationAPIAdapter.privateBrowsing.test.ts | 117 ++++++++++++++++++ .../router/src/core/NavigationAPIAdapter.ts | 115 ++++++++++++----- 2 files changed, 199 insertions(+), 33 deletions(-) create mode 100644 packages/router/src/__tests__/NavigationAPIAdapter.privateBrowsing.test.ts diff --git a/packages/router/src/__tests__/NavigationAPIAdapter.privateBrowsing.test.ts b/packages/router/src/__tests__/NavigationAPIAdapter.privateBrowsing.test.ts new file mode 100644 index 0000000..3a41c47 --- /dev/null +++ b/packages/router/src/__tests__/NavigationAPIAdapter.privateBrowsing.test.ts @@ -0,0 +1,117 @@ +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { NavigationAPIAdapter } from "../core/NavigationAPIAdapter.js"; +import { setupNavigationMock, cleanupNavigationMock } from "./setup.js"; +import { route } from "../route.js"; +import { internalRoutes } from "../types.js"; +import { clearLoaderCache } from "../core/loaderCache.js"; + +const routes = internalRoutes([ + { path: "/", component: () => null }, + { path: "/about", component: () => null }, + { path: "/contact", component: () => null }, +]); + +let mockNav: ReturnType; +let adapter: NavigationAPIAdapter; + +beforeEach(() => { + mockNav = setupNavigationMock("http://localhost/"); + adapter = new NavigationAPIAdapter(); +}); + +afterEach(() => { + cleanupNavigationMock(); + clearLoaderCache(); +}); + +describe("WebKit Private Browsing simulation", () => { + it("getSnapshot returns the destination URL even when currentEntry stays stale", async () => { + adapter.setupInterception(() => routes); + const initialEntryId = mockNav.currentEntry.id; + + await mockNav.__simulateInterceptedNavigation("http://localhost/about", { + privateBrowsing: true, + }); + + expect(mockNav.currentEntry.id).toBe(initialEntryId); + expect(mockNav.currentEntry.url).toBe("http://localhost/"); + expect(adapter.getSnapshot()?.url.href).toBe("http://localhost/about"); + }); + + it("subsequent intercepted navigations update the resolved URL", async () => { + adapter.setupInterception(() => routes); + + await mockNav.__simulateInterceptedNavigation("http://localhost/about", { + privateBrowsing: true, + }); + expect(adapter.getSnapshot()?.url.href).toBe("http://localhost/about"); + + await mockNav.__simulateInterceptedNavigation("http://localhost/contact", { + privateBrowsing: true, + }); + expect(adapter.getSnapshot()?.url.href).toBe("http://localhost/contact"); + }); + + it("notifies subscribers via navigatesuccess when currententrychange does not fire", async () => { + const seen: string[] = []; + adapter.subscribe(() => { + seen.push(adapter.getSnapshot()?.url.href ?? ""); + }); + adapter.setupInterception(() => routes); + + await mockNav.__simulateInterceptedNavigation("http://localhost/about", { + privateBrowsing: true, + }); + + expect(seen).toEqual(["http://localhost/about"]); + }); +}); + +describe("#committedDestination scoping", () => { + it("does not leak across unrelated traversals", async () => { + adapter.setupInterception(() => routes); + + await mockNav.__simulateInterceptedNavigation("http://localhost/about"); + expect(adapter.getSnapshot()?.url.href).toBe("http://localhost/about"); + + mockNav.__simulateTraversal(0); + + expect(adapter.getSnapshot()?.url.href).toBe("http://localhost/"); + }); + + it("does not leak when subsequent non-intercepted navigation changes entry id", async () => { + adapter.setupInterception(() => routes); + + await mockNav.__simulateInterceptedNavigation("http://localhost/about"); + mockNav.__simulateNavigation("http://localhost/contact"); + + expect(adapter.getSnapshot()?.url.href).toBe("http://localhost/contact"); + }); +}); + +describe("dispose cleanup with composite cache keys", () => { + it("clears entry-scoped loader cache when an entry is disposed", async () => { + let loaderCalls = 0; + const routesWithLoader = internalRoutes([ + route({ + path: "/about", + component: () => null, + loader: () => { + loaderCalls += 1; + return { ok: true }; + }, + }), + ]); + + adapter.setupInterception(() => routesWithLoader); + + await mockNav.__simulateInterceptedNavigation("http://localhost/about"); + expect(loaderCalls).toBe(1); + + const aboutEntry = mockNav.__getEntry(1); + aboutEntry.__dispose(); + + await mockNav.__simulateInterceptedNavigation("http://localhost/about"); + expect(loaderCalls).toBe(2); + }); +}); diff --git a/packages/router/src/core/NavigationAPIAdapter.ts b/packages/router/src/core/NavigationAPIAdapter.ts index 4e30b6a..70d9070 100644 --- a/packages/router/src/core/NavigationAPIAdapter.ts +++ b/packages/router/src/core/NavigationAPIAdapter.ts @@ -40,30 +40,56 @@ export function resetNavigationState(): void { export class NavigationAPIAdapter implements RouterAdapter { // Cache the snapshot to ensure referential stability for useSyncExternalStore #cachedSnapshot: LocationEntry | null = null; + #cachedHref: string | null = null; #cachedEntryId: string | null = null; + // Destination URL captured from the most recent intercepted navigation, + // scoped to the entry.id observed at the time of intercept. Honored only + // when the current entry.id still matches, so it cannot leak across + // unrelated traversals or non-intercepted entry changes. + // + // WebKit Private Browsing workaround: after an intercepted navigation, + // `currentEntry.url` stays stale while `event.destination.url` reflects + // the real URL. Upstream bug: https://bugs.webkit.org/show_bug.cgi?id=314976 + #committedDestination: { entryId: string; href: string } | null = null; // Ephemeral info from the current navigation event (not persisted in history) #currentNavigationInfo: unknown = undefined; - // Per-entry reload counters, used to generate unique cache keys + // Per-(entry, URL) reload counters, used to generate unique cache keys // so that loaders re-execute on reload instead of returning cached results. - // Keyed by NavigationHistoryEntry.id. #reloadCounts = new Map(); getSnapshot(): LocationEntry | null { const entry = navigation.currentEntry; - if (!entry?.url) { + if (!entry) { return null; } - // Return cached snapshot if entry hasn't changed - if (this.#cachedEntryId === entry.id && this.#cachedSnapshot) { + const stickyHref = + this.#committedDestination?.entryId === entry.id + ? this.#committedDestination.href + : null; + const actualHref = stickyHref ?? entry.url; + if (!actualHref) { + return null; + } + + // Return cached snapshot if neither URL nor entry identity changed + if ( + this.#cachedHref === actualHref && + this.#cachedEntryId === entry.id && + this.#cachedSnapshot + ) { return this.#cachedSnapshot; } - // Create new snapshot and cache it + // Composite key changes when either entry identity (replace) or URL + // (Private Browsing where entry.id is stale) changes, so loaders for + // a fresh navigation are not served from a stale slot. + this.#cachedHref = actualHref; this.#cachedEntryId = entry.id; + const composite = `${entry.id}|${actualHref}`; this.#cachedSnapshot = { - url: new URL(entry.url), - key: this.#effectiveKey(entry.id), + url: new URL(actualHref), + key: this.#effectiveKey(composite), entryId: entry.id, entryKey: entry.key, state: entry.getState(), @@ -88,6 +114,19 @@ export class NavigationAPIAdapter implements RouterAdapter { { signal: controller.signal }, ); + // Fallback notifier for WebKit Private Browsing where currententrychange + // does not fire after an intercepted navigation. + navigation.addEventListener( + "navigatesuccess", + () => { + callback("navigation"); + // currententrychange may have been skipped; ensure new entries + // still get dispose subscriptions. + this.#subscribeToDisposeEvents(controller.signal); + }, + { signal: controller.signal }, + ); + // Subscribe to dispose events on all existing entries this.#subscribeToDisposeEvents(controller.signal); @@ -120,14 +159,18 @@ export class NavigationAPIAdapter implements RouterAdapter { this.#subscribedEntryIds.add(entry.id); const entryId = entry.id; + const entryUrl = entry.url; entry.addEventListener( "dispose", () => { // clearLoaderCacheForEntry uses prefix matching, so it also clears - // reload-keyed caches (e.g., entryId:r1:0, entryId:r2:0, etc.) - clearLoaderCacheForEntry(entryId); + // reload-keyed caches (e.g., composite:r1, composite:r2, etc.) + if (entryUrl) { + const composite = `${entryId}|${entryUrl}`; + clearLoaderCacheForEntry(composite); + this.#reloadCounts.delete(composite); + } this.#subscribedEntryIds.delete(entryId); - this.#reloadCounts.delete(entryId); }, { signal }, ); @@ -135,13 +178,12 @@ export class NavigationAPIAdapter implements RouterAdapter { } /** - * Compute the effective cache key for a given entry. - * Includes a reload suffix when the entry has been reloaded, - * so loaders get a fresh cache key and re-execute. + * Compute the effective cache key for a composite (`${entry.id}|${url}`), + * appending a reload suffix so loaders re-execute on reload. */ - #effectiveKey(entryId: string): string { - const count = this.#reloadCounts.get(entryId) ?? 0; - return count > 0 ? `${entryId}:r${count}` : entryId; + #effectiveKey(composite: string): string { + const count = this.#reloadCounts.get(composite) ?? 0; + return count > 0 ? `${composite}:r${count}` : composite; } navigate(to: string, options?: NavigateOptions): void { @@ -241,22 +283,20 @@ export class NavigationAPIAdapter implements RouterAdapter { // Route match, so intercept - // On reload, increment the per-entry reload count so loaders get a - // fresh cache key and re-execute. The old cache (under the previous - // key) remains intact for the pending UI shown during the React - // transition. Stale reload caches (2+ generations old) are pruned. + // On reload, increment the per-(entry, URL) reload count so loaders + // get a fresh cache key and re-execute. The immediately previous + // generation is kept because it may be the committed state shown as + // pending UI during the React transition; older ones are pruned. if (event.navigationType === "reload") { - const entryId = navigation.currentEntry!.id; - const oldCount = this.#reloadCounts.get(entryId) ?? 0; - // Prune reload cache from 2 generations ago. The immediately - // previous generation is kept because it may be the committed - // state shown as pending UI during the new transition. + const reloadKey = `${navigation.currentEntry!.id}|${event.destination.url}`; + const oldCount = this.#reloadCounts.get(reloadKey) ?? 0; if (oldCount >= 2) { - clearLoaderCacheForEntry(`${entryId}:r${oldCount - 1}`); + clearLoaderCacheForEntry(`${reloadKey}:r${oldCount - 1}`); } - this.#reloadCounts.set(entryId, oldCount + 1); - // Invalidate snapshot so getSnapshot() picks up the new reload key + this.#reloadCounts.set(reloadKey, oldCount + 1); this.#cachedSnapshot = null; + this.#cachedHref = null; + this.#cachedEntryId = null; } // Abort initial load's loaders if this is the first navigation @@ -274,9 +314,17 @@ export class NavigationAPIAdapter implements RouterAdapter { ); } - // Compute effective cache key inside the handler where - // currentEntry already points to the correct (possibly new) entry. - const effectiveKey = this.#effectiveKey(currentEntry.id); + // Don't invalidate the cache here: getSnapshot's cache check + // already returns the same reference when the resolved URL is + // unchanged (normal mode), avoiding an extra render when + // navigatesuccess fires after currententrychange. + this.#committedDestination = { + entryId: currentEntry.id, + href: event.destination.url, + }; + + const composite = `${currentEntry.id}|${event.destination.url}`; + const effectiveKey = this.#effectiveKey(composite); let actionResult: unknown = undefined; @@ -292,7 +340,7 @@ export class NavigationAPIAdapter implements RouterAdapter { }); } // Revalidate loaders after action — clear cache so loaders re-execute - clearLoaderCacheForEntry(currentEntry.id); + clearLoaderCacheForEntry(composite); } const request = createLoaderRequest(url); @@ -332,6 +380,7 @@ export class NavigationAPIAdapter implements RouterAdapter { updateCurrentEntryState(state: unknown): void { // Invalidate cached snapshot BEFORE updating, so the subscriber gets fresh state this.#cachedSnapshot = null; + this.#cachedHref = null; navigation.updateCurrentEntry({ state }); // Note: updateCurrentEntry fires currententrychange, so subscribers are notified automatically }