diff --git a/src/app/hooks/createAccountDataListStore.ts b/src/app/hooks/createAccountDataListStore.ts new file mode 100644 index 000000000..57aa4bc46 --- /dev/null +++ b/src/app/hooks/createAccountDataListStore.ts @@ -0,0 +1,130 @@ +import { useEffect, useState } from 'react'; +import { ClientEvent, ClientEventHandlerMap, MatrixClient } from 'matrix-js-sdk'; +import { AccountDataEvent } from '../../types/matrix/accountData'; +import { getAccountData, setAccountData } from '../utils/accountData'; + +// Generic, concurrency-safe store for a single global account-data event that +// holds a mutable collection (a list or a record). It is the shared engine +// behind useBookmarks / useReminders / useUserNotes. +// +// Why a module-scoped singleton (per store, per client): +// +// These hooks are mounted by many components at once (e.g. useBookmarks() lives +// on every message row). A per-instance snapshot + write queue would only +// serialize writes within a single instance, so two edits issued from different +// instances before the server echo lands would each compute from a stale +// snapshot and clobber each other — setAccountData replaces the whole content +// with no server-side merge. We therefore keep ONE shared `latest` snapshot and +// ONE write queue per store, keyed off the active MatrixClient, and serialize +// every write through that queue (the lost-update fix). Do not weaken this. + +// How the differing payload shapes are parameterized: +// - `read` maps the raw account-data content (C | undefined) to the in-memory +// value T (e.g. `content?.bookmarks ?? []`, or `content ?? {}` for notes). +// - `write` maps the in-memory value T back to the content object to persist +// (e.g. `{ bookmarks: value }`, or just `value` for the flat notes record). +export type AccountDataListStoreConfig = { + eventType: AccountDataEvent | string; + read: (content: C | undefined) => T; + write: (value: T) => object; +}; + +export type AccountDataListStore = { + /** React hook: subscribe to the store and return the latest value. */ + useValue: (mx: MatrixClient) => T; + /** Read the current shared snapshot without subscribing. */ + getLatest: (mx: MatrixClient) => T; + /** + * Serialize a write. `compute` receives the latest snapshot and returns the + * next value; writes run one at a time so none is computed from stale data. + */ + enqueueWrite: (mx: MatrixClient, compute: (current: T) => T) => Promise; +}; + +export function createAccountDataListStore({ + eventType, + read, + write, +}: AccountDataListStoreConfig): AccountDataListStore { + type ModuleState = { + mx: MatrixClient; + latest: T; + writeQueue: Promise; + listeners: Set<(value: T) => void>; + onAccountData: ClientEventHandlerMap[ClientEvent.AccountData]; + }; + + let moduleState: ModuleState | null = null; + + const readLatest = (mx: MatrixClient): T => read(getAccountData(mx, eventType)); + + // Lazily initialize the shared state for the given client. On a client change + // (login/logout swaps the MatrixClient) we tear down the old subscription and + // re-initialize against the new client so we never leak or double-subscribe. + function ensureModuleState(mx: MatrixClient): ModuleState { + if (moduleState && moduleState.mx === mx) { + return moduleState; + } + + if (moduleState) { + moduleState.mx.removeListener(ClientEvent.AccountData, moduleState.onAccountData); + } + + const state: ModuleState = { + mx, + latest: readLatest(mx), + writeQueue: Promise.resolve(), + listeners: new Set(), + // Reassigned below once `state` is captured. + onAccountData: () => undefined, + }; + + state.onAccountData = (evt) => { + if (evt.getType() === eventType) { + const value = read(evt.getContent() as C); + state.latest = value; + state.listeners.forEach((listener) => listener(value)); + } + }; + + mx.on(ClientEvent.AccountData, state.onAccountData); + moduleState = state; + return state; + } + + function enqueueWrite(mx: MatrixClient, compute: (current: T) => T): Promise { + const state = ensureModuleState(mx); + const run = state.writeQueue.then(async () => { + const next = compute(state.latest); + state.latest = next; + state.listeners.forEach((listener) => listener(next)); + await setAccountData(mx, eventType, write(next)); + }); + // Keep the chain alive even if one write rejects, but propagate the + // rejection to this caller so it can react (e.g. retry). + state.writeQueue = run.catch(() => undefined); + return run; + } + + function useValue(mx: MatrixClient): T { + const [value, setValue] = useState(() => ensureModuleState(mx).latest); + + // Subscribe to the shared module state. A single AccountData listener is + // installed per client (in ensureModuleState); each hook instance only + // registers a local setter and unregisters it on unmount / client change. + useEffect(() => { + const state = ensureModuleState(mx); + setValue(state.latest); + state.listeners.add(setValue); + return () => { + state.listeners.delete(setValue); + }; + }, [mx]); + + return value; + } + + const getLatest = (mx: MatrixClient): T => ensureModuleState(mx).latest; + + return { useValue, getLatest, enqueueWrite }; +} diff --git a/src/app/hooks/useBookmarks.ts b/src/app/hooks/useBookmarks.ts index 63bcdce54..3c8c4341d 100644 --- a/src/app/hooks/useBookmarks.ts +++ b/src/app/hooks/useBookmarks.ts @@ -1,7 +1,6 @@ -import { useCallback, useEffect, useState } from 'react'; -import { ClientEvent, ClientEventHandlerMap, MatrixClient } from 'matrix-js-sdk'; +import { useCallback } from 'react'; import { useMatrixClient } from './useMatrixClient'; -import { getAccountData, setAccountData } from '../utils/accountData'; +import { createAccountDataListStore } from './createAccountDataListStore'; export type Bookmark = { roomId: string; @@ -18,78 +17,14 @@ type BookmarksContent = { bookmarks: Bookmark[]; }; -function readBookmarks(mx: MatrixClient): Bookmark[] { - return getAccountData(mx, BOOKMARKS_KEY)?.bookmarks ?? []; -} - -// Module-scoped serialization state. -// -// useBookmarks() is mounted once per message row (dozens of live instances), so -// a per-instance latest/queue would only serialize writes within a single row — -// bookmarking message A then message B from different rows (before the server -// echo lands) would let B compute from a stale snapshot and clobber A -// (setAccountData replaces the whole content, no server merge). We therefore -// keep a single shared latest ref + write queue, keyed off the active client. -type BookmarksModuleState = { - mx: MatrixClient; - latest: Bookmark[]; - writeQueue: Promise; - listeners: Set<(list: Bookmark[]) => void>; - onAccountData: ClientEventHandlerMap[ClientEvent.AccountData]; -}; - -let moduleState: BookmarksModuleState | null = null; - -// Lazily initialize the shared state for the given client. On a client change -// (login/logout swaps the MatrixClient) we tear down the old subscription and -// re-initialize against the new client so we never leak or double-subscribe. -function ensureModuleState(mx: MatrixClient): BookmarksModuleState { - if (moduleState && moduleState.mx === mx) { - return moduleState; - } - - if (moduleState) { - moduleState.mx.removeListener(ClientEvent.AccountData, moduleState.onAccountData); - } - - const state: BookmarksModuleState = { - mx, - latest: readBookmarks(mx), - writeQueue: Promise.resolve(), - listeners: new Set(), - // Reassigned below once `state` is captured. - onAccountData: () => undefined, - }; - - state.onAccountData = (evt) => { - if (evt.getType() === BOOKMARKS_KEY) { - const list = evt.getContent()?.bookmarks ?? []; - state.latest = list; - state.listeners.forEach((listener) => listener(list)); - } - }; - - mx.on(ClientEvent.AccountData, state.onAccountData); - moduleState = state; - return state; -} - -function enqueueBookmarkWrite( - mx: MatrixClient, - compute: (current: Bookmark[]) => Bookmark[], -): Promise { - const state = ensureModuleState(mx); - const run = state.writeQueue.then(async () => { - const next = compute(state.latest); - state.latest = next; - state.listeners.forEach((listener) => listener(next)); - await setAccountData(mx, BOOKMARKS_KEY, { bookmarks: next }); - }); - // Keep the chain alive even if one write rejects, but propagate the - // rejection to this caller so it can react (e.g. retry). - state.writeQueue = run.catch(() => undefined); - return run; -} +// Shared, concurrency-safe store. See createAccountDataListStore for why the +// snapshot + write queue must be module-scoped (writes are serialized to avoid +// lost updates, since setAccountData replaces the whole content with no merge). +const bookmarksStore = createAccountDataListStore({ + eventType: BOOKMARKS_KEY, + read: (content) => content?.bookmarks ?? [], + write: (bookmarks) => ({ bookmarks }), +}); export function useBookmarks(): { bookmarks: Bookmark[]; @@ -98,23 +33,11 @@ export function useBookmarks(): { isBookmarked: (eventId: string) => boolean; } { const mx = useMatrixClient(); - const [bookmarks, setBookmarks] = useState(() => ensureModuleState(mx).latest); - - // Subscribe to the shared module state. A single AccountData listener is - // installed per client (in ensureModuleState); each hook instance only - // registers a local setter and unregisters it on unmount / client change. - useEffect(() => { - const state = ensureModuleState(mx); - setBookmarks(state.latest); - state.listeners.add(setBookmarks); - return () => { - state.listeners.delete(setBookmarks); - }; - }, [mx]); + const bookmarks = bookmarksStore.useValue(mx); const addBookmark = useCallback( (b: Bookmark) => - enqueueBookmarkWrite(mx, (current) => { + bookmarksStore.enqueueWrite(mx, (current) => { // Avoid duplicates const filtered = current.filter((bk) => bk.eventId !== b.eventId); let next = [b, ...filtered]; @@ -128,7 +51,7 @@ export function useBookmarks(): { const removeBookmark = useCallback( (eventId: string) => - enqueueBookmarkWrite(mx, (current) => current.filter((bk) => bk.eventId !== eventId)), + bookmarksStore.enqueueWrite(mx, (current) => current.filter((bk) => bk.eventId !== eventId)), [mx], ); diff --git a/src/app/hooks/useReminders.ts b/src/app/hooks/useReminders.ts index 18d090515..9002e4f61 100644 --- a/src/app/hooks/useReminders.ts +++ b/src/app/hooks/useReminders.ts @@ -1,7 +1,6 @@ -import { useCallback, useEffect, useState } from 'react'; -import { ClientEvent, ClientEventHandlerMap, MatrixClient } from 'matrix-js-sdk'; +import { useCallback } from 'react'; import { useMatrixClient } from './useMatrixClient'; -import { getAccountData, setAccountData } from '../utils/accountData'; +import { createAccountDataListStore } from './createAccountDataListStore'; export type Reminder = { roomId: string; @@ -16,78 +15,14 @@ type RemindersContent = { reminders: Reminder[]; }; -function readReminders(mx: MatrixClient): Reminder[] { - return getAccountData(mx, REMINDERS_KEY)?.reminders ?? []; -} - -// Module-scoped serialization state. -// -// The latest snapshot and the write queue must be shared across every hook -// instance: ReminderMonitor (auto-removes fired reminders) and RemindMeDialog -// (adds reminders) mount separate hooks, and a per-instance queue would let a -// remove and an add race across instances and clobber each other (setAccountData -// replaces the whole content, no server merge). We therefore keep a single -// shared queue + latest ref, keyed off the active MatrixClient. -type ReminderModuleState = { - mx: MatrixClient; - latest: Reminder[]; - writeQueue: Promise; - listeners: Set<(list: Reminder[]) => void>; - onAccountData: ClientEventHandlerMap[ClientEvent.AccountData]; -}; - -let moduleState: ReminderModuleState | null = null; - -// Lazily initialize the shared state for the given client. On a client change -// (login/logout swaps the MatrixClient) we tear down the old subscription and -// re-initialize against the new client so we never leak or double-subscribe. -function ensureModuleState(mx: MatrixClient): ReminderModuleState { - if (moduleState && moduleState.mx === mx) { - return moduleState; - } - - if (moduleState) { - moduleState.mx.removeListener(ClientEvent.AccountData, moduleState.onAccountData); - } - - const state: ReminderModuleState = { - mx, - latest: readReminders(mx), - writeQueue: Promise.resolve(), - listeners: new Set(), - // Reassigned below once `state` is captured. - onAccountData: () => undefined, - }; - - state.onAccountData = (evt) => { - if (evt.getType() === REMINDERS_KEY) { - const list = evt.getContent()?.reminders ?? []; - state.latest = list; - state.listeners.forEach((listener) => listener(list)); - } - }; - - mx.on(ClientEvent.AccountData, state.onAccountData); - moduleState = state; - return state; -} - -function enqueueReminderWrite( - mx: MatrixClient, - compute: (current: Reminder[]) => Reminder[], -): Promise { - const state = ensureModuleState(mx); - const run = state.writeQueue.then(async () => { - const next = compute(state.latest); - state.latest = next; - state.listeners.forEach((listener) => listener(next)); - await setAccountData(mx, REMINDERS_KEY, { reminders: next }); - }); - // Keep the chain alive even if one write rejects, but propagate the - // rejection to this caller so it can react (e.g. retry). - state.writeQueue = run.catch(() => undefined); - return run; -} +// Shared, concurrency-safe store. See createAccountDataListStore for why the +// snapshot + write queue must be module-scoped (writes are serialized to avoid +// lost updates, since setAccountData replaces the whole content with no merge). +const remindersStore = createAccountDataListStore({ + eventType: REMINDERS_KEY, + read: (content) => content?.reminders ?? [], + write: (reminders) => ({ reminders }), +}); export function useReminders(): { reminders: Reminder[]; @@ -96,34 +31,22 @@ export function useReminders(): { getReminders: () => Reminder[]; } { const mx = useMatrixClient(); - const [reminders, setReminders] = useState(() => ensureModuleState(mx).latest); - - // Subscribe to the shared module state. A single AccountData listener is - // installed per client (in ensureModuleState); each hook instance only - // registers a local setter and unregisters it on unmount / client change. - useEffect(() => { - const state = ensureModuleState(mx); - setReminders(state.latest); - state.listeners.add(setReminders); - return () => { - state.listeners.delete(setReminders); - }; - }, [mx]); + const reminders = remindersStore.useValue(mx); const addReminder = useCallback( - (r: Reminder) => enqueueReminderWrite(mx, (current) => [...current, r]), + (r: Reminder) => remindersStore.enqueueWrite(mx, (current) => [...current, r]), [mx], ); const removeReminder = useCallback( (eventId: string, timestamp: number) => - enqueueReminderWrite(mx, (current) => + remindersStore.enqueueWrite(mx, (current) => current.filter((r) => !(r.eventId === eventId && r.timestamp === timestamp)), ), [mx], ); - const getReminders = useCallback(() => ensureModuleState(mx).latest, [mx]); + const getReminders = useCallback(() => remindersStore.getLatest(mx), [mx]); return { reminders, addReminder, removeReminder, getReminders }; } diff --git a/src/app/hooks/useUserNotes.ts b/src/app/hooks/useUserNotes.ts index 1423ea668..49e156f0b 100644 --- a/src/app/hooks/useUserNotes.ts +++ b/src/app/hooks/useUserNotes.ts @@ -1,111 +1,37 @@ -import { useCallback, useEffect, useState } from 'react'; -import { ClientEvent, ClientEventHandlerMap, MatrixClient } from 'matrix-js-sdk'; +import { useCallback } from 'react'; import { useMatrixClient } from './useMatrixClient'; -import { getAccountData, setAccountData } from '../utils/accountData'; +import { createAccountDataListStore } from './createAccountDataListStore'; const NOTES_KEY = 'io.lotus.user_notes'; export const USER_NOTE_MAX_LENGTH = 500; type UserNotesContent = Record; -function readNotes(mx: MatrixClient): UserNotesContent { - return getAccountData(mx, NOTES_KEY) ?? {}; -} - -// Module-scoped serialization state. +// Shared, concurrency-safe store. See createAccountDataListStore for why the +// snapshot + write queue must be module-scoped (writes are serialized to avoid +// lost updates, since setAccountData replaces the whole content with no merge). // -// useUserNotes() can be mounted by many components at once, so a per-instance -// latest/queue would only serialize writes within one instance. Notes for -// different users saved from different instances (before the server echo lands) -// would each compute from a stale snapshot and clobber each other, since -// setAccountData replaces the whole record with no server merge. We therefore -// keep a single shared latest record + write queue, keyed off the active client. -type UserNotesModuleState = { - mx: MatrixClient; - latest: UserNotesContent; - writeQueue: Promise; - listeners: Set<(record: UserNotesContent) => void>; - onAccountData: ClientEventHandlerMap[ClientEvent.AccountData]; -}; - -let moduleState: UserNotesModuleState | null = null; - -// Lazily initialize the shared state for the given client. On a client change -// (login/logout swaps the MatrixClient) we tear down the old subscription and -// re-initialize against the new client so we never leak or double-subscribe. -function ensureModuleState(mx: MatrixClient): UserNotesModuleState { - if (moduleState && moduleState.mx === mx) { - return moduleState; - } - - if (moduleState) { - moduleState.mx.removeListener(ClientEvent.AccountData, moduleState.onAccountData); - } - - const state: UserNotesModuleState = { - mx, - latest: readNotes(mx), - writeQueue: Promise.resolve(), - listeners: new Set(), - // Reassigned below once `state` is captured. - onAccountData: () => undefined, - }; - - state.onAccountData = (evt) => { - if (evt.getType() === NOTES_KEY) { - const record = evt.getContent() ?? {}; - state.latest = record; - state.listeners.forEach((listener) => listener(record)); - } - }; - - mx.on(ClientEvent.AccountData, state.onAccountData); - moduleState = state; - return state; -} - -function enqueueNotesWrite( - mx: MatrixClient, - compute: (current: UserNotesContent) => UserNotesContent, -): Promise { - const state = ensureModuleState(mx); - const run = state.writeQueue.then(async () => { - const next = compute(state.latest); - state.latest = next; - state.listeners.forEach((listener) => listener(next)); - await setAccountData(mx, NOTES_KEY, next); - }); - // Keep the chain alive even if one write rejects, but propagate the - // rejection to this caller so it can react (e.g. retry). - state.writeQueue = run.catch(() => undefined); - return run; -} +// Notes are a flat Record rather than a wrapped list, so `read` +// and `write` pass the content through unchanged. +const notesStore = createAccountDataListStore({ + eventType: NOTES_KEY, + read: (content) => content ?? {}, + write: (notes) => notes, +}); export function useUserNotes(): { getNote: (userId: string) => string; setNote: (userId: string, note: string) => Promise; } { const mx = useMatrixClient(); - const [notes, setNotes] = useState(() => ensureModuleState(mx).latest); - - // Subscribe to the shared module state. A single AccountData listener is - // installed per client (in ensureModuleState); each hook instance only - // registers a local setter and unregisters it on unmount / client change. - useEffect(() => { - const state = ensureModuleState(mx); - setNotes(state.latest); - state.listeners.add(setNotes); - return () => { - state.listeners.delete(setNotes); - }; - }, [mx]); + const notes = notesStore.useValue(mx); const getNote = useCallback((userId: string) => notes[userId] ?? '', [notes]); const setNote = useCallback( (userId: string, note: string) => { const trimmed = note.trim().slice(0, USER_NOTE_MAX_LENGTH); - return enqueueNotesWrite(mx, (current) => { + return notesStore.enqueueWrite(mx, (current) => { const updated = { ...current }; if (trimmed) { updated[userId] = trimmed;