Skip to content

Commit b37d887

Browse files
Brooooooklynclaude
andauthored
fix(pages): keep the committed page when a history write fails (#598)
Fixes #562. A Pages navigation wrote history last. When `pushState` or `replaceState` threw (Safari's rate limit, a `SecurityError`), the new page had already taken over, but the address bar still showed the old URL and the visit never went idle. ``` before after ────── ───── render B render B commit B: commit B: dispose A, hand over Deferred values write history ── throws ─┐ head, title, scroll key → B (nothing else changed) │ write history ── throws dispose B only │ show A again ◄────────────┘ result: page B, URL A, head B, result: page A, URL A, head A, A's Deferred values gone, visit rejects with the error, stuck on "loading" navigation goes idle ``` ## What changed ``` navigation-engine.ts write history first, right after the #507 guard on throw: dispose only the new page, rethrow scroll-restoration.ts take the new scroll key only once the entry exists client.ts (Vue, Svelte, Solid) put the committed page back when the last visit to settle finds a failed one on screen react pages-client.ts on a failed commit: end the visit, dispatch 'fail' (shows the committed page), reject router.visit() hash visits a failed write rejects the visit (loading → idle) instead of throwing out of router.visit() ``` The engine has to write first. Disposing the old page, handing over its Deferred values and updating the head can't be undone, so a rollback after the fact isn't possible. Overlapping visits: ``` B renders ── B's write fails C still loading → B stays until C settles C commits → C shown (normal) C fails too → A shown again hash visit settles while B renders, then B's write fails → A shown again ``` Kept as before: the #507 guard runs first, #506 (history for every visit that isn't back/forward), and #527/#566 (a same-URL form submission replaces its entry). ## Tests - `pages-client.test.ts`, new `failed history writes (#562)` block. It covers a failed push, a failed replace, a same-URL POST (form state kept), a working visit after a failure, an older visit failing while a newer one loads (the newer one commits, or fails too), a hash visit settling first, a failed hash visit, and the engine called the way React calls it (a second `commit()` is a no-op, and the old page still gets its streamed Deferred values). - `pages-scroll.test.ts`: after a failed write, positions are saved under the current entry's key. - `react-pages-client.test.ts` (browser): a Link push and a `replace` Link whose write fails. The page stays, the URL stays, state goes idle, and the next click works. - Mutation checks: each source change was put back to `main` one at a time (engine, core client, the hash-visit-first branch, hash shell, scroll key, React adapter). Each one made a test fail. - `vp run check`, `vp lint`, `vp fmt`, `vp run test` and one full `vp test run` pass. ## Choices to check - **React `useForm` has already reported success** when the shell arrives, before the commit. If the write then fails, the form keeps its result, the page rolls back, and the error goes to `reportError` so it isn't dropped silently. Making `useForm` wait for the commit would change its timing for every submission. - **The head and title now change after the history entry is written**, not before. Browsers ignore the `title` argument, so this shouldn't show. - **Not in this PR:** Vue, Svelte and Solid `<Form>`/Link don't catch a rejected visit, so a failed write there is an unhandled rejection (as with any other visit error today). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 parent 7cbb16e commit b37d887

7 files changed

Lines changed: 413 additions & 25 deletions

File tree

‎packages/react/src/runtime/pages-client.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -487,11 +487,25 @@ function App({
487487
dispatch({ type: 'finish', resource, location: currentUrl() });
488488
})
489489
.catch((error) => {
490+
// The history write failed, so the address bar still shows the committed page.
491+
// End this visit and show that page again; a newer visit keeps its own state.
492+
const resolveViewTransition = viewTransitionWaiters.get(resource.id);
493+
if (resolveViewTransition) {
494+
viewTransitionWaiters.delete(resource.id);
495+
resolveViewTransition();
496+
}
490497
const visitSettler = pendingVisitSettlers.get(resource.id);
491498
if (visitSettler) {
492499
pendingVisitSettlers.delete(resource.id);
493500
visitSettler.reject(error);
501+
} else {
502+
// A form visit has already settled with its action result.
503+
reportError(error);
504+
}
505+
if (activePendingNavigationId === resource.id) {
506+
activePendingNavigationId = null;
494507
}
508+
dispatch({ type: 'fail', resource, location: currentUrl() });
495509
});
496510
},
497511
[],

‎packages/react/test/integration/react-pages-client.test.ts‎

Lines changed: 53 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { execSync, type ChildProcess } from 'node:child_process';
22
import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
33
import { join } from 'node:path';
44
import { type Browser, type Page } from 'playwright';
5-
import { describe, expect, it, beforeAll, afterAll } from 'vite-plus/test';
5+
import { describe, expect, it, beforeAll, afterAll, vi } from 'vite-plus/test';
66
import {
77
killChild,
88
spawnPlaygroundServer,
@@ -594,6 +594,58 @@ function registerClientTests(getBase: () => string, coverage: 'smoke' | 'full')
594594
await page.context().close();
595595
}
596596
});
597+
// The next pushState or replaceState call throws, like a browser that rate-limits history.
598+
async function failNextHistoryWrite(page: Page, method: 'pushState' | 'replaceState') {
599+
await page.evaluate((name) => {
600+
const original = History.prototype[name];
601+
History.prototype[name] = function () {
602+
History.prototype[name] = original;
603+
throw new DOMException('History blocked by test', 'SecurityError');
604+
};
605+
}, method);
606+
}
607+
608+
async function waitForIdle(page: Page) {
609+
await page.waitForFunction(
610+
() => document.querySelector('[data-testid="navigation-state"]')?.textContent === 'idle',
611+
);
612+
}
613+
614+
for (const [link, method] of [
615+
['onclick-link', 'pushState'],
616+
['replace-link', 'replaceState'],
617+
] as const) {
618+
it(`${link} keeps the current page when ${method} fails`, async () => {
619+
const page = await newPage();
620+
const errors: Array<string> = [];
621+
page.on('pageerror', (error) => {
622+
errors.push(error.message);
623+
});
624+
625+
try {
626+
await page.goto(BASE + '/link-behavior');
627+
await waitForHydration(page);
628+
const heading = await page.textContent('h1');
629+
await failNextHistoryWrite(page, method);
630+
631+
await page.click(`[data-testid="${link}"]`);
632+
await vi.waitFor(() => expect(errors).toContain('History blocked by test'), {
633+
timeout: 10_000,
634+
});
635+
await waitForIdle(page);
636+
expect(await page.textContent('h1')).toBe(heading);
637+
expect(page.url()).toBe(BASE + '/link-behavior');
638+
639+
await page.click(`[data-testid="${link}"]`);
640+
await page.waitForURL(BASE + '/users');
641+
await page.waitForFunction(() =>
642+
document.querySelector('h1')?.textContent?.includes('Users'),
643+
);
644+
} finally {
645+
await page.context().close();
646+
}
647+
});
648+
}
597649
}
598650

599651
describe('react-pages client (dev)', () => {

‎packages/void/src/pages/client.ts‎

Lines changed: 63 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {
1212
createPageNavigationEngine,
1313
type PageData,
1414
type PageNavigationEngineConfig,
15+
type PageNavigationResult,
1516
type VisitOptions,
1617
type VoidRouterAdapter as PageNavigationAdapter,
1718
type PrefetchOptions,
@@ -335,6 +336,18 @@ export async function createVoidRouter(config: VoidRouterConfig): Promise<{
335336
let routeUrl = currentUrl();
336337
let routeParams = engine.initialPageData.params ?? {};
337338
let activeNavigationId: number | null = null;
339+
// The page in the address bar, and the page on screen. They differ while a visit has
340+
// rendered but not committed, and after its commit fails.
341+
type RenderedPage = Pick<PageNavigationResult, 'pageData' | 'component' | 'layouts'>;
342+
let committedPage: RenderedPage = {
343+
pageData: engine.initialPageData,
344+
component: engine.initialComponent,
345+
layouts: engine.initialLayouts,
346+
};
347+
let renderedPage = committedPage;
348+
// Set when the page on screen is one whose commit failed; the last visit to settle puts
349+
// the committed page back.
350+
let failedRender: { preserveState: boolean } | null = null;
338351
const setRouteState = (url: string, params: Record<string, string> = routeParams) => {
339352
routeUrl = url;
340353
routeParams = params;
@@ -358,10 +371,14 @@ export async function createVoidRouter(config: VoidRouterConfig): Promise<{
358371
result.pageData.params ?? {},
359372
);
360373

361-
const applyUpdate = () =>
362-
config.adapter.applyUpdate(result.pageData, result.component, result.layouts, {
363-
preserveState: resource.options.preserveState ?? resource.method !== 'GET',
374+
const preserveState = resource.options.preserveState ?? resource.method !== 'GET';
375+
const applyUpdate = () => {
376+
renderedPage = result;
377+
failedRender = null;
378+
return config.adapter.applyUpdate(result.pageData, result.component, result.layouts, {
379+
preserveState,
364380
});
381+
};
365382

366383
let applied = true;
367384
const useTransition = resource.options.viewTransition ?? config.viewTransitions;
@@ -380,9 +397,24 @@ export async function createVoidRouter(config: VoidRouterConfig): Promise<{
380397
}
381398

382399
// A newer visit can commit while this page is being applied; its URL stays.
383-
if (!applied || !(await resource.commit())) {
400+
let committed = false;
401+
if (applied) {
402+
try {
403+
committed = await resource.commit();
404+
} catch (error) {
405+
if (renderedPage === result) {
406+
failedRender = { preserveState };
407+
}
408+
throw error;
409+
}
410+
}
411+
if (!committed) {
384412
throw new DOMException('Navigation aborted', 'AbortError');
385413
}
414+
committedPage = result;
415+
if (renderedPage === result) {
416+
failedRender = null;
417+
}
386418
if (activeNavigationId === resource.id) {
387419
setRouteState(currentUrl(), result.pageData.params ?? {});
388420
}
@@ -397,10 +429,33 @@ export async function createVoidRouter(config: VoidRouterConfig): Promise<{
397429
}
398430
throw error;
399431
} finally {
400-
if (activeNavigationId === resource.id) {
401-
activeNavigationId = null;
402-
setRouteState(currentUrl());
403-
setNavigation(idleNavigationState());
432+
// A same-page hash visit can settle before this one, so no visit may be active.
433+
const ownsNavigation = activeNavigationId === resource.id;
434+
if (ownsNavigation || activeNavigationId === null) {
435+
let restored = false;
436+
if (failedRender && renderedPage !== committedPage) {
437+
const { preserveState } = failedRender;
438+
failedRender = null;
439+
renderedPage = committedPage;
440+
restored = true;
441+
const page = committedPage;
442+
try {
443+
await config.adapter.applyUpdate(page.pageData, page.component, page.layouts, {
444+
preserveState,
445+
});
446+
} catch (error) {
447+
// The visit already rejects with the history error; keep that one.
448+
reportError(error);
449+
}
450+
}
451+
// A newer visit may have started while the committed page was applied.
452+
if (ownsNavigation && activeNavigationId === resource.id) {
453+
activeNavigationId = null;
454+
setRouteState(currentUrl(), restored ? (committedPage.pageData.params ?? {}) : undefined);
455+
setNavigation(idleNavigationState());
456+
} else if (restored && activeNavigationId === null) {
457+
setRouteState(currentUrl(), committedPage.pageData.params ?? {});
458+
}
404459
}
405460
}
406461
}

‎packages/void/src/pages/navigation-engine.ts‎

Lines changed: 29 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -598,10 +598,10 @@ export async function createPageNavigationEngine(
598598
options: VisitOptions,
599599
writeHistory: boolean,
600600
): void {
601-
_currentFetchUrl = effectiveFetchUrl;
602601
if (writeHistory) {
603602
scrollRestoration.writeHistory(effectiveFetchUrl + hash, !!options.replace);
604603
}
604+
_currentFetchUrl = effectiveFetchUrl;
605605
scrollRestoration.commit();
606606
restoreScroll(options._scrollPosition, hash);
607607
}
@@ -864,6 +864,26 @@ export async function createPageNavigationEngine(
864864
if (resourceId < _urlNavigationId) {
865865
return false;
866866
}
867+
// Write history before anything else changes. Disposing the previous page and handing
868+
// over its Deferred values cannot be undone, so a failed write must find them untouched.
869+
// preserveState only controls component state; back/forward traversal already moved history.
870+
if (!options._popstate) {
871+
const historyUrl = commitTarget.fetchUrl + commitTarget.hash;
872+
const resource = getResource();
873+
const currentDocumentUrl = window.location.pathname + window.location.search;
874+
const replaceHistory =
875+
!!options.replace ||
876+
(resource.method !== 'GET' && historyUrl === currentDocumentUrl + commitTarget.hash);
877+
try {
878+
scrollRestoration.writeHistory(historyUrl, replaceHistory);
879+
} catch (error) {
880+
if (_pendingResource?.id === resourceId) {
881+
_pendingResource = null;
882+
}
883+
resource.dispose();
884+
throw error;
885+
}
886+
}
867887
_urlNavigationId = resourceId;
868888
if (_pendingResource?.id === resourceId) {
869889
_pendingResource = null;
@@ -910,17 +930,6 @@ export async function createPageNavigationEngine(
910930
}
911931
applyClientHead(result.pageData.head as HeadDescriptor | undefined);
912932
_currentFetchUrl = commitTarget.fetchUrl;
913-
914-
// preserveState only controls component state; back/forward traversal already moved history.
915-
if (!options._popstate) {
916-
const historyUrl = commitTarget.fetchUrl + commitTarget.hash;
917-
const resource = getResource();
918-
const currentDocumentUrl = window.location.pathname + window.location.search;
919-
const replaceHistory =
920-
!!options.replace ||
921-
(resource.method !== 'GET' && historyUrl === currentDocumentUrl + commitTarget.hash);
922-
scrollRestoration.writeHistory(historyUrl, replaceHistory);
923-
}
924933
scrollRestoration.commit();
925934

926935
requestAnimationFrame(() => {
@@ -989,7 +998,13 @@ export async function createPageNavigationEngine(
989998
Boolean(hash || options._popstate);
990999

9911000
if (isSamePageHashNavigation) {
992-
commitHashNavigation(effectiveFetchUrl, hash, options, !options._popstate);
1001+
// A failed history write ends this visit like any other failed visit: through its shell.
1002+
let shell: Promise<undefined> = Promise.resolve(undefined);
1003+
try {
1004+
commitHashNavigation(effectiveFetchUrl, hash, options, !options._popstate);
1005+
} catch (error) {
1006+
shell = Promise.reject(error);
1007+
}
9931008
return {
9941009
id,
9951010
url,
@@ -998,7 +1013,7 @@ export async function createPageNavigationEngine(
9981013
method,
9991014
options,
10001015
immediate: true,
1001-
shell: Promise.resolve(undefined),
1016+
shell,
10021017
abort() {},
10031018
// Same-page hash visits commit right away. They do not stop an older visit
10041019
// whose page is already on screen from committing its URL.

‎packages/void/src/pages/scroll-restoration.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -95,13 +95,15 @@ export function createScrollRestoration(): ScrollRestoration {
9595

9696
writeHistory(url: string, replace: boolean): void {
9797
// A replaced entry keeps its key: its position is saved again when it is left.
98-
renderedKey = (replace ? readKey(history.state) : null) ?? newKey();
99-
const state = { ...(replace ? history.state : {}), __voidScrollKey: renderedKey };
98+
const key = (replace ? readKey(history.state) : null) ?? newKey();
99+
const state = { ...(replace ? history.state : {}), __voidScrollKey: key };
100100
if (replace) {
101101
history.replaceState(state, '', url);
102102
} else {
103103
history.pushState(state, '', url);
104104
}
105+
// Only once the entry exists: if the write throws, positions keep the current entry's key.
106+
renderedKey = key;
105107
},
106108

107109
commit(): void {

0 commit comments

Comments
 (0)