From 48b1e879d2a278c59d8823929c203a5fc51da8bf Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 8 Oct 2026 11:46:48 +0200 Subject: [PATCH] fix(offline): an online edit waits behind an older queued write to the same entity Holding later queued writes behind a parked one only ordered writes that were in the queue. Online, the repos send an edit straight to the server, so an edit made while an older write to the same visit, tour or driving settings was backing off or parked landed first, and Try again (or the next flush) then replayed the older write over it. Those writes carry no version token, so nothing refused the stale replay. The repos of visits, day clears, tours and driving settings now ask the queue before an online write. While an older write to the same entity is pending, in flight, parked or a conflict, the new write is queued too, with the same optimistic Dexie write as offline, and a flush is started so it goes out as soon as the one ahead of it does. When the driving settings left the cache since the older write, they are read in first. Places and packing items keep their direct online path: they send the base token, so a stale replay is refused with a 409 and becomes a conflict. What this does not cover, and the docs now say so: a collaborator's change to a token-less entity is overwritten by the retried write, as by any replayed offline write of those resources, and the planner's two time edits that call the API directly are not held. The wording of the parking commit also named budget items, which never go through the queue. --- client/CLAUDE.md | 2 +- client/src/repo/assignmentRepo.test.ts | 54 ++++++++++++++++++- client/src/repo/assignmentRepo.ts | 13 +++-- .../src/repo/roadtripPreferencesRepo.test.ts | 13 +++++ client/src/repo/roadtripPreferencesRepo.ts | 6 ++- client/src/repo/tourRepo.test.ts | 18 ++++++- client/src/repo/tourRepo.ts | 3 +- client/src/sync/mutationQueue.ts | 46 ++++++++++++++-- client/tests/unit/sync/mutationQueue.test.ts | 28 ++++++++++ wiki/Offline-Mode-and-PWA.md | 2 +- 10 files changed, 170 insertions(+), 15 deletions(-) diff --git a/client/CLAUDE.md b/client/CLAUDE.md index 16505f55d..56b73f0b2 100644 --- a/client/CLAUDE.md +++ b/client/CLAUDE.md @@ -35,7 +35,7 @@ The layering is **component → feature hook → store/slice → `repo/` → `ap - **`src/repo/`** — per-entity repositories mediating between the network and the offline cache: online, call REST and upsert into Dexie; offline, read Dexie. Always go through a repo for trip data. - **`src/api/client.ts`** — the single Axios instance, typed from `@trek/shared`. New API surface goes in a per-domain `api/.ts`, not the god module. - **`src/db/offlineDb.ts`** — the Dexie schema. Database and table names are the on-device contract — renaming one orphans every user's cache. -- **`src/sync/`** — `mutationQueue.ts`: offline writes do an optimistic Dexie write (temporary **negative id** for creates), then `flush()` replays REST on reconnect with the mutation's UUID as the **`X-Idempotency-Key`** header (matching the server interceptor) and reconciles the row. Writes to one entity keep their order: a write parked as `failed` or `conflict` holds back the later writes to the same `resource` + entity id until the user retries, resolves or discards it, and only that action deletes a parked row (trip eviction never does). `flush()` takes the Web Lock `trek-mutation-flush:` so only one tab replays at a time; a tab that finds it taken waits for it, so `flush()` never resolves while another tab is still replaying and a re-seed chained onto it cannot lay older server rows over those writes (the in-tab flag alone where the API is missing). The rest of the directory drives reconnection sync, the auth gate that stops background loops before logout swaps the DB, offline preferences, and map pre-download (raster and vector prefetchers). +- **`src/sync/`** — `mutationQueue.ts`: offline writes do an optimistic Dexie write (temporary **negative id** for creates), then `flush()` replays REST on reconnect with the mutation's UUID as the **`X-Idempotency-Key`** header (matching the server interceptor) and reconciles the row. Writes to one entity keep their order: a write parked as `failed` or `conflict` holds back the later writes to the same `resource` + entity id until the user retries, resolves or discards it, and only that action deletes a parked row (trip eviction never does). Repos whose writes carry no version token (visits, day clears, tours, driving settings) ask `mutationQueue.mustQueue()` before an online write and queue it while an older write to the entity is still queued, so a retried write cannot land over a newer one made here online; places and packing items send `X-Base-Updated-At`, so the server refuses the stale replay with a 409 instead. `flush()` takes the Web Lock `trek-mutation-flush:` so only one tab replays at a time; a tab that finds it taken waits for it, so `flush()` never resolves while another tab is still replaying and a re-seed chained onto it cannot lay older server rows over those writes (the in-tab flag alone where the API is missing). The rest of the directory drives reconnection sync, the auth gate that stops background loops before logout swaps the DB, offline preferences, and map pre-download (raster and vector prefetchers). ## Rules for new code diff --git a/client/src/repo/assignmentRepo.test.ts b/client/src/repo/assignmentRepo.test.ts index a78916a9e..546e24de1 100644 --- a/client/src/repo/assignmentRepo.test.ts +++ b/client/src/repo/assignmentRepo.test.ts @@ -11,7 +11,7 @@ import type { Assignment, Day } from '../types' vi.mock('../api/assignmentEndDay', () => ({ saveAssignmentEndDay: vi.fn() })) vi.mock('../sync/networkMode', () => ({ isEffectivelyOffline: vi.fn(() => true) })) vi.mock('../sync/authGate', () => ({ isAuthed: () => true })) -vi.mock('../api/client', () => ({ apiClient: { request: vi.fn() }, assignmentsApi: { updateTime: vi.fn() } })) +vi.mock('../api/client', () => ({ apiClient: { request: vi.fn() }, assignmentsApi: { updateTime: vi.fn(), clearDay: vi.fn() } })) const assignment = { id: 7, day_id: 1, place_id: 2, order_index: 0, assignment_time: '07:00', place: { id: 2, name: 'Berlin' } } as Assignment beforeEach(async () => { @@ -79,3 +79,55 @@ describe('assignment time persistence', () => { expect((await offlineDb.days.get(1))?.assignments?.[0]).toEqual(assignment) }) }) + +describe('an online edit of a visit with an older write still queued', () => { + const first = { place_time: '07:00', end_time: null } + const second = { place_time: '09:00', end_time: null } + + /** Queue the first edit offline, then come back online. */ + async function queueFirst(status: 'pending' | 'failed') { + await assignmentRepo.setTimes(9, assignment, first) + if (status === 'failed') await offlineDb.mutationQueue.toCollection().modify({ status: 'failed', attempts: 8 }) + vi.mocked(isEffectivelyOffline).mockReturnValue(false) + vi.mocked(apiClient.request).mockImplementation(async ({ data }) => ({ data: { assignment: { ...assignment, assignment_time: (data as typeof first).place_time } } })) + } + + const sentTimes = () => vi.mocked(apiClient.request).mock.calls.map(([req]) => (req.data as typeof first).place_time) + + it('waits behind a parked one, so Try again cannot replay the older time over it', async () => { + await queueFirst('failed') + + await assignmentRepo.setTimes(9, assignment, second) + // Not sent past the parked write, not even by the flush it started. + await new Promise(resolve => setTimeout(resolve, 20)) + expect(assignmentsApi.updateTime).not.toHaveBeenCalled() + expect(apiClient.request).not.toHaveBeenCalled() + expect((await offlineDb.days.get(1))?.assignments?.[0].assignment_time).toBe('09:00') + + await mutationQueue.retryFailed() + expect(sentTimes()).toEqual(['07:00', '09:00']) + expect(await offlineDb.mutationQueue.count()).toBe(0) + expect((await offlineDb.days.get(1))?.assignments?.[0].assignment_time).toBe('09:00') + }) + + it('goes out right after an older one that was only waiting to be sent', async () => { + await queueFirst('pending') + + await assignmentRepo.setTimes(9, assignment, second) + + await vi.waitFor(async () => expect(await offlineDb.mutationQueue.count()).toBe(0)) + expect(assignmentsApi.updateTime).not.toHaveBeenCalled() + expect(sentTimes()).toEqual(['07:00', '09:00']) + }) + + it('holds a day clear behind an older clear of the same day', async () => { + await assignmentRepo.clearDay(9, 1) + await offlineDb.mutationQueue.toCollection().modify({ status: 'failed', attempts: 8 }) + vi.mocked(isEffectivelyOffline).mockReturnValue(false) + + await assignmentRepo.clearDay(9, 1) + + expect(assignmentsApi.clearDay).not.toHaveBeenCalled() + expect((await offlineDb.mutationQueue.toArray()).map(m => m.status).sort()).toEqual(['failed', 'pending']) + }) +}) diff --git a/client/src/repo/assignmentRepo.ts b/client/src/repo/assignmentRepo.ts index b17795cfc..72f8d008f 100644 --- a/client/src/repo/assignmentRepo.ts +++ b/client/src/repo/assignmentRepo.ts @@ -4,7 +4,6 @@ import { assignmentsApi } from '../api/client' import { offlineDb } from '../db/offlineDb' import { cacheAssignment } from '../db/cacheAssignment' import { generateUUID, mutationQueue } from '../sync/mutationQueue' -import { isEffectivelyOffline } from '../sync/networkMode' import type { Assignment } from '../types' /** Start and End of one visit, as the time route stores them: null is no time. */ @@ -20,7 +19,7 @@ export const assignmentRepo = { * replay, so a place added meanwhile by someone else goes with the rest. */ async clearDay(tripId: number | string, dayId: number): Promise { - if (!isEffectivelyOffline()) { + if (!(await mutationQueue.mustQueue('dayAssignments', dayId))) { await assignmentsApi.clearDay(tripId, dayId) await offlineDb.days.where('id').equals(dayId).modify(day => { day.assignments = [] }) return @@ -33,10 +32,11 @@ export const assignmentRepo = { }) await offlineDb.days.where('id').equals(dayId).modify(day => { day.assignments = [] }) }) + mutationQueue.sendSoon() }, async setEndDay(tripId: number | string, assignment: Assignment, endDay: boolean): Promise { - if (!isEffectivelyOffline()) { + if (!(await mutationQueue.mustQueue('assignments', assignment.id))) { const saved = await saveAssignmentEndDay(tripId, assignment.id, { end_day: endDay }) await cacheAssignment(saved) return saved @@ -50,12 +50,13 @@ export const assignmentRepo = { }) await cacheAssignment(updated) }) + mutationQueue.sendSoon() return updated }, /** Takes a stop out of the day's route or puts it back (#2532), offline too. */ async setRouteExcluded(tripId: number | string, assignment: Assignment, excluded: boolean): Promise { - if (!isEffectivelyOffline()) { + if (!(await mutationQueue.mustQueue('assignments', assignment.id))) { const saved = assignmentSchema.parse((await assignmentsApi.setRouteExcluded(tripId, assignment.id, excluded)).assignment) await cacheAssignment(saved) return saved @@ -69,6 +70,7 @@ export const assignmentRepo = { }) await cacheAssignment(updated) }) + mutationQueue.sendSoon() return updated }, @@ -79,7 +81,7 @@ export const assignmentRepo = { * other in as it stands; leaving it out would clear it. */ async setTimes(tripId: number | string, assignment: Assignment, times: AssignmentTimes): Promise { - if (!isEffectivelyOffline()) { + if (!(await mutationQueue.mustQueue('assignments', assignment.id))) { const saved = assignmentSchema.parse((await assignmentsApi.updateTime(tripId, assignment.id, times)).assignment) await cacheAssignment(saved) return saved @@ -93,6 +95,7 @@ export const assignmentRepo = { }) await cacheAssignment(updated) }) + mutationQueue.sendSoon() return updated }, } diff --git a/client/src/repo/roadtripPreferencesRepo.test.ts b/client/src/repo/roadtripPreferencesRepo.test.ts index 0694a22fa..08921e964 100644 --- a/client/src/repo/roadtripPreferencesRepo.test.ts +++ b/client/src/repo/roadtripPreferencesRepo.test.ts @@ -41,4 +41,17 @@ describe('trip driving preferences offline cache', () => { await expect(roadtripPreferencesRepo.update(9, { roadtrip_range_km: 999 })).rejects.toThrow('Denied') expect((await offlineDb.roadtripPreferences.get(9))?.preferences.roadtrip_range_km).toBe(120) }) + it('queues an online change behind an older one that is parked, reading the settings in if the cache lost them', async () => { + await roadtripPreferencesRepo.update(9, { roadtrip_range_km: 200 }) + await offlineDb.mutationQueue.toCollection().modify({ status: 'failed', attempts: 8 }) + await offlineDb.roadtripPreferences.delete(9) + vi.mocked(isEffectivelyOffline).mockReturnValue(false) + vi.mocked(apiClient.get).mockResolvedValue({ data: { tripId: 9, preferences: { roadtrip_day_start: '08:00', roadtrip_day_end: '18:00', roadtrip_range_km: 150 } } }) + + expect((await roadtripPreferencesRepo.update(9, { roadtrip_range_km: 300 })).roadtrip_range_km).toBe(300) + + expect(apiClient.put).not.toHaveBeenCalled() + expect((await offlineDb.mutationQueue.toArray()).map(m => [m.status, (m.body as { roadtrip_range_km: number }).roadtrip_range_km]).sort()) + .toEqual([['failed', 200], ['pending', 300]]) + }) }) diff --git a/client/src/repo/roadtripPreferencesRepo.ts b/client/src/repo/roadtripPreferencesRepo.ts index ad1cd7821..2f59fa103 100644 --- a/client/src/repo/roadtripPreferencesRepo.ts +++ b/client/src/repo/roadtripPreferencesRepo.ts @@ -34,17 +34,21 @@ export const roadtripPreferencesRepo = { const save = (writes.get(tripId) ?? Promise.resolve({})).catch(() => ({})).then(async () => { if (offlineDb.name !== cacheName) throw new Error('Account changed.') const validated = roadtripPreferencesUpdateSchema.parse(patch) - if (!isEffectivelyOffline()) { + if (!(await mutationQueue.mustQueue('roadtripPreferences', tripId))) { const saved = await apiClient.put(`/trips/${tripId}/roadtrip/preferences`, validated) if (offlineDb.name !== cacheName) throw new Error('Account changed.') await offlineDb.roadtripPreferences.put(roadtripPreferencesResponseSchema.parse(saved.data)) } else { + // Online and behind an older queued write: the trip may have left the + // cache since that write was made, so read the settings in first. + if (!isEffectivelyOffline() && !(await offlineDb.roadtripPreferences.get(tripId))) await roadtripPreferencesRepo.read(tripId) const next = { ...await cachedRoadtripPreferences(tripId), ...validated } if (next.roadtrip_day_start && next.roadtrip_day_end && next.roadtrip_day_end <= next.roadtrip_day_start) throw new Error('Day end must be later than day start.') await mutationQueue.enqueue({ id: generateUUID(), tripId, method: 'PUT', url: `/trips/${tripId}/roadtrip/preferences`, body: validated, resource: 'roadtripPreferences', entityId: tripId, }) + mutationQueue.sendSoon() } return cachedRoadtripPreferences(tripId) }) diff --git a/client/src/repo/tourRepo.test.ts b/client/src/repo/tourRepo.test.ts index dff7a1d0b..dd7518818 100644 --- a/client/src/repo/tourRepo.test.ts +++ b/client/src/repo/tourRepo.test.ts @@ -1,4 +1,4 @@ -// FE-REPO-TOUR-001 to FE-REPO-TOUR-012 +// FE-REPO-TOUR-001 to FE-REPO-TOUR-014 // Tours through the offline core: read-through to Dexie, offline create and // edit written there and queued, and the online paths keeping the copy fresh. import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' @@ -189,6 +189,22 @@ describe('tourRepo.update', () => { expect(await offlineDb.tours.get(40)).toMatchObject({ name: 'Edited', waypoints: body.waypoints }) expect(await offlineDb.tours.get(50)).toMatchObject({ trip_id: 3 }) }) + + it('FE-REPO-TOUR-014: online, an edit of a tour with an older write parked waits behind it in the queue', async () => { + await offlineDb.tours.put({ ...tour(), trip_id: 3 }) + await offlineDb.mutationQueue.put({ + id: 'parked-tour', tripId: 3, method: 'PUT', url: '/trips/3/tours/40', body, createdAt: 1, + status: 'failed', attempts: 8, lastError: 'boom', resource: 'tours', entityId: 40, + }) + let sent = 0 + server.use(http.put('/api/trips/3/tours/40', () => { sent++; return HttpResponse.json({ tour: tour(), waypoints: body.waypoints }) })) + + const result = await tourRepo.update(3, 40, { ...body, name: 'Renamed' }) + + expect(result.tour.name).toBe('Renamed') + expect(sent).toBe(0) + expect((await offlineDb.mutationQueue.toArray()).map(m => m.status).sort()).toEqual(['failed', 'pending']) + }) }) describe('replaceTripTours', () => { diff --git a/client/src/repo/tourRepo.ts b/client/src/repo/tourRepo.ts index c2b5a73b5..a73cb9f75 100644 --- a/client/src/repo/tourRepo.ts +++ b/client/src/repo/tourRepo.ts @@ -130,7 +130,7 @@ export const tourRepo = { }, async update(tripId: number | string, placeId: number, body: TourCreateRequest): Promise { - if (isEffectivelyOffline()) { + if (await mutationQueue.mustQueue('tours', placeId)) { const tid = Number(tripId) const existing = await offlineDb.tours.get(placeId) const tour: CachedTour = { @@ -153,6 +153,7 @@ export const tourRepo = { entityId: placeId, ...(isTemp ? { tempEntityId: placeId } : {}), }) + mutationQueue.sendSoon() return { tour: toListItem(tour), waypoints: body.waypoints } } const result = await toursApi.update(tripId, placeId, body) diff --git a/client/src/sync/mutationQueue.ts b/client/src/sync/mutationQueue.ts index 950cb3fda..c16539dbe 100644 --- a/client/src/sync/mutationQueue.ts +++ b/client/src/sync/mutationQueue.ts @@ -230,6 +230,38 @@ export const mutationQueue = { return item.id }, + /** + * Does a write to this entity have to join the queue? Offline, always. Online + * too while an older write to the same entity is still queued, backing off, + * parked as failed or waiting as a conflict: sent straight to the server the + * new write would land first, and Try again or the next flush would replay + * the older one over it. Queued, it waits behind the older write and keeps + * the order the user made them in. The repos of visits, a day's visits, tours + * and the driving settings ask this, because those writes carry no version + * token; a place or packing item sends one, so the server refuses the older + * write (409) and it becomes a conflict instead. + */ + async mustQueue(resource: string, entityId: number): Promise { + if (isEffectivelyOffline()) return true + const key = `${resource}:${entityId}` + const older = await offlineDb.mutationQueue + .where('status') + .anyOf(['pending', 'syncing', 'failed', 'conflict']) + .filter(m => entityKey(m) === key) + .first() + return older !== undefined + }, + + /** + * Start a flush for a write that joined the queue while online (see + * mustQueue). It goes out as soon as the write it waits for is through, not + * at the next trigger. Offline this does nothing. + */ + sendSoon(): void { + if (isEffectivelyOffline()) return + this.flush().catch(console.error) + }, + /** * Drain the queue: replay each pending mutation against the server in FIFO order. * Stops on the first network error (retried on the next trigger). 4xx answers @@ -528,10 +560,16 @@ export const mutationQueue = { }, /** - * Put every parked change back in line, from a fresh start, and flush. The - * later writes to the same entities waited behind them, so the replay keeps - * the order they were made in, and a place or packing item still sends the - * token it was edited against, which a newer server version refuses (409). + * Put every parked change back in line, from a fresh start, and flush. + * The later writes to the same entity made on this device waited behind it: + * offline ones in the queue and, for visits, tours and the driving settings, + * online ones too (mustQueue), so they go out after it in the order they were + * made. A place or packing item still sends the token it was edited against, + * so any newer version on the server, a collaborator's or one saved here + * online, refuses it (409) and it becomes a conflict. A write without a token + * overwrites what someone else changed on the server meanwhile, as every + * replayed offline write of those resources does, and so do writes made here + * that skip the repos (the planner's two direct time edits of a visit). */ async retryFailed(): Promise { await offlineDb.mutationQueue diff --git a/client/tests/unit/sync/mutationQueue.test.ts b/client/tests/unit/sync/mutationQueue.test.ts index 41a9af4cf..5701587bc 100644 --- a/client/tests/unit/sync/mutationQueue.test.ts +++ b/client/tests/unit/sync/mutationQueue.test.ts @@ -715,6 +715,34 @@ describe('mutationQueue: writes to one entity keep their order around a parked o }); }); +describe('mutationQueue.mustQueue: a write to an entity with an older one still in the queue', () => { + const visit = (status: 'pending' | 'syncing' | 'failed' | 'conflict', entityId = 7) => ({ + id: generateUUID(), tripId: 1, method: 'PUT' as const, url: `/trips/1/assignments/${entityId}/time`, + body: {}, createdAt: 1, status, attempts: 0, lastError: null, resource: 'assignments', entityId, + }); + + it('is always true offline', async () => { + Object.defineProperty(navigator, 'onLine', { value: false }); + expect(await mutationQueue.mustQueue('assignments', 7)).toBe(true); + }); + + it('is false online while nothing is queued for the entity', async () => { + await offlineDb.mutationQueue.put(visit('failed', 8)); + await offlineDb.mutationQueue.put({ ...visit('failed'), resource: 'places' }); + expect(await mutationQueue.mustQueue('assignments', 7)).toBe(false); + }); + + it.each(['pending', 'syncing', 'failed', 'conflict'] as const)('is true online behind a %s write to the entity', async status => { + await offlineDb.mutationQueue.put(visit(status)); + expect(await mutationQueue.mustQueue('assignments', 7)).toBe(true); + }); + + it('is true for an edit of an offline create that has not synced yet', async () => { + await mutationQueue.enqueue({ id: generateUUID(), tripId: 1, method: 'POST', url: '/trips/1/tours', body: {}, resource: 'tours', tempId: -42 }); + expect(await mutationQueue.mustQueue('tours', -42)).toBe(true); + }); +}); + describe('mutationQueue.flush: one tab at a time', () => { /** A Web Locks stand-in: one holder per name, later requests wait their turn. */ function fakeLocks() { diff --git a/wiki/Offline-Mode-and-PWA.md b/wiki/Offline-Mode-and-PWA.md index 200eacb63..5c9c0d6a2 100644 --- a/wiki/Offline-Mode-and-PWA.md +++ b/wiki/Offline-Mode-and-PWA.md @@ -95,7 +95,7 @@ If a change you made offline collides with a newer change on the server, it is s ### Stats & cache -The stats panel shows cached trips, pending changes, conflicts and failed changes. A change the server keeps refusing with a server error is retried with growing gaps (from 30 seconds up to about two hours) and only holds back the other changes of its own trip; after eight attempts it counts as failed. Later changes to the same item (the same place, packing item, visit, tour or driving settings) wait behind a failed change or an open conflict instead of overtaking it, so they still reach the server in the order you made them. Failed changes come with **Try again** and **Discard**: trying again sends them first and then the changes that waited behind them; discarding keeps the server's version, sends the changes that waited, and the next sync puts the server's version back on the device. **Clear cache** removes all offline data from IndexedDB after you confirm it in TREK's dialog (you can re-sync any time while online). Each cached trip entry shows its date range, place/file count and last successful sync. +The stats panel shows cached trips, pending changes, conflicts and failed changes. A change the server keeps refusing with a server error is retried with growing gaps (from 30 seconds up to about two hours) and only holds back the other changes of its own trip; after eight attempts it counts as failed. Later changes you make on this device to the same item (the same place, packing item, visit, tour or driving settings) wait behind a failed change or an open conflict instead of overtaking it, so they reach the server in the order you made them; for visits, tours and driving settings that includes changes you make while online. Places and packing items remember which version you edited, so trying a failed change again never overwrites a newer version saved in the meantime, on this device or by another member of the trip: it turns into a conflict for you to decide. Visits, tours and driving settings carry no such version, so trying again overwrites what another member changed on the same item in the meantime, the same as any change sent after being offline. Failed changes come with **Try again** and **Discard**: trying again sends them first and then the changes that waited behind them; discarding keeps the server's version, sends the changes that waited, and the next sync puts the server's version back on the device. **Clear cache** removes all offline data from IndexedDB after you confirm it in TREK's dialog (you can re-sync any time while online). Each cached trip entry shows its date range, place/file count and last successful sync. ## Limitations