fix(reminders): RMW race, reliable removal, stable poll interval (N113/N114/N115)
- N113: mutations compute from a local ref kept in sync with server echoes, and writes serialize through a promise queue, so rapid add/remove no longer reads a stale baseline and clobbers a prior write. - N114: ReminderMonitor shows each toast once (firedRef) but retries the account-data removal on later ticks if it fails (removingRef released on error) — a failed removal no longer permanently swallows the reminder. - N115: the 30s poll interval reads reminders/mDirects via refs and drops them from the effect deps, so it's created once instead of resetting its countdown on every reminder sync (which could indefinitely defer a near-due reminder). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -1,4 +1,4 @@
|
|||||||
import { useCallback, useEffect, useState } from 'react';
|
import { useCallback, useEffect, useRef, useState } from 'react';
|
||||||
import { MatrixClient } from 'matrix-js-sdk';
|
import { MatrixClient } from 'matrix-js-sdk';
|
||||||
import { useMatrixClient } from './useMatrixClient';
|
import { useMatrixClient } from './useMatrixClient';
|
||||||
import { useAccountDataCallback } from './useAccountDataCallback';
|
import { useAccountDataCallback } from './useAccountDataCallback';
|
||||||
@@ -32,39 +32,64 @@ export function useReminders(): {
|
|||||||
const mx = useMatrixClient();
|
const mx = useMatrixClient();
|
||||||
const [reminders, setReminders] = useState<Reminder[]>(() => readReminders(mx));
|
const [reminders, setReminders] = useState<Reminder[]>(() => readReminders(mx));
|
||||||
|
|
||||||
|
// Authoritative local snapshot used to compute mutations. Reading
|
||||||
|
// mx.getAccountData() per-mutation is racy: two quick add/remove calls both
|
||||||
|
// read the same stale baseline and the second write clobbers the first
|
||||||
|
// (N113). We instead mutate from this ref, kept in sync with server echoes.
|
||||||
|
const latestRef = useRef<Reminder[]>(reminders);
|
||||||
|
// Serialize writes so overlapping setAccountData calls can't land out of
|
||||||
|
// order on the server (last-write-wins would otherwise drop data).
|
||||||
|
const writeQueueRef = useRef<Promise<unknown>>(Promise.resolve());
|
||||||
|
|
||||||
|
const applyServerState = useCallback((list: Reminder[]) => {
|
||||||
|
latestRef.current = list;
|
||||||
|
setReminders(list);
|
||||||
|
}, []);
|
||||||
|
|
||||||
useAccountDataCallback(
|
useAccountDataCallback(
|
||||||
mx,
|
mx,
|
||||||
useCallback(
|
useCallback(
|
||||||
(evt) => {
|
(evt) => {
|
||||||
if (evt.getType() === REMINDERS_KEY) {
|
if (evt.getType() === REMINDERS_KEY) {
|
||||||
setReminders(evt.getContent<RemindersContent>()?.reminders ?? []);
|
applyServerState(evt.getContent<RemindersContent>()?.reminders ?? []);
|
||||||
}
|
}
|
||||||
},
|
},
|
||||||
[setReminders],
|
[applyServerState],
|
||||||
),
|
),
|
||||||
);
|
);
|
||||||
|
|
||||||
// Re-read on mx change
|
// Re-read on mx change
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
setReminders(readReminders(mx));
|
applyServerState(readReminders(mx));
|
||||||
}, [mx]);
|
}, [mx, applyServerState]);
|
||||||
|
|
||||||
const addReminder = useCallback(
|
const enqueueWrite = useCallback(
|
||||||
async (r: Reminder) => {
|
(compute: (current: Reminder[]) => Reminder[]): Promise<void> => {
|
||||||
const current = readReminders(mx);
|
const run = writeQueueRef.current.then(async () => {
|
||||||
const next = [...current, r];
|
const next = compute(latestRef.current);
|
||||||
await (mx as any).setAccountData(REMINDERS_KEY, { reminders: next });
|
latestRef.current = next;
|
||||||
|
setReminders(next);
|
||||||
|
await (mx as any).setAccountData(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).
|
||||||
|
writeQueueRef.current = run.catch(() => undefined);
|
||||||
|
return run;
|
||||||
},
|
},
|
||||||
[mx],
|
[mx],
|
||||||
);
|
);
|
||||||
|
|
||||||
|
const addReminder = useCallback(
|
||||||
|
(r: Reminder) => enqueueWrite((current) => [...current, r]),
|
||||||
|
[enqueueWrite],
|
||||||
|
);
|
||||||
|
|
||||||
const removeReminder = useCallback(
|
const removeReminder = useCallback(
|
||||||
async (eventId: string, timestamp: number) => {
|
(eventId: string, timestamp: number) =>
|
||||||
const current = readReminders(mx);
|
enqueueWrite((current) =>
|
||||||
const next = current.filter((r) => !(r.eventId === eventId && r.timestamp === timestamp));
|
current.filter((r) => !(r.eventId === eventId && r.timestamp === timestamp)),
|
||||||
await (mx as any).setAccountData(REMINDERS_KEY, { reminders: next });
|
),
|
||||||
},
|
[enqueueWrite],
|
||||||
[mx],
|
|
||||||
);
|
);
|
||||||
|
|
||||||
const getReminders = useCallback(() => reminders, [reminders]);
|
const getReminders = useCallback(() => reminders, [reminders]);
|
||||||
|
|||||||
@@ -390,16 +390,26 @@ function ReminderMonitor() {
|
|||||||
const setToast = useSetAtom(toastQueueAtom);
|
const setToast = useSetAtom(toastQueueAtom);
|
||||||
const mDirects = useAtomValue(mDirectAtom);
|
const mDirects = useAtomValue(mDirectAtom);
|
||||||
const firedRef = useRef<Set<string>>(new Set());
|
const firedRef = useRef<Set<string>>(new Set());
|
||||||
|
const removingRef = useRef<Set<string>>(new Set());
|
||||||
|
// Read the latest reminders / DM map via refs so the poll interval below is
|
||||||
|
// created once — not torn down and restarted (which resets its 30s countdown
|
||||||
|
// and can indefinitely defer a near-due reminder) on every reminder sync (N115).
|
||||||
|
const remindersRef = useRef(reminders);
|
||||||
|
remindersRef.current = reminders;
|
||||||
|
const mDirectsRef = useRef(mDirects);
|
||||||
|
mDirectsRef.current = mDirects;
|
||||||
|
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
const check = () => {
|
const check = () => {
|
||||||
const now = Date.now();
|
const now = Date.now();
|
||||||
reminders.forEach((r) => {
|
remindersRef.current.forEach((r) => {
|
||||||
|
if (r.timestamp > now) return;
|
||||||
const key = `${r.eventId}-${r.timestamp}`;
|
const key = `${r.eventId}-${r.timestamp}`;
|
||||||
if (r.timestamp <= now && !firedRef.current.has(key)) {
|
// Show the toast exactly once.
|
||||||
|
if (!firedRef.current.has(key)) {
|
||||||
firedRef.current.add(key);
|
firedRef.current.add(key);
|
||||||
const room = mx.getRoom(r.roomId);
|
const room = mx.getRoom(r.roomId);
|
||||||
const hashPath = mDirects.has(r.roomId)
|
const hashPath = mDirectsRef.current.has(r.roomId)
|
||||||
? getDirectRoomPath(r.roomId, r.eventId)
|
? getDirectRoomPath(r.roomId, r.eventId)
|
||||||
: getHomeRoomPath(r.roomId, r.eventId);
|
: getHomeRoomPath(r.roomId, r.eventId);
|
||||||
setToast({
|
setToast({
|
||||||
@@ -410,7 +420,15 @@ function ReminderMonitor() {
|
|||||||
roomId: r.roomId,
|
roomId: r.roomId,
|
||||||
hashPath,
|
hashPath,
|
||||||
});
|
});
|
||||||
removeReminder(r.eventId, r.timestamp);
|
}
|
||||||
|
// Persist the removal, retrying on a later tick if it fails — without
|
||||||
|
// re-showing the toast (N114). The server echo drops it from
|
||||||
|
// `reminders` once the write lands.
|
||||||
|
if (!removingRef.current.has(key)) {
|
||||||
|
removingRef.current.add(key);
|
||||||
|
removeReminder(r.eventId, r.timestamp).catch(() => {
|
||||||
|
removingRef.current.delete(key);
|
||||||
|
});
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
};
|
};
|
||||||
@@ -425,7 +443,7 @@ function ReminderMonitor() {
|
|||||||
clearInterval(interval);
|
clearInterval(interval);
|
||||||
document.removeEventListener('visibilitychange', onVisible);
|
document.removeEventListener('visibilitychange', onVisible);
|
||||||
};
|
};
|
||||||
}, [mx, reminders, setToast, removeReminder, mDirects]);
|
}, [mx, setToast, removeReminder]);
|
||||||
|
|
||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user