diff --git a/apps/web/src/lib/mobile/MobileRuntimeProvider.test.tsx b/apps/web/src/lib/mobile/MobileRuntimeProvider.test.tsx index 0628523ad..31bd2a287 100644 --- a/apps/web/src/lib/mobile/MobileRuntimeProvider.test.tsx +++ b/apps/web/src/lib/mobile/MobileRuntimeProvider.test.tsx @@ -1,7 +1,7 @@ import { setNavigationAuthority, useNavigationStore } from "@openmapx/core"; import { MOBILE_PROTOCOL_MAX, MOBILE_PROTOCOL_MIN } from "@openmapx/core/navigation"; import { render, screen, waitFor } from "@testing-library/react"; -import { afterEach, describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { MobileRuntimeProvider } from "./MobileRuntimeProvider"; import { CHANNEL_GLOBAL } from "./mobileShellEnvironment"; import { useMobileRuntime, useNavigationModeAvailable } from "./useMobileRuntime"; @@ -354,6 +354,41 @@ describe("MobileRuntimeProvider store integration", () => { expect(state.progress).toEqual({ alongMeters: 100 }); }); + it("updates the store when a snapshot arrives, never while React renders", async () => { + // Applying the store inside a state updater ran it during the provider's + // render, updating every map layer subscribed to the store mid-render. + function RouteSubscriber() { + const revision = useNavigationStore((s) => s.nativeRevision); + return {revision ?? "none"}; + } + const errors = vi.spyOn(console, "error").mockImplementation(() => {}); + const shell = shellScope(); + render( + + + + , + ); + deliverHello(shell); + await waitFor(() => expect(text("state")).toBe("native-compatible")); + + shell.deliver(snapshot()); + shell.deliver( + snapshot({ type: "progress", revision: 5, kind: undefined, progress: { alongMeters: 200 } }), + ); + shell.deliver( + snapshot({ type: "progress", revision: 6, kind: undefined, progress: { alongMeters: 300 } }), + ); + + await waitFor(() => expect(text("store-revision")).toBe("6")); + expect(text("revision")).toBe("6"); + const renderPhaseUpdates = errors.mock.calls.filter((call) => + String(call[0]).includes("while rendering a different component"), + ); + errors.mockRestore(); + expect(renderPhaseUpdates).toEqual([]); + }); + it("advances the store by one revision on a delta", async () => { const shell = shellScope(); mount(shell.scope); diff --git a/apps/web/src/lib/mobile/MobileRuntimeProvider.tsx b/apps/web/src/lib/mobile/MobileRuntimeProvider.tsx index 3349f8fb0..ba7b04eff 100644 --- a/apps/web/src/lib/mobile/MobileRuntimeProvider.tsx +++ b/apps/web/src/lib/mobile/MobileRuntimeProvider.tsx @@ -4,6 +4,7 @@ import { setNavigationAuthority, useNavigationStore } from "@openmapx/core"; import { createContext, type ReactNode, + useCallback, useContext, useEffect, useMemo, @@ -85,7 +86,16 @@ export function MobileRuntimeProvider({ children, webBuildId, scope }: MobileRun const inShell = useMemo(() => isInstalledShell(scope ?? globalThis), [scope]); const [state, setState] = useState(inShell ? "negotiating" : "browser"); const [handshake, setHandshake] = useState(null); - const [readModel, setReadModel] = useState(null); + const [readModel, setReadModelState] = useState(null); + // The read model as of the last message, for the message handler. Bridge + // messages are applied as they arrive, never inside a state updater: React + // runs updaters while rendering, and applying the navigation store there + // updated every component subscribed to it in the middle of a render. + const readModelRef = useRef(null); + const setReadModel = useCallback((next: NativeReadModel | null) => { + readModelRef.current = next; + setReadModelState(next); + }, []); const clientRef = useRef(null); const [commands, setCommands] = useState(null); @@ -117,28 +127,26 @@ export function MobileRuntimeProvider({ children, webBuildId, scope }: MobileRun if (message.type === "snapshot.update") { const incoming = envelopeOf((message.payload as { snapshot?: unknown }).snapshot); if (!incoming) return; - setReadModel((current) => { - const outcome = applyNativeSnapshot(current, incoming); - if (!outcome.ok) { - // A gap or a changed route means the page is behind; ask for the - // whole thing rather than render an interpolation. - if (outcome.reason === "need-full-snapshot") void nativeCommands.requestSnapshot(); - return current; - } - // The store re-checks the step against what it actually rendered, - // which is what catches a batched render dropping one in between. - const projection = projectionOf(outcome.model, incoming.type); - const store = useNavigationStore.getState(); - const applied = - incoming.type === "full" - ? store.applyNativeFullSnapshot(projection) - : store.applyNativeDelta(projection); - if (applied === "needs-full-snapshot") { - void nativeCommands.requestSnapshot(); - return current; - } - return outcome.model; - }); + const outcome = applyNativeSnapshot(readModelRef.current, incoming); + if (!outcome.ok) { + // A gap or a changed route means the page is behind; ask for the + // whole thing rather than render an interpolation. + if (outcome.reason === "need-full-snapshot") void nativeCommands.requestSnapshot(); + return; + } + // The store re-checks the step against what it actually rendered, + // which is what catches a batched render dropping one in between. + const projection = projectionOf(outcome.model, incoming.type); + const store = useNavigationStore.getState(); + const applied = + incoming.type === "full" + ? store.applyNativeFullSnapshot(projection) + : store.applyNativeDelta(projection); + if (applied === "needs-full-snapshot") { + void nativeCommands.requestSnapshot(); + return; + } + setReadModel(outcome.model); return; } @@ -169,11 +177,13 @@ export function MobileRuntimeProvider({ children, webBuildId, scope }: MobileRun .getState() .applyNativeEvent({ eventId, type: String(message.type) }); if (outcome !== "applied") return; - setReadModel((current) => (current ? rememberEvent(current, eventId) : current)); + const current = readModelRef.current; + if (current) setReadModel(rememberEvent(current, eventId)); // Acknowledged immediately: the durable copy lives natively, and leaving // it unacknowledged would replay it on every reconnect. void nativeCommands.acknowledgeEvents([eventId]); - setReadModel((current) => (current ? forgetEvents(current, [eventId]) : current)); + const remembered = readModelRef.current; + if (remembered) setReadModel(forgetEvents(remembered, [eventId])); } }); @@ -200,7 +210,7 @@ export function MobileRuntimeProvider({ children, webBuildId, scope }: MobileRun clientRef.current = null; setCommands(null); }; - }, [inShell, webBuildId, scope]); + }, [inShell, webBuildId, scope, setReadModel]); // Reconcile whenever the page could have missed something: returning to the // foreground, or coming back online. Native state wins in both directions — diff --git a/apps/web/src/lib/mobile/useForegroundLocation.test.tsx b/apps/web/src/lib/mobile/useForegroundLocation.test.tsx new file mode 100644 index 000000000..b0e9f0d39 --- /dev/null +++ b/apps/web/src/lib/mobile/useForegroundLocation.test.tsx @@ -0,0 +1,42 @@ +import { renderHook } from "@testing-library/react"; +import { describe, expect, it, vi } from "vitest"; + +const runtime = vi.hoisted(() => ({ + value: { + browserAuthority: false, + state: "native-compatible", + client: {}, + readModel: { revision: 1 }, + handshake: null, + } as Record, +})); + +vi.mock("./MobileRuntimeProvider", () => ({ + useMobileRuntimeContext: () => runtime.value, +})); + +const { useForegroundLocation } = await import("./useForegroundLocation"); + +describe("useForegroundLocation", () => { + it("keeps one function while native snapshots arrive", () => { + // The map is built from this function: a new one on every snapshot tore the + // map down and rebuilt it every few seconds during native navigation. + const view = renderHook(() => useForegroundLocation()); + const first = view.result.current; + + runtime.value = { ...runtime.value, readModel: { revision: 2 } }; + view.rerender(); + + expect(view.result.current).toBe(first); + }); + + it("changes when where it would ask for a fix changes", () => { + const view = renderHook(() => useForegroundLocation()); + const first = view.result.current; + + runtime.value = { ...runtime.value, browserAuthority: true, state: "browser" }; + view.rerender(); + + expect(view.result.current).not.toBe(first); + }); +}); diff --git a/apps/web/src/lib/mobile/useForegroundLocation.ts b/apps/web/src/lib/mobile/useForegroundLocation.ts index 25473ca2b..afa6aec64 100644 --- a/apps/web/src/lib/mobile/useForegroundLocation.ts +++ b/apps/web/src/lib/mobile/useForegroundLocation.ts @@ -18,21 +18,25 @@ import { useMobileRuntimeContext } from "./MobileRuntimeProvider"; * native-authority split exists to remove. */ export function useForegroundLocation() { - const runtime = useMobileRuntimeContext(); + // Only what decides where a fix comes from. The runtime context changes with + // every native snapshot, and the map is built from this function, so + // depending on the whole context rebuilt the map every few seconds while + // navigating in the installed app. + const { browserAuthority, client, state } = useMobileRuntimeContext(); const counter = useRef(0); return useCallback( async (options: ForegroundLocationOptions = {}): Promise => { - if (runtime.browserAuthority) return browserForegroundFix(options); - if (!runtime.client || runtime.state !== "native-compatible") { + if (browserAuthority) return browserForegroundFix(options); + if (!client || state !== "native-compatible") { return { status: "unavailable" }; } counter.current += 1; // Per-request so a slow answer to an abandoned request cannot move the map // under someone who has since asked again. const requestId = `fix-${counter.current}`; - return nativeForegroundFix(runtime.client, requestId, options); + return nativeForegroundFix(client, requestId, options); }, - [runtime], + [browserAuthority, client, state], ); } diff --git a/apps/web/src/lib/navigation/useNavCamera.test.tsx b/apps/web/src/lib/navigation/useNavCamera.test.tsx index db44a87fb..2d56ca9be 100644 --- a/apps/web/src/lib/navigation/useNavCamera.test.tsx +++ b/apps/web/src/lib/navigation/useNavCamera.test.tsx @@ -712,6 +712,68 @@ describe("useNavCamera lifecycle", () => { expect(replacement.state.cameraTransitions.some((t) => t.method === "jumpTo")).toBe(true); }); + /** + * The page swaps in a new map without clearing `mapReady` (a style rebuild, + * or the map being re-created), so navigation can start in the gap where + * `mapRef` is empty. `styleVersion` is what announces the new map. + */ + async function mountDuringMapReplacement() { + const frames = createFrameHarness(CLOCK_START); + vi.stubGlobal("requestAnimationFrame", frames.request); + vi.stubGlobal("cancelAnimationFrame", frames.cancel); + vi.spyOn(performance, "now").mockImplementation(() => frames.now()); + mapContextTest.mapRef.current = null; + mapContextTest.mapReady = true; + startRoute(); + const view = renderHook(() => useNavCamera()); + await act(async () => { + await Promise.resolve(); + }); + const fake = createFakeMap({ zoom: 10 }); + mapContextTest.mapRef.current = fake.map; + mapContextTest.styleVersion += 1; + view.rerender(); + await act(async () => { + await Promise.resolve(); + }); + return { frames, fake, view }; + } + + it("draws the puck when navigation starts while the map is being replaced", async () => { + const { frames, fake } = await mountDuringMapReplacement(); + applyFix(START_ALONG); + frames.flush(60, FRAME_MS); + + expect(markerTest.markers[0]?.map).toBe(fake.map); + expect(fake.state.cameraTransitions.some((t) => t.method === "jumpTo")).toBe(true); + }); + + it("enters follow on a map that arrives after navigation started", async () => { + const { fake } = await mountDuringMapReplacement(); + expect(fake.state.cameraTransitions.some((t) => t.method === "easeTo")).toBe(true); + }); + + it("enters follow again on a replacement map, and not on a style reload", async () => { + const harness = await mountNavCamera(); + applyFix(START_ALONG); + harness.frames.flush(60, FRAME_MS); + const eases = (fake: FakeMap) => + fake.state.cameraTransitions.filter((t) => t.method === "easeTo").length; + const before = eases(harness.fake); + + // A style reload keeps the map, and the camera the user is following. + mapContextTest.styleVersion += 1; + harness.rerender(); + expect(eases(harness.fake)).toBe(before); + + // A new map starts from its own default camera and needs follow re-entered. + const replacement = createFakeMap({ zoom: 10 }); + mapContextTest.mapRef.current = replacement.map; + mapContextTest.styleVersion += 1; + harness.rerender(); + expect(eases(replacement)).toBe(1); + }); + it("does not attach a marker that arrives after navigation stopped", async () => { const frames = createFrameHarness(CLOCK_START); vi.stubGlobal("requestAnimationFrame", frames.request); diff --git a/apps/web/src/lib/navigation/useNavCamera.ts b/apps/web/src/lib/navigation/useNavCamera.ts index 60fa7a31f..db16601fa 100644 --- a/apps/web/src/lib/navigation/useNavCamera.ts +++ b/apps/web/src/lib/navigation/useNavCamera.ts @@ -229,6 +229,8 @@ export function useNavCamera(): void { // in-progress gesture before it crosses the pan/zoom threshold — the reason a // pinch or drag wouldn't take hold while the puck (and camera) were moving. const userInteractingRef = useRef(false); + // The map follow was last entered on. + const followMapRef = useRef(null); const active = status === "navigating" || status === "rerouting"; @@ -244,10 +246,11 @@ export function useNavCamera(): void { }); }, []); - // Create the heading-puck marker once the map is ready; remove on unmount. + // Create the heading-puck marker once; remove on unmount. It does not need a + // map: the frame loop attaches it to whichever map is current, so a trip that + // starts while the map is being replaced still gets its puck. useEffect(() => { - const map = mapRef?.current; - if (!map || !mapReady || markerRef.current) return; + if (markerRef.current) return; let destroyed = false; void import("maplibre-gl") .then((maplibregl) => { @@ -272,7 +275,7 @@ export function useNavCamera(): void { return () => { destroyed = true; }; - }, [mapRef, mapReady, requestFrame]); + }, [requestFrame]); useEffect(() => { return () => { @@ -332,42 +335,60 @@ export function useNavCamera(): void { // (Re)entering follow: establish zoom + pitch with a short ease, then let the // per-frame loop take over centre/bearing once it settles. + const enterFollow = useCallback( + (map: maplibregl.Map) => { + followMapRef.current = map; + const prog = useNavigationStore.getState().progress; + if (prog && displayedRef.current === null) { + displayedRef.current = prog.alongMeters; + bearingRef.current = prog.bearing; + } + // Respect a user-chosen zoom on re-entry (recenter); otherwise establish the + // default follow zoom. + const enterZoom = userZoomedRef.current + ? map.getZoom() + : Math.max(map.getZoom(), NAV_ENTER_ZOOM); + const enterPadding = getCameraPaddingTarget(map); + map.easeTo( + { + zoom: enterZoom, + pitch: PITCH[mode] ?? 0, + padding: enterPadding, + duration: prefersReducedMotion() ? 0 : ENTER_EASE_MS, + }, + { programmatic: true }, + ); + // Seed the eased follow-zoom and padding so the per-frame loop continues + // from here. + displayedZoomRef.current = enterZoom; + displayedPitchRef.current = PITCH[mode] ?? 0; + displayedPaddingRef.current = enterPadding; + paddingTargetRef.current = enterPadding; + settleUntilRef.current = performance.now() + SETTLE_MS; + // Forget the published poses: a recenter or a mode change is a + // discontinuity, and the first frame after the ease must draw it. + lastPuckRef.current = null; + lastCamRef.current = null; + requestFrame(); + }, + [mode, requestFrame], + ); + useEffect(() => { const map = mapRef?.current; if (!map || !active || cameraMode !== "follow") return; - const prog = useNavigationStore.getState().progress; - if (prog && displayedRef.current === null) { - displayedRef.current = prog.alongMeters; - bearingRef.current = prog.bearing; - } - // Respect a user-chosen zoom on re-entry (recenter); otherwise establish the - // default follow zoom. - const enterZoom = userZoomedRef.current - ? map.getZoom() - : Math.max(map.getZoom(), NAV_ENTER_ZOOM); - const enterPadding = getCameraPaddingTarget(map); - map.easeTo( - { - zoom: enterZoom, - pitch: PITCH[mode] ?? 0, - padding: enterPadding, - duration: prefersReducedMotion() ? 0 : ENTER_EASE_MS, - }, - { programmatic: true }, - ); - // Seed the eased follow-zoom and padding so the per-frame loop continues - // from here. - displayedZoomRef.current = enterZoom; - displayedPitchRef.current = PITCH[mode] ?? 0; - displayedPaddingRef.current = enterPadding; - paddingTargetRef.current = enterPadding; - settleUntilRef.current = performance.now() + SETTLE_MS; - // Forget the published poses: a recenter or a mode change is a - // discontinuity, and the first frame after the ease must draw it. - lastPuckRef.current = null; - lastCamRef.current = null; - requestFrame(); - }, [mapRef, active, cameraMode, mode, requestFrame]); + enterFollow(map); + }, [mapRef, active, cameraMode, enterFollow]); + + // A replacement map arrives without `mapReady` changing, and starts from its + // own default camera, so follow is entered again on it. `styleVersion` is what + // announces it; a style reload on the same map keeps the camera as it is. + // biome-ignore lint/correctness/useExhaustiveDependencies: styleVersion and mapReady are the triggers; the body reads the current map and store. + useEffect(() => { + const map = mapRef?.current; + if (!map || !active || cameraMode !== "follow" || followMapRef.current === map) return; + enterFollow(map); + }, [styleVersion, mapReady]); // Chrome moving under the map (a sheet detent, a rewrapped banner) changes // the padding target; the next frame resolves it once and eases toward it.