From dd7cfca0b252fd898e148de3c5067f1267baee6c Mon Sep 17 00:00:00 2001 From: Maurice Date: Fri, 9 Oct 2026 10:53:41 +0200 Subject: [PATCH] refactor(collab): share the notes list and attachment upload between desktop and phone useCollabNotesData holds the note list with its first load and the live note events, and the attachment upload both shells run after saving a note. Writing notes, their colours and categories stays in each shell, since the desktop panel keeps empty categories and colour batches the phone tab does not have. A failed load still empties the desktop list only. --- client/src/components/Collab/CollabNotes.tsx | 72 ++------ .../components/Collab/CollabNotes.types.ts | 1 + .../Collab/useCollabNotesData.test.ts | 167 ++++++++++++++++++ .../components/Collab/useCollabNotesData.ts | 95 ++++++++++ .../mobile/screens/trip/tabs/MCollabNotes.tsx | 62 ++----- 5 files changed, 284 insertions(+), 113 deletions(-) create mode 100644 client/src/components/Collab/useCollabNotesData.test.ts create mode 100644 client/src/components/Collab/useCollabNotesData.ts diff --git a/client/src/components/Collab/CollabNotes.tsx b/client/src/components/Collab/CollabNotes.tsx index 31331097b..a4a563bbe 100644 --- a/client/src/components/Collab/CollabNotes.tsx +++ b/client/src/components/Collab/CollabNotes.tsx @@ -1,4 +1,4 @@ -import { useState, useEffect, useCallback, useId, useMemo } from 'react' +import { useState, useCallback, useId, useMemo } from 'react' import Markdown from 'react-markdown' import remarkGfm from 'remark-gfm' import remarkBreaks from 'remark-breaks' @@ -10,13 +10,13 @@ import { DialogButton, DialogFooter, DialogHeader, DialogSection, DialogShell, D import { collabApi } from '../../api/client' import { useCanDo } from '../../store/permissionsStore' import { useTripStore } from '../../store/tripStore' -import { addListener, removeListener } from '../../api/websocket' import { useTranslation } from '../../i18n' import { useToast } from '../shared/Toast' import ConfirmDialog from '../shared/ConfirmDialog' import EmptyState from '../shared/EmptyState' import type { User } from '../../types' import type { CollabNote } from './CollabNotes.types' +import { useCollabNotesData } from './useCollabNotesData' import { FONT, NOTE_COLORS } from './CollabNotes.constants' import { NoteFormModal } from './CollabNotesFormModal' import { CategorySettingsModal } from './CollabNotesCategorySettingsModal' @@ -43,8 +43,7 @@ function useCollabNotes({ tripId, currentUser }: CollabNotesProps) { const can = useCanDo() const trip = useTripStore((s) => s.trip) const canEdit = can('collab_edit', trip) - const [notes, setNotes] = useState([]) - const [loading, setLoading] = useState(true) + const { notes, setNotes, loading, uploadNoteFiles } = useCollabNotesData({ tripId, t, toast, resetOnLoadError: true }) const [showNewModal, setShowNewModal] = useState(false) const [editingNote, setEditingNote] = useState(null) const [viewingNote, setViewingNote] = useState(null) @@ -77,49 +76,6 @@ function useCollabNotes({ tripId, currentUser }: CollabNotesProps) { return NOTE_COLORS[Object.keys(categoryColors).length % NOTE_COLORS.length].value } - // ── Load notes on mount ── - useEffect(() => { - if (!tripId) return - let cancelled = false - setLoading(true) - collabApi.getNotes(tripId) - .then(data => { if (!cancelled) setNotes(data?.notes || data || []) }) - .catch(() => { if (!cancelled) setNotes([]) }) - .finally(() => { if (!cancelled) setLoading(false) }) - return () => { cancelled = true } - }, [tripId]) - - // ── WebSocket real-time sync ── - useEffect(() => { - if (!tripId) return - - const handler = (msg) => { - // The panel is not remounted on a trip change, so an event still in flight - // from the trip we just left must not land in this list. - if (String(msg?.tripId) !== String(tripId)) return - if (msg.type === 'collab:note:created' && msg.note) { - setNotes(prev => { - if (prev.some(n => n.id === msg.note.id)) return prev - return [msg.note, ...prev] - }) - } - if (msg.type === 'collab:note:updated' && msg.note) { - setNotes(prev => - prev.map(n => (n.id === msg.note.id ? { ...n, ...msg.note } : n)) - ) - } - if (msg.type === 'collab:note:deleted') { - const deletedId = msg.noteId || msg.id - if (deletedId) { - setNotes(prev => prev.filter(n => n.id !== deletedId)) - } - } - } - - addListener(handler) - return () => removeListener(handler) - }, [tripId]) - // ── Actions ── const handleCreateNote = useCallback(async (data) => { const pendingFiles = data._pendingFiles || [] @@ -135,11 +91,7 @@ function useCollabNotes({ tripId, currentUser }: CollabNotesProps) { const note = created.note || created // Upload pending files if (pendingFiles.length > 0 && note.id) { - for (const file of pendingFiles) { - const fd = new FormData() - fd.append('file', file) - try { await collabApi.uploadNoteFile(tripId, note.id, fd) } catch (err) { console.error('Failed to upload note attachment:', err); toast.error(t('common.error')) } - } + await uploadNoteFiles(note.id, pendingFiles, err => console.error('Failed to upload note attachment:', err)) // Reload note with attachments const fresh = await collabApi.getNotes(tripId) if (fresh?.notes) setNotes(fresh.notes) @@ -151,7 +103,7 @@ function useCollabNotes({ tripId, currentUser }: CollabNotesProps) { return [note, ...prev] }) } - }, [tripId, toast, t]) + }, [tripId, toast, t, setNotes, uploadNoteFiles]) const handleUpdateNote = useCallback(async (noteId, data, opts: { silent?: boolean } = {}) => { let result @@ -168,7 +120,7 @@ function useCollabNotes({ tripId, currentUser }: CollabNotesProps) { prev.map(n => (n.id === noteId ? { ...n, ...updated } : n)) ) } - }, [tripId, toast, t]) + }, [tripId, toast, t, setNotes]) // A colour or a rename is N single-note writes; if one of them is rejected the // rest still have to run, and the list has to be re-read so it stops showing a @@ -178,7 +130,7 @@ function useCollabNotes({ tripId, currentUser }: CollabNotesProps) { const fresh = await collabApi.getNotes(tripId) setNotes(fresh?.notes || fresh || []) } catch {} - }, [tripId]) + }, [tripId, setNotes]) const saveCategoryColors = useCallback(async (newMap) => { let failed = 0 @@ -224,16 +176,12 @@ function useCollabNotes({ tripId, currentUser }: CollabNotesProps) { delete data._pendingFiles await handleUpdateNote(editingNote.id, data) if (pendingFiles.length > 0) { - for (const file of pendingFiles) { - const fd = new FormData() - fd.append('file', file) - try { await collabApi.uploadNoteFile(tripId, editingNote.id, fd) } catch { toast.error(t('common.error')) } - } + await uploadNoteFiles(editingNote.id, pendingFiles) const fresh = await collabApi.getNotes(tripId) if (fresh?.notes) setNotes(fresh.notes) window.dispatchEvent(new Event('collab-files-changed')) } - }, [editingNote, tripId, handleUpdateNote, toast, t]) + }, [editingNote, tripId, handleUpdateNote, setNotes, uploadNoteFiles]) const handleDeleteNoteFile = useCallback(async (noteId, fileId) => { try { await collabApi.deleteNoteFile(tripId, noteId, fileId) } catch { toast.error(t('common.error')) } @@ -249,7 +197,7 @@ function useCollabNotes({ tripId, currentUser }: CollabNotesProps) { } setNotes(prev => prev.filter(n => n.id !== noteId)) window.dispatchEvent(new Event('collab-files-changed')) - }, [tripId, toast, t]) + }, [tripId, toast, t, setNotes]) // ── Derived data ── const categories = [...new Set(notes.map(n => n.category).filter(Boolean))] diff --git a/client/src/components/Collab/CollabNotes.types.ts b/client/src/components/Collab/CollabNotes.types.ts index c61f3d0bb..bf9c18f7a 100644 --- a/client/src/components/Collab/CollabNotes.types.ts +++ b/client/src/components/Collab/CollabNotes.types.ts @@ -21,6 +21,7 @@ export interface CollabNote { avatar: string | null user_id: number created_at: string + updated_at?: string author?: { username: string; avatar: string | null } user?: { username: string; avatar: string | null } files?: NoteFile[] diff --git a/client/src/components/Collab/useCollabNotesData.test.ts b/client/src/components/Collab/useCollabNotesData.test.ts new file mode 100644 index 000000000..04bfb8330 --- /dev/null +++ b/client/src/components/Collab/useCollabNotesData.test.ts @@ -0,0 +1,167 @@ +// FE-COLLAB-NOTESDATA-001 to FE-COLLAB-NOTESDATA-008: the note list behind the desktop +// Collab panel (empties on a failed load) and the phone's notes tab (keeps the list), with +// the live note events and the attachment upload both run after saving a note. +import { act, renderHook, waitFor } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { collabApi } from '../../api/client'; +import { addListener, removeListener } from '../../api/websocket'; +import type { CollabNoteData } from './collabModel'; +import { useCollabNotesData, type CollabNotesDataOptions } from './useCollabNotesData'; + +const t = (key: string) => key; + +function note(id: number, over: Partial = {}): CollabNoteData { + return { + id, + trip_id: 1, + user_id: 2, + title: `Note ${id}`, + content: null, + category: null, + color: null, + website: null, + pinned: false, + username: 'alice', + avatar: null, + avatar_url: null, + created_at: '2026-05-01T10:00:00Z', + attachments: [], + ...over, + }; +} + +function setup(over: Partial = {}) { + const toast = { success: vi.fn(), error: vi.fn(), warning: vi.fn(), info: vi.fn() }; + const hook = renderHook( + (props: Partial) => useCollabNotesData({ tripId: 1, t, toast, ...over, ...props }), + { initialProps: {} } + ); + return { ...hook, toast }; +} + +async function loaded(over: Partial = {}) { + const view = setup(over); + await waitFor(() => expect(view.result.current.loading).toBe(false)); + return view; +} + +function wsHandler(): (event: Record) => void { + const calls = vi.mocked(addListener).mock.calls; + return calls[calls.length - 1][0] as (event: Record) => void; +} + +beforeEach(() => { + vi.mocked(addListener).mockClear(); + vi.mocked(removeListener).mockClear(); + vi.spyOn(collabApi, 'getNotes').mockResolvedValue({ notes: [note(1), note(2)] }); + vi.spyOn(collabApi, 'uploadNoteFile').mockResolvedValue({}); +}); + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe('useCollabNotesData', () => { + it('FE-COLLAB-NOTESDATA-001: loads the notes, from an object or a bare array', async () => { + const { result } = setup(); + expect(result.current.loading).toBe(true); + await waitFor(() => expect(result.current.loading).toBe(false)); + expect(collabApi.getNotes).toHaveBeenCalledWith(1); + expect(result.current.notes.map((n) => n.id)).toEqual([1, 2]); + + vi.mocked(collabApi.getNotes).mockResolvedValue([note(3)]); + const bare = await loaded(); + expect(bare.result.current.notes.map((n) => n.id)).toEqual([3]); + + vi.mocked(collabApi.getNotes).mockResolvedValue({}); + const empty = await loaded(); + expect(empty.result.current.notes).toEqual([]); + }); + + it('FE-COLLAB-NOTESDATA-002: without a trip nothing loads or listens', () => { + const { result } = setup({ tripId: 0 }); + expect(collabApi.getNotes).not.toHaveBeenCalled(); + expect(addListener).not.toHaveBeenCalled(); + expect(result.current.loading).toBe(true); + }); + + it('FE-COLLAB-NOTESDATA-003: a failed reload empties the desktop list and keeps the phone list', async () => { + const desktop = await loaded({ resetOnLoadError: true }); + const phone = await loaded(); + vi.mocked(collabApi.getNotes).mockRejectedValue(new Error('boom')); + + desktop.rerender({ tripId: 2 }); + phone.rerender({ tripId: 2 }); + await waitFor(() => expect(desktop.result.current.loading).toBe(false)); + await waitFor(() => expect(phone.result.current.loading).toBe(false)); + expect(desktop.result.current.notes).toEqual([]); + expect(phone.result.current.notes.map((n) => n.id)).toEqual([1, 2]); + }); + + it('FE-COLLAB-NOTESDATA-004: live events create, update and delete, only for this trip', async () => { + const { result } = await loaded(); + const handler = wsHandler(); + act(() => { + handler({ type: 'collab:note:created', tripId: 2, note: note(7) }); + handler({ type: 'collab:note:created', tripId: '1', note: note(7) }); + handler({ type: 'collab:note:created', tripId: 1, note: note(7) }); + handler({ type: 'collab:note:updated', tripId: 1, note: { id: 1, title: 'renamed' } }); + handler({ type: 'collab:note:updated', tripId: 1 }); + }); + expect(result.current.notes.map((n) => n.id)).toEqual([7, 1, 2]); + // An update folds into the note it names. + expect(result.current.notes[1]).toMatchObject({ title: 'renamed', username: 'alice' }); + + act(() => { + handler({ type: 'collab:note:deleted', tripId: 1, noteId: 7 }); + handler({ type: 'collab:note:deleted', tripId: 1, id: 1 }); + handler({ type: 'collab:note:deleted', tripId: 1 }); + }); + expect(result.current.notes.map((n) => n.id)).toEqual([2]); + }); + + it('FE-COLLAB-NOTESDATA-005: unmounting removes the live listener', async () => { + const { unmount } = await loaded(); + const handler = wsHandler(); + unmount(); + expect(removeListener).toHaveBeenCalledWith(handler); + }); + + it('FE-COLLAB-NOTESDATA-006: attachments upload one by one as a file field', async () => { + const { result, toast } = await loaded(); + const a = new File(['a'], 'a.pdf'); + const b = new File(['b'], 'b.png'); + await act(async () => { + await result.current.uploadNoteFiles(5, [a, b]); + }); + const calls = vi.mocked(collabApi.uploadNoteFile).mock.calls; + expect(calls.map((c) => [c[0], c[1]])).toEqual([ + [1, 5], + [1, 5], + ]); + expect(((calls[0][2] as FormData).get('file') as File).name).toBe(a.name); + expect(((calls[1][2] as FormData).get('file') as File).name).toBe(b.name); + expect(toast.error).not.toHaveBeenCalled(); + }); + + it('FE-COLLAB-NOTESDATA-007: a refused file is reported and toasted, the rest still go', async () => { + const err = new Error('too big'); + vi.mocked(collabApi.uploadNoteFile).mockRejectedValueOnce(err); + const onFileError = vi.fn(); + const { result, toast } = await loaded(); + await act(async () => { + await result.current.uploadNoteFiles(5, [new File(['a'], 'a.pdf'), new File(['b'], 'b.pdf')], onFileError); + }); + expect(collabApi.uploadNoteFile).toHaveBeenCalledTimes(2); + expect(onFileError).toHaveBeenCalledWith(err); + expect(toast.error).toHaveBeenCalledTimes(1); + expect(toast.error).toHaveBeenCalledWith('common.error'); + }); + + it('FE-COLLAB-NOTESDATA-008: setNotes lets a shell write the list it reloaded', async () => { + const { result } = await loaded(); + act(() => result.current.setNotes([note(9)])); + expect(result.current.notes.map((n) => n.id)).toEqual([9]); + }); +}); diff --git a/client/src/components/Collab/useCollabNotesData.ts b/client/src/components/Collab/useCollabNotesData.ts new file mode 100644 index 000000000..19eddc006 --- /dev/null +++ b/client/src/components/Collab/useCollabNotesData.ts @@ -0,0 +1,95 @@ +import { useCallback, useEffect, useState } from 'react'; + +import { collabApi } from '../../api/client'; +import { addListener, removeListener } from '../../api/websocket'; +import type { useTranslation } from '../../i18n'; +import type { useToast } from '../shared/Toast'; +import type { CollabNoteData } from './collabModel'; + +export interface CollabNotesDataOptions { + tripId: number; + t: ReturnType['t']; + toast: ReturnType; + /** The desktop panel outlives a trip change, so a failed load there empties the list. */ + resetOnLoadError?: boolean; +} + +/** + * The trip notes behind the desktop Collab panel and the phone's notes tab, each reading + * the rows through its own note type: the list as the server has it, kept current by the + * WebSocket note events, and the attachment upload both run after a note is saved. Writing + * a note, its colours and its categories stays with each shell, which differ there. Collab + * has no store slice, so the notes live here. + */ +export function useCollabNotesData({ + tripId, + t, + toast, + resetOnLoadError = false, +}: CollabNotesDataOptions) { + const [notes, setNotes] = useState([]); + const [loading, setLoading] = useState(true); + + useEffect(() => { + if (!tripId) return; + let cancelled = false; + setLoading(true); + collabApi + .getNotes(tripId) + .then((data: N[] | { notes?: N[] } | null) => { + if (!cancelled) setNotes(Array.isArray(data) ? data : data?.notes || []); + }) + .catch(() => { + if (!cancelled && resetOnLoadError) setNotes([]); + }) + .finally(() => { + if (!cancelled) setLoading(false); + }); + return () => { + cancelled = true; + }; + }, [tripId, resetOnLoadError]); + + useEffect(() => { + if (!tripId) return; + const handler = (msg: Record) => { + // An event still in flight from a trip just left must not land in this list. + if (String(msg?.tripId) !== String(tripId)) return; + const note = msg.note as N | undefined; + if (msg.type === 'collab:note:created' && note) { + setNotes((prev) => (prev.some((n) => n.id === note.id) ? prev : [note, ...prev])); + } + if (msg.type === 'collab:note:updated' && note) { + setNotes((prev) => prev.map((n) => (n.id === note.id ? { ...n, ...note } : n))); + } + if (msg.type === 'collab:note:deleted') { + const deletedId = (msg.noteId || msg.id) as number | undefined; + if (deletedId) setNotes((prev) => prev.filter((n) => n.id !== deletedId)); + } + }; + addListener(handler); + return () => removeListener(handler); + }, [tripId]); + + /** + * Uploads a saved note's attachments one by one; a file the server refuses is toasted + * and the rest still go. `onFileError` sees each refusal before the toast. + */ + const uploadNoteFiles = useCallback( + async (noteId: number, files: File[], onFileError?: (err: unknown) => void) => { + for (const file of files) { + const fd = new FormData(); + fd.append('file', file); + try { + await collabApi.uploadNoteFile(tripId, noteId, fd); + } catch (err) { + onFileError?.(err); + toast.error(t('common.error')); + } + } + }, + [tripId, toast, t] + ); + + return { notes, setNotes, loading, uploadNoteFiles }; +} diff --git a/client/src/mobile/screens/trip/tabs/MCollabNotes.tsx b/client/src/mobile/screens/trip/tabs/MCollabNotes.tsx index 333f9a23b..c1a1d2c13 100644 --- a/client/src/mobile/screens/trip/tabs/MCollabNotes.tsx +++ b/client/src/mobile/screens/trip/tabs/MCollabNotes.tsx @@ -6,7 +6,7 @@ import { sanitizedMarkdownPlugins, sanitizedMarkdownComponents } from '../../../ import { Check, ExternalLink, FileText, Paperclip, Pin, PinOff, Plus, StickyNote, Trash2, X } from 'lucide-react' import MDancingTrek from '../../../components/MDancingTrek' import { collabApi } from '../../../../api/client' -import { addListener, removeListener } from '../../../../api/websocket' +import { useCollabNotesData } from '../../../../components/Collab/useCollabNotesData' import { openFile } from '../../../../utils/fileDownload' import { safeExternalHref } from '../../../../utils/safeUrl' import MSheet from '../../../components/MSheet' @@ -47,9 +47,9 @@ function linkHost(url: string): string { } /** - * Trip-tab Collab / Notes. Same architecture as MCollabChat: own state, own - * `collabApi` calls, own WebSocket listener — no `tripStore`/`tripActions` - * (10-tab-databindings.md §8.4). The demo only has a placeholder for this + * Trip-tab Collab / Notes. The list and its WebSocket listener come from + * useCollabNotesData, the hook the desktop panel reads too; no + * `tripStore`/`tripActions` (10-tab-databindings.md §8.4). The demo only has a placeholder for this * sub-tab, so the card/filter/form design below is new (spec 03 §6.4 audit * item), built in the same visual language as MTransportsTab. */ @@ -58,44 +58,12 @@ export default function MCollabNotes({ planner }: MCollabNotesProps) { const canEdit = planner.can('collab_edit', planner.trip) const canUploadFiles = planner.can('file_upload', planner.trip) - const [notes, setNotes] = useState([]) - const [loading, setLoading] = useState(true) + const { notes, setNotes, loading, uploadNoteFiles } = useCollabNotesData({ tripId, t, toast }) const [activeCategory, setActiveCategory] = useState(null) const [formTarget, setFormTarget] = useState(null) const [viewingNote, setViewingNote] = useState(null) const [pendingDeleteId, setPendingDeleteId] = useState(null) - // ── Load ── - useEffect(() => { - let cancelled = false - setLoading(true) - collabApi.getNotes(tripId).then((data: GetNotesResponse) => { - if (!cancelled) setNotes(data.notes || []) - }).catch(() => { /* leave notes empty */ }).finally(() => { if (!cancelled) setLoading(false) }) - return () => { cancelled = true } - }, [tripId]) - - // ── WebSocket (own listener, not handleRemoteEvent) ── - useEffect(() => { - const handler = (event: Record) => { - if (String(event.tripId) !== String(tripId)) return - if (event.type === 'collab:note:created') { - const note = event.note as CollabNoteData - setNotes(prev => (prev.some(n => n.id === note.id) ? prev : [note, ...prev])) - } - if (event.type === 'collab:note:updated') { - const note = event.note as CollabNoteData - setNotes(prev => prev.map(n => (n.id === note.id ? { ...n, ...note } : n))) - } - if (event.type === 'collab:note:deleted') { - const noteId = event.noteId as number - setNotes(prev => prev.filter(n => n.id !== noteId)) - } - } - addListener(handler) - return () => removeListener(handler) - }, [tripId]) - const categories = noteCategoriesList(notes) const colorMap = buildCategoryColorMap(notes) const sorted = sortNotes(notes, activeCategory) @@ -113,17 +81,13 @@ export default function MCollabNotes({ planner }: MCollabNotesProps) { throw new Error('create failed') } if (data.pendingFiles.length > 0) { - for (const file of data.pendingFiles) { - const fd = new FormData() - fd.append('file', file) - try { await collabApi.uploadNoteFile(tripId, created.id, fd) } catch { toast.error(t('common.error')) } - } + await uploadNoteFiles(created.id, data.pendingFiles) const fresh = (await collabApi.getNotes(tripId)) as GetNotesResponse setNotes(fresh.notes || []) return } setNotes(prev => (prev.some(n => n.id === created.id) ? prev : [created, ...prev])) - }, [tripId, toast, t]) + }, [tripId, toast, t, setNotes, uploadNoteFiles]) const handleUpdate = useCallback(async ( noteId: number, @@ -139,17 +103,13 @@ export default function MCollabNotes({ planner }: MCollabNotesProps) { throw new Error('update failed') } if (pendingFiles.length > 0) { - for (const file of pendingFiles) { - const fd = new FormData() - fd.append('file', file) - try { await collabApi.uploadNoteFile(tripId, noteId, fd) } catch { toast.error(t('common.error')) } - } + await uploadNoteFiles(noteId, pendingFiles) const fresh = (await collabApi.getNotes(tripId)) as GetNotesResponse setNotes(fresh.notes || []) return } if (updated) setNotes(prev => prev.map(n => (n.id === noteId ? { ...n, ...updated } : n))) - }, [tripId, toast, t]) + }, [tripId, toast, t, setNotes, uploadNoteFiles]) const handleDelete = useCallback(async (noteId: number) => { try { @@ -158,7 +118,7 @@ export default function MCollabNotes({ planner }: MCollabNotesProps) { } catch { toast.error(t('common.error')) } - }, [tripId, toast, t]) + }, [tripId, toast, t, setNotes]) // Returns whether the file is really gone — the form sheet only drops the // chip once the server confirmed it. @@ -173,7 +133,7 @@ export default function MCollabNotes({ planner }: MCollabNotesProps) { toast.error(t('common.error')) return false } - }, [tripId, toast, t]) + }, [tripId, toast, t, setNotes]) const openNote = (note: CollabNoteData) => (canEdit ? setFormTarget(note) : setViewingNote(note))