From 5dbcc4af40aedf01ba7bc8e67aa14a6bd2a4bdbe Mon Sep 17 00:00:00 2001 From: Maurice Date: Mon, 12 Oct 2026 18:24:18 +0200 Subject: [PATCH] fix(todo): tasks added, ticked, edited, reordered or deleted offline wait in the queue and sync once the connection is back Every task write went straight to the API, so offline a new task failed with "Network Error" and was lost, and a tick flipped back. Tasks now go through todoRepo and the mutation queue like packing items: a temporary row at the end of the list offline, the edits and the delete of it wait for its real id, and an online write to a task that still has an older one queued joins the queue behind it, since task writes carry no version token. A reorder offline is queued without the tasks that have no server id yet. Desktop and phone both write through the store, so both are covered. Refs #2601 --- client/src/repo/todoRepo.offline.test.ts | 167 +++++++++++++++++++++ client/src/repo/todoRepo.ts | 128 ++++++++++++++++ client/src/store/slices/todoSlice.ts | 12 +- client/tests/unit/slices/todoSlice.test.ts | 32 ++++ 4 files changed, 333 insertions(+), 6 deletions(-) create mode 100644 client/src/repo/todoRepo.offline.test.ts diff --git a/client/src/repo/todoRepo.offline.test.ts b/client/src/repo/todoRepo.offline.test.ts new file mode 100644 index 000000000..c2ba96dab --- /dev/null +++ b/client/src/repo/todoRepo.offline.test.ts @@ -0,0 +1,167 @@ +// FE-REPO-TODO-OFF-001 to FE-REPO-TODO-OFF-007 +// Offline write paths (#2601), todoRepo.test.ts covers the reads. +import 'fake-indexeddb/auto'; +import { http, HttpResponse } from 'msw'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { buildTodoItem } from '../../tests/helpers/factories'; +import { server } from '../../tests/helpers/msw/server'; +import type { QueuedMutation } from '../db/offlineDb'; +import { clearAll, offlineDb } from '../db/offlineDb'; +import { setAuthed } from '../sync/authGate'; +import { mutationQueue } from '../sync/mutationQueue'; +import { todoRepo } from './todoRepo'; + +function setOnline(v: boolean): void { + Object.defineProperty(navigator, 'onLine', { value: v, writable: true, configurable: true }); +} + +async function queue(): Promise { + return (await offlineDb.mutationQueue.toArray()).sort((a, b) => a.createdAt - b.createdAt); +} + +beforeEach(async () => { + await clearAll(); + mutationQueue._resetFlushing(); + setOnline(false); +}); + +afterEach(() => { + vi.restoreAllMocks(); + setAuthed(false); + setOnline(true); +}); + +describe('todoRepo — offline', () => { + it('FE-REPO-TODO-OFF-001: a new task gets a temp row at the end of the list and a queued POST', async () => { + await offlineDb.todoItems.bulkPut([ + buildTodoItem({ id: 10, trip_id: 4, sort_order: 0 }), + buildTodoItem({ id: 11, trip_id: 4, sort_order: 3 }), + buildTodoItem({ id: 12, trip_id: 5, sort_order: 9 }), + ]); + + const { item } = await todoRepo.create(4, { name: 'Passport', category: 'Docs', priority: 2, due_date: null }); + + expect(item.id).toBeLessThan(0); + expect(item).toMatchObject({ + trip_id: 4, + name: 'Passport', + category: 'Docs', + checked: 0, + sort_order: 4, + priority: 2, + due_date: null, + description: null, + assigned_user_id: null, + }); + expect(await offlineDb.todoItems.get(item.id)).toMatchObject({ name: 'Passport' }); + const [mutation] = await queue(); + expect(mutation).toMatchObject({ + method: 'POST', + url: '/trips/4/todo', + resource: 'todoItems', + tempId: item.id, + body: { name: 'Passport', category: 'Docs', priority: 2, due_date: null }, + }); + }); + + it('FE-REPO-TODO-OFF-002: a tick, an edit and a delete of an unsynced task wait for its id', async () => { + const { item } = await todoRepo.create(4, { name: 'Visa' }); + const { item: ticked } = await todoRepo.update(4, item.id, { checked: true }); + expect(ticked).toMatchObject({ id: item.id, checked: 1 }); + const { item: edited } = await todoRepo.update(4, item.id, { name: 'Visa form', description: null }); + expect(edited).toMatchObject({ name: 'Visa form', checked: 1 }); + await todoRepo.delete(4, item.id); + + expect(await offlineDb.todoItems.get(item.id)).toBeUndefined(); + const [, tick, edit, del] = await queue(); + expect(tick).toMatchObject({ + method: 'PUT', + url: '/trips/4/todo/{id}', + tempEntityId: item.id, + entityId: item.id, + body: { checked: true }, + }); + expect(edit).toMatchObject({ method: 'PUT', url: '/trips/4/todo/{id}', tempEntityId: item.id }); + expect(del).toMatchObject({ method: 'DELETE', url: '/trips/4/todo/{id}', tempEntityId: item.id }); + }); + + it('FE-REPO-TODO-OFF-003: an edit of a synced task keeps the fields it does not name', async () => { + await offlineDb.todoItems.put( + buildTodoItem({ id: 20, trip_id: 4, name: 'Insurance', priority: 1, category: 'Docs' }) + ); + const { item } = await todoRepo.update(4, 20, { name: 'Travel insurance', category: null }); + expect(item).toMatchObject({ id: 20, name: 'Travel insurance', priority: 1, category: null }); + const [mutation] = await queue(); + expect(mutation).toMatchObject({ method: 'PUT', url: '/trips/4/todo/20', entityId: 20, resource: 'todoItems' }); + }); + + it('FE-REPO-TODO-OFF-004: a reorder is written here and queued without the tasks that have no server id yet', async () => { + await offlineDb.todoItems.bulkPut([ + buildTodoItem({ id: 30, trip_id: 4, sort_order: 0 }), + buildTodoItem({ id: 31, trip_id: 4, sort_order: 1 }), + ]); + await todoRepo.reorder(4, [31, -7, 30]); + expect((await offlineDb.todoItems.get(31))?.sort_order).toBe(0); + expect((await offlineDb.todoItems.get(30))?.sort_order).toBe(2); + const [mutation] = await queue(); + expect(mutation).toMatchObject({ method: 'PUT', url: '/trips/4/todo/reorder', body: { orderedIds: [31, 30] } }); + expect(mutation.resource).toBeUndefined(); + }); + + it('FE-REPO-TODO-OFF-005: online, an edit of a task with a queued write joins the queue behind it', async () => { + await offlineDb.todoItems.put(buildTodoItem({ id: 40, trip_id: 4, name: 'Old' })); + await todoRepo.update(4, 40, { name: 'Offline' }); + setOnline(true); + let sent = 0; + server.use( + http.put('/api/trips/4/todo/40', () => { + sent += 1; + return HttpResponse.error(); + }) + ); + const sendSoon = vi.spyOn(mutationQueue, 'sendSoon').mockImplementation(() => {}); + + await todoRepo.update(4, 40, { name: 'Online' }); + + expect(sent).toBe(0); + expect(sendSoon).toHaveBeenCalled(); + expect((await queue()).map((m) => (m.body as { name: string }).name)).toEqual(['Offline', 'Online']); + }); + + it('FE-REPO-TODO-OFF-006: online, a write goes straight out and lands in the cache', async () => { + setOnline(true); + server.use( + http.post('/api/trips/4/todo', () => + HttpResponse.json({ item: buildTodoItem({ id: 50, trip_id: 4, name: 'Tickets' }) }) + ) + ); + const { item } = await todoRepo.create(4, { name: 'Tickets' }); + expect(item.id).toBe(50); + expect(await offlineDb.todoItems.get(50)).toMatchObject({ name: 'Tickets' }); + expect(await queue()).toEqual([]); + }); + + it('FE-REPO-TODO-OFF-007: the replay moves a new task to its real id and the queued tick follows it', async () => { + const { item } = await todoRepo.create(4, { name: 'Adapter' }); + await todoRepo.update(4, item.id, { checked: true }); + const puts: string[] = []; + server.use( + http.post('/api/trips/4/todo', () => + HttpResponse.json({ item: buildTodoItem({ id: 60, trip_id: 4, name: 'Adapter' }) }) + ), + http.put('/api/trips/4/todo/:id', ({ params }) => { + puts.push(String(params.id)); + return HttpResponse.json({ item: buildTodoItem({ id: 60, trip_id: 4, name: 'Adapter', checked: 1 }) }); + }) + ); + setAuthed(true); + setOnline(true); + + await mutationQueue.flush(); + + expect(puts).toEqual(['60']); + expect(await offlineDb.todoItems.get(item.id)).toBeUndefined(); + expect(await offlineDb.todoItems.get(60)).toMatchObject({ checked: 1 }); + expect(await queue()).toEqual([]); + }); +}); diff --git a/client/src/repo/todoRepo.ts b/client/src/repo/todoRepo.ts index 27f1b584c..62136b06f 100644 --- a/client/src/repo/todoRepo.ts +++ b/client/src/repo/todoRepo.ts @@ -1,8 +1,65 @@ import { todoApi } from '../api/client' import { offlineDb, replaceTripRows } from '../db/offlineDb' +import { mutationQueue, generateUUID, nextTempId } from '../sync/mutationQueue' +import { isEffectivelyOffline } from '../sync/networkMode' import { onlineThenCache } from './withOfflineFallback' import type { TodoItem } from '../types' +import type { TodoCreateItemRequest, TodoUpdateItemRequest } from '@trek/shared' +/** + * The task an offline create leaves on screen until the queue replays it. The + * defaults are the ones the server fills in, and it goes to the end of the list + * as a new task does there. + */ +async function localItem(tempId: number, tripId: number, data: TodoCreateItemRequest): Promise { + const rows = await offlineDb.todoItems.where('trip_id').equals(tripId).toArray() + const last = rows.reduce((max, row) => Math.max(max, row.sort_order ?? -1), -1) + return { + id: tempId, + trip_id: tripId, + name: data.name, + category: data.category || null, + checked: 0, + sort_order: last + 1, + due_date: data.due_date || null, + description: data.description || null, + assigned_user_id: data.assigned_user_id || null, + priority: data.priority || 0, + } +} + +/** The fields of an edit as the row keeps them: the tick as 0/1, a null priority as none. */ +function editedFields(data: TodoUpdateItemRequest): Partial { + const { checked, priority, ...rest } = data + return { + ...rest, + ...(checked === undefined ? {} : { checked: checked ? 1 : 0 }), + ...(priority === undefined ? {} : { priority: priority ?? 0 }), + } as Partial +} + +/** Queue a write to one task, held behind any older write to it. */ +async function enqueueItemWrite(tripId: number | string, id: number, method: 'PUT' | 'DELETE', body: unknown): Promise { + await mutationQueue.enqueue({ + id: generateUUID(), + tripId: Number(tripId), + method, + url: `/trips/${tripId}/todo/${id < 0 ? '{id}' : id}`, + body, + resource: 'todoItems', + entityId: id, + ...(id < 0 ? { tempEntityId: id } : {}), + }) + mutationQueue.sendSoon() +} + +/** + * Tasks through the offline core. Reads fall back to the Dexie copy; a write + * made offline lands there and in the mutation queue and replays once the + * connection is back. Task writes carry no version token, so an online write to + * a task that still has an older one queued joins the queue behind it + * (mustQueue). + */ export const todoRepo = { async list(tripId: number | string): Promise<{ items: TodoItem[] }> { return onlineThenCache( @@ -17,4 +74,75 @@ export const todoRepo = { }), ) }, + + async create(tripId: number | string, data: TodoCreateItemRequest): Promise<{ item: TodoItem }> { + if (!isEffectivelyOffline()) { + const result: { item: TodoItem } = await todoApi.create(tripId, data) + await offlineDb.todoItems.put(result.item) + return result + } + const tid = Number(tripId) + const tempId = nextTempId() + const item = await localItem(tempId, tid, data) + await offlineDb.transaction('rw', offlineDb.todoItems, offlineDb.mutationQueue, async () => { + await offlineDb.todoItems.put(item) + await mutationQueue.enqueue({ + id: generateUUID(), + tripId: tid, + method: 'POST', + url: `/trips/${tripId}/todo`, + body: data, + resource: 'todoItems', + tempId, + }) + }) + return { item } + }, + + async update(tripId: number | string, id: number, data: TodoUpdateItemRequest): Promise<{ item: TodoItem }> { + if (!(await mutationQueue.mustQueue('todoItems', id))) { + const result: { item: TodoItem } = await todoApi.update(tripId, id, data) + await offlineDb.todoItems.put(result.item) + return result + } + const before = await offlineDb.todoItems.get(id) + const item = { ...(before ?? {}), ...editedFields(data), id, trip_id: Number(tripId) } as TodoItem + await offlineDb.todoItems.put(item) + await enqueueItemWrite(tripId, id, 'PUT', data) + return { item } + }, + + async delete(tripId: number | string, id: number): Promise { + if (!(await mutationQueue.mustQueue('todoItems', id))) { + await todoApi.delete(tripId, id) + await offlineDb.todoItems.delete(id) + return + } + await offlineDb.todoItems.delete(id) + await enqueueItemWrite(tripId, id, 'DELETE', undefined) + }, + + /** + * The new order of the tasks. Offline it waits in the queue; a task created + * offline has no server id to name yet, so it keeps the place the server + * gives a new one, at the end. + */ + async reorder(tripId: number | string, orderedIds: number[]): Promise { + if (!isEffectivelyOffline()) { + await todoApi.reorder(tripId, orderedIds) + return + } + await offlineDb.transaction('rw', offlineDb.todoItems, offlineDb.mutationQueue, async () => { + for (const [index, id] of orderedIds.entries()) { + await offlineDb.todoItems.update(id, { sort_order: index }) + } + await mutationQueue.enqueue({ + id: generateUUID(), + tripId: Number(tripId), + method: 'PUT', + url: `/trips/${tripId}/todo/reorder`, + body: { orderedIds: orderedIds.filter(id => id > 0) }, + }) + }) + }, } diff --git a/client/src/store/slices/todoSlice.ts b/client/src/store/slices/todoSlice.ts index 03092d1c3..fb6ebd83c 100644 --- a/client/src/store/slices/todoSlice.ts +++ b/client/src/store/slices/todoSlice.ts @@ -1,4 +1,4 @@ -import { todoApi } from '../../api/client' +import { todoRepo } from '../../repo/todoRepo' import type { StoreApi } from 'zustand' import type { TripStoreState } from '../tripStore' import type { TodoItem } from '../../types' @@ -20,7 +20,7 @@ export interface TodoSlice { export const createTodoSlice = (set: SetState, get: GetState): TodoSlice => ({ addTodoItem: async (tripId, data) => { try { - const result = await todoApi.create(tripId, data) + const result = await todoRepo.create(tripId, data) set(state => ({ todoItems: [...state.todoItems, result.item] })) return result.item } catch (err: unknown) { @@ -30,7 +30,7 @@ export const createTodoSlice = (set: SetState, get: GetState): TodoSlice => ({ updateTodoItem: async (tripId, id, data) => { try { - const result = await todoApi.update(tripId, id, data) + const result = await todoRepo.update(tripId, id, data) set(state => ({ todoItems: state.todoItems.map(item => item.id === id ? result.item : item) })) @@ -44,7 +44,7 @@ export const createTodoSlice = (set: SetState, get: GetState): TodoSlice => ({ const prev = get().todoItems set(state => ({ todoItems: state.todoItems.filter(item => item.id !== id) })) try { - await todoApi.delete(tripId, id) + await todoRepo.delete(tripId, id) } catch (err: unknown) { set({ todoItems: prev }) throw new Error(getApiErrorMessage(err, 'Error deleting todo')) @@ -58,7 +58,7 @@ export const createTodoSlice = (set: SetState, get: GetState): TodoSlice => ({ ) })) try { - await todoApi.update(tripId, id, { checked }) + await todoRepo.update(tripId, id, { checked }) } catch (err: unknown) { // The caller fires this optimistically and doesn't await, so rolling back // silently would just flip the checkbox with no explanation. Surface it. @@ -85,7 +85,7 @@ export const createTodoSlice = (set: SetState, get: GetState): TodoSlice => ({ return { todoItems: [...reordered, ...remaining] } }) try { - await todoApi.reorder(tripId, orderedIds) + await todoRepo.reorder(tripId, orderedIds) } catch (err: unknown) { set({ todoItems: prev }) notify(getApiErrorMessage(err, 'Error reordering todos'), 'error') diff --git a/client/tests/unit/slices/todoSlice.test.ts b/client/tests/unit/slices/todoSlice.test.ts index 123426bc3..9f2061f98 100644 --- a/client/tests/unit/slices/todoSlice.test.ts +++ b/client/tests/unit/slices/todoSlice.test.ts @@ -148,3 +148,35 @@ describe('todoSlice', () => { }); }); }); + +describe('todoSlice offline (#2601)', () => { + const setOnline = (v: boolean) => + Object.defineProperty(navigator, 'onLine', { value: v, writable: true, configurable: true }); + + it('FE-TODO-008: offline a task is added, ticked, edited, reordered and deleted without an error', async () => { + const { clearAll, offlineDb } = await import('../../../src/db/offlineDb'); + await clearAll(); + const synced = buildTodoItem({ id: 10, trip_id: 1, sort_order: 0 }); + await offlineDb.todoItems.put(synced); + seedStore(useTripStore, { todoItems: [synced] }); + setOnline(false); + try { + const added = await useTripStore.getState().addTodoItem(1, { name: 'Offline task' }); + expect(added.id).toBeLessThan(0); + expect(useTripStore.getState().todoItems.map(i => i.name)).toEqual([synced.name, 'Offline task']); + + await useTripStore.getState().toggleTodoItem(1, added.id, true); + await useTripStore.getState().updateTodoItem(1, added.id, { name: 'Offline task, renamed' }); + expect(useTripStore.getState().todoItems[1]).toMatchObject({ name: 'Offline task, renamed', checked: 1 }); + + await useTripStore.getState().reorderTodoItems(1, [added.id, 10]); + expect(useTripStore.getState().todoItems.map(i => i.id)).toEqual([added.id, 10]); + + await useTripStore.getState().deleteTodoItem(1, added.id); + expect(useTripStore.getState().todoItems.map(i => i.id)).toEqual([10]); + expect((await offlineDb.mutationQueue.orderBy('createdAt').toArray()).map(m => m.method)).toEqual(['POST', 'PUT', 'PUT', 'PUT', 'DELETE']); + } finally { + setOnline(true); + } + }); +});