From 80c94b048f6c1c838514cd4d827dc02b1f9fba67 Mon Sep 17 00:00:00 2001 From: Florian Date: Wed, 4 Nov 2026 01:59:38 +0100 Subject: [PATCH] fix(navigation): keep the map and its follow camera through native navigation In the installed app, turn-by-turn navigation showed the route overview with no position marker, and the camera never followed. useForegroundLocation memoised its callback on the whole mobile runtime context, which changes with every native snapshot. MapCanvas builds the map in an effect that depends on that callback, so the map was destroyed and rebuilt every few seconds while navigating, each time at the stored overview and with a fresh first-fix request. The nav camera created its marker and entered follow only when mapReady changed, which a rebuild does not do, so a trip started during a rebuild never got either. - useForegroundLocation depends only on authority, client and state. - The nav camera creates its marker without needing a map (the frame loop attaches it to the current one) and enters follow again when a new map instance appears, but not on a style reload of the same map. - MobileRuntimeProvider applies bridge snapshots and events when they arrive instead of inside a state updater, which React runs while rendering and which updated store subscribers mid-render. --- .../lib/mobile/MobileRuntimeProvider.test.tsx | 37 +++++++- .../src/lib/mobile/MobileRuntimeProvider.tsx | 62 +++++++----- .../lib/mobile/useForegroundLocation.test.tsx | 42 ++++++++ .../src/lib/mobile/useForegroundLocation.ts | 14 ++- .../src/lib/navigation/useNavCamera.test.tsx | 62 ++++++++++++ apps/web/src/lib/navigation/useNavCamera.ts | 95 +++++++++++-------- 6 files changed, 243 insertions(+), 69 deletions(-) create mode 100644 apps/web/src/lib/mobile/useForegroundLocation.test.tsx 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.