From 14c7102552c02d0112ceaf24fbae34b62d14b230 Mon Sep 17 00:00:00 2001 From: LongYinan Date: Mon, 26 Oct 2026 20:48:54 +0800 Subject: [PATCH] fix(pages): keep the committed page when a history write fails A navigation commit wrote history last. If pushState or replaceState threw (Safari's rate limit, a SecurityError), the new page had already taken over the head, scroll key and Deferred values while the address bar still showed the old URL, and the visit never went idle. Write history first in the engine commit, so a failed write leaves the committed page untouched and only disposes the new one. The core client (Vue, Svelte, Solid) and the React adapter then show the committed page again and end the visit. A same-page hash visit whose write fails now rejects through its shell instead of throwing synchronously. Fixes #562. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/react/src/runtime/pages-client.ts | 14 ++ .../integration/react-pages-client.test.ts | 54 ++++- packages/void/src/pages/client.ts | 71 +++++- packages/void/src/pages/navigation-engine.ts | 43 ++-- packages/void/src/pages/scroll-restoration.ts | 6 +- packages/void/test/unit/pages-client.test.ts | 226 ++++++++++++++++++ packages/void/test/unit/pages-scroll.test.ts | 24 ++ 7 files changed, 413 insertions(+), 25 deletions(-) diff --git a/packages/react/src/runtime/pages-client.ts b/packages/react/src/runtime/pages-client.ts index 4cbcceeb2..ef1daf104 100644 --- a/packages/react/src/runtime/pages-client.ts +++ b/packages/react/src/runtime/pages-client.ts @@ -487,11 +487,25 @@ function App({ dispatch({ type: 'finish', resource, location: currentUrl() }); }) .catch((error) => { + // The history write failed, so the address bar still shows the committed page. + // End this visit and show that page again; a newer visit keeps its own state. + const resolveViewTransition = viewTransitionWaiters.get(resource.id); + if (resolveViewTransition) { + viewTransitionWaiters.delete(resource.id); + resolveViewTransition(); + } const visitSettler = pendingVisitSettlers.get(resource.id); if (visitSettler) { pendingVisitSettlers.delete(resource.id); visitSettler.reject(error); + } else { + // A form visit has already settled with its action result. + reportError(error); + } + if (activePendingNavigationId === resource.id) { + activePendingNavigationId = null; } + dispatch({ type: 'fail', resource, location: currentUrl() }); }); }, [], diff --git a/packages/react/test/integration/react-pages-client.test.ts b/packages/react/test/integration/react-pages-client.test.ts index f40d9baf4..f936a54e4 100644 --- a/packages/react/test/integration/react-pages-client.test.ts +++ b/packages/react/test/integration/react-pages-client.test.ts @@ -2,7 +2,7 @@ import { execSync, type ChildProcess } from 'node:child_process'; import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; import { type Browser, type Page } from 'playwright'; -import { describe, expect, it, beforeAll, afterAll } from 'vite-plus/test'; +import { describe, expect, it, beforeAll, afterAll, vi } from 'vite-plus/test'; import { killChild, spawnPlaygroundServer, @@ -594,6 +594,58 @@ function registerClientTests(getBase: () => string, coverage: 'smoke' | 'full') await page.context().close(); } }); + // The next pushState or replaceState call throws, like a browser that rate-limits history. + async function failNextHistoryWrite(page: Page, method: 'pushState' | 'replaceState') { + await page.evaluate((name) => { + const original = History.prototype[name]; + History.prototype[name] = function () { + History.prototype[name] = original; + throw new DOMException('History blocked by test', 'SecurityError'); + }; + }, method); + } + + async function waitForIdle(page: Page) { + await page.waitForFunction( + () => document.querySelector('[data-testid="navigation-state"]')?.textContent === 'idle', + ); + } + + for (const [link, method] of [ + ['onclick-link', 'pushState'], + ['replace-link', 'replaceState'], + ] as const) { + it(`${link} keeps the current page when ${method} fails`, async () => { + const page = await newPage(); + const errors: Array = []; + page.on('pageerror', (error) => { + errors.push(error.message); + }); + + try { + await page.goto(BASE + '/link-behavior'); + await waitForHydration(page); + const heading = await page.textContent('h1'); + await failNextHistoryWrite(page, method); + + await page.click(`[data-testid="${link}"]`); + await vi.waitFor(() => expect(errors).toContain('History blocked by test'), { + timeout: 10_000, + }); + await waitForIdle(page); + expect(await page.textContent('h1')).toBe(heading); + expect(page.url()).toBe(BASE + '/link-behavior'); + + await page.click(`[data-testid="${link}"]`); + await page.waitForURL(BASE + '/users'); + await page.waitForFunction(() => + document.querySelector('h1')?.textContent?.includes('Users'), + ); + } finally { + await page.context().close(); + } + }); + } } describe('react-pages client (dev)', () => { diff --git a/packages/void/src/pages/client.ts b/packages/void/src/pages/client.ts index bf9ecd0c0..fe166862d 100644 --- a/packages/void/src/pages/client.ts +++ b/packages/void/src/pages/client.ts @@ -12,6 +12,7 @@ import { createPageNavigationEngine, type PageData, type PageNavigationEngineConfig, + type PageNavigationResult, type VisitOptions, type VoidRouterAdapter as PageNavigationAdapter, type PrefetchOptions, @@ -335,6 +336,18 @@ export async function createVoidRouter(config: VoidRouterConfig): Promise<{ let routeUrl = currentUrl(); let routeParams = engine.initialPageData.params ?? {}; let activeNavigationId: number | null = null; + // The page in the address bar, and the page on screen. They differ while a visit has + // rendered but not committed, and after its commit fails. + type RenderedPage = Pick; + let committedPage: RenderedPage = { + pageData: engine.initialPageData, + component: engine.initialComponent, + layouts: engine.initialLayouts, + }; + let renderedPage = committedPage; + // Set when the page on screen is one whose commit failed; the last visit to settle puts + // the committed page back. + let failedRender: { preserveState: boolean } | null = null; const setRouteState = (url: string, params: Record = routeParams) => { routeUrl = url; routeParams = params; @@ -358,10 +371,14 @@ export async function createVoidRouter(config: VoidRouterConfig): Promise<{ result.pageData.params ?? {}, ); - const applyUpdate = () => - config.adapter.applyUpdate(result.pageData, result.component, result.layouts, { - preserveState: resource.options.preserveState ?? resource.method !== 'GET', + const preserveState = resource.options.preserveState ?? resource.method !== 'GET'; + const applyUpdate = () => { + renderedPage = result; + failedRender = null; + return config.adapter.applyUpdate(result.pageData, result.component, result.layouts, { + preserveState, }); + }; let applied = true; const useTransition = resource.options.viewTransition ?? config.viewTransitions; @@ -380,9 +397,24 @@ export async function createVoidRouter(config: VoidRouterConfig): Promise<{ } // A newer visit can commit while this page is being applied; its URL stays. - if (!applied || !(await resource.commit())) { + let committed = false; + if (applied) { + try { + committed = await resource.commit(); + } catch (error) { + if (renderedPage === result) { + failedRender = { preserveState }; + } + throw error; + } + } + if (!committed) { throw new DOMException('Navigation aborted', 'AbortError'); } + committedPage = result; + if (renderedPage === result) { + failedRender = null; + } if (activeNavigationId === resource.id) { setRouteState(currentUrl(), result.pageData.params ?? {}); } @@ -397,10 +429,33 @@ export async function createVoidRouter(config: VoidRouterConfig): Promise<{ } throw error; } finally { - if (activeNavigationId === resource.id) { - activeNavigationId = null; - setRouteState(currentUrl()); - setNavigation(idleNavigationState()); + // A same-page hash visit can settle before this one, so no visit may be active. + const ownsNavigation = activeNavigationId === resource.id; + if (ownsNavigation || activeNavigationId === null) { + let restored = false; + if (failedRender && renderedPage !== committedPage) { + const { preserveState } = failedRender; + failedRender = null; + renderedPage = committedPage; + restored = true; + const page = committedPage; + try { + await config.adapter.applyUpdate(page.pageData, page.component, page.layouts, { + preserveState, + }); + } catch (error) { + // The visit already rejects with the history error; keep that one. + reportError(error); + } + } + // A newer visit may have started while the committed page was applied. + if (ownsNavigation && activeNavigationId === resource.id) { + activeNavigationId = null; + setRouteState(currentUrl(), restored ? (committedPage.pageData.params ?? {}) : undefined); + setNavigation(idleNavigationState()); + } else if (restored && activeNavigationId === null) { + setRouteState(currentUrl(), committedPage.pageData.params ?? {}); + } } } } diff --git a/packages/void/src/pages/navigation-engine.ts b/packages/void/src/pages/navigation-engine.ts index 1ff237104..c0d95e7fc 100644 --- a/packages/void/src/pages/navigation-engine.ts +++ b/packages/void/src/pages/navigation-engine.ts @@ -598,10 +598,10 @@ export async function createPageNavigationEngine( options: VisitOptions, writeHistory: boolean, ): void { - _currentFetchUrl = effectiveFetchUrl; if (writeHistory) { scrollRestoration.writeHistory(effectiveFetchUrl + hash, !!options.replace); } + _currentFetchUrl = effectiveFetchUrl; scrollRestoration.commit(); restoreScroll(options._scrollPosition, hash); } @@ -864,6 +864,26 @@ export async function createPageNavigationEngine( if (resourceId < _urlNavigationId) { return false; } + // Write history before anything else changes. Disposing the previous page and handing + // over its Deferred values cannot be undone, so a failed write must find them untouched. + // preserveState only controls component state; back/forward traversal already moved history. + if (!options._popstate) { + const historyUrl = commitTarget.fetchUrl + commitTarget.hash; + const resource = getResource(); + const currentDocumentUrl = window.location.pathname + window.location.search; + const replaceHistory = + !!options.replace || + (resource.method !== 'GET' && historyUrl === currentDocumentUrl + commitTarget.hash); + try { + scrollRestoration.writeHistory(historyUrl, replaceHistory); + } catch (error) { + if (_pendingResource?.id === resourceId) { + _pendingResource = null; + } + resource.dispose(); + throw error; + } + } _urlNavigationId = resourceId; if (_pendingResource?.id === resourceId) { _pendingResource = null; @@ -910,17 +930,6 @@ export async function createPageNavigationEngine( } applyClientHead(result.pageData.head as HeadDescriptor | undefined); _currentFetchUrl = commitTarget.fetchUrl; - - // preserveState only controls component state; back/forward traversal already moved history. - if (!options._popstate) { - const historyUrl = commitTarget.fetchUrl + commitTarget.hash; - const resource = getResource(); - const currentDocumentUrl = window.location.pathname + window.location.search; - const replaceHistory = - !!options.replace || - (resource.method !== 'GET' && historyUrl === currentDocumentUrl + commitTarget.hash); - scrollRestoration.writeHistory(historyUrl, replaceHistory); - } scrollRestoration.commit(); requestAnimationFrame(() => { @@ -989,7 +998,13 @@ export async function createPageNavigationEngine( Boolean(hash || options._popstate); if (isSamePageHashNavigation) { - commitHashNavigation(effectiveFetchUrl, hash, options, !options._popstate); + // A failed history write ends this visit like any other failed visit: through its shell. + let shell: Promise = Promise.resolve(undefined); + try { + commitHashNavigation(effectiveFetchUrl, hash, options, !options._popstate); + } catch (error) { + shell = Promise.reject(error); + } return { id, url, @@ -998,7 +1013,7 @@ export async function createPageNavigationEngine( method, options, immediate: true, - shell: Promise.resolve(undefined), + shell, abort() {}, // Same-page hash visits commit right away. They do not stop an older visit // whose page is already on screen from committing its URL. diff --git a/packages/void/src/pages/scroll-restoration.ts b/packages/void/src/pages/scroll-restoration.ts index 158814cd5..495ee3481 100644 --- a/packages/void/src/pages/scroll-restoration.ts +++ b/packages/void/src/pages/scroll-restoration.ts @@ -95,13 +95,15 @@ export function createScrollRestoration(): ScrollRestoration { writeHistory(url: string, replace: boolean): void { // A replaced entry keeps its key: its position is saved again when it is left. - renderedKey = (replace ? readKey(history.state) : null) ?? newKey(); - const state = { ...(replace ? history.state : {}), __voidScrollKey: renderedKey }; + const key = (replace ? readKey(history.state) : null) ?? newKey(); + const state = { ...(replace ? history.state : {}), __voidScrollKey: key }; if (replace) { history.replaceState(state, '', url); } else { history.pushState(state, '', url); } + // Only once the entry exists: if the write throws, positions keep the current entry's key. + renderedKey = key; }, commit(): void { diff --git a/packages/void/test/unit/pages-client.test.ts b/packages/void/test/unit/pages-client.test.ts index 45c256eba..ec646773f 100644 --- a/packages/void/test/unit/pages-client.test.ts +++ b/packages/void/test/unit/pages-client.test.ts @@ -1726,4 +1726,230 @@ describe('superseded navigation commits (#507)', () => { expect(page.documentStub).toMatchObject({ title: 'B' }); expect(page.rejectDeferred).not.toHaveBeenCalled(); }); + + describe('failed history writes (#562)', () => { + function failNext(method: 'pushState' | 'replaceState') { + const error = new DOMException('History blocked', 'SecurityError'); + vi.mocked(history[method]).mockImplementationOnce(() => { + throw error; + }); + return error; + } + + function states(onNavigationChange: ReturnType) { + return onNavigationChange.mock.calls.map((call) => (call[0] as { state: string }).state); + } + + it('keeps the committed page, URL and head when pushState fails', async () => { + const page = setup(); + const onNavigationChange = vi.fn(); + const r = await router(page, { onNavigationChange }); + const error = failNext('pushState'); + + await expect(r.visit('/b')).rejects.toBe(error); + + expect(states(onNavigationChange)).toEqual(['loading', 'idle']); + expect(window.location.pathname).toBe('/'); + expect(r.url.pathname).toBe('/'); + expect(page.documentStub).toMatchObject({ title: 'Home' }); + expect(page.rendered).toEqual(['/b', '']); + expect(page.applyUpdate).toHaveBeenNthCalledWith( + 2, + expect.objectContaining({ component: 'home' }), + expect.anything(), + expect.anything(), + { preserveState: false }, + ); + // Only B's own pending Deferred value is rejected. + expect(page.rejectDeferred).toHaveBeenCalledTimes(1); + }); + + it('navigates normally after a failed write', async () => { + const page = setup(); + const r = await router(page); + failNext('pushState'); + await r.visit('/b').catch(() => {}); + + await expect(r.visit('/a')).resolves.toMatchObject({ url: '/a' }); + expect(vi.mocked(history.pushState).mock.calls.map((call) => call[2])).toEqual(['/b', '/a']); + expect(window.location.pathname).toBe('/a'); + expect(page.documentStub).toMatchObject({ title: 'A' }); + }); + + it('keeps the committed page when replaceState fails for replace: true', async () => { + const page = setup(); + const r = await router(page); + const error = failNext('replaceState'); + + await expect(r.visit('/a', { replace: true })).rejects.toBe(error); + + expect(window.location.pathname).toBe('/'); + expect(page.rendered).toEqual(['/a', '']); + expect(page.documentStub).toMatchObject({ title: 'Home' }); + }); + + it('keeps form state when a same-URL submission cannot replace its entry', async () => { + const page = setup(); + const r = await router(page); + await r.visit('/a'); + const committedA = page.applyUpdate.mock.calls[0][0]; + const error = failNext('replaceState'); + + await expect(r.visit('/a', { method: 'post', data: { name: 'x' } })).rejects.toBe(error); + + expect(history.pushState).toHaveBeenCalledTimes(1); + expect(page.applyUpdate).toHaveBeenLastCalledWith( + committedA, + expect.anything(), + expect.anything(), + { preserveState: true }, + ); + expect(window.location.pathname).toBe('/a'); + }); + + it('leaves the page to a newer visit when an older commit fails', async () => { + const page = setup(); + const b = gate(); + const a = gate(); + page.updateGates.set('/b', b.promise); + page.fetchGates.set('/a', a.promise); + const onNavigationChange = vi.fn(); + const r = await router(page, { onNavigationChange }); + + const visitB = r.visit('/b'); + await entered(page, '/b'); + const visitA = r.visit('/a'); + const error = failNext('pushState'); + b.release(); + await expect(visitB).rejects.toBe(error); + + // A is still loading, so B stays on screen until A settles. + expect(page.rendered).toEqual(['/b']); + expect(states(onNavigationChange).at(-1)).toBe('loading'); + + a.release(); + await expect(visitA).resolves.toMatchObject({ url: '/a' }); + expect(page.rendered).toEqual(['/b', '/a']); + expect(window.location.pathname).toBe('/a'); + expect(page.documentStub).toMatchObject({ title: 'A' }); + expect(states(onNavigationChange).at(-1)).toBe('idle'); + }); + + it('restores the committed page once a newer visit fails too', async () => { + const page = setup(); + const b = gate(); + const a = gate(); + page.updateGates.set('/b', b.promise); + const fetchPage = fetch; + vi.stubGlobal( + 'fetch', + vi.fn(async (url: string, init?: RequestInit) => { + if (init?.method?.toUpperCase() === 'POST') { + await a.promise; + return new Response('down', { status: 500 }); + } + return fetchPage(url, init); + }), + ); + const onNavigationChange = vi.fn(); + const r = await router(page, { onNavigationChange }); + + const visitB = r.visit('/b'); + await entered(page, '/b'); + const submitA = r.visit('/a', { method: 'post', data: { name: 'x' } }); + const error = failNext('pushState'); + b.release(); + await expect(visitB).rejects.toBe(error); + a.release(); + await expect(submitA).rejects.toThrow(); + + expect(page.rendered).toEqual(['/b', '']); + expect(window.location.pathname).toBe('/'); + expect(page.documentStub).toMatchObject({ title: 'Home' }); + expect(states(onNavigationChange).at(-1)).toBe('idle'); + }); + + it('restores the committed page when a hash visit settled first', async () => { + const page = setup(); + const b = gate(); + page.updateGates.set('/b', b.promise); + const onNavigationChange = vi.fn(); + const r = await router(page, { onNavigationChange }); + + const visitB = r.visit('/b'); + await entered(page, '/b'); + await r.visit('#x'); + const error = failNext('pushState'); + b.release(); + + await expect(visitB).rejects.toBe(error); + expect(page.rendered).toEqual(['/b', '']); + expect(window.location.pathname + window.location.hash).toBe('/#x'); + expect(page.documentStub).toMatchObject({ title: 'Home' }); + expect(states(onNavigationChange).at(-1)).toBe('idle'); + }); + + it('ends a hash visit whose history write fails like any failed visit', async () => { + const page = setup(); + const onNavigationChange = vi.fn(); + const r = await router(page, { onNavigationChange }); + const error = failNext('pushState'); + + const visit = r.visit('#x'); + await expect(visit).rejects.toBe(error); + expect(states(onNavigationChange)).toEqual(['loading', 'idle']); + expect(window.location.hash).toBe(''); + + await r.visit('/#y'); + expect(fetch).not.toHaveBeenCalled(); + expect(window.location.hash).toBe('#y'); + }); + + it('leaves the engine ready for the next visit, as the React adapter calls it', async () => { + const page = setup(); + const initialPage = { + component: 'home', + props: { stats: null }, + deferredKeys: { stats: 'stats' }, + }; + const getElementById = page.documentStub.getElementById.getMockImplementation()!; + page.documentStub.getElementById.mockImplementation((id: string) => + id === '__VOID_PAGE_DATA__' + ? { textContent: JSON.stringify(initialPage) } + : getElementById(id), + ); + const onDeferredUpdate = vi.fn(); + const engine = await createPageNavigationEngine({ + adapter: { + createDeferred: () => ({ loading: true }), + isLoading: () => true, + resolveDeferred: (_ref, value) => value, + rejectDeferred: page.rejectDeferred, + onDeferredUpdate, + }, + components: { a: loadComponent, b: loadComponent, home: loadComponent }, + layoutTree: {}, + routeMeta: {}, + matchRoute: (url) => (url.startsWith('/a') ? 'a' : 'b'), + viewTransitions: false, + prefetchHoverDelay: 75, + prefetchDefaultCacheFor: parseCacheFor('30s'), + }); + const error = failNext('pushState'); + + const b = engine.prepareVisit('/b'); + await b.shell; + await expect(b.commit()).rejects.toBe(error); + await expect(b.commit()).resolves.toBe(false); + + // The home page still owns the Deferred values streamed into its HTML. + (window as unknown as DeferredTestWindow).__resolveDeferred('stats', 1); + expect(onDeferredUpdate).toHaveBeenLastCalledWith(expect.objectContaining({ stats: 1 })); + + const a = engine.prepareVisit('/a'); + await a.shell; + await expect(a.commit()).resolves.toBe(true); + expect(window.location.pathname).toBe('/a'); + }); + }); }); diff --git a/packages/void/test/unit/pages-scroll.test.ts b/packages/void/test/unit/pages-scroll.test.ts index b73fc3063..808d6590f 100644 --- a/packages/void/test/unit/pages-scroll.test.ts +++ b/packages/void/test/unit/pages-scroll.test.ts @@ -241,4 +241,28 @@ describe('Pages scroll restoration', () => { page.window.dispatchEvent(Object.assign(new Event('popstate'), { state })); expect(page.window.scrollY).toBe(3500); }); + + it('saves positions under the current entry when a history write fails', async () => { + const page = await setup(); + page.engine.start(); + page.engine.commitInitialRender(); + const key = page.history.state?.__voidScrollKey; + vi.stubGlobal( + 'fetch', + vi.fn(async () => + Response.json({ component: 'home', props: {} }, { headers: { 'X-VoidPages': 'true' } }), + ), + ); + const error = new DOMException('History blocked', 'SecurityError'); + page.history.pushState.mockImplementationOnce(() => { + throw error; + }); + const visit = page.engine.prepareVisit('/next'); + await visit.shell; + await expect(visit.commit()).rejects.toBe(error); + + page.window.scrollY = 640; + page.window.dispatchEvent(new Event('pagehide')); + expect([...page.storage]).toEqual([[`void:scroll:${key}`, '{"x":0,"y":640}']]); + }); });