mirror of
https://github.com/multica-ai/multica.git
synced 2026-08-03 03:03:56 +02:00
fix(web): answer canGoBack from the browser alone, never from push counts
Review found the counter still lied, one level deeper: it counted `router.push` calls, and a call is not a committed history entry. Next drops a push to `replaceState` when the canonical URL is unchanged (app-router.js, the `pendingPush && href !== canonicalUrl` branch), so clicking a self-link — the breadcrumb on an issue detail page, a pin to the issue you are on — incremented depth with no entry behind it, and a delete from there could still step off the app. Fixing the count needs per-entry markers, which means depending on both React effect ordering (our provider's effect runs before app-router's history commit) and Next's own preserveCustomHistoryState behaviour. Two rounds of review have now found holes in hand-rolled history tracking; a third layer of it is not the way to buy this guarantee. So stop deriving. `canGoBack` is the Navigation API's answer or `false`. Browsers without it take the fallback path, which is exactly what they did before any of this existed — nobody regresses, and the "wrong true walks the user out of the app" failure is now unreachable by construction. Drops the tracker, the popstate wiring and the push wrapper: the `multica:navigate` bridge returns to its original shape. Co-authored-by: multica-agent <github@multica.ai>
This commit is contained in:
Bohan-J
@@ -1,107 +1,47 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { createInAppHistoryTracker, probeNavigationApi } from "./in-app-history";
|
||||
import { afterEach, describe, expect, it } from "vitest";
|
||||
import { canGoBackInApp } from "./in-app-history";
|
||||
|
||||
describe("createInAppHistoryTracker", () => {
|
||||
it("trusts the Navigation API when the browser exposes it", () => {
|
||||
expect(createInAppHistoryTracker(() => true).canGoBack()).toBe(true);
|
||||
// False even though nothing was pushed is the point: arriving from an
|
||||
// external site leaves same-origin history empty.
|
||||
expect(createInAppHistoryTracker(() => false).canGoBack()).toBe(false);
|
||||
});
|
||||
const win = window as unknown as { navigation?: unknown };
|
||||
|
||||
it("ignores its own depth once the Navigation API answers", () => {
|
||||
const tracker = createInAppHistoryTracker(() => false);
|
||||
tracker.recordPush();
|
||||
function withNavigationApi(navigation: unknown) {
|
||||
win.navigation = navigation;
|
||||
}
|
||||
|
||||
expect(tracker.canGoBack()).toBe(false);
|
||||
});
|
||||
|
||||
describe("without the Navigation API", () => {
|
||||
const makeTracker = () => createInAppHistoryTracker(() => undefined);
|
||||
|
||||
it("reports nothing to go back to before any in-app push", () => {
|
||||
expect(makeTracker().canGoBack()).toBe(false);
|
||||
});
|
||||
|
||||
it("reports history once the adapter has pushed in this document", () => {
|
||||
const tracker = makeTracker();
|
||||
tracker.recordPush();
|
||||
|
||||
expect(tracker.canGoBack()).toBe(true);
|
||||
});
|
||||
|
||||
// Regression: the depth used to only ever grow, so this sequence left the
|
||||
// user on the document's first entry still claiming a page behind it —
|
||||
// and `back()` would then step off Multica entirely.
|
||||
it("reports no history after a push is undone by a browser Back", () => {
|
||||
const tracker = makeTracker();
|
||||
tracker.recordPush();
|
||||
tracker.recordTraversal();
|
||||
|
||||
expect(tracker.canGoBack()).toBe(false);
|
||||
});
|
||||
|
||||
it("stays negative-proof across more traversals than pushes", () => {
|
||||
const tracker = makeTracker();
|
||||
tracker.recordPush();
|
||||
tracker.recordTraversal();
|
||||
tracker.recordTraversal();
|
||||
tracker.recordTraversal();
|
||||
|
||||
expect(tracker.canGoBack()).toBe(false);
|
||||
|
||||
// One push must be enough to make it answer again — a clamped-out
|
||||
// counter that had gone deeply negative would swallow it.
|
||||
tracker.recordPush();
|
||||
expect(tracker.canGoBack()).toBe(true);
|
||||
});
|
||||
|
||||
it("keeps reporting history while pushes outnumber traversals", () => {
|
||||
const tracker = makeTracker();
|
||||
tracker.recordPush();
|
||||
tracker.recordPush();
|
||||
tracker.recordTraversal();
|
||||
|
||||
expect(tracker.canGoBack()).toBe(true);
|
||||
});
|
||||
|
||||
// Forward re-enters an entry that does have a page behind it, but we
|
||||
// cannot tell forward from back without the Navigation API. Reporting
|
||||
// "no history" here costs a legitimate back() and is the safe direction.
|
||||
it("is conservative after a browser Forward", () => {
|
||||
const tracker = makeTracker();
|
||||
tracker.recordPush();
|
||||
tracker.recordTraversal(); // back
|
||||
tracker.recordTraversal(); // forward
|
||||
|
||||
expect(tracker.canGoBack()).toBe(false);
|
||||
});
|
||||
});
|
||||
afterEach(() => {
|
||||
delete win.navigation;
|
||||
});
|
||||
|
||||
describe("probeNavigationApi", () => {
|
||||
it("returns undefined when the browser has no Navigation API", () => {
|
||||
// jsdom ships no `window.navigation`, which is the fallback path itself.
|
||||
expect(probeNavigationApi()).toBeUndefined();
|
||||
describe("canGoBackInApp", () => {
|
||||
it("reports the Navigation API's answer when the browser has one", () => {
|
||||
withNavigationApi({ canGoBack: true });
|
||||
|
||||
expect(canGoBackInApp()).toBe(true);
|
||||
});
|
||||
|
||||
it("reads canGoBack when the Navigation API is present", () => {
|
||||
const win = window as unknown as { navigation?: unknown };
|
||||
win.navigation = { canGoBack: true };
|
||||
try {
|
||||
expect(probeNavigationApi()).toBe(true);
|
||||
} finally {
|
||||
delete win.navigation;
|
||||
}
|
||||
// Not "nothing was pushed yet" — arriving from an external site leaves the
|
||||
// same-origin run empty, which is the whole reason we ask the browser.
|
||||
it("reports false when the Navigation API says there is no same-origin entry", () => {
|
||||
withNavigationApi({ canGoBack: false });
|
||||
|
||||
expect(canGoBackInApp()).toBe(false);
|
||||
});
|
||||
|
||||
it("ignores a `navigation` global that is not the Navigation API", () => {
|
||||
const win = window as unknown as { navigation?: unknown };
|
||||
win.navigation = { somethingElse: true };
|
||||
try {
|
||||
expect(probeNavigationApi()).toBeUndefined();
|
||||
} finally {
|
||||
delete win.navigation;
|
||||
}
|
||||
// jsdom ships no `window.navigation`. Callers then take their fallback path,
|
||||
// which is what these browsers did before any of this existed — deliberately
|
||||
// no guessing, because a wrong `true` walks the user out of the app.
|
||||
it("reports false when the browser has no Navigation API", () => {
|
||||
expect(canGoBackInApp()).toBe(false);
|
||||
});
|
||||
|
||||
it("reports false for a `navigation` global that is not the Navigation API", () => {
|
||||
withNavigationApi({ somethingElse: true });
|
||||
|
||||
expect(canGoBackInApp()).toBe(false);
|
||||
});
|
||||
|
||||
it("reports false for a non-boolean canGoBack", () => {
|
||||
withNavigationApi({ canGoBack: "yes" });
|
||||
|
||||
expect(canGoBackInApp()).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -3,59 +3,37 @@
|
||||
*
|
||||
* Callers that want to step back (deleting an issue returns the user to the
|
||||
* list they opened it from) must not step back off the app when the current
|
||||
* page was opened cold — a shared link, a pasted URL, a new tab. The browser
|
||||
* knows the answer; `history.length` does not, because it counts entries from
|
||||
* other origins too.
|
||||
* page was opened cold — a shared link, a pasted URL, a new tab.
|
||||
*
|
||||
* The Navigation API answers exactly the right question: `entries()` spans
|
||||
* only the contiguous same-origin run around the current entry, so arriving
|
||||
* from an external site reports `canGoBack === false` even though the browser
|
||||
* technically has somewhere to go.
|
||||
* Only the browser can answer this, and exactly one API does: the Navigation
|
||||
* API's `entries()` spans just the contiguous same-origin run around the
|
||||
* current entry, so arriving from an external site reports `canGoBack` false
|
||||
* even though the browser technically has somewhere to go. `history.length`
|
||||
* is no substitute — it counts other origins' entries too.
|
||||
*
|
||||
* Where it is unavailable we track our own depth: how many in-app pushes are
|
||||
* still "below" the current entry. A push adds one; any history traversal
|
||||
* (browser Back/Forward, or our own `back()`) may have moved us off that
|
||||
* entry, so it takes one away. Since reaching the document's first entry
|
||||
* requires traversing back at least as many times as we pushed, a positive
|
||||
* depth can never be claimed while sitting on it — the failure direction is
|
||||
* always "report no history and use the fallback", never "step off the app".
|
||||
* Going forward again is the cost: the depth stays spent, so we fall back
|
||||
* where a step back would in fact have been fine.
|
||||
* Nothing else here is a guess. Counting the adapter's own `router.push`
|
||||
* calls looks like a workable fallback and is not: a push is a request, not a
|
||||
* committed history entry. Next drops it to `replaceState` when the canonical
|
||||
* URL is unchanged (`app-router.js`, the `pendingPush && href !== canonicalUrl`
|
||||
* branch), and pushes can also be superseded or abandoned mid-transition. Any
|
||||
* count derived from calls can therefore claim history that does not exist,
|
||||
* and a wrong `true` walks the user out of Multica — the precise failure this
|
||||
* exists to prevent. So where the browser cannot answer we report `false`,
|
||||
* and callers take their fallback path.
|
||||
*/
|
||||
|
||||
/** Minimal shape of the Navigation API — not in TypeScript's DOM lib yet. */
|
||||
type NavigationApi = { canGoBack: boolean };
|
||||
|
||||
/** `undefined` when the browser can't tell us, so callers can fall back. */
|
||||
export function probeNavigationApi(): boolean | undefined {
|
||||
if (typeof window === "undefined") return undefined;
|
||||
/**
|
||||
* Whether a step back stays inside the app. `false` whenever the browser has
|
||||
* no Navigation API: the caller then navigates to its fallback, which is the
|
||||
* behaviour those browsers had before any of this existed.
|
||||
*/
|
||||
export function canGoBackInApp(): boolean {
|
||||
if (typeof window === "undefined") return false;
|
||||
const navigation = (window as { navigation?: unknown }).navigation as
|
||||
| NavigationApi
|
||||
| undefined;
|
||||
return typeof navigation?.canGoBack === "boolean"
|
||||
? navigation.canGoBack
|
||||
: undefined;
|
||||
}
|
||||
|
||||
export function createInAppHistoryTracker(
|
||||
probe: () => boolean | undefined = probeNavigationApi,
|
||||
) {
|
||||
let depth = 0;
|
||||
return {
|
||||
/** An in-app push: the entry it creates has this page behind it. */
|
||||
recordPush(): void {
|
||||
depth += 1;
|
||||
},
|
||||
/**
|
||||
* A history traversal — `popstate`. We can't tell forward from back
|
||||
* without the Navigation API, so assume the direction that can only cost
|
||||
* us a `back()` we were entitled to, never one we weren't.
|
||||
*/
|
||||
recordTraversal(): void {
|
||||
depth = Math.max(0, depth - 1);
|
||||
},
|
||||
canGoBack(): boolean {
|
||||
return probe() ?? depth > 0;
|
||||
},
|
||||
};
|
||||
return navigation?.canGoBack === true;
|
||||
}
|
||||
|
||||
@@ -6,8 +6,8 @@
|
||||
* deployment's own origin. Desktop answers it by opening a tab; the web must
|
||||
* answer it with a router push, or those links silently do nothing.
|
||||
*/
|
||||
import { describe, expect, it, vi, beforeEach } from "vitest";
|
||||
import { act, render } from "@testing-library/react";
|
||||
import { describe, expect, it, vi, afterEach, beforeEach } from "vitest";
|
||||
import { render } from "@testing-library/react";
|
||||
|
||||
const router = vi.hoisted(() => ({
|
||||
push: vi.fn(),
|
||||
@@ -31,10 +31,6 @@ function navigate(path: string) {
|
||||
);
|
||||
}
|
||||
|
||||
function popstate() {
|
||||
window.dispatchEvent(new PopStateEvent("popstate"));
|
||||
}
|
||||
|
||||
function renderAdapter(): () => NavigationAdapter {
|
||||
let adapter: NavigationAdapter | null = null;
|
||||
function Probe() {
|
||||
@@ -84,59 +80,46 @@ describe("WebNavigationProvider internal link bridge", () => {
|
||||
|
||||
/**
|
||||
* `canGoBack` decides whether a page whose subject was just deleted steps back
|
||||
* or replaces with a fallback. Getting it wrong in the optimistic direction
|
||||
* walks the user out of Multica, so the wiring — not just the tracker — needs
|
||||
* covering. jsdom has no Navigation API, which is exactly the fallback path.
|
||||
* or replaces with a fallback. A wrong `true` walks the user out of Multica,
|
||||
* so the adapter must expose the browser's own answer and nothing derived.
|
||||
*/
|
||||
describe("WebNavigationProvider in-app history", () => {
|
||||
// The tracker is document-scoped, so normalize it instead of assuming a
|
||||
// fresh module: traversals clamp at zero, so draining always lands on "no
|
||||
// in-app history" no matter what earlier tests pushed.
|
||||
function drainToColdOpen() {
|
||||
for (let i = 0; i < 5; i += 1) popstate();
|
||||
}
|
||||
describe("WebNavigationProvider canGoBack", () => {
|
||||
const win = window as unknown as { navigation?: unknown };
|
||||
|
||||
it("reports no history for a cold open", () => {
|
||||
afterEach(() => {
|
||||
delete win.navigation;
|
||||
});
|
||||
|
||||
it("passes through the Navigation API's answer", () => {
|
||||
win.navigation = { canGoBack: true };
|
||||
|
||||
expect(renderAdapter()().canGoBack!()).toBe(true);
|
||||
});
|
||||
|
||||
it("reads the answer live rather than freezing it at render", () => {
|
||||
win.navigation = { canGoBack: true };
|
||||
const adapter = renderAdapter();
|
||||
drainToColdOpen();
|
||||
|
||||
win.navigation = { canGoBack: false };
|
||||
|
||||
expect(adapter().canGoBack!()).toBe(false);
|
||||
});
|
||||
|
||||
it("reports history once the user has navigated in-app", () => {
|
||||
// Regression (PR review, twice): a count of `push` calls stood in for real
|
||||
// history here. It could not — Next drops a push to `replaceState` when the
|
||||
// canonical URL is unchanged, so a push that committed no entry still made
|
||||
// this claim `true`. Calling push must not move this answer at all.
|
||||
it("is unmoved by a push that committed no history entry", () => {
|
||||
win.navigation = { canGoBack: false };
|
||||
const adapter = renderAdapter();
|
||||
drainToColdOpen();
|
||||
|
||||
act(() => adapter().push("/acme/issues/MUL-1"));
|
||||
|
||||
expect(adapter().canGoBack!()).toBe(true);
|
||||
});
|
||||
|
||||
// Regression (PR review): a push followed by the browser's own Back button
|
||||
// left the user on the entry the document opened at while `canGoBack` still
|
||||
// said true — `back()` from there leaves the app.
|
||||
it("reports no history after a browser Back undoes the push", () => {
|
||||
const adapter = renderAdapter();
|
||||
drainToColdOpen();
|
||||
|
||||
act(() => adapter().push("/acme/issues/MUL-1"));
|
||||
popstate();
|
||||
adapter().push("/acme/issues");
|
||||
|
||||
expect(router.push).toHaveBeenCalledWith("/acme/issues");
|
||||
expect(adapter().canGoBack!()).toBe(false);
|
||||
});
|
||||
|
||||
it("stops tracking traversals once unmounted", () => {
|
||||
const adapter = renderAdapter();
|
||||
drainToColdOpen();
|
||||
act(() => adapter().push("/acme/issues/MUL-1"));
|
||||
|
||||
const { unmount } = render(
|
||||
<WebNavigationProvider>{null}</WebNavigationProvider>,
|
||||
);
|
||||
unmount();
|
||||
// The remaining mounted provider still listens, so this must still count.
|
||||
popstate();
|
||||
|
||||
expect(adapter().canGoBack!()).toBe(false);
|
||||
it("reports false where the browser cannot answer, so callers use the fallback", () => {
|
||||
expect(renderAdapter()().canGoBack!()).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,16 +1,12 @@
|
||||
"use client";
|
||||
|
||||
import { Suspense, useCallback, useEffect } from "react";
|
||||
import { Suspense, useEffect } from "react";
|
||||
import { useRouter, usePathname, useSearchParams } from "next/navigation";
|
||||
import {
|
||||
NavigationProvider,
|
||||
type NavigationAdapter,
|
||||
} from "@multica/views/navigation";
|
||||
import { createInAppHistoryTracker } from "./in-app-history";
|
||||
|
||||
// Document-scoped, like the browser history it shadows: a remount must not
|
||||
// convince us there is somewhere to go back to.
|
||||
const inAppHistory = createInAppHistoryTracker();
|
||||
import { canGoBackInApp } from "./in-app-history";
|
||||
|
||||
/**
|
||||
* Web half of the `multica:navigate` bridge — the event shared content
|
||||
@@ -19,30 +15,16 @@ const inAppHistory = createInAppHistoryTracker();
|
||||
* equivalent is a router push in place. Without this the event has no listener
|
||||
* and such links do nothing at all.
|
||||
*/
|
||||
function useInternalLinkHandler(push: (path: string) => void) {
|
||||
function useInternalLinkHandler(router: ReturnType<typeof useRouter>) {
|
||||
useEffect(() => {
|
||||
const handler = (e: Event) => {
|
||||
const path = (e as CustomEvent<{ path?: string }>).detail?.path;
|
||||
if (!path) return;
|
||||
push(path);
|
||||
router.push(path);
|
||||
};
|
||||
window.addEventListener("multica:navigate", handler);
|
||||
return () => window.removeEventListener("multica:navigate", handler);
|
||||
}, [push]);
|
||||
}
|
||||
|
||||
/**
|
||||
* Keep the in-app depth honest about history traversals — the browser's own
|
||||
* Back/Forward buttons and `router.back()` both land here. Without it a push
|
||||
* followed by a browser Back would still look like "there is a page behind
|
||||
* us" while sitting on the entry the document opened at.
|
||||
*/
|
||||
function useHistoryTraversalTracking() {
|
||||
useEffect(() => {
|
||||
const onPopState = () => inAppHistory.recordTraversal();
|
||||
window.addEventListener("popstate", onPopState);
|
||||
return () => window.removeEventListener("popstate", onPopState);
|
||||
}, []);
|
||||
}, [router]);
|
||||
}
|
||||
|
||||
function NavigationProviderInner({
|
||||
@@ -53,21 +35,13 @@ function NavigationProviderInner({
|
||||
const router = useRouter();
|
||||
const pathname = usePathname();
|
||||
const searchParams = useSearchParams();
|
||||
const push = useCallback(
|
||||
(path: string) => {
|
||||
inAppHistory.recordPush();
|
||||
router.push(path);
|
||||
},
|
||||
[router],
|
||||
);
|
||||
useInternalLinkHandler(push);
|
||||
useHistoryTraversalTracking();
|
||||
useInternalLinkHandler(router);
|
||||
|
||||
const adapter: NavigationAdapter = {
|
||||
push,
|
||||
push: router.push,
|
||||
replace: router.replace,
|
||||
back: router.back,
|
||||
canGoBack: inAppHistory.canGoBack,
|
||||
canGoBack: canGoBackInApp,
|
||||
pathname,
|
||||
searchParams: new URLSearchParams(searchParams.toString()),
|
||||
getShareableUrl: (path: string) =>
|
||||
|
||||
@@ -15,9 +15,14 @@ import { useNavigation } from "./context";
|
||||
*
|
||||
* `fallback` covers the case where there is nothing to go back to: a shared
|
||||
* link opened cold, a new tab, a pasted URL. Stepping back from those would
|
||||
* leave Multica entirely, so we navigate to `fallback` instead. Adapters that
|
||||
* can't report history (test stubs, the desktop issue window) always take this
|
||||
* branch, which is exactly the pre-existing behaviour.
|
||||
* leave Multica entirely, so we navigate to `fallback` instead.
|
||||
*
|
||||
* It also covers every platform that cannot answer the question — the desktop
|
||||
* issue window, test stubs, and browsers with no Navigation API. That branch
|
||||
* is the behaviour those surfaces had before this hook existed, so a platform
|
||||
* going quiet costs the user a better destination and nothing more. Only ever
|
||||
* answer `canGoBack` from something that knows; a hopeful guess here is what
|
||||
* puts a user outside the app.
|
||||
*
|
||||
* Never `push`: the page we are leaving is dead, so its URL must not be left
|
||||
* on the back stack for the back button to land on a 404. Stepping back still
|
||||
|
||||
Reference in New Issue
Block a user