From 47a4276e847baf916ef9bd47544081f6781049b6 Mon Sep 17 00:00:00 2001 From: Maurice Date: Sun, 23 Aug 2026 10:07:24 +0200 Subject: [PATCH] fix(client): make the offline core survive a logout and stop clobbering itself logout() unregistered every sync trigger, and the only place that registers them is App's mount effect - which never runs again, because logout navigates through the router rather than reloading. Log out, log back in, go offline, edit something: it queues and nothing ever flushes it. No online listener, no visibilitychange, no 30s heartbeat, no WS reconnect hook. Register on auth success instead; registerSyncTriggers is already idempotent. The background sync loops only checked the auth gate on entry. Logout swaps the Dexie handle to the shared anonymous database, so a trip loop or a blob download still in flight afterwards wrote the logged-out account's itinerary and documents into it - the database that survives logout and greets the next person on a shared device. Check the gate per iteration. And the remote place:updated applier replaced the whole embedded place, while the local reducer right next to it preserves the assignment's own times. Same event, two answers, and the remote one wipes a scheduled 09:00-10:30 off the itinerary when someone else renames the place. One merge helper now, used by both, so they cannot drift again. The test that should have caught it seeded null times and asserted only the name. Also: moving a day note deleted before it created, and restored the note only locally when the create failed; t() interpolation was corrupted by a '$' in a parameter value; a 429 upload was treated as permanently failed; and the geolocation watch was started inside a state updater. --- client/src/api/oauthScopes.test.ts | 27 ++++++++++++-- client/src/hooks/useGeolocation.test.ts | 21 +++++++++++ client/src/hooks/useGeolocation.ts | 37 +++++++++++++------ client/src/i18n/TranslationContext.tsx | 6 ++- client/src/store/authStore.ts | 6 ++- client/src/store/slices/dayNotesSlice.ts | 11 +++++- client/src/store/slices/placesSlice.ts | 10 ++++- client/src/store/slices/remoteEventHandler.ts | 3 +- client/src/sync/tripSyncManager.ts | 21 ++++++++++- client/src/utils/uploadQueue.test.ts | 16 ++++++++ client/src/utils/uploadQueue.ts | 4 ++ client/tests/setup.ts | 12 +++++- .../tests/unit/mswUnhandledRequests.test.ts | 10 +++++ .../unit/remoteEventHandler/places.test.ts | 23 ++++++++++++ client/tests/unit/sync/syncTriggers.test.ts | 4 ++ 15 files changed, 187 insertions(+), 24 deletions(-) create mode 100644 client/tests/unit/mswUnhandledRequests.test.ts diff --git a/client/src/api/oauthScopes.test.ts b/client/src/api/oauthScopes.test.ts index 443c30b64..81712c63f 100644 --- a/client/src/api/oauthScopes.test.ts +++ b/client/src/api/oauthScopes.test.ts @@ -1,7 +1,20 @@ -// FE-OAUTH-SCOPES-001 to FE-OAUTH-SCOPES-010 +// FE-OAUTH-SCOPES-001 to FE-OAUTH-SCOPES-011 import { describe, it, expect } from 'vitest' +import { readFileSync } from 'node:fs' +import { dirname, resolve } from 'node:path' +import { fileURLToPath } from 'node:url' import { SCOPE_GROUPS, ALL_SCOPES, SCOPE_GROUP_NAMES, getScopesByGroup } from './oauthScopes' +// The consent page mirrors the server's scope list by hand, so read the server +// file and compare. Parsing keeps this a client-only test (no server import). +function readServerScopes(): string[] { + const here = dirname(fileURLToPath(import.meta.url)) + const src = readFileSync(resolve(here, '../../../server/src/mcp/scopes.ts'), 'utf8') + const block = src.slice(src.indexOf('export const SCOPES = {')) + const body = block.slice(0, block.indexOf('} as const;')) + return [...body.matchAll(/^\s*[A-Z_]+:\s*'([a-z]+:[a-z]+)'/gm)].map(m => m[1]) +} + describe('SCOPE_GROUPS', () => { it('FE-OAUTH-SCOPES-001: contains all expected scope keys', () => { const expected = [ @@ -34,8 +47,16 @@ describe('SCOPE_GROUPS', () => { }) describe('ALL_SCOPES', () => { - it('FE-OAUTH-SCOPES-003: contains exactly 29 scopes', () => { - expect(ALL_SCOPES).toHaveLength(29) + it('FE-OAUTH-SCOPES-003: contains exactly as many scopes as the server defines', () => { + expect(ALL_SCOPES).toHaveLength(readServerScopes().length) + }) + + it('FE-OAUTH-SCOPES-011: matches server/src/mcp/scopes.ts exactly', () => { + const serverScopes = readServerScopes() + // Guards the parse itself: a moved file or a reformatted list must fail + // here rather than quietly compare two empty sets. + expect(serverScopes.length).toBeGreaterThan(0) + expect(new Set(ALL_SCOPES)).toEqual(new Set(serverScopes)) }) it('FE-OAUTH-SCOPES-004: matches Object.keys(SCOPE_GROUPS)', () => { diff --git a/client/src/hooks/useGeolocation.test.ts b/client/src/hooks/useGeolocation.test.ts index 0f3bba137..1f9bc118a 100644 --- a/client/src/hooks/useGeolocation.test.ts +++ b/client/src/hooks/useGeolocation.test.ts @@ -1,4 +1,5 @@ // FE-HOOK-GEO-001 to FE-HOOK-GEO-023 +import { StrictMode } from 'react'; import { act, renderHook } from '@testing-library/react'; import { GeoOnceError, getCurrentPositionOnce, useGeolocation } from './useGeolocation'; @@ -221,6 +222,26 @@ describe('useGeolocation', () => { expect(clearWatch).toHaveBeenCalledWith(7); }); + it('FE-HOOK-GEO-015b: the updater form sees the freshest mode and starts one watch', async () => { + // The side effects run outside the state updater (React may invoke an + // updater more than once per commit, and startWatch awaits the iOS + // orientation prompt before it records the watch id), so the previous mode + // comes from a ref that setMode keeps in step within a batch. + const requestPermission = vi.fn().mockResolvedValue('granted'); + vi.stubGlobal('DeviceOrientationEvent', Object.assign(function () {}, { requestPermission })); + + const { result } = renderHook(() => useGeolocation(), { wrapper: StrictMode }); + + await act(async () => { + result.current.setMode('show'); + result.current.setMode(prev => (prev === 'show' ? 'follow' : 'off')); + await Promise.resolve(); + }); + + expect(result.current.mode).toBe('follow'); + expect(watchPosition).toHaveBeenCalledTimes(1); + }); + it('FE-HOOK-GEO-016: asks iOS for orientation permission and proceeds either way', async () => { const requestPermission = vi.fn().mockResolvedValue('denied'); vi.stubGlobal('DeviceOrientationEvent', Object.assign(function () {}, { requestPermission })); diff --git a/client/src/hooks/useGeolocation.ts b/client/src/hooks/useGeolocation.ts index e6f991082..565699208 100644 --- a/client/src/hooks/useGeolocation.ts +++ b/client/src/hooks/useGeolocation.ts @@ -93,12 +93,20 @@ export function getCurrentPositionOnce( export function useGeolocation(): UseGeolocationReturn { const [position, setPosition] = useState(null) const [mode, setModeState] = useState('off') + // What setMode's updater form reads instead of a state updater's argument: + // React may run an updater more than once per commit, and subscribing from + // inside one would leak a watch (see setMode below). + const modeRef = useRef(mode) + modeRef.current = mode const [error, setError] = useState(null) const watchIdRef = useRef(null) + // True between the start of startWatch and the watchPosition call it awaits. + const startingRef = useRef(false) const orientationHandlerRef = useRef<((e: DeviceOrientationEvent) => void) | null>(null) const headingRef = useRef(null) const stopWatch = useCallback(() => { + startingRef.current = false if (watchIdRef.current !== null) { try { navigator.geolocation.clearWatch(watchIdRef.current) } catch { /* noop */ } watchIdRef.current = null @@ -116,6 +124,11 @@ export function useGeolocation(): UseGeolocationReturn { setError('Geolocation is not supported in this browser') return false } + // Already watching, or still waiting on the iOS prompt below: a second + // subscription would overwrite watchIdRef and leave the first one running + // with nobody left holding its id. + if (startingRef.current || watchIdRef.current !== null) return true + startingRef.current = true setError(null) // iOS: ask for orientation permission up front; on Android and desktop @@ -157,6 +170,7 @@ export function useGeolocation(): UseGeolocationReturn { window.addEventListener('deviceorientationabsolute', onOrientation as EventListener) window.addEventListener('deviceorientation', onOrientation as EventListener) + startingRef.current = false watchIdRef.current = navigator.geolocation.watchPosition( (pos) => { setPosition({ @@ -189,17 +203,18 @@ export function useGeolocation(): UseGeolocationReturn { }, [stopWatch]) const setMode = useCallback((m: TrackingMode | ((prev: TrackingMode) => TrackingMode)) => { - setModeState(prev => { - const next = typeof m === 'function' ? m(prev) : m - if (next === 'off') { - stopWatch() - setPosition(null) - } else if (watchIdRef.current === null) { - // started externally but no watch yet — start it - startWatch() - } - return next - }) + const next = typeof m === 'function' ? m(modeRef.current) : m + // Kept in step here as well, so two setMode calls batched into one commit + // still see each other's result. + modeRef.current = next + setModeState(next) + if (next === 'off') { + stopWatch() + setPosition(null) + } else if (watchIdRef.current === null) { + // started externally but no watch yet — start it + void startWatch() + } }, [startWatch, stopWatch]) const cycleMode = useCallback(async () => { diff --git a/client/src/i18n/TranslationContext.tsx b/client/src/i18n/TranslationContext.tsx index d320f3e1d..efacfb112 100644 --- a/client/src/i18n/TranslationContext.tsx +++ b/client/src/i18n/TranslationContext.tsx @@ -133,7 +133,9 @@ export function TranslationProvider({ children }: TranslationProviderProps) { let val: string = (strings[key] ?? en[key] ?? key) as string if (params) { Object.entries(params).forEach(([k, v]) => { - val = val.replace(new RegExp(`\\{${k}\\}`, 'g'), String(v)) + // Function replacement: a value carrying `$&` or `$1` (a trip named + // "A$&B") would otherwise be expanded as a replacement pattern. + val = val.replace(new RegExp(`\\{${k}\\}`, 'g'), () => String(v)) }) } return val @@ -145,7 +147,7 @@ export function TranslationProvider({ children }: TranslationProviderProps) { Object.entries(params).forEach(([k, v]) => { // Escape BEFORE substitution so a user-controlled value with `<` or // `&` cannot break out of the surrounding template's markup. - val = val.replace(new RegExp(`\\{${k}\\}`, 'g'), escapeHtml(String(v))) + val = val.replace(new RegExp(`\\{${k}\\}`, 'g'), () => escapeHtml(String(v))) }) } // Then re-sanitise the fully-built string: even if a translator ships a diff --git a/client/src/store/authStore.ts b/client/src/store/authStore.ts index 4355d424c..224f67fa1 100644 --- a/client/src/store/authStore.ts +++ b/client/src/store/authStore.ts @@ -7,7 +7,7 @@ import { getApiErrorMessage } from '../types' import { tripSyncManager } from '../sync/tripSyncManager' import { reopenForUser, deleteCurrentUserDb } from '../db/offlineDb' import { setAuthed } from '../sync/authGate' -import { unregisterSyncTriggers } from '../sync/syncTriggers' +import { registerSyncTriggers, unregisterSyncTriggers } from '../sync/syncTriggers' import { useSystemNoticeStore } from './systemNoticeStore.js' import { clearAppearanceSnapshot } from '../theme/applyAppearance' import { clearAllPluginSessions } from './pluginStore' @@ -95,6 +95,10 @@ async function onAuthSuccess(userId: number): Promise { } catch (err) { console.error('[auth] failed to open user-scoped offline DB', err) } + // logout() tears the triggers down, and App's mount effect never runs again in + // an SPA session, so a second login in the same tab would leave the mutation + // queue without a flush trigger. Re-registering is a no-op while they are up. + registerSyncTriggers() } export const useAuthStore = create()( diff --git a/client/src/store/slices/dayNotesSlice.ts b/client/src/store/slices/dayNotesSlice.ts index 1ca9578cd..7d795acc3 100644 --- a/client/src/store/slices/dayNotesSlice.ts +++ b/client/src/store/slices/dayNotesSlice.ts @@ -112,7 +112,9 @@ export const createDayNotesSlice = (set: SetState, get: GetState): DayNotesSlice })) try { - await dayNotesApi.delete(tripId, fromDayId, noteId) + // There is no atomic move on the server, so the destructive half goes last: + // if the create were second and failed, the note would already be gone for + // good and the rollback below would only fake it back into the store. // Every field the note carries, not just the ones it had when this was // written: a move is a delete plus a create, so anything omitted here is // silently dropped — which is how a coloured note lost its colour on the @@ -120,6 +122,13 @@ export const createDayNotesSlice = (set: SetState, get: GetState): DayNotesSlice const result = await dayNotesApi.create(tripId, toDayId, { text: note.text, time: note.time, icon: note.icon, color: note.color ?? null, sort_order, }) + try { + await dayNotesApi.delete(tripId, fromDayId, noteId) + } catch (delErr: unknown) { + // The source survived, so drop the copy rather than leave a duplicate behind. + await dayNotesApi.delete(tripId, toDayId, result.note.id).catch(() => {}) + throw delErr + } set(s => ({ dayNotes: { ...s.dayNotes, diff --git a/client/src/store/slices/placesSlice.ts b/client/src/store/slices/placesSlice.ts index fe7069883..8dc2c8b88 100644 --- a/client/src/store/slices/placesSlice.ts +++ b/client/src/store/slices/placesSlice.ts @@ -19,6 +19,14 @@ export interface PlacesSlice { updatePlacesMany: (tripId: number | string, placeIds: number[], patch: Partial) => Promise } +/** Swap the pool place embedded in an assignment while keeping the assignment's + * own times: the day view shows COALESCE(assignment_time, place_time), so + * overwriting them with the pool row would wipe a per-stop schedule. Exported + * because the WS applier in remoteEventHandler has to merge the same way. */ +export function mergeAssignmentPlace(a: Assignment, place: Place): Assignment { + return { ...a, place: { ...place, place_time: a.place.place_time, end_time: a.place.end_time } } +} + /** Replace a place in the pool and in every day-assignment that embeds it, * preserving the assignment's own times (mirrors updatePlace's reconciliation). */ function applyUpdatedPlace(set: SetState, placeId: number, place: Place): void { @@ -28,7 +36,7 @@ function applyUpdatedPlace(set: SetState, placeId: number, place: Place): void { for (const [dayId, items] of Object.entries(state.assignments)) { if (items.some((a: Assignment) => a.place?.id === placeId)) { updatedAssignments[dayId] = items.map((a: Assignment) => - a.place?.id === placeId ? { ...a, place: { ...place, place_time: a.place.place_time, end_time: a.place.end_time } } : a + a.place?.id === placeId ? mergeAssignmentPlace(a, place) : a ) changed = true } diff --git a/client/src/store/slices/remoteEventHandler.ts b/client/src/store/slices/remoteEventHandler.ts index 1615ce179..d7877ee61 100644 --- a/client/src/store/slices/remoteEventHandler.ts +++ b/client/src/store/slices/remoteEventHandler.ts @@ -4,6 +4,7 @@ import type { TripStoreState } from '../tripStore' import type { Assignment, Place, Day, DayNote, PackingItem, TodoItem, BudgetItem, BudgetItemMember, Reservation, Trip, TripFile, WebSocketEvent } from '../../types' import { offlineDb } from '../../db/offlineDb' import { useAuthStore } from '../authStore' +import { mergeAssignmentPlace } from './placesSlice' type SetState = StoreApi['setState'] type GetState = StoreApi['getState'] @@ -212,7 +213,7 @@ export const STATE_APPLIERS: Partial> assignments: Object.fromEntries( Object.entries(state.assignments).map(([dayId, items]) => [ dayId, - items.map(a => a.place?.id === (payload.place as Place).id ? { ...a, place: payload.place as Place } : a) + items.map(a => a.place?.id === (payload.place as Place).id ? mergeAssignmentPlace(a, payload.place as Place) : a) ]) ), }), diff --git a/client/src/sync/tripSyncManager.ts b/client/src/sync/tripSyncManager.ts index 0d5b3a15a..d38acd18e 100644 --- a/client/src/sync/tripSyncManager.ts +++ b/client/src/sync/tripSyncManager.ts @@ -52,8 +52,15 @@ interface TripBundle { // ── Helpers ─────────────────────────────────────────────────────────────────── +// Trip dates are plain local calendar dates, so format ours the same way. +// toISOString() would hand back the UTC day and drop an ongoing trip a day +// early west of UTC (and keep a finished one a day too long east of it). +function ymd(d: Date): string { + return `${d.getFullYear()}-${String(d.getMonth() + 1).padStart(2, '0')}-${String(d.getDate()).padStart(2, '0')}` +} + function todayStr(): string { - return new Date().toISOString().slice(0, 10) + return ymd(new Date()) } /** @@ -76,7 +83,7 @@ function isStale(trip: Trip): boolean { if (!trip.end_date) return false const cutoff = new Date() cutoff.setDate(cutoff.getDate() - 7) - return trip.end_date < cutoff.toISOString().slice(0, 10) + return trip.end_date < ymd(cutoff) } function isPhoto(file: TripFile): boolean { @@ -123,6 +130,9 @@ async function cacheFilesForTrip(tripId: number, files: TripFile[]): Promise { expect(res.failed).toEqual([f]) }) + it('FE-W4UPQ-007b: retries a 429 rather than dropping the file', async () => { + let calls = 0 + const uploadOne = vi.fn(async (f: File) => { + calls++ + // A proxy in front of a self-hosted instance rate-limits the batch. + if (calls === 1) throw { response: { status: 429 } } + return [f.name] + }) + + const res = await uploadFilesResilient([file('a.txt')], uploadOne, { retries: 1 }) + + expect(calls).toBe(2) + expect(res.succeeded).toEqual(['a.txt']) + expect(res.failed).toEqual([]) + }) + it('FE-W4UPQ-008: retries a 5xx up to the limit then gives up', async () => { const uploadOne = vi.fn(async () => { throw { response: { status: 502 } } }) const f = file('a.txt') diff --git a/client/src/utils/uploadQueue.ts b/client/src/utils/uploadQueue.ts index de2f92b8e..721cc2d77 100644 --- a/client/src/utils/uploadQueue.ts +++ b/client/src/utils/uploadQueue.ts @@ -22,6 +22,10 @@ const sleep = (ms: number) => new Promise(r => setTimeout(r, ms)) function isRetryable(err: unknown): boolean { if (err && typeof err === 'object' && 'response' in err) { const status = (err as { response?: { status?: number } }).response?.status + // 408/425/429: timeout / too-early / rate-limited. TREK's own upload routes + // do not send them, but a proxy in front of a self-hosted instance does, and + // dropping the file for one of those loses a photo we could have re-sent. + if (status === 408 || status === 425 || status === 429) return true if (status !== undefined && status >= 400 && status < 500) return false } return true diff --git a/client/tests/setup.ts b/client/tests/setup.ts index dcbe38674..d0375bea4 100644 --- a/client/tests/setup.ts +++ b/client/tests/setup.ts @@ -21,8 +21,16 @@ vi.mock('../src/api/websocket', () => ({ removeListener: vi.fn(), })); -// MSW lifecycle -beforeAll(() => server.listen({ onUnhandledRequest: 'warn' })); +// MSW lifecycle. A cross-origin request nobody mocked is the dangerous kind: 'warn' +// lets it through, so the runner really talks to frankfurter or a tile server and settles +// a promise after the test environment is gone (see handlers/external.ts). Those fail now. +// An unhandled same-origin call stays a warning: that is a missing handler, not egress. +beforeAll(() => server.listen({ + onUnhandledRequest: (request, print) => { + if (new URL(request.url).origin === location.origin) print.warning(); + else print.error(); + }, +})); afterEach(() => { server.resetHandlers(); cleanup(); diff --git a/client/tests/unit/mswUnhandledRequests.test.ts b/client/tests/unit/mswUnhandledRequests.test.ts new file mode 100644 index 000000000..76eabe011 --- /dev/null +++ b/client/tests/unit/mswUnhandledRequests.test.ts @@ -0,0 +1,10 @@ +import { describe, it, expect } from 'vitest'; + +// Guards the lifecycle option in tests/setup.ts: a cross-origin call nobody mocked must +// fail here rather than actually leaving the runner (see msw/handlers/external.ts for the +// FX widget that once did, and answered after its test environment was gone). +describe('msw unhandled requests', () => { + it('FE-MSW-EGRESS-001: an unmocked cross-origin request is refused', async () => { + await expect(fetch('https://unmocked.example/rates')).rejects.toThrow(); + }); +}); diff --git a/client/tests/unit/remoteEventHandler/places.test.ts b/client/tests/unit/remoteEventHandler/places.test.ts index 58d5c9ea9..9fb244581 100644 --- a/client/tests/unit/remoteEventHandler/places.test.ts +++ b/client/tests/unit/remoteEventHandler/places.test.ts @@ -51,6 +51,29 @@ describe('remoteEventHandler > places', () => { expect(assignments['10'][0].place?.name).toBe('Cascaded Update'); }); + it('FE-WSEVT-PLACE-004b: place:updated keeps the assignment its own times', () => { + // The day view renders COALESCE(assignment_time, place_time), so the copy + // embedded in an assignment carries the per-stop schedule. A broadcast about + // the pool row must not overwrite it (the local reducer merges the same way). + const place = buildPlace({ id: 1, name: 'Original', place_time: '08:00', end_time: '09:00' }); + const assignment = buildAssignment({ + id: 100, + day_id: 10, + place: { ...place, place_time: '14:00', end_time: '15:30' }, + }); + useTripStore.setState({ places: [place], assignments: { '10': [assignment] } }); + + useTripStore.getState().handleRemoteEvent({ + type: 'place:updated', + place: buildPlace({ id: 1, name: 'Renamed', place_time: '08:00', end_time: '09:00' }), + }); + + const embedded = useTripStore.getState().assignments['10'][0].place!; + expect(embedded.name).toBe('Renamed'); + expect(embedded.place_time).toBe('14:00'); + expect(embedded.end_time).toBe('15:30'); + }); + it('FE-WSEVT-PLACE-005: place:deleted removes place from places array', () => { seedData(); useTripStore.getState().handleRemoteEvent({ type: 'place:deleted', placeId: 1 }); diff --git a/client/tests/unit/sync/syncTriggers.test.ts b/client/tests/unit/sync/syncTriggers.test.ts index fb1aa6eb3..b2826fca2 100644 --- a/client/tests/unit/sync/syncTriggers.test.ts +++ b/client/tests/unit/sync/syncTriggers.test.ts @@ -82,6 +82,10 @@ describe('syncTriggers', () => { expect(flush).toHaveBeenCalled(); expect(syncAll).toHaveBeenCalled(); expect(hydrate).toHaveBeenCalledWith('7'); + // The order is the point: re-seeding Dexie before the queue drains would + // overwrite edits that have not been sent yet. + expect(flush.mock.invocationCallOrder[0]).toBeLessThan(syncAll.mock.invocationCallOrder[0]); + expect(flush.mock.invocationCallOrder[0]).toBeLessThan(hydrate.mock.invocationCallOrder[0]); }); it('online event retries the settings load when it has not yet succeeded (#1618)', async () => {