From 72c284467b59d109ef8dc47b5e138b1c99ecc31e Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 8 Oct 2026 13:44:24 +0200 Subject: [PATCH] fix(client): cache a visit note saved from the place dialog The place dialog stopped reloading the days after a save, so the new visit note only reached the Zustand store. Nothing wrote it to Dexie, the tab never sees its own assignment:updated echo, and the time save before it cached the server row with the old note. A trip reopened offline then showed the old note until the next full sync. The note now goes through assignmentRepo.setNotes, which caches the saved row on its day online and queues the write behind any other write of the same visit offline, like setTimes does. A new store action applies it, so the page hook no longer patches the store by hand. usePlaceEdits drops its direct API import and leaves the offline baseline. --- client/scripts/offline-baseline.json | 1 - .../pages/tripPlanner/usePlaceEdits.test.ts | 9 +++-- client/src/pages/tripPlanner/usePlaceEdits.ts | 20 +++------- .../pages/tripPlanner/useTripPlanner.test.tsx | 5 ++- client/src/repo/assignmentRepo.test.ts | 30 +++++++++++++- client/src/repo/assignmentRepo.ts | 24 +++++++++++ client/src/store/slices/assignmentsSlice.ts | 20 ++++++++++ .../unit/slices/assignmentsSlice.test.ts | 40 +++++++++++++++++++ 8 files changed, 129 insertions(+), 20 deletions(-) diff --git a/client/scripts/offline-baseline.json b/client/scripts/offline-baseline.json index 91cbfddd0..41dd224bd 100644 --- a/client/scripts/offline-baseline.json +++ b/client/scripts/offline-baseline.json @@ -206,7 +206,6 @@ "src/pages/settings/useSettings.ts", "src/pages/sharedTrip/useSharedTrip.ts", "src/pages/tripPlanner/useBookingEdits.ts", - "src/pages/tripPlanner/usePlaceEdits.ts", "src/pages/tripPlanner/usePlaceFormOpeners.ts", "src/pages/tripPlanner/usePlannerDialogs.ts", "src/pages/tripPlanner/useRoadtripStopEdits.ts", diff --git a/client/src/pages/tripPlanner/usePlaceEdits.test.ts b/client/src/pages/tripPlanner/usePlaceEdits.test.ts index 64332db55..bb2b45830 100644 --- a/client/src/pages/tripPlanner/usePlaceEdits.test.ts +++ b/client/src/pages/tripPlanner/usePlaceEdits.test.ts @@ -41,6 +41,7 @@ function makeActions() { assignPlaceToDay: vi.fn(async () => ({ id: 555 })), refreshDays: vi.fn(async () => undefined), setAssignmentTimes: vi.fn(async () => undefined), + setAssignmentNotes: vi.fn(async () => undefined), addFile: vi.fn(async () => undefined), deletePlace: vi.fn(async (): Promise<{ tourPlaceIds?: number[] } | undefined> => undefined), deletePlacesMany: vi.fn(async (): Promise<{ tourPlaceIds?: number[] } | undefined> => undefined), @@ -191,9 +192,10 @@ describe('usePlaceEdits', () => { // The times go through the store and the visit's repo, never straight to the API. expect(actions.setAssignmentTimes).toHaveBeenCalledWith(42, 10, 101, { place_time: '09:00', end_time: null }) expect(updateTime).not.toHaveBeenCalled() - expect(updateNotes).toHaveBeenCalledWith(42, 101, { notes: null }) - // The note lands on the visit without reloading the days over a queued time. - expect(useTripStore.getState().assignments[10][0].notes).toBeNull() + // The note goes through the store and the visit's repo too, which caches it on the day, + // and the days are not reloaded over a queued time. + expect(actions.setAssignmentNotes).toHaveBeenCalledWith(42, 10, 101, null) + expect(updateNotes).not.toHaveBeenCalled() expect(actions.refreshDays).not.toHaveBeenCalled() expect(actions.addFile).toHaveBeenCalledTimes(2) expect(toast.error).toHaveBeenCalledWith('files.uploadError') @@ -208,6 +210,7 @@ describe('usePlaceEdits', () => { expect(actions.updatePlace).toHaveBeenCalledWith(42, 2, { name: 'Castle' }) expect(actions.setAssignmentTimes).not.toHaveBeenCalled() + expect(actions.setAssignmentNotes).not.toHaveBeenCalled() expect(updateTime).not.toHaveBeenCalled() expect(actions.refreshDays).not.toHaveBeenCalled() }) diff --git a/client/src/pages/tripPlanner/usePlaceEdits.ts b/client/src/pages/tripPlanner/usePlaceEdits.ts index 506618b15..de1ac9b4c 100644 --- a/client/src/pages/tripPlanner/usePlaceEdits.ts +++ b/client/src/pages/tripPlanner/usePlaceEdits.ts @@ -1,5 +1,4 @@ import { useCallback, useMemo, useState } from 'react' -import { assignmentsApi } from '../../api/client' import { roadtripInsertion } from '../../components/Roadtrip/dayWindow' import { reanchorAfterInsert } from '../../components/Roadtrip/roadtripModel' import type { usePlaceSelection } from '../../hooks/usePlaceSelection' @@ -110,19 +109,12 @@ export function usePlaceEdits(options: PlaceEditsOptions) { await tripActions.setAssignmentTimes(tripId, Number(dayKey), editingAssignmentId, { place_time: place_time || null, end_time: end_time || null, }) - } - // The form only includes assignment_notes when the user changed it, so - // an untouched note never produces a PUT (#2163). '' clears like null. - // The note is set on the visit in place: reloading the days would show - // the server's time while the new one still waits in the queue. - if (assignment_notes !== undefined) { - const notes = assignment_notes || null - await assignmentsApi.updateNotes(tripId, editingAssignmentId, { notes }) - useTripStore.setState(state => ({ - assignments: Object.fromEntries(Object.entries(state.assignments).map(([key, items]) => [ - key, items.map(a => (a.id === editingAssignmentId ? { ...a, notes } : a)), - ])), - })) + // The form only includes assignment_notes when the user changed it, so + // an untouched note never produces a PUT (#2163). '' clears like null. + // Through the visit's repo as well, so the cached day keeps the new note. + if (assignment_notes !== undefined) { + await tripActions.setAssignmentNotes(tripId, Number(dayKey), editingAssignmentId, assignment_notes || null) + } } } // Upload pending files with place_id diff --git a/client/src/pages/tripPlanner/useTripPlanner.test.tsx b/client/src/pages/tripPlanner/useTripPlanner.test.tsx index 077ca54f0..2b14ecde1 100644 --- a/client/src/pages/tripPlanner/useTripPlanner.test.tsx +++ b/client/src/pages/tripPlanner/useTripPlanner.test.tsx @@ -203,7 +203,10 @@ beforeEach(() => { vi.spyOn(tripsApi, 'getMembers').mockResolvedValue({ owner: null, members: [] }) vi.spyOn(accommodationsApi, 'list').mockResolvedValue({ accommodations: [] }) vi.spyOn(assignmentsApi, 'updateTime').mockResolvedValue({}) - vi.spyOn(assignmentsApi, 'updateNotes').mockResolvedValue({}) + // The note route answers with the saved visit, which the repo caches on the day. + vi.spyOn(assignmentsApi, 'updateNotes').mockImplementation(async (_tripId, id, { notes }) => ({ + assignment: buildAssignment({ id, day_id: 7, place: buildPlace({ id: 1, lat: 1, lng: 2 }), notes }), + })) vi.spyOn(airtrailApi, 'sync').mockResolvedValue({ changed: 0 }) vi.spyOn(mapsApi, 'reverse').mockResolvedValue({ name: '', address: '' } as never) vi.spyOn(mapsApi, 'search').mockResolvedValue({ places: [] } as never) diff --git a/client/src/repo/assignmentRepo.test.ts b/client/src/repo/assignmentRepo.test.ts index 91682effa..6de6f5b08 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(), clearDay: vi.fn() } })) +vi.mock('../api/client', () => ({ apiClient: { request: vi.fn() }, assignmentsApi: { updateTime: vi.fn(), updateNotes: 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 () => { @@ -80,6 +80,34 @@ describe('assignment time persistence', () => { }) }) +describe('assignment note persistence', () => { + it('saves through the note route online and caches the saved row on the day', async () => { + vi.mocked(isEffectivelyOffline).mockReturnValue(false) + vi.mocked(assignmentsApi.updateNotes).mockResolvedValue({ assignment: { ...assignment, notes: 'Bring cash' } }) + const saved = await assignmentRepo.setNotes(9, assignment, 'Bring cash') + expect(assignmentsApi.updateNotes).toHaveBeenCalledWith(9, 7, { notes: 'Bring cash' }) + expect(saved.notes).toBe('Bring cash') + expect((await offlineDb.days.get(1))?.assignments?.[0]).toMatchObject({ notes: 'Bring cash', assignment_time: '07:00' }) + expect(await offlineDb.mutationQueue.count()).toBe(0) + }) + + it('stores the offline note with a replayable write', async () => { + const saved = await assignmentRepo.setNotes(9, assignment, null) + expect(saved.notes).toBeNull() + expect((await offlineDb.days.get(1))?.assignments?.[0].notes).toBeNull() + expect(await offlineDb.mutationQueue.toArray()).toEqual([expect.objectContaining({ + url: '/trips/9/assignments/7/notes', method: 'PUT', body: { notes: null }, resource: 'assignments', entityId: 7, + })]) + }) + + it('keeps the cached visit unchanged after a refused save', async () => { + vi.mocked(isEffectivelyOffline).mockReturnValue(false) + vi.mocked(assignmentsApi.updateNotes).mockRejectedValue(new Error('Denied')) + await expect(assignmentRepo.setNotes(9, assignment, 'x')).rejects.toThrow('Denied') + 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 } diff --git a/client/src/repo/assignmentRepo.ts b/client/src/repo/assignmentRepo.ts index 72f8d008f..6741e7d5b 100644 --- a/client/src/repo/assignmentRepo.ts +++ b/client/src/repo/assignmentRepo.ts @@ -98,4 +98,28 @@ export const assignmentRepo = { mutationQueue.sendSoon() return updated }, + /** + * The note a visit carries on its day (#2163). Saved through here rather than straight + * to the API so the cached day holds the new note too: the tab never sees its own + * `assignment:updated` echo, and a trip reopened offline reads its days from the cache. + * A write of the same visit still waiting in the queue keeps this one behind it. + */ + async setNotes(tripId: number | string, assignment: Assignment, notes: string | null): Promise { + if (!(await mutationQueue.mustQueue('assignments', assignment.id))) { + const saved = assignmentSchema.parse((await assignmentsApi.updateNotes(tripId, assignment.id, { notes })).assignment) + await cacheAssignment(saved) + return saved + } + const updated = { ...assignment, notes } + await offlineDb.transaction('rw', offlineDb.days, offlineDb.mutationQueue, async () => { + await mutationQueue.enqueue({ + id: generateUUID(), tripId: Number(tripId), method: 'PUT', + url: `/trips/${tripId}/assignments/${assignment.id}/notes`, + body: { notes }, resource: 'assignments', entityId: assignment.id, + }) + await cacheAssignment(updated) + }) + mutationQueue.sendSoon() + return updated + }, } diff --git a/client/src/store/slices/assignmentsSlice.ts b/client/src/store/slices/assignmentsSlice.ts index bdf2d9e3c..abf7ae547 100644 --- a/client/src/store/slices/assignmentsSlice.ts +++ b/client/src/store/slices/assignmentsSlice.ts @@ -13,6 +13,7 @@ export interface AssignmentsSlice { setAssignmentEndDay: (tripId: number | string, dayId: number, assignmentId: number, endDay: boolean) => Promise setAssignmentRouteExcluded: (tripId: number | string, dayId: number, assignmentId: number, excluded: boolean) => Promise setAssignmentTimes: (tripId: number | string, dayId: number, assignmentId: number, times: AssignmentTimes) => Promise + setAssignmentNotes: (tripId: number | string, dayId: number, assignmentId: number, notes: string | null) => Promise assignPlaceToDay: (tripId: number | string, dayId: number | string, placeId: number | string, position?: number | null) => Promise removeAssignment: (tripId: number | string, dayId: number | string, assignmentId: number) => Promise clearDayAssignments: (tripId: number | string, dayId: number) => Promise @@ -77,6 +78,25 @@ export const createAssignmentsSlice = (set: SetState, get: GetState): Assignment throw err } }, + setAssignmentNotes: async (tripId, dayId, assignmentId, notes) => { + const assignment = get().assignments[String(dayId)]?.find(a => a.id === assignmentId) + if (!assignment || assignmentId < 0) return + // Only the note is patched: the saved row carries the place as the server has it, + // and the visit's times may still wait in the queue ahead of this write. + const apply = (value: string | null) => set(state => ({ + assignments: Object.fromEntries(Object.entries(state.assignments).map(([key, items]) => [ + key, items.map(a => a.id === assignmentId ? { ...a, notes: value } : a), + ])), + })) + apply(notes) + try { + const saved = await assignmentRepo.setNotes(tripId, assignment, notes) + apply(saved.notes ?? null) + } catch (err: unknown) { + apply(assignment.notes ?? null) + throw err + } + }, assignPlaceToDay: async (tripId, dayId, placeId, position) => { const state = get() const place = state.places.find(p => p.id === Number.parseInt(String(placeId))) diff --git a/client/tests/unit/slices/assignmentsSlice.test.ts b/client/tests/unit/slices/assignmentsSlice.test.ts index eed11a9f5..04ced9bc0 100644 --- a/client/tests/unit/slices/assignmentsSlice.test.ts +++ b/client/tests/unit/slices/assignmentsSlice.test.ts @@ -427,6 +427,46 @@ describe('assignmentsSlice', () => { }); }); + describe('setAssignmentNotes', () => { + const visit = () => buildAssignment({ id: 5, day_id: 1, place_id: 10, notes: 'old', assignment_time: '09:00' }); + + it('FE-ASSIGN-030: shows the note at once and keeps the one the server saved', async () => { + seedStore(useTripStore, { assignments: { '1': [visit()] } }); + let seen: unknown; + server.use( + http.put('/api/trips/1/assignments/5/notes', async ({ request }) => { + seen = await request.json(); + expect(useTripStore.getState().assignments['1'][0].notes).toBe('Bring cash'); + return HttpResponse.json({ assignment: { ...visit(), notes: 'Bring cash' } }); + }), + ); + + await useTripStore.getState().setAssignmentNotes(1, 1, 5, 'Bring cash'); + + expect(seen).toEqual({ notes: 'Bring cash' }); + expect(useTripStore.getState().assignments['1'][0]).toMatchObject({ notes: 'Bring cash', assignment_time: '09:00' }); + }); + + it('FE-ASSIGN-031: a refused write puts the old note back', async () => { + seedStore(useTripStore, { assignments: { '1': [visit()] } }); + server.use(http.put('/api/trips/1/assignments/5/notes', () => HttpResponse.json({ error: 'no' }, { status: 403 }))); + + await expect(useTripStore.getState().setAssignmentNotes(1, 1, 5, null)).rejects.toBeTruthy(); + + expect(useTripStore.getState().assignments['1'][0].notes).toBe('old'); + }); + + it('FE-ASSIGN-032: a visit it cannot find, or one not yet saved, is left alone', async () => { + const unsaved = { ...visit(), id: -3 }; + seedStore(useTripStore, { assignments: { '1': [unsaved] } }); + + await useTripStore.getState().setAssignmentNotes(1, 1, 5, 'x'); + await useTripStore.getState().setAssignmentNotes(1, 1, -3, 'x'); + + expect(useTripStore.getState().assignments['1'][0]).toEqual(unsaved); + }); + }); + describe('setAssignmentTimes', () => { // The pool place still carries an End of its own, which the visit shows once its // own override is gone, exactly as a fresh read of the day would.