From e7851e79ff3eea4905369c82a4d4ef43c166cad4 Mon Sep 17 00:00:00 2001 From: Maurice Date: Sat, 8 Aug 2026 21:58:42 +0200 Subject: [PATCH] fix(client): honour the start page on a device that never mirrored it The mirror answered "dashboard" for a browser that had simply never been told, and RootRedirect took that as the user's choice - so the setting was ignored on every device except the one that set it, and on that one too after a logout cleared the mirror. The setting itself was stored correctly the whole time, which is what made it look like nothing happened. readStartDestination now says "unknown" rather than guessing, and a device in that state waits for the real settings instead. That costs one wait, once per device; afterwards the mirror answers and the launch is as fast as before. Bounded by a timeout, because loadSettings deliberately leaves isLoaded false on a failed request so it can retry - without the floor, a launch with no backend would sit on the spinner forever. --- client/src/App.test.tsx | 42 ++++++++++++++++++- client/src/App.tsx | 31 ++++++++++---- client/src/utils/startDestination.ts | 26 +++++++++--- .../tests/unit/utils/startDestination.test.ts | 26 +++++++++--- 4 files changed, 103 insertions(+), 22 deletions(-) diff --git a/client/src/App.test.tsx b/client/src/App.test.tsx index 2d9cb475f..5b69b26cd 100644 --- a/client/src/App.test.tsx +++ b/client/src/App.test.tsx @@ -1,5 +1,5 @@ import React from 'react' -import { render, screen, waitFor } from '@testing-library/react' +import { render, screen, waitFor, act } from '@testing-library/react' import { MemoryRouter } from 'react-router' import { describe, it, expect, beforeEach, vi } from 'vitest' import { http, HttpResponse } from 'msw' @@ -8,6 +8,7 @@ import { useAuthStore } from './store/authStore' import { useSettingsStore } from './store/settingsStore' import { resetAllStores } from '../tests/helpers/store' import { buildUser, buildSettings } from '../tests/helpers/factories' +import { SETTINGS_WAIT_MS } from './utils/startDestination' import App from './App' // ── Mock page components ─────────────────────────────────────────────────────── @@ -397,3 +398,42 @@ describe('Version cache-busting', () => { await waitFor(() => expect(reload).toHaveBeenCalled()) }) }) + +// Regression: a device that has never mirrored the preference must not treat +// "nothing mirrored" as "user wants the dashboard". +describe('RootRedirect — preference not mirrored on this device', () => { + it('FE-COMP-APP-031: honours the server setting when localStorage is empty', async () => { + seedAuth({ isAuthenticated: true, user: buildUser() }) + // Cold start: settings are still in flight when RootRedirect first runs. + useSettingsStore.setState({ isLoaded: false }) + server.use(http.get('/api/trips/active', () => HttpResponse.json({ trip: { id: 42, title: 'Japan' } }))) + + renderApp('/') + + // ...and land a moment later, exactly as loadSettings() does. + await act(async () => { + useSettingsStore.setState({ + isLoaded: true, + settings: buildSettings({ start_page: 'active_trip', start_trip_tab: 'finanzplan' }), + }) + }) + + await waitFor(() => expect(screen.getByText('TripPlanner')).toBeInTheDocument()) + }) + + // loadSettings leaves isLoaded false on a failed request so it can retry, so + // the wait above needs a floor or a launch with no backend never resolves. + it('FE-COMP-APP-032: gives up on the settings after the timeout and opens the dashboard', async () => { + vi.useFakeTimers({ shouldAdvanceTime: true }) + seedAuth({ isAuthenticated: true, user: buildUser() }) + useSettingsStore.setState({ isLoaded: false }) + + renderApp('/') + expect(screen.queryByText('Dashboard')).not.toBeInTheDocument() + + await act(async () => { vi.advanceTimersByTime(SETTINGS_WAIT_MS + 100) }) + + await waitFor(() => expect(screen.getByText('Dashboard')).toBeInTheDocument()) + vi.useRealTimers() + }) +}) diff --git a/client/src/App.tsx b/client/src/App.tsx index 4a5013acc..a200addd8 100644 --- a/client/src/App.tsx +++ b/client/src/App.tsx @@ -20,7 +20,7 @@ import { lazyWithRetry } from './utils/lazyWithRetry' import { useIsPhone } from './mobile/useIsPhone' import { TranslationProvider, useTranslation } from './i18n' import { authApi, tripsApi } from './api/client' -import { readStartDestination, tripStartPath, DEFAULT_START_PAGE, DEFAULT_START_TRIP_TAB } from './utils/startDestination' +import { readStartDestination, tripStartPath, DEFAULT_START_PAGE, DEFAULT_START_TRIP_TAB, SETTINGS_WAIT_MS } from './utils/startDestination' import { usePermissionsStore, PermissionLevel } from './store/permissionsStore' import { useInAppNotificationListener } from './hooks/useInAppNotificationListener.ts' import { registerSyncTriggers, unregisterSyncTriggers } from './sync/syncTriggers' @@ -179,31 +179,44 @@ function ViewportRoute({ phone: Phone, desktop: Desktop }: { * here rather than on /dashboard — landing on the dashboard directly has to keep * working, or there would be no way back to it. * - * The preference is read from the localStorage mirror, so the default path stays - * exactly as fast as before: no settings request to wait for. Only someone who - * asked for 'active_trip' pays for the one lookup that finds their trip, and a - * failure there just falls back to the dashboard. + * Once this device has seen the preference it comes from the localStorage + * mirror, so the common launch waits for nothing. A device that has never seen + * it waits for the settings instead of assuming the default — the preference + * lives on the server, and guessing here would ignore it on every browser that + * didn't set it. Only 'active_trip' costs the one lookup that finds the trip, + * and any failure along the way lands on the dashboard. */ function RootRedirect() { const { isAuthenticated, isLoading } = useAuthStore() const settingsLoaded = useSettingsStore((s) => s.isLoaded) const settings = useSettingsStore((s) => s.settings) const [target, setTarget] = useState(null) + // Bounds the wait below: loadSettings deliberately leaves isLoaded false on a + // failed request so it can retry, which would otherwise strand a launch that + // started offline on the spinner. + const [settingsGaveUp, setSettingsGaveUp] = useState(false) + + useEffect(() => { + if (settingsLoaded) return + const timer = setTimeout(() => setSettingsGaveUp(true), SETTINGS_WAIT_MS) + return () => clearTimeout(timer) + }, [settingsLoaded]) useEffect(() => { if (isLoading || !isAuthenticated || target) return - // Straight after login the real settings are already in the store, so use - // them; on a cold start they aren't, and waiting would defeat the point. + const mirrored = readStartDestination() + if (!mirrored && !settingsLoaded && !settingsGaveUp) return + // Loaded settings are the truth; the mirror is what we knew last time. const { page, tab } = settingsLoaded ? { page: settings.start_page ?? DEFAULT_START_PAGE, tab: settings.start_trip_tab ?? DEFAULT_START_TRIP_TAB } - : readStartDestination() + : (mirrored ?? { page: DEFAULT_START_PAGE, tab: DEFAULT_START_TRIP_TAB }) if (page !== 'active_trip') { setTarget('/dashboard'); return } let cancelled = false tripsApi.active() .then(({ trip }) => { if (!cancelled) setTarget(trip ? tripStartPath(trip.id, tab) : '/dashboard') }) .catch(() => { if (!cancelled) setTarget('/dashboard') }) return () => { cancelled = true } - }, [isLoading, isAuthenticated, settingsLoaded, target]) + }, [isLoading, isAuthenticated, settingsLoaded, settingsGaveUp, settings, target]) if (isLoading || (isAuthenticated && !target)) { return ( diff --git a/client/src/utils/startDestination.ts b/client/src/utils/startDestination.ts index ea5d461b1..36cfc6945 100644 --- a/client/src/utils/startDestination.ts +++ b/client/src/utils/startDestination.ts @@ -19,6 +19,13 @@ export const DEFAULT_START_TRIP_TAB = 'plan' */ export const START_DESTINATION_ROUTE = '/' +/** + * How long a device that has never seen the preference waits for the settings + * before giving up and opening the dashboard. Long enough for a normal request, + * short enough that a launch with no backend isn't stuck staring at a spinner. + */ +export const SETTINGS_WAIT_MS = 2500 + export interface StartDestination { page: StartPage tab: string @@ -36,17 +43,24 @@ const TAB_KEY = 'trek_start_trip_tab' const isStartPage = (value: unknown): value is StartPage => value === 'dashboard' || value === 'active_trip' -export function readStartDestination(): StartDestination { +/** + * What this device last knew, or null if it has never been told — a fresh + * browser, another machine, or one that has logged out since. + * + * The null case matters: the preference lives on the server, so "this device + * doesn't know" is not the same as "this user wants the dashboard". Callers + * have to wait for the real settings rather than assume the default, or the + * setting is silently ignored on every device that didn't set it. + */ +export function readStartDestination(): StartDestination | null { try { const page = localStorage.getItem(PAGE_KEY) + if (!isStartPage(page)) return null const tab = localStorage.getItem(TAB_KEY) - return { - page: isStartPage(page) ? page : DEFAULT_START_PAGE, - tab: isTripTabId(tab) ? tab : DEFAULT_START_TRIP_TAB, - } + return { page, tab: isTripTabId(tab) ? tab : DEFAULT_START_TRIP_TAB } } catch { // Private-mode Safari and friends throw on access, not just on write. - return { page: DEFAULT_START_PAGE, tab: DEFAULT_START_TRIP_TAB } + return null } } diff --git a/client/tests/unit/utils/startDestination.test.ts b/client/tests/unit/utils/startDestination.test.ts index 4de37375e..b9ce12b5f 100644 --- a/client/tests/unit/utils/startDestination.test.ts +++ b/client/tests/unit/utils/startDestination.test.ts @@ -18,8 +18,12 @@ afterEach(() => { }) describe('readStartDestination', () => { - it('FE-UTIL-START-001: falls back to the dashboard when nothing was ever mirrored', () => { - expect(readStartDestination()).toEqual({ page: DEFAULT_START_PAGE, tab: DEFAULT_START_TRIP_TAB }) + // Not the dashboard: the preference lives on the server, so a device that has + // never seen it has to say "I don't know" and let the caller wait for the + // settings. Answering 'dashboard' here ignored the setting on every device + // that hadn't set it — including any browser that had logged out since. + it('FE-UTIL-START-001: reports "unknown" when nothing was ever mirrored', () => { + expect(readStartDestination()).toBeNull() }) it('FE-UTIL-START-002: returns what was mirrored', () => { @@ -27,19 +31,27 @@ describe('readStartDestination', () => { expect(readStartDestination()).toEqual({ page: 'active_trip', tab: 'finanzplan' }) }) + it('FE-UTIL-START-002b: an explicitly mirrored dashboard is an answer, not "unknown"', () => { + rememberStartDestination({ start_page: 'dashboard' }) + expect(readStartDestination()).toEqual({ page: DEFAULT_START_PAGE, tab: DEFAULT_START_TRIP_TAB }) + }) + // A value a user (or an older/newer build) could have written by hand must not // become a route we then navigate to. it('FE-UTIL-START-003: ignores values that are not a known page or tab', () => { localStorage.setItem('trek_start_page', 'somewhere_else') + expect(readStartDestination()).toBeNull() + + localStorage.setItem('trek_start_page', 'active_trip') localStorage.setItem('trek_start_trip_tab', '../../etc/passwd') - expect(readStartDestination()).toEqual({ page: DEFAULT_START_PAGE, tab: DEFAULT_START_TRIP_TAB }) + expect(readStartDestination()).toEqual({ page: 'active_trip', tab: DEFAULT_START_TRIP_TAB }) }) it('FE-UTIL-START-004: survives a browser that refuses storage access', () => { vi.spyOn(Storage.prototype, 'getItem').mockImplementation(() => { throw new Error('SecurityError') }) - expect(readStartDestination()).toEqual({ page: DEFAULT_START_PAGE, tab: DEFAULT_START_TRIP_TAB }) + expect(readStartDestination()).toBeNull() }) }) @@ -65,10 +77,12 @@ describe('rememberStartDestination', () => { }) describe('forgetStartDestination', () => { - it('FE-UTIL-START-008: leaves the next account on this browser with the default', () => { + it('FE-UTIL-START-008: leaves the next account on this browser knowing nothing', () => { rememberStartDestination({ start_page: 'active_trip', start_trip_tab: 'finanzplan' }) forgetStartDestination() - expect(readStartDestination()).toEqual({ page: DEFAULT_START_PAGE, tab: DEFAULT_START_TRIP_TAB }) + // "unknown", so the next login waits for its own settings instead of + // inheriting the previous account's preference. + expect(readStartDestination()).toBeNull() }) })