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 () => {