mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-22 23:16:46 +00:00
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.
This commit is contained in:
+41
-1
@@ -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()
|
||||
})
|
||||
})
|
||||
|
||||
+22
-9
@@ -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<string | null>(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 (
|
||||
|
||||
@@ -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
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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()
|
||||
})
|
||||
})
|
||||
|
||||
|
||||
Reference in New Issue
Block a user