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}']]); + }); });