diff --git a/LOTUS_TODO.md b/LOTUS_TODO.md index 7abf5074d..7e929e65b 100644 --- a/LOTUS_TODO.md +++ b/LOTUS_TODO.md @@ -62,9 +62,11 @@ Built and gate-green; verify per [LOTUS_TESTING.md](./LOTUS_TESTING.md), then gr ## ๐Ÿ”ด Open โ€” Actionable -### ๐Ÿ› Unread/read-receipt flakiness (reported 2026-07, live) โ€” INVESTIGATING +### โœ… Unread/read-receipt flakiness (reported 2026-07) โ€” FIXED (pending prod QA) -Room unread indicators are **inconsistent**: reading a message sometimes clears the dot, sometimes leaves it stuck, and **sometimes the unread comes back after it was read** (resurrects). This is the same subsystem as the Wave-1 fixes (T1 `markThreadAsRead` root receipt, N1 favicon double-count, N2 dedupe cache, N4 blanket-DELETE) plus the newly-added **Mark as Unread (MSC2867)** `markedUnreadAtom` โ€” a prime suspect for interference. Look closely at: `state/room/roomToUnread.ts` (handleReceipt recompute vs DELETE, RESET triggers), `utils/notifications.ts markAsRead`, `features/room/thread/threadReceipt.ts`, `state/room/markedUnread.ts` (its Receipt listener + the `unread || markedUnread` dot), and `threadIdForReceipt`/unthreaded-receipt handling. Repro: read a message โ†’ dot lingers or returns. Planning session. +Room unread dots were inconsistent: reading a message sometimes cleared the dot, sometimes left it stuck, sometimes it resurrected. Root cause (confirmed by tracing + diffing upstream cinny `dev`): **our own "N4" change.** `handleReceipt` recomputed via `getUnreadInfo`, which reads `room.getUnreadNotificationCount()` โ€” server-computed and **stale on the synchronous synthetic receipt echo** (SDK only zeroes it immediately when the last event is your own message) โ†’ it PUT the stale non-zero count back โ†’ stuck/resurrecting. Compounded by `hasUnread = !!unread` lighting the dot on any present map entry, incl. phantom `{0,0}` PUTs from our `UnreadNotifications` listener. Plus a Mark-as-Unread (MSC2867) flag that never cleared on opening an already-read room (no receipt โ†’ no auto-clear). + +**Fix:** `roomToUnread.ts` โ€” `handleReceipt` reverts to upstream's optimistic `DELETE` on own receipt; reducer collapses `{0,0}` PUT โ†’ DELETE. `notifications.ts markAsRead` clears the marked-unread flag directly. `markedUnread.ts onReceipt` gated to main/unthreaded receipts (`myMainReceiptPresent`). Unit tests added; 700/700 pass, typecheck + build clean. Deploy + manual QA (read โ†’ dot clears & stays; thread read; mark-unread โ†’ open โ†’ clears; reconnect no resurrect). ### ๐Ÿงจ Encryption / E2EE โ€” โš ๏ธ EXTREME COMPLEXITY ยท ๐Ÿง  PLANNING SESSION REQUIRED diff --git a/src/app/state/room/markedUnread.test.ts b/src/app/state/room/markedUnread.test.ts index c1dec552e..4ebbd954e 100644 --- a/src/app/state/room/markedUnread.test.ts +++ b/src/app/state/room/markedUnread.test.ts @@ -1,7 +1,7 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; import { MatrixEvent } from 'matrix-js-sdk'; -import { receiptIsMine, setMarkedUnread } from './markedUnread'; +import { myMainReceiptPresent, receiptIsMine, setMarkedUnread } from './markedUnread'; // MSC2867 mark-as-unread: reading a room (our own receipt) clears the flag, so // `receiptIsMine` must detect only OUR receipt and ignore others'. And a write @@ -33,6 +33,28 @@ test('receiptIsMine: tolerates empty / malformed content', () => { assert.equal(receiptIsMine(receiptEvent({ $x: {} }), ME), false); }); +// myMainReceiptPresent gates the auto-clear to main-timeline reads, so reading a +// single thread does not wipe the whole-room marked-unread flag. +test('myMainReceiptPresent: true for an unthreaded receipt (no thread_id)', () => { + const event = receiptEvent({ $abc: { 'm.read': { [ME]: { ts: 1 } } } }); + assert.equal(myMainReceiptPresent(event, ME), true); +}); + +test('myMainReceiptPresent: true for a thread_id "main" receipt', () => { + const event = receiptEvent({ $abc: { 'm.read': { [ME]: { ts: 1, thread_id: 'main' } } } }); + assert.equal(myMainReceiptPresent(event, ME), true); +}); + +test('myMainReceiptPresent: false for a thread-scoped receipt', () => { + const event = receiptEvent({ $abc: { 'm.read': { [ME]: { ts: 1, thread_id: '$root:server' } } } }); + assert.equal(myMainReceiptPresent(event, ME), false); +}); + +test('myMainReceiptPresent: false when only another user has a main receipt', () => { + const event = receiptEvent({ $abc: { 'm.read': { [OTHER]: { ts: 1 } } } }); + assert.equal(myMainReceiptPresent(event, ME), false); +}); + test('setMarkedUnread writes both the stable and unstable keys with the flag', async () => { const calls: Array<{ type: string; content: unknown }> = []; const mx = { diff --git a/src/app/state/room/markedUnread.ts b/src/app/state/room/markedUnread.ts index a4ec897f6..da3a89873 100644 --- a/src/app/state/room/markedUnread.ts +++ b/src/app/state/room/markedUnread.ts @@ -9,7 +9,7 @@ import { AccountDataEvent } from '../../../types/matrix/accountData'; // flag round-trips across the ecosystem. const UNSTABLE_MARKED_UNREAD = 'com.famedly.marked_unread'; -const readMarkedUnread = (room: Room): boolean => { +export const readMarkedUnread = (room: Room): boolean => { const stable = room.getAccountData(AccountDataEvent.MarkedUnread)?.getContent()?.unread; if (typeof stable === 'boolean') return stable; return room.getAccountData(UNSTABLE_MARKED_UNREAD)?.getContent()?.unread === true; @@ -41,6 +41,22 @@ export const receiptIsMine = (event: MatrixEvent, userId: string): boolean => { ); }; +// True only when OUR receipt in this event is for the main timeline โ€” either +// unthreaded (no thread_id) or thread_id "main". A receipt scoped to a specific +// thread (thread_id === ) must NOT clear the whole-room marked +// flag, since only that one thread was read. +export const myMainReceiptPresent = (event: MatrixEvent, userId: string): boolean => { + const content = event.getContent(); + return Object.keys(content).some((eventId) => + Object.keys(content[eventId] ?? {}).some((receiptType) => { + const receipt = content[eventId][receiptType]?.[userId]; + if (!receipt) return false; + const threadId = (receipt as { thread_id?: string }).thread_id; + return threadId === undefined || threadId === 'main'; + }), + ); +}; + export const useBindMarkedUnreadAtom = (mx: MatrixClient, anAtom: typeof markedUnreadAtom) => { const setAtom = useSetAtom(anAtom); @@ -65,12 +81,15 @@ export const useBindMarkedUnreadAtom = (mx: MatrixClient, anAtom: typeof markedU const onAccountData: RoomEventHandlerMap[RoomEvent.AccountData] = (_event, room) => { syncRoom(room); }; - // Reading a room clears its marked-unread flag (MSC2867): when our own read - // receipt lands for a room that's currently marked, clear it. + // Reading a room clears its marked-unread flag (MSC2867): when our own + // MAIN-timeline read receipt lands for a room that's currently marked, clear + // it. Gated to main/unthreaded receipts so reading a single thread doesn't + // wipe the whole-room flag. (This also fires for receipts from our other + // devices; the local read path clears via markAsRead in notifications.ts.) const onReceipt: RoomEventHandlerMap[RoomEvent.Receipt] = (event, room) => { const myId = mx.getUserId(); if (!myId || !readMarkedUnread(room)) return; - if (receiptIsMine(event, myId)) { + if (myMainReceiptPresent(event, myId)) { setMarkedUnread(mx, room.roomId, false).catch(() => undefined); } }; diff --git a/src/app/state/room/roomToUnread.test.ts b/src/app/state/room/roomToUnread.test.ts index 93fb48390..2bc924617 100644 --- a/src/app/state/room/roomToUnread.test.ts +++ b/src/app/state/room/roomToUnread.test.ts @@ -116,6 +116,23 @@ test('PUT with unchanged counts is skipped (same map reference)', () => { assert.equal(before, after); }); +test('PUT of { total: 0, highlight: 0 } removes the room (collapses to DELETE)', () => { + const store = createStore(); + store.set(roomToUnreadAtom, { type: 'PUT', unreadInfo: info('!r:s', 3, 1) }); + // A phantom zero-count PUT (e.g. UnreadNotifications after the server zeroes + // counts) must clear the entry, not leave a stuck dot. + store.set(roomToUnreadAtom, { type: 'PUT', unreadInfo: info('!r:s', 0, 0) }); + assert.equal(get(store).has('!r:s'), false); +}); + +test('PUT of { 0, 0 } on an absent room is a no-op (same map reference)', () => { + const store = createStore(); + const before = get(store); + store.set(roomToUnreadAtom, { type: 'PUT', unreadInfo: info('!r:s', 0, 0) }); + assert.equal(before, get(store)); + assert.equal(get(store).has('!r:s'), false); +}); + // --------------------------------------------------------------------------- // roomToUnreadAtom: PUT with parent aggregation // --------------------------------------------------------------------------- diff --git a/src/app/state/room/roomToUnread.ts b/src/app/state/room/roomToUnread.ts index efdeb1409..3ef52d5b6 100644 --- a/src/app/state/room/roomToUnread.ts +++ b/src/app/state/room/roomToUnread.ts @@ -24,7 +24,6 @@ import { getUnreadInfo, getUnreadInfos, isNotificationEvent, - roomHaveUnread, } from '../../utils/room'; import { roomToParentsAtom } from './roomToParents'; import { useStateEventCallback } from '../../hooks/useStateEventCallback'; @@ -139,6 +138,27 @@ export const roomToUnreadAtom = atom + deleteUnreadInfo( + draftRoomToUnread, + getAllParents(get(roomToParentsAtom), unreadInfo.roomId), + unreadInfo.roomId, + ), + ), + ); + } + return; + } const currentUnread = get(baseRoomToUnread).get(unreadInfo.roomId); if (currentUnread && unreadEqual(currentUnread, unreadInfoToUnread(unreadInfo))) { // Do not update if unread data has not changes @@ -256,20 +276,16 @@ export const useBindRoomToUnreadAtom = (mx: MatrixClient, unreadAtom: typeof roo ), ); if (isMyReceipt) { - // Don't blanket-DELETE the room's unread on any receipt: a THREADED - // receipt (reading one thread) would wipe the room's still-valid - // main-timeline badge, and if the room was already read no - // UnreadNotifications PUT follows to restore it. Recompute instead โ€” - // DELETE only when the room is genuinely fully read. - const info = getUnreadInfo( - room, - getMutedThreads(threadNotificationsRef.current, room.roomId), - ); - if (info.total === 0 && info.highlight === 0 && !roomHaveUnread(mx, room)) { - setUnreadAtom({ type: 'DELETE', roomId: room.roomId }); - } else { - setUnreadAtom({ type: 'PUT', unreadInfo: info }); - } + // Optimistically clear on our own receipt (upstream cinny behavior). + // Do NOT recompute from getUnreadInfo here: getUnreadNotificationCount is + // server-computed and STALE on the synchronous synthetic receipt echo + // (the SDK only zeroes it immediately when the last live event is our own + // message), so recomputing PUTs the stale non-zero count back โ†’ the dot + // sticks / resurrects. The RoomEvent.UnreadNotifications listener below + // re-asserts the accurate badge (incl. restoring the main badge after a + // thread read) once the server acks, and a { 0, 0 } PUT collapses to a + // DELETE in the reducer. + setUnreadAtom({ type: 'DELETE', roomId: room.roomId }); } }; mx.on(RoomEvent.Receipt, handleReceipt); diff --git a/src/app/utils/notifications.test.ts b/src/app/utils/notifications.test.ts index 2a952463f..86cfe2916 100644 --- a/src/app/utils/notifications.test.ts +++ b/src/app/utils/notifications.test.ts @@ -20,16 +20,22 @@ type RoomOpts = { readUpTo?: string | null; threads?: any[]; threadUnread?: Record; + markedUnread?: boolean; }; const setup = (opts: RoomOpts) => { const calls: ReceiptCall[] = []; + const accountDataWrites: Array<{ type: string; content: any }> = []; const room = { getLiveTimeline: () => ({ getEvents: () => opts.timeline ?? [] }), getEventReadUpTo: () => opts.readUpTo ?? null, getThreads: () => opts.threads ?? [], getThreadUnreadNotificationCount: (threadId: string, _type: NotificationCountType) => opts.threadUnread?.[threadId] ?? 0, + getAccountData: (type: string) => + opts.markedUnread && type === 'm.marked_unread' + ? { getContent: () => ({ unread: true }) } + : undefined, }; const mx = { getRoom: () => room, @@ -38,8 +44,12 @@ const setup = (opts: RoomOpts) => { calls.push({ eventId: event.getId(), receiptType, unthreaded }); return {}; }, + setRoomAccountData: async (_roomId: string, type: string, content: any) => { + accountDataWrites.push({ type, content }); + return {}; + }, } as any; - return { mx, calls }; + return { mx, calls, accountDataWrites }; }; test('main timeline: unthreaded receipt at the latest event', async () => { @@ -107,6 +117,27 @@ test('everything read: no receipts sent', async () => { assert.equal(calls.length, 0); }); +test('marked-unread + already fully read: clears the flag even though no receipt is sent', async () => { + const { mx, calls, accountDataWrites } = setup({ + timeline: [evt('a'), evt('b')], + readUpTo: 'b', // nothing newer โ†’ no receipt + markedUnread: true, + }); + await markAsRead(mx, '!r:server', false); + assert.equal(calls.length, 0); // no receipt (the stuck-dot case) + // ...but the marked-unread flag is cleared directly (both keys, unread:false) + assert.ok(accountDataWrites.some((w) => w.type === 'm.marked_unread' && w.content.unread === false)); +}); + +test('not marked-unread: markAsRead does not touch account data', async () => { + const { mx, accountDataWrites } = setup({ + timeline: [evt('a'), evt('b')], + readUpTo: 'a', + }); + await markAsRead(mx, '!r:server', false); + assert.equal(accountDataWrites.length, 0); +}); + test('sending thread reply is skipped', async () => { const t = thread('$root', evt('$reply', true)); // isSending โ†’ skip const { mx, calls } = setup({ diff --git a/src/app/utils/notifications.ts b/src/app/utils/notifications.ts index 2156d3415..1930e3f89 100644 --- a/src/app/utils/notifications.ts +++ b/src/app/utils/notifications.ts @@ -1,11 +1,19 @@ import { MatrixClient, NotificationCountType, ReceiptType } from 'matrix-js-sdk'; import { getSettings } from '../state/settings'; +import { readMarkedUnread, setMarkedUnread } from '../state/room/markedUnread'; export async function markAsRead(mx: MatrixClient, roomId: string, privateReceipt: boolean) { const { privateReadReceipts } = getSettings(); const room = mx.getRoom(roomId); if (!room) return; + // Reading a room clears an explicit "mark as unread" (MSC2867). The binder's + // receipt-driven auto-clear does NOT fire when the room is already fully read + // (no receipt is sent below in that case), so clear it directly here. + if (readMarkedUnread(room)) { + setMarkedUnread(mx, roomId, false).catch(() => undefined); + } + const receiptType = privateReceipt || privateReadReceipts ? ReceiptType.ReadPrivate : ReceiptType.Read;