diff --git a/apps/web/platform/in-app-history.test.ts b/apps/web/platform/in-app-history.test.ts index c930b8895a..66ccf33a74 100644 --- a/apps/web/platform/in-app-history.test.ts +++ b/apps/web/platform/in-app-history.test.ts @@ -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); }); }); diff --git a/apps/web/platform/in-app-history.ts b/apps/web/platform/in-app-history.ts index 2d985da8b7..57fbf17886 100644 --- a/apps/web/platform/in-app-history.ts +++ b/apps/web/platform/in-app-history.ts @@ -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; } diff --git a/apps/web/platform/navigation.test.tsx b/apps/web/platform/navigation.test.tsx index e397652aae..92ea813c51 100644 --- a/apps/web/platform/navigation.test.tsx +++ b/apps/web/platform/navigation.test.tsx @@ -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( - {null}, - ); - 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); }); }); diff --git a/apps/web/platform/navigation.tsx b/apps/web/platform/navigation.tsx index 43975e1dca..b682b688d2 100644 --- a/apps/web/platform/navigation.tsx +++ b/apps/web/platform/navigation.tsx @@ -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) { 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) => diff --git a/packages/views/navigation/use-back-or-replace.ts b/packages/views/navigation/use-back-or-replace.ts index 694550e25e..81ec056f55 100644 --- a/packages/views/navigation/use-back-or-replace.ts +++ b/packages/views/navigation/use-back-or-replace.ts @@ -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