-
Notifications
You must be signed in to change notification settings - Fork 326
fix(app-router): guard stale popstate scroll restoration #1253
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.
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
Changes from all commits
aad163a
f78bbd2
ad33016
334fbd4
b7289c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| type RestoreScrollPosition = (state: unknown) => void; | ||
| type NavigateRsc = ( | ||
| href: string, | ||
| redirectDepth?: number, | ||
| navigationKind?: "navigate" | "traverse" | "refresh", | ||
| ) => Promise<void>; | ||
|
|
||
| type BrowserPopstateRestoreDeps = { | ||
| getActiveNavigationId: () => number; | ||
| getPendingNavigation: () => Promise<void> | null | undefined; | ||
| getNavigate: () => NavigateRsc | undefined; | ||
| isCurrentNavigation: (navId: number) => boolean; | ||
| notifyAppRouterTransitionStart: (href: string) => void; | ||
| restorePopstateScrollPosition: RestoreScrollPosition; | ||
| setPendingNavigation: (pendingNavigation: Promise<void> | null) => void; | ||
| }; | ||
|
|
||
| export function createPopstateRestoreHandler( | ||
| deps: BrowserPopstateRestoreDeps, | ||
| ): (event: PopStateEvent) => void { | ||
| return (event) => { | ||
| deps.notifyAppRouterTransitionStart(window.location.href); | ||
| const navigate = deps.getNavigate(); | ||
| const pendingNavigation = navigate?.(window.location.href, 0, "traverse") ?? Promise.resolve(); | ||
| const popstateNavId = deps.getActiveNavigationId(); | ||
|
|
||
| deps.setPendingNavigation(pendingNavigation); | ||
|
|
||
| void pendingNavigation.finally(() => { | ||
| if (deps.isCurrentNavigation(popstateNavId)) { | ||
| deps.restorePopstateScrollPosition(event.state); | ||
| } | ||
|
|
||
| if (deps.getPendingNavigation() === pendingNavigation) { | ||
| deps.setPendingNavigation(null); | ||
| } | ||
| }); | ||
| }; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ import { | |
| hydrateRootInTransition, | ||
| } from "../packages/vinext/src/server/app-browser-hydration.js"; | ||
| import { createAppBrowserNavigationController } from "../packages/vinext/src/server/app-browser-navigation-controller.js"; | ||
| import { createPopstateRestoreHandler } from "../packages/vinext/src/server/app-browser-popstate.js"; | ||
| import { | ||
| VINEXT_RSC_COMPATIBILITY_ID_HEADER, | ||
| resolveRscCompatibilityNavigationDecision, | ||
|
|
@@ -251,6 +252,16 @@ function stubWindow(href: string) { | |
| return { assign, replace, storage }; | ||
| } | ||
|
|
||
| function createDeferred(): { resolve: () => void; promise: Promise<void> } { | ||
| let resolve: () => void = () => { | ||
| throw new Error("Promise was not initialized"); | ||
| }; | ||
| const promise = new Promise<void>((resolveInner) => { | ||
| resolve = resolveInner; | ||
| }); | ||
| return { promise, resolve }; | ||
| } | ||
|
|
||
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| vi.unstubAllGlobals(); | ||
|
|
@@ -2993,6 +3004,99 @@ describe("app browser entry previousNextUrl helpers", () => { | |
| }); | ||
| }); | ||
|
|
||
| describe("createPopstateRestoreHandler", () => { | ||
| it("restores scroll only after the latest popstate navigation commits", async () => { | ||
| const restoreCalls: unknown[] = []; | ||
| const firstNavigation = createDeferred(); | ||
| const secondNavigation = createDeferred(); | ||
| let popstateCalls = 0; | ||
| const popstate = vi.fn(() => { | ||
| popstateCalls += 1; | ||
| if (popstateCalls === 1) { | ||
| return firstNavigation.promise; | ||
| } | ||
| return secondNavigation.promise; | ||
| }); | ||
| let activeNavigationId = 0; | ||
|
|
||
| stubWindow("https://example.com/feed"); | ||
| window.__VINEXT_RSC_PENDING__ = null; | ||
|
|
||
| const handler = createPopstateRestoreHandler({ | ||
| getActiveNavigationId: () => activeNavigationId, | ||
| getNavigate: () => { | ||
|
Comment on lines
+3025
to
+3027
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| activeNavigationId += 1; | ||
| return () => popstate(); | ||
| }, | ||
| getPendingNavigation: () => window.__VINEXT_RSC_PENDING__, | ||
| isCurrentNavigation: (navId) => navId === activeNavigationId, | ||
| notifyAppRouterTransitionStart: () => {}, | ||
| restorePopstateScrollPosition: (scrollState) => { | ||
| restoreCalls.push(scrollState); | ||
| }, | ||
| setPendingNavigation: (pendingNavigation) => { | ||
| window.__VINEXT_RSC_PENDING__ = pendingNavigation; | ||
| }, | ||
| }); | ||
|
|
||
| handler({ state: { __vinext_scrollY: 10 } } as PopStateEvent); | ||
| handler({ state: { __vinext_scrollY: 20 } } as PopStateEvent); | ||
|
|
||
| expect(window.__VINEXT_RSC_PENDING__).toBe(secondNavigation.promise); | ||
|
|
||
| secondNavigation.resolve(); | ||
| await secondNavigation.promise; | ||
| await Promise.resolve(); | ||
|
|
||
| expect(restoreCalls).toEqual([{ __vinext_scrollY: 20 }]); | ||
| expect(window.__VINEXT_RSC_PENDING__).toBeNull(); | ||
|
|
||
| firstNavigation.resolve(); | ||
| await firstNavigation.promise; | ||
| await Promise.resolve(); | ||
|
|
||
| expect(restoreCalls).toEqual([{ __vinext_scrollY: 20 }]); | ||
| expect(window.__VINEXT_RSC_PENDING__).toBeNull(); | ||
| }); | ||
|
|
||
| it("clears __VINEXT_RSC_PENDING__ when a stale popstate navigation settles", async () => { | ||
| const restoreCalls: unknown[] = []; | ||
| const navigation = createDeferred(); | ||
| let activeNavigationId = 1; | ||
|
|
||
| stubWindow("https://example.com/feed"); | ||
| window.__VINEXT_RSC_PENDING__ = null; | ||
|
|
||
| const handler = createPopstateRestoreHandler({ | ||
| getActiveNavigationId: () => activeNavigationId, | ||
| getNavigate: () => { | ||
| activeNavigationId = 1; | ||
| return () => navigation.promise; | ||
| }, | ||
| getPendingNavigation: () => window.__VINEXT_RSC_PENDING__, | ||
| isCurrentNavigation: (navId) => navId === activeNavigationId, | ||
| notifyAppRouterTransitionStart: () => {}, | ||
| restorePopstateScrollPosition: (scrollState) => { | ||
| restoreCalls.push(scrollState); | ||
| }, | ||
| setPendingNavigation: (pendingNavigation) => { | ||
| window.__VINEXT_RSC_PENDING__ = pendingNavigation; | ||
| }, | ||
| }); | ||
|
|
||
| handler({ state: { __vinext_scrollY: 10 } } as PopStateEvent); | ||
| expect(window.__VINEXT_RSC_PENDING__).toBe(navigation.promise); | ||
|
|
||
| activeNavigationId = 2; | ||
| navigation.resolve(); | ||
| await navigation.promise; | ||
| await Promise.resolve(); | ||
|
|
||
| expect(restoreCalls).toEqual([]); | ||
| expect(window.__VINEXT_RSC_PENDING__).toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("devOnCaughtError (hydrateRoot dev handler)", () => { | ||
| it("ignores redirect sentinels handled by RedirectBoundary", () => { | ||
| const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {}); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ import { waitForAppRouterHydration } from "../helpers"; | |
|
|
||
| const BASE = "http://localhost:4174"; | ||
| const VISITED_CACHE_MARKER = "__VINEXT_VISITED_CACHE_MARKER__"; | ||
| const RSC_NAVIGATION_PROMISE_MARKER = "__VINEXT_TEST_RSC_NAVIGATION_PROMISE__"; | ||
|
|
||
| async function pushAppRoute(page: Page, pathname: string): Promise<void> { | ||
| await page.evaluate((target) => { | ||
|
|
@@ -14,6 +15,47 @@ async function pushAppRoute(page: Page, pathname: string): Promise<void> { | |
| }, pathname); | ||
| } | ||
|
|
||
| async function captureRscNavigationPromises(page: Page): Promise<void> { | ||
| await page.evaluate((marker) => { | ||
| const navigate = window.__VINEXT_RSC_NAVIGATE__; | ||
| if (typeof navigate !== "function") { | ||
| throw new Error("window.__VINEXT_RSC_NAVIGATE__ is not installed"); | ||
| } | ||
|
|
||
| const wrappedNavigate: typeof navigate = ( | ||
| href, | ||
| redirectDepth, | ||
| navigationKind, | ||
| historyUpdateMode, | ||
| previousNextUrlOverride, | ||
| programmaticTransition, | ||
| ) => { | ||
| const pendingNavigation = navigate( | ||
| href, | ||
| redirectDepth, | ||
| navigationKind, | ||
| historyUpdateMode, | ||
| previousNextUrlOverride, | ||
| programmaticTransition, | ||
| ); | ||
| Reflect.set(window, marker, pendingNavigation); | ||
| return pendingNavigation; | ||
| }; | ||
|
|
||
| window.__VINEXT_RSC_NAVIGATE__ = wrappedNavigate; | ||
| }, RSC_NAVIGATION_PROMISE_MARKER); | ||
| } | ||
|
|
||
| async function waitForLastRscNavigation(page: Page): Promise<void> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good stabilization. The |
||
| await page.waitForFunction( | ||
| (marker) => Reflect.get(window, marker), | ||
| RSC_NAVIGATION_PROMISE_MARKER, | ||
| ); | ||
| await page.evaluate(async (marker) => { | ||
| await Reflect.get(window, marker); | ||
| }, RSC_NAVIGATION_PROMISE_MARKER); | ||
| } | ||
|
|
||
| test.describe("App Router RSC compatibility navigation", () => { | ||
| test("replays same-build visited RSC payloads instead of refetching or reloading", async ({ | ||
| page, | ||
|
|
@@ -28,9 +70,13 @@ test.describe("App Router RSC compatibility navigation", () => { | |
|
|
||
| await page.goto(`${BASE}/`); | ||
| await waitForAppRouterHydration(page); | ||
| await captureRscNavigationPromises(page); | ||
|
|
||
| await pushAppRoute(page, "/about"); | ||
| await expect(page.locator("h1")).toHaveText("About"); | ||
| // router.push commits visible UI before the RSC navigation promise has | ||
| // finished seeding the visited-response cache this test asserts on. | ||
| await waitForLastRscNavigation(page); | ||
| expect(aboutRscRequests).toHaveLength(1); | ||
|
|
||
| await page.evaluate((marker) => { | ||
|
|
@@ -42,6 +88,7 @@ test.describe("App Router RSC compatibility navigation", () => { | |
| router.push("/"); | ||
| }, VISITED_CACHE_MARKER); | ||
| await expect(page.locator("h1")).toHaveText("Welcome to App Router"); | ||
| await waitForLastRscNavigation(page); | ||
|
|
||
| await pushAppRoute(page, "/about"); | ||
| await expect(page.locator("h1")).toHaveText("About"); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: The correctness of this line depends on
navigate?.()callingbeginNavigation()synchronously before returning the promise, so thatgetActiveNavigationId()on the next line reads the id that this popstate allocated. That's true today (navigateRscisasyncandbeginNavigation()is the first synchronous statement), but it's a subtle ordering contract between two files.Consider adding a brief comment here noting the invariant, e.g.:
This would help future readers understand why
getActiveNavigationIdis called afternavigaterather than before.