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

@@ -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<QueuedMutation[]> {
+  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([]);
+  });
+});

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<TodoItem> {
+  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<TodoItem> {
+  const { checked, priority, ...rest } = data
+  return {
+    ...rest,
+    ...(checked === undefined ? {} : { checked: checked ? 1 : 0 }),
+    ...(priority === undefined ? {} : { priority: priority ?? 0 }),
+  } as Partial<TodoItem>
+}
+
+/** 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<void> {
+  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<void> {
+    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<void> {
+    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) },
+      })
+    })
+  },
 }

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')

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);
+    }
+  });
+});