Skip to content

Commit cd480dd

Browse files
Brooooooklynclaude
andauthored
fix(pages): keep a newer visit's URL when an older one finishes later (#548)
Fixes #507 ## Summary An older visit whose page update was still running could commit after a newer visit had already committed. It then wrote its URL, head and scroll over the newer page, and disposed the newer page's Deferred props. ``` before: visit('/a') ── applyUpdate(A) ……… slow ……… commit(A) ✗ URL /a, title A, B's Deferred rejected visit('/b') ── applyUpdate(B) ── commit(B) page /b after: commit(A) sees that a newer visit already set the URL → skipped, commit() returns false ``` ### Rule: the address bar only moves forward `makeCommit()` in `packages/void/src/pages/navigation-engine.ts` tracks the newest visit whose URL is in the address bar: | sets it | why | | --- | --- | | a successful commit | its URL was just written | | a back/forward visit, when it starts | the browser has already moved | | ~~a same-page hash visit~~ | it renders nothing, so a page already on screen still commits its URL | An older visit commits only if no newer visit has set the URL. `resource.commit()` now resolves `true`/`false`. ### Cases | case | result | | --- | --- | | A finishes after B committed (the issue) | A skipped; page, URL, head, scroll and B's Deferred props stay on B | | A finishes while B is still loading | A commits `/a` (its page is on screen), then B commits `/b` | | the new page's mount code calls `visit('/b', { replace: true })` | A pushes `/a`, B replaces it; the previous entry is kept | | Back pressed while A is being applied | A never pushes, so forward history is kept | | a hash link (or Back to a same-page hash entry) during A's update | A still commits `/a`, as before, so page and URL match | I first tried "skip A if any newer visit started" (what Inertia and SvelteKit do). Review showed it breaks redirects made from a page's mount code: the redirect's `replace` then overwrote the previous page's entry. So the rule only looks at visits that have already set the URL. ### Callers - **`client.ts`** (Vue, Svelte, Solid): a skipped visit throws an AbortError inside `applyNavigationResource`. The existing catch settles it like any abandoned visit: GET resolves `undefined`, other methods reject with AbortError (`useForm` already ignores that). A view-transition callback that runs after a newer visit started now skips `applyUpdate`, so the older page cannot paint over the newer one. - **React** (`pages-client.ts`): `finishNavigation` uses the result. A skipped visit settles the same way and does not dispatch `finish`. No docs change: nothing changes for users except the bug. ## Tests New `describe('superseded navigation commits (#507)')` in `packages/void/test/unit/pages-client.test.ts` (10 cases): - normal visits in order (control) - the issue's repro: URL, `router.url`, title, scroll and B's Deferred prop stay on B - A finishing before B commits - a redirect started from the new page's mount - a hash visit during A's update, and Back to a same-page hash entry during it - Back to another page during A's update - a superseded form submission rejects with AbortError - a late view-transition callback - the engine called the way the React adapter calls it Each guard has a test that fails when the guard is removed. 5 of them fail on `main`. `vp test run` passed except one platform browser test that timed out at 5s under full load. That test runs with JavaScript turned off, so this change doesn't reach it; it passes alone and in its project run twice. `vp run test`, `vp run check`, `vp lint` and `vp fmt` pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 parent 70c275d commit cd480dd

4 files changed

Lines changed: 330 additions & 10 deletions

File tree

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

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -437,6 +437,7 @@ function App({
437437
// with the browser URL, so one that renders the query falls back to a client render.
438438
React.startTransition(() => setHydrated(true));
439439
}
440+
return true;
440441
},
441442
dispose() {},
442443
}),
@@ -459,7 +460,7 @@ function App({
459460
committedResources.add(resource.id);
460461
resource
461462
.commit()
462-
.then(() => {
463+
.then((committed) => {
463464
const resolveViewTransition = viewTransitionWaiters.get(resource.id);
464465
if (resolveViewTransition) {
465466
viewTransitionWaiters.delete(resource.id);
@@ -468,7 +469,17 @@ function App({
468469
const visitSettler = pendingVisitSettlers.get(resource.id);
469470
if (visitSettler) {
470471
pendingVisitSettlers.delete(resource.id);
471-
visitSettler.resolve(result.pageData);
472+
if (committed) {
473+
visitSettler.resolve(result.pageData);
474+
} else if (resource.method.toUpperCase() === 'GET') {
475+
// A newer visit committed first. Settle like any abandoned visit.
476+
visitSettler.resolve(undefined);
477+
} else {
478+
visitSettler.reject(new DOMException('Navigation aborted', 'AbortError'));
479+
}
480+
}
481+
if (!committed) {
482+
return;
472483
}
473484
if (activePendingNavigationId === resource.id) {
474485
activePendingNavigationId = null;

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

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -357,15 +357,26 @@ export async function createVoidRouter(config: VoidRouterConfig): Promise<{
357357
preserveState: resource.options.preserveState ?? resource.method !== 'GET',
358358
});
359359

360+
let applied = true;
360361
const useTransition = resource.options.viewTransition ?? config.viewTransitions;
361362
if (useTransition && document.startViewTransition) {
362-
const transition = document.startViewTransition(() => applyUpdate());
363+
// The callback runs on a later frame. A newer visit started by then renders instead.
364+
const transition = document.startViewTransition(() => {
365+
if (activeNavigationId !== resource.id) {
366+
applied = false;
367+
return;
368+
}
369+
return applyUpdate();
370+
});
363371
await transition.updateCallbackDone;
364372
} else {
365373
await applyUpdate();
366374
}
367375

368-
await resource.commit();
376+
// A newer visit can commit while this page is being applied; its URL stays.
377+
if (!applied || !(await resource.commit())) {
378+
throw new DOMException('Navigation aborted', 'AbortError');
379+
}
369380
if (activeNavigationId === resource.id) {
370381
setRouteState(currentUrl(), result.pageData.params ?? {});
371382
}

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

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,8 @@ export type PageNavigationResource = {
104104
immediate: boolean;
105105
shell: Promise<PageNavigationResult | undefined>;
106106
abort(): void;
107-
commit(): Promise<void>;
107+
/** Resolves false when a newer visit has already set the URL, so nothing was committed. */
108+
commit(): Promise<boolean>;
108109
dispose(): void;
109110
};
110111

@@ -252,6 +253,9 @@ export async function createPageNavigationEngine(
252253

253254
const _prefetchCache = new PrefetchCache();
254255
let _nextNavigationId = 0;
256+
// The newest visit whose URL is in the address bar: the last commit, or a back/forward
257+
// visit, which the browser has already moved to. An older visit must not commit over it.
258+
let _urlNavigationId = 0;
255259
let _pendingResource: PageNavigationResource | null = null;
256260
let _committedResource: PageNavigationResource | null = null;
257261
let _committedDeferredRegistry: DeferredRegistry | null = null;
@@ -853,17 +857,23 @@ export async function createPageNavigationEngine(
853857
deferredRegistry: DeferredRegistry,
854858
resourceId: number,
855859
getResource: () => PageNavigationResource,
856-
): () => Promise<void> {
860+
): () => Promise<boolean> {
857861
let committed = false;
858862
return async () => {
859863
if (committed) {
860-
return;
864+
return false;
861865
}
862866
committed = true;
863867
const result = await shell;
864868
if (!result) {
865-
return;
869+
return false;
870+
}
871+
// A newer visit committed while this one was being applied. Committing now would put
872+
// back this page's URL, head, scroll and Deferred ownership over the newer page.
873+
if (resourceId < _urlNavigationId) {
874+
return false;
866875
}
876+
_urlNavigationId = resourceId;
867877
if (_pendingResource?.id === resourceId) {
868878
_pendingResource = null;
869879
}
@@ -922,6 +932,7 @@ export async function createPageNavigationEngine(
922932
restoreScroll(options._scrollPosition, commitTarget.hash);
923933
}
924934
});
935+
return true;
925936
};
926937
}
927938

@@ -993,11 +1004,21 @@ export async function createPageNavigationEngine(
9931004
immediate: true,
9941005
shell: Promise.resolve(undefined),
9951006
abort() {},
996-
async commit() {},
1007+
// Same-page hash visits commit right away. They do not stop an older visit
1008+
// whose page is already on screen from committing its URL.
1009+
async commit() {
1010+
return true;
1011+
},
9971012
dispose() {},
9981013
};
9991014
}
10001015

1016+
if (options._popstate) {
1017+
// The browser has already moved to this entry. A same-page hash entry is left out
1018+
// above: it renders nothing, so an older page on screen still commits its URL.
1019+
_urlNavigationId = id;
1020+
}
1021+
10011022
const controller = new AbortController();
10021023
const deferredRegistry = new DeferredRegistry();
10031024
const commitTarget = { fetchUrl: effectiveFetchUrl, hash };
@@ -1012,7 +1033,7 @@ export async function createPageNavigationEngine(
10121033
deferredRegistry,
10131034
);
10141035

1015-
let commitResource: () => Promise<void> = async () => {};
1036+
let commitResource: () => Promise<boolean> = async () => false;
10161037
const stop = () => {
10171038
controller.abort(abortError());
10181039
deferredRegistry.dispose();

0 commit comments

Comments
 (0)