diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index ff664059c4..c4fb4e91cc 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -39,6 +39,10 @@ const overrides = new Map([ ["src-tauri/src/nostr_convert.rs", 1126], ["src/shared/api/relayClientSession.ts", 1022], ["src-tauri/src/migration.rs", 1295], + // onMarkRead prop-pair completion (mirrors the onMarkUnread prop already + // threaded here) — a 1-line overage, not generic debt growth. Approved + // override; still queued to split with the rest of this list. + ["src/features/messages/ui/MessageThreadPanel.tsx", 1002], ]); await runFileSizeCheck({ diff --git a/desktop/src/app/AppShell.tsx b/desktop/src/app/AppShell.tsx index 0531f7c72e..4544c48b49 100644 --- a/desktop/src/app/AppShell.tsx +++ b/desktop/src/app/AppShell.tsx @@ -29,6 +29,7 @@ import { useOpenDmMutation, } from "@/features/channels/hooks"; import { useUnreadChannels } from "@/features/channels/useUnreadChannels"; +import { msgContextKey } from "@/features/channels/readState/readStateFormat"; import { useMembershipNotifications } from "@/features/channels/useMembershipNotifications"; import { useFeedItemState } from "@/features/home/useFeedItemState"; import { getThreadReference } from "@/features/messages/lib/threading"; @@ -367,6 +368,21 @@ export function AppShell() { }, [markChannelRead], ); + + // Per-message read frontier (LP4 v3): effective(msg:) folds through the + // channel, so a channel-read clears messages older than the top-level frontier. + const getMessageReadAt = React.useCallback( + (messageId: string) => getChannelReadAt(msgContextKey(messageId)), + [getChannelReadAt], + ); + const markMessageRead = React.useCallback( + (messageId: string, timestamp: number) => + markChannelRead( + msgContextKey(messageId), + new Date(timestamp * 1_000).toISOString(), + ), + [markChannelRead], + ); const threadActivityFeedItems = useThreadActivityFeedItems( threadActivityItems, mutedRootIds, @@ -479,9 +495,10 @@ export function AppShell() { [goSettings], ); - const handleCloseSettings = React.useCallback(() => { - closeSettings(); - }, [closeSettings]); + const handleCloseSettings = React.useCallback( + () => closeSettings(), + [closeSettings], + ); // Section switches rewrite the settings entry rather than stacking one // history entry per section, so back always exits settings in one step. @@ -605,13 +622,12 @@ export function AppShell() { }; }, []); - const handleOpenNewDm = React.useCallback(() => { - setIsNewDmOpen(true); - }, []); + const handleOpenNewDm = React.useCallback(() => setIsNewDmOpen(true), []); - const handleOpenCreateChannel = React.useCallback(() => { - setIsCreateChannelOpen(true); - }, []); + const handleOpenCreateChannel = React.useCallback( + () => setIsCreateChannelOpen(true), + [], + ); React.useLayoutEffect(() => { if (settingsOpen) { @@ -721,12 +737,12 @@ export function AppShell() { markChannelRead, markChannelUnread, openCreateChannel: handleOpenCreateChannel, - openChannelManagement: () => { - setIsChannelManagementOpen(true); - }, + openChannelManagement: () => setIsChannelManagementOpen(true), getChannelReadAt, getThreadReadAt, markThreadRead, + getMessageReadAt, + markMessageRead, readStateVersion, setContextParentResolver, followThread: handleFollowThread, diff --git a/desktop/src/app/AppShellContext.tsx b/desktop/src/app/AppShellContext.tsx index a565397d1c..c83f302b16 100644 --- a/desktop/src/app/AppShellContext.tsx +++ b/desktop/src/app/AppShellContext.tsx @@ -25,6 +25,12 @@ type AppShellContextValue = { getThreadReadAt: (rootId: string, channelId?: string | null) => number | null; // Advance the thread read frontier to the given unix-seconds timestamp. markThreadRead: (rootId: string, timestamp: number) => void; + // Per-message read frontier as unix-seconds timestamp, or null when never + // read. Uses `msg:` context keys folded through the active channel by the + // parent resolver (LP4 v3 per-message badge model). + getMessageReadAt: (messageId: string) => number | null; + // Advance a single message's read marker to the given unix-seconds timestamp. + markMessageRead: (messageId: string, timestamp: number) => void; // Bump-counter that invalidates whenever the read marker changes. Include // in memo deps that consume getChannelReadAt. readStateVersion: number; @@ -50,6 +56,8 @@ const AppShellContext = React.createContext({ getChannelReadAt: () => null, getThreadReadAt: () => null, markThreadRead: () => {}, + getMessageReadAt: () => null, + markMessageRead: () => {}, readStateVersion: 0, setContextParentResolver: () => {}, followThread: () => {}, diff --git a/desktop/src/features/channels/lib/subtreeCreatedAt.test.mjs b/desktop/src/features/channels/lib/subtreeCreatedAt.test.mjs deleted file mode 100644 index ef316fc41f..0000000000 --- a/desktop/src/features/channels/lib/subtreeCreatedAt.test.mjs +++ /dev/null @@ -1,144 +0,0 @@ -import assert from "node:assert/strict"; -import test from "node:test"; - -import { computeThreadUnreadMarker } from "../../messages/lib/unreadMarker.ts"; -import { - buildDirectRepliesByParentId, - subtreeMaxCreatedAt, -} from "./subtreeCreatedAt.ts"; - -// Tree: w(100) -// ├── deep1(400) ── deep2(500) -// └── sib(300) -// `deep1` is the deep branch (subtree-max 500); `sib` is a shallower sibling -// whose only reply (300) is chronologically older than the deep tail. -function fixture() { - const directReplyIdsByParentId = new Map([ - ["w", ["deep1", "sib"]], - ["deep1", ["deep2"]], - ]); - const createdAtByMessageId = new Map([ - ["w", 100], - ["deep1", 400], - ["deep2", 500], - ["sib", 300], - ]); - const replies = [ - { id: "sib", createdAt: 300 }, - { id: "deep1", createdAt: 400 }, - { id: "deep2", createdAt: 500 }, - ]; - return { directReplyIdsByParentId, createdAtByMessageId, replies }; -} - -test("subtreeMaxCreatedAt_branchWithDescendants_returnsDeepestCreatedAt", () => { - const { directReplyIdsByParentId, createdAtByMessageId } = fixture(); - - const result = subtreeMaxCreatedAt( - "deep1", - directReplyIdsByParentId, - createdAtByMessageId, - ); - - // Includes the descendant deep2(500), not just deep1's own 400. - assert.equal(result, 500); -}); - -test("subtreeMaxCreatedAt_leafBranch_returnsOwnCreatedAt", () => { - const { directReplyIdsByParentId, createdAtByMessageId } = fixture(); - - const result = subtreeMaxCreatedAt( - "sib", - directReplyIdsByParentId, - createdAtByMessageId, - ); - - assert.equal(result, 300); -}); - -test("subtreeMaxCreatedAt_absentMessage_returnsNull", () => { - const { directReplyIdsByParentId, createdAtByMessageId } = fixture(); - - const result = subtreeMaxCreatedAt( - "ghost", - directReplyIdsByParentId, - createdAtByMessageId, - ); - - // Null signals the caller to skip the read-state write. - assert.equal(result, null); -}); - -// Invariant 3: expanding the deep branch advances the single monotonic frontier -// to the branch subtree-max (500), which consumes the chronologically-older -// unexpanded sibling (300) too. This is the accepted single-frontier semantic. -test("expandDeepBranch_advancesFrontierToSubtreeMax_consumesOlderSibling", () => { - const { directReplyIdsByParentId, createdAtByMessageId, replies } = fixture(); - - const frontier = subtreeMaxCreatedAt( - "deep1", - directReplyIdsByParentId, - createdAtByMessageId, - ); - const marker = computeThreadUnreadMarker(replies, frontier); - - assert.equal(frontier, 500); - // Everything at or below 500 is read — including sib(300), never expanded. - assert.equal(marker.firstUnreadReplyId, null); - assert.equal(marker.unreadCount, 0); -}); - -// Invariant 1: the session divider is computed from the open-time frontier -// SNAPSHOT, the badge/consume from the LIVE frontier. After expand advances the -// live frontier to the subtree-max (500), the two clocks deliberately diverge: -// the live frontier reports everything consumed, while the divider — read from -// the frozen open-time snapshot (100) — stays pinned on the first unread reply. -// This is what keeps the divider from moving mid-session when you expand. -test("expandAfterOpen_dividerFromSnapshot_holds_whileLiveFrontierConsumes", () => { - const { directReplyIdsByParentId, createdAtByMessageId, replies } = fixture(); - - const openSnapshot = 100; - const liveFrontierAfterExpand = subtreeMaxCreatedAt( - "deep1", - directReplyIdsByParentId, - createdAtByMessageId, - ); - - const dividerFromSnapshot = computeThreadUnreadMarker(replies, openSnapshot); - const consumeFromLive = computeThreadUnreadMarker( - replies, - liveFrontierAfterExpand, - ); - - // Divider stays on the first unread, computed against the frozen snapshot... - assert.equal(dividerFromSnapshot.firstUnreadReplyId, "sib"); - assert.equal(dividerFromSnapshot.unreadCount, 3); - // ...even though the live frontier has consumed the whole branch. - assert.equal(consumeFromLive.firstUnreadReplyId, null); -}); - -test("buildDirectRepliesByParentId_groupsDirectRepliesByParent_inOrder", () => { - const messages = [ - { id: "root", parentId: null, createdAt: 100 }, - { id: "r1", parentId: "root", createdAt: 200 }, - { id: "deep", parentId: "r1", createdAt: 300 }, - { id: "r2", parentId: "root", createdAt: 250 }, - ]; - const index = buildDirectRepliesByParentId(messages); - // Only DIRECT children, in timeline order — not transitive descendants. - assert.deepEqual( - index.get("root")?.map((m) => m.id), - ["r1", "r2"], - ); - assert.deepEqual( - index.get("r1")?.map((m) => m.id), - ["deep"], - ); - // A top-level message with no replies is absent (the seed/count guard). - assert.equal(index.has("r2"), false); -}); - -test("buildDirectRepliesByParentId_topLevelOnly_returnsEmpty", () => { - const messages = [{ id: "root", parentId: null, createdAt: 100 }]; - assert.equal(buildDirectRepliesByParentId(messages).size, 0); -}); diff --git a/desktop/src/features/channels/lib/subtreeCreatedAt.ts b/desktop/src/features/channels/lib/subtreeCreatedAt.ts index d115962bde..f426b50456 100644 --- a/desktop/src/features/channels/lib/subtreeCreatedAt.ts +++ b/desktop/src/features/channels/lib/subtreeCreatedAt.ts @@ -1,36 +1,17 @@ /** - * Newest `createdAt` across a thread branch: the message itself plus every - * descendant, walked through the direct-children adjacency map. Drilling into a - * branch advances the thread read frontier to this value, so it determines how - * far "expanding consumes unread" reaches. Returns null when the message is - * absent from the timeline so the caller can skip the read-state write. + * Reply-graph builders for the per-message thread badge model (LP4 v3). Each + * maps the loaded timeline into an index a badge consumer reads in O(1): direct + * children by parent, replies by their resolved thread root, createdAt by id, + * and the descendant id walk. The old `subtreeMaxCreatedAt` frontier-advance + * helper is gone — read state is now per-message (`effective(msg:)`), so no + * subtree ceiling is computed. */ -export function subtreeMaxCreatedAt( - messageId: string, - directReplyIdsByParentId: ReadonlyMap, - createdAtByMessageId: ReadonlyMap, -): number | null { - const ownCreatedAt = createdAtByMessageId.get(messageId); - if (ownCreatedAt === undefined) return null; - - let maxCreatedAt = ownCreatedAt; - const pendingIds = [...(directReplyIdsByParentId.get(messageId) ?? [])]; - while (pendingIds.length > 0) { - const currentId = pendingIds.pop(); - if (!currentId) continue; - const createdAt = createdAtByMessageId.get(currentId); - if (createdAt !== undefined && createdAt > maxCreatedAt) { - maxCreatedAt = createdAt; - } - pendingIds.push(...(directReplyIdsByParentId.get(currentId) ?? [])); - } - return maxCreatedAt; -} /** Minimal timeline shape the adjacency/createdAt builders read. */ interface ReplyGraphMessage { id: string; parentId?: string | null; + rootId?: string | null; createdAt: number; } @@ -49,19 +30,24 @@ export function buildDirectReplyIdsByParentId( } /** - * Maps each parent message id to its direct-reply objects in timeline order. - * Built once so per-thread badge consumers resolve direct replies in O(1) - * instead of re-scanning the whole timeline per top-level message. + * Maps each thread root id to every reply that resolves to it by `rootId`, + * in timeline order. Unlike the parent-keyed maps above, this groups by the + * reply's own `rootId` (getThreadReference: the `root` e-tag that travels with + * the event), so a deep reply lands under its true root even when an + * intermediate ancestor is absent from the loaded window. Root-keyed badge + * consumers use this to roll up severed orphans the parent-chain walk misses. + * Top-level messages (no rootId) and self-referential roots are excluded. */ -export function buildDirectRepliesByParentId( +export function buildRepliesByRootId( messages: readonly T[], ): Map { const map = new Map(); for (const message of messages) { - if (!message.parentId) continue; - const currentReplies = map.get(message.parentId) ?? []; + const rootId = message.rootId; + if (!rootId || rootId === message.id) continue; + const currentReplies = map.get(rootId) ?? []; currentReplies.push(message); - map.set(message.parentId, currentReplies); + map.set(rootId, currentReplies); } return map; } diff --git a/desktop/src/features/channels/lib/threadBadgeCollapseOnOpen.test.mjs b/desktop/src/features/channels/lib/threadBadgeCollapseOnOpen.test.mjs index d88a49f3b3..ba621db60c 100644 --- a/desktop/src/features/channels/lib/threadBadgeCollapseOnOpen.test.mjs +++ b/desktop/src/features/channels/lib/threadBadgeCollapseOnOpen.test.mjs @@ -2,147 +2,124 @@ import assert from "node:assert/strict"; import test from "node:test"; import { computeThreadBadgeCounts } from "./threadBadgeCounts.ts"; -import { nextThreadBadgeFrontier } from "./threadBadgeFrontier.ts"; -import { - buildCreatedAtByMessageId, - buildDirectRepliesByParentId, - buildDirectReplyIdsByParentId, - subtreeMaxCreatedAt, -} from "./subtreeCreatedAt.ts"; +import { buildRepliesByRootId } from "./subtreeCreatedAt.ts"; -// End-to-end model of the mark-read-on-thread-open pipeline in -// useChannelUnreadState. The open effect computes a read ceiling for the thread -// head, markThreadRead advances the thread-OWN marker toward it (monotonic, per -// advanceContext), the badge frontier snapshot then advances toward that live -// marker (seedThreadBadgeFrontiers -> nextThreadBadgeFrontier), and the -// summary badge counts the whole subtree against that snapshot -// (computeThreadBadgeCounts). The fix changed the open ceiling from the -// direct-replies max (head + direct children) to the full-subtree max -// (subtreeMaxCreatedAt); these tests pin that the badge collapses to 0 on open -// whether or not the OWN marker actually advances. +// Open-at-level contract (LP4 v3). Opening a thread no longer collapses the +// whole subtree badge (#1118's behavior, deliberately reversed). The on-open +// effect marks read ONLY the replies revealed on open — each gets its own +// msg: marker advanced to its createdAt — so a reply in a still-collapsed +// branch keeps its badge until it too is revealed. The root summary badge +// (computeThreadBadgeCounts) reads effective(msg:) live: a reply counts +// iff createdAt > readAt, so reading one reply never clears another. -const msg = (id, parentId, createdAt, pubkey = "author") => ({ +// rootId travels with every reply (getThreadReference's `root` e-tag), so a +// nested reply rolls up to the thread root even when an ancestor is collapsed. +const msg = (id, parentId, createdAt = 100, pubkey = "author", rootId) => ({ id, parentId, + rootId: rootId ?? parentId ?? id, createdAt, pubkey, }); -// The ceiling the open effect now writes: full-subtree max over the head. -const openCeiling = (rootId, messages) => - subtreeMaxCreatedAt( - rootId, - buildDirectReplyIdsByParentId(messages), - buildCreatedAtByMessageId(messages), - ); +const countAll = () => true; + +// Model the on-open mark-read effect: each revealed reply's msg: marker is +// advanced to its own createdAt (useChannelUnreadState's open effect maps +// markMessageRead(id, createdAt) over the visible set). A reply absent from the +// revealed set was never read, so its resolver returns null and it stays +// unread. Returns the live per-message getReadAt resolver after the open. +function openMarksRevealed(messages, revealedIds) { + const revealed = new Set(revealedIds); + const createdAtById = new Map(messages.map((m) => [m.id, m.createdAt])); + return (id) => (revealed.has(id) ? (createdAtById.get(id) ?? null) : null); +} -// Drive one thread-open through the pipeline. `priorOwnMarker` is the thread's -// OWN read marker before this open (null = never read). Returns the resulting -// badge count for the root after open. -const badgeAfterOpen = (rootId, messages, priorOwnMarker, currentPubkey) => { - const ceiling = openCeiling(rootId, messages); - // markThreadRead -> advanceContext: monotonic max of prior own marker and the - // new ceiling. A null ceiling means no replies; the effect early-returns. - const liveMarker = - ceiling === null - ? priorOwnMarker - : priorOwnMarker === null - ? ceiling - : Math.max(priorOwnMarker, ceiling); - // seedThreadBadgeFrontiers advances the snapshot toward the live marker. - const frontier = nextThreadBadgeFrontier(undefined, liveMarker); - return computeThreadBadgeCounts( +const rootBadge = (messages, getReadAt, currentPubkey) => + computeThreadBadgeCounts( messages, - buildDirectRepliesByParentId(messages), - new Map([[rootId, frontier]]), - () => true, + buildRepliesByRootId(messages), + getReadAt, + countAll, currentPubkey, - ).get(rootId); -}; + ).get("root"); -test("openThreadWithUnreadNestedReply_advancesFrontierToSubtreeMax", () => { - // root -> a(100) -> b(200): the unread lives in nested reply b. - // The OLD direct-replies ceiling stopped at a(100) (b is a grandchild, not a - // direct reply of root); only subtree-max reaches the nested b(200). +test("openRevealingOnlyDirectChild_keepsCollapsedGrandchildBadge", () => { + // root -> a -> b: opening reveals direct child a but b is nested under a + // still-collapsed branch. The OLD whole-subtree-on-open would have cleared + // the badge entirely; v3 marks only a read, so b keeps the root badge lit. const messages = [ msg("root", null, 50), msg("a", "root", 100), - msg("b", "a", 200), + msg("b", "a", 200, "author", "root"), ]; - const ids = buildDirectReplyIdsByParentId(messages); - const createdAt = buildCreatedAtByMessageId(messages); - assert.equal(subtreeMaxCreatedAt("root", ids, createdAt), 200); + assert.equal(rootBadge(messages, openMarksRevealed(messages, ["a"])), 1); }); -test("openThreadWithUnreadNestedReply_collapsesBadgeToZero", () => { - // The reported bug: before the fix the frontier sat at the direct-replies - // ceiling (100) and the nested reply b(200) kept the badge lit. The fix - // advances to subtree-max (200), so the badge recomputes to 0 on open. +test("openRevealingWholeSubtree_clearsRootBadge", () => { + // When every reply is revealed on open, each is marked read and the badge + // clears — the only case the old subtree-collapse and the new open-at-level + // agree on. const messages = [ msg("root", null, 50), msg("a", "root", 100), - msg("b", "a", 200), + msg("b", "a", 200, "author", "root"), ]; - assert.equal(badgeAfterOpen("root", messages, null), undefined); + assert.equal( + rootBadge(messages, openMarksRevealed(messages, ["a", "b"])), + undefined, + ); }); -test("openThreadWithUnreadNestedReply_oldDirectCeilingLeftBadgeLit", () => { - // Regression guard: the OLD behavior advanced the frontier only to the - // direct-replies ceiling — max over root(50) and its DIRECT reply a(100), - // i.e. 100. The nested grandchild b(200) was excluded, so the badge stayed - // lit (count=1). This pins the exact gap the fix closes: had the fix been - // reverted to that ceiling, the badge would NOT clear. The subtree-max - // ceiling (200) is asserted to clear the badge in the test above. +test("openRevealingOneBranch_keepsOtherCollapsedBranchBadge", () => { + // root -> {a -> a1, c -> c1}: opening reveals branch a (a, a1) but leaves + // branch c collapsed. The two unread replies under c keep the root badge. const messages = [ msg("root", null, 50), msg("a", "root", 100), - msg("b", "a", 200), + msg("a1", "a", 110, "author", "root"), + msg("c", "root", 120), + msg("c1", "c", 130, "author", "root"), ]; - const oldDirectCeiling = 100; - const frontier = nextThreadBadgeFrontier(undefined, oldDirectCeiling); - const count = computeThreadBadgeCounts( - messages, - buildDirectRepliesByParentId(messages), - new Map([["root", frontier]]), - () => true, - ).get("root"); - assert.equal(count, 1); + assert.equal( + rootBadge(messages, openMarksRevealed(messages, ["a", "a1"])), + 2, + ); }); -test("ownMarkerAlreadyAtSubtreeMax_stillCollapsesBadgeToZero", () => { - // Prior-session expand synced the OWN marker to subtree-max BEFORE this - // session's first open. markThreadRead's advance is then a no-op - // (advanceContext early-returns, no notify), but the badge still reads 0 - // because the frontier snapshot is seeded from the live marker on render, - // independent of whether the advance notified. Pins the no-op-return path. +test("newerReplyAfterOpen_relightsRootBadge", () => { + // Open marks a(100) read at its createdAt. A newer reply b(200) arrives in + // the same revealed branch; the predicate is strictly createdAt > readAt, so + // b is unread against a's marker and the badge relights. Models a reply + // landing after the open snapshot without re-marking. const messages = [ msg("root", null, 50), msg("a", "root", 100), - msg("b", "a", 200), + msg("b", "root", 200), ]; - assert.equal(badgeAfterOpen("root", messages, 200), undefined); + // Only a was present/revealed at open; b is unread (never marked). + assert.equal(rootBadge(messages, openMarksRevealed(messages, ["a"])), 1); }); test("openThreadWhereOnlyUnreadIsOwnReply_neverShowsBadge", () => { - // Will "commented back": a nested reply authored by the current user. Self - // authored replies are excluded from the count, so no badge ever shows and - // the fix is inert — the badge is already absent before and after open. + // A nested reply authored by the current user. Self-authored replies are + // excluded from the count, so no badge shows regardless of read state — the + // open-at-level change is inert here. const messages = [ msg("root", null, 50), msg("a", "root", 100, "other"), - msg("b", "a", 200, "ME"), + msg("b", "a", 200, "ME", "root"), ]; - // Frontier below every reply (never read) — only "other"'s reply a counts. - const beforeOpen = computeThreadBadgeCounts( - messages, - buildDirectRepliesByParentId(messages), - new Map([["root", null]]), - () => true, - "me", - ).get("root"); - assert.equal(beforeOpen, 1); - // After open the frontier reaches subtree-max (200), clearing a as well. - assert.equal(badgeAfterOpen("root", messages, null, "me"), undefined); + // Nothing revealed (never read), only "other"'s reply a could count. + assert.equal( + rootBadge(messages, () => null, "me"), + 1, + ); + // After revealing a, only the self-authored b remains — no badge. + assert.equal( + rootBadge(messages, openMarksRevealed(messages, ["a"]), "me"), + undefined, + ); }); test("openThreadWhereEveryUnreadIsOwnReply_inertNoBadgeEver", () => { @@ -150,15 +127,14 @@ test("openThreadWhereEveryUnreadIsOwnReply_inertNoBadgeEver", () => { const messages = [ msg("root", null, 50), msg("a", "root", 100, "ME"), - msg("b", "a", 200, "ME"), + msg("b", "a", 200, "ME", "root"), ]; - const before = computeThreadBadgeCounts( - messages, - buildDirectRepliesByParentId(messages), - new Map([["root", null]]), - () => true, - "me", - ).get("root"); - assert.equal(before, undefined); - assert.equal(badgeAfterOpen("root", messages, null, "me"), undefined); + assert.equal( + rootBadge(messages, () => null, "me"), + undefined, + ); + assert.equal( + rootBadge(messages, openMarksRevealed(messages, ["a", "b"]), "me"), + undefined, + ); }); diff --git a/desktop/src/features/channels/lib/threadBadgeCounts.test.mjs b/desktop/src/features/channels/lib/threadBadgeCounts.test.mjs index 5214ab781b..577dc9bf87 100644 --- a/desktop/src/features/channels/lib/threadBadgeCounts.test.mjs +++ b/desktop/src/features/channels/lib/threadBadgeCounts.test.mjs @@ -2,93 +2,162 @@ import assert from "node:assert/strict"; import test from "node:test"; import { computeThreadBadgeCounts } from "./threadBadgeCounts.ts"; -import { buildDirectRepliesByParentId } from "./subtreeCreatedAt.ts"; +import { buildRepliesByRootId } from "./subtreeCreatedAt.ts"; -// Minimal TimelineMessage shape the badge counter reads: id, parentId, +// Minimal TimelineMessage shape the badge counter reads: id, parentId, rootId, // createdAt, pubkey. createdAt defaults high so replies count unread against a -// null frontier unless a test sets it lower. -const msg = (id, parentId, createdAt = 100, pubkey = "author") => ({ +// never-read resolver unless a test sets it lower. `rootId` defaults to the parent +// (mirroring getThreadReference's `rootTag?.[1] ?? parentId` fallback), which is +// correct for a DIRECT reply (parent IS the root); nested replies must pass +// their true thread root explicitly, exactly as getThreadReference resolves the +// `root` e-tag that travels with every event regardless of which ancestors are +// loaded. The roll-up groups by that rootId, so a severed orphan still tallies +// at its true root. +const msg = (id, parentId, createdAt = 100, pubkey = "author", rootId) => ({ id, parentId, + rootId: rootId ?? parentId ?? id, createdAt, pubkey, }); const countAll = () => true; -const counts = (messages, frontiers, isNotified = countAll, currentPubkey) => + +// LP4 v3: badges read a per-message resolver, not a per-root frontier. These +// helpers translate the legacy test intents into resolvers: +// - `neverRead` — no message has been read (the old null-frontier case). +// - `readLineByRoot(map)` — a uniform read-line per thread root, applied to +// every reply that resolves to that root by rootId. Reproduces the old +// "frontier covers part of the subtree" cases without a single global line. +const neverRead = () => null; +function readLineByRoot(messages, frontiersByRoot) { + const lineByMessageId = new Map(); + for (const message of messages) { + const root = message.rootId ?? message.parentId ?? message.id; + const line = frontiersByRoot.get(root); + if (line !== undefined) lineByMessageId.set(message.id, line); + } + return (id) => lineByMessageId.get(id) ?? null; +} + +const counts = (messages, getReadAt, isNotified = countAll, currentPubkey) => computeThreadBadgeCounts( messages, - buildDirectRepliesByParentId(messages), - frontiers, + buildRepliesByRootId(messages), + getReadAt, isNotified, currentPubkey, ); test("computeThreadBadgeCounts_directRepliesOnly_countsEach", () => { const messages = [msg("root", null), msg("a", "root"), msg("b", "root")]; - assert.equal(counts(messages, undefined).get("root"), 2); + assert.equal(counts(messages, neverRead).get("root"), 2); }); test("computeThreadBadgeCounts_nestedReply_countsTowardRoot", () => { - // root -> a -> b: b is a reply-to-a-reply. Pre-fix it lived under a's key - // and was never tallied toward root; the subtree walk must count it. - const messages = [msg("root", null), msg("a", "root"), msg("b", "a")]; - assert.equal(counts(messages, undefined).get("root"), 2); + // root -> a -> b: b is a reply-to-a-reply. It carries the thread root in its + // rootId, so the root-keyed roll-up tallies it toward root, not toward a. + const messages = [ + msg("root", null), + msg("a", "root"), + msg("b", "a", 100, "author", "root"), + ]; + assert.equal(counts(messages, neverRead).get("root"), 2); }); test("computeThreadBadgeCounts_deepChain_countsWholeSubtree", () => { - // root -> a -> b -> c -> d: every descendant tallies toward the root. + // root -> a -> b -> c -> d: every descendant carries rootId "root" and tallies + // toward the root. const messages = [ msg("root", null), msg("a", "root"), - msg("b", "a"), - msg("c", "b"), - msg("d", "c"), + msg("b", "a", 100, "author", "root"), + msg("c", "b", 100, "author", "root"), + msg("d", "c", 100, "author", "root"), ]; - assert.equal(counts(messages, undefined).get("root"), 4); + assert.equal(counts(messages, neverRead).get("root"), 4); }); test("computeThreadBadgeCounts_branchingSubtree_countsAllBranches", () => { - // root -> a -> {b, c}; root -> d. Four descendants across two branches. + // root -> a -> {b, c}; root -> d. Four descendants across two branches, all + // carrying rootId "root". const messages = [ msg("root", null), msg("a", "root"), - msg("b", "a"), - msg("c", "a"), + msg("b", "a", 100, "author", "root"), + msg("c", "a", 100, "author", "root"), msg("d", "root"), ]; - assert.equal(counts(messages, undefined).get("root"), 4); + assert.equal(counts(messages, neverRead).get("root"), 4); }); test("computeThreadBadgeCounts_rootWithNoReplies_omitted", () => { const messages = [msg("root", null)]; - assert.equal(counts(messages, undefined).has("root"), false); + assert.equal(counts(messages, neverRead).has("root"), false); }); test("computeThreadBadgeCounts_notNotified_omitted", () => { - const messages = [msg("root", null), msg("a", "root"), msg("b", "a")]; - assert.equal(counts(messages, undefined, () => false).size, 0); + const messages = [ + msg("root", null), + msg("a", "root"), + msg("b", "a", 100, "author", "root"), + ]; + assert.equal(counts(messages, neverRead, () => false).size, 0); }); -test("computeThreadBadgeCounts_frontierCoversNestedReplies_excludesRead", () => { - // Frontier 150: a (100) is read, only nested b (200) remains unread. +test("computeThreadBadgeCounts_readLineCoversNestedReplies_excludesRead", () => { + // Read-line 150 across the root's subtree: a (100) is read, only nested + // b (200) remains unread. const messages = [ msg("root", null), msg("a", "root", 100), - msg("b", "a", 200), + msg("b", "a", 200, "author", "root"), ]; - const frontiers = new Map([["root", 150]]); - assert.equal(counts(messages, frontiers).get("root"), 1); + const readAt = readLineByRoot(messages, new Map([["root", 150]])); + assert.equal(counts(messages, readAt).get("root"), 1); }); -test("computeThreadBadgeCounts_frontierCoversWholeSubtree_omitsRoot", () => { +test("computeThreadBadgeCounts_readLineCoversWholeSubtree_omitsRoot", () => { const messages = [ msg("root", null), msg("a", "root", 100), - msg("b", "a", 120), + msg("b", "a", 120, "author", "root"), ]; - const frontiers = new Map([["root", 150]]); - assert.equal(counts(messages, frontiers).has("root"), false); + const readAt = readLineByRoot(messages, new Map([["root", 150]])); + assert.equal(counts(messages, readAt).has("root"), false); +}); + +test("computeThreadBadgeCounts_perMessageMarker_readDeepReplyKeepsSiblingBadge", () => { + // The per-message model's defining case: marking the deep reply b read + // leaves direct sibling a unread independently — a single subtree frontier + // could not express "b read but a not". + const messages = [ + msg("root", null), + msg("a", "root", 100), + msg("b", "a", 200, "author", "root"), + ]; + const readAt = (id) => (id === "b" ? 200 : null); + assert.equal(counts(messages, readAt).get("root"), 1); +}); + +test("computeThreadBadgeCounts_forcedUnread_relightsReadReply", () => { + // Session-local mark-unread forces a (read by its marker) back to unread. + const messages = [ + msg("root", null), + msg("a", "root", 100), + msg("b", "a", 200, "author", "root"), + ]; + const allRead = () => 1000; + const isForcedUnread = (id) => id === "a"; + const result = computeThreadBadgeCounts( + messages, + buildRepliesByRootId(messages), + allRead, + countAll, + undefined, + isForcedUnread, + ); + assert.equal(result.get("root"), 1); }); test("computeThreadBadgeCounts_selfAuthoredNestedReply_notCounted", () => { @@ -96,20 +165,74 @@ test("computeThreadBadgeCounts_selfAuthoredNestedReply_notCounted", () => { const messages = [ msg("root", null), msg("a", "root", 100, "other"), - msg("b", "a", 200, "ME"), + msg("b", "a", 200, "ME", "root"), ]; - assert.equal(counts(messages, undefined, countAll, "me").get("root"), 1); + assert.equal(counts(messages, neverRead, countAll, "me").get("root"), 1); }); test("computeThreadBadgeCounts_multipleRoots_eachCountsOwnSubtree", () => { const messages = [ msg("root1", null), msg("a", "root1"), - msg("b", "a"), + msg("b", "a", 100, "author", "root1"), msg("root2", null), msg("c", "root2"), ]; - const result = counts(messages, undefined); + const result = counts(messages, neverRead); assert.equal(result.get("root1"), 2); assert.equal(result.get("root2"), 1); }); + +// --- LP4 Case 1: orphaned subtree from a broken parent chain rolls up --- +// +// The roll-up groups each reply under its `rootId` (buildRepliesByRootId). +// Pagination / load windows can drop an intermediate ancestor, severing the +// parent chain — but every reply still carries its true rootId (the `root` +// e-tag travels with the event, getThreadReference), so a deep reply tallies at +// its real root even when the middle ancestor is absent from the loaded array. +// +// These two tests pin the exact trigger — a missing middle ancestor — and the +// orphan-immune roll-up that counts it anyway. The third is the full-chain +// control, identical to the broken-chain result by construction. + +test("computeThreadBadgeCounts_brokenParentChain_orphanedReplyRollsUpToRoot", () => { + // Full thread is root -> a -> b -> c, but intermediate ancestor `b` is NOT in + // the loaded array (unloaded by the timeline window). `c` is genuinely unread + // and carries rootId "root", so the root-keyed roll-up tallies both `a` and + // `c`: count 2, the same as if the chain were intact. The old parentId-walk + // orphaned `c` (keyed under absent "b") and undercounted to 1. + const loaded = [ + msg("root", null), + msg("a", "root"), + // msg("b", "a") — intentionally absent: unloaded intermediate ancestor. + msg("c", "b", 100, "author", "root"), + ]; + assert.equal(counts(loaded, neverRead).get("root"), 2); +}); + +test("computeThreadBadgeCounts_brokenParentChain_orphanedSoleReply_showsBadge", () => { + // Sharper form: root's ONLY unread content is the deep reply `c`, whose + // intermediate ancestor `b` is unloaded. `c` carries rootId "root", so the + // roll-up still groups it under root and the badge shows count 1. The old + // parentId-walk produced NO badge at all (root unreachable to its sole reply). + const loaded = [ + msg("root", null), + // msg("b", "root") — intentionally absent: unloaded intermediate ancestor. + msg("c", "b", 100, "author", "root"), + ]; + assert.equal(counts(loaded, neverRead).get("root"), 1); +}); + +test("computeThreadBadgeCounts_fullParentChain_orphanRollsUp_DESIRED", () => { + // Control: the SAME thread with the intermediate ancestor `b` present. The + // chain root -> a -> b -> c is intact and every descendant carries rootId + // "root", so the root badge counts 3 — the baseline the broken-chain cases + // above match by rolling severed orphans up by rootId. + const loaded = [ + msg("root", null), + msg("a", "root"), + msg("b", "a", 100, "author", "root"), + msg("c", "b", 100, "author", "root"), + ]; + assert.equal(counts(loaded, neverRead).get("root"), 3); +}); diff --git a/desktop/src/features/channels/lib/threadBadgeCounts.ts b/desktop/src/features/channels/lib/threadBadgeCounts.ts index fb78c84267..a7933c6e18 100644 --- a/desktop/src/features/channels/lib/threadBadgeCounts.ts +++ b/desktop/src/features/channels/lib/threadBadgeCounts.ts @@ -1,72 +1,52 @@ import { computeThreadUnreadMarker } from "@/features/messages/lib/unreadMarker"; import type { TimelineMessage } from "@/features/messages/types"; -/** - * All reply messages in a root's subtree — direct children plus every deeper - * descendant, walked through the direct-replies adjacency map. A reply-to-a- - * reply must count toward the root's badge, so the badge tally needs the whole - * subtree rather than the root's direct children alone. - * - * Terminates without a visited-set: buildDirectRepliesByParentId places each - * message under exactly one parent key, and the only caller seeds the walk from - * true roots (parentId === null), so a node in a malformed parent cycle — whose - * members all key off each other, never off a root — is unreachable from any - * root's bucket. Seeding from a non-root id, or a builder that filed one node - * under two keys, would break that invariant. - */ -function collectSubtreeReplies( - rootId: string, - directRepliesByParentId: ReadonlyMap, -): TimelineMessage[] { - const replies: TimelineMessage[] = []; - const pending = [...(directRepliesByParentId.get(rootId) ?? [])]; - while (pending.length > 0) { - const reply = pending.pop(); - if (!reply) continue; - replies.push(reply); - pending.push(...(directRepliesByParentId.get(reply.id) ?? [])); - } - return replies; -} - /** * Per-thread unread reply counts for the summary rows in the main timeline. * * Counts are computed only for threads the user has notification interest in - * (`isNotified`) and measured against the per-root frontier snapshot rather - * than the live marker, so badges stay stable for the session (see - * nextThreadBadgeFrontier for the snapshot-advance-on-read rationale). The - * count spans the root's WHOLE subtree, so a reply nested under another reply - * still tallies toward the root's badge. + * (`isNotified`). The count spans the root's WHOLE subtree, so a reply nested + * under another reply still tallies toward the root's badge. + * + * Subtree membership is keyed on each reply's `rootId` rather than walked + * through the parent chain: a reply whose intermediate ancestor is absent from + * the loaded window still carries its true rootId (getThreadReference), so it + * rolls up to the root the parent-chain walk could never reach. For an intact + * chain every descendant carries the root's rootId, so the tally is identical + * to the old adjacency walk. Each reply has exactly one rootId, so it is + * counted once and a malformed parent cycle keys off no root. + * + * Unread is decided per-reply against `getReadAt` (LP4 v3): each reply lights + * iff `createdAt > effective(msg:)`, so reading one reply never clears + * another and a collapsed-branch reply keeps its badge until revealed. * * @param messages Top-level timeline entries in chronological order. - * @param directRepliesByParentId Direct replies keyed by parent id, walked to - * collect each root's full descendant subtree. - * @param frontiers Per-root read frontier in unix seconds, or null/undefined - * when the thread was never read (every reply counts unread). + * @param repliesByRootId Replies grouped by their resolved thread root id. + * @param getReadAt Per-message read resolver; `null` means never read. * @param isNotified Whether a thread root is one the user is notified for. * @param currentPubkey Replies authored by this pubkey never count as unread. + * @param isForcedUnread Session-local OR-overlay: a reply forced unread this + * session counts regardless of its marker (per-message mark-unread). */ export function computeThreadBadgeCounts( messages: TimelineMessage[], - directRepliesByParentId: ReadonlyMap, - frontiers: ReadonlyMap | undefined, + repliesByRootId: ReadonlyMap, + getReadAt: (messageId: string) => number | null, isNotified: (rootId: string) => boolean, currentPubkey?: string, + isForcedUnread: (messageId: string) => boolean = () => false, ): Map { const counts = new Map(); for (const message of messages) { if (message.parentId) continue; if (!isNotified(message.id)) continue; - const subtreeReplies = collectSubtreeReplies( - message.id, - directRepliesByParentId, - ); - if (subtreeReplies.length === 0) continue; + const subtreeReplies = repliesByRootId.get(message.id); + if (!subtreeReplies || subtreeReplies.length === 0) continue; const { unreadCount } = computeThreadUnreadMarker( subtreeReplies, - frontiers?.get(message.id) ?? null, + getReadAt, currentPubkey, + isForcedUnread, ); if (unreadCount > 0) { counts.set(message.id, unreadCount); diff --git a/desktop/src/features/channels/lib/threadBadgeFrontier.test.mjs b/desktop/src/features/channels/lib/threadBadgeFrontier.test.mjs deleted file mode 100644 index 89817f2c88..0000000000 --- a/desktop/src/features/channels/lib/threadBadgeFrontier.test.mjs +++ /dev/null @@ -1,98 +0,0 @@ -import assert from "node:assert/strict"; -import test from "node:test"; - -import { nextThreadBadgeFrontier } from "./threadBadgeFrontier.ts"; -import { seedThreadBadgeFrontiers } from "./threadBadgeFrontier.ts"; -import { buildDirectRepliesByParentId } from "./subtreeCreatedAt.ts"; - -const msg = (id, parentId) => ({ id, parentId }); -const seedAll = () => true; -const seed = (frontiers, messages, isNotified, getReadAt) => - seedThreadBadgeFrontiers( - frontiers, - messages, - buildDirectRepliesByParentId(messages), - isNotified, - getReadAt, - ); - -test("nextThreadBadgeFrontier_unseededNullMarker_seedsNull", () => { - // Thread never read: snapshot seeds to null (everything unread). - assert.equal(nextThreadBadgeFrontier(undefined, null), null); -}); - -test("nextThreadBadgeFrontier_unseededWithMarker_seedsToMarker", () => { - assert.equal(nextThreadBadgeFrontier(undefined, 100), 100); -}); - -test("nextThreadBadgeFrontier_readAdvancesMarker_advancesSnapshot", () => { - // Snapshot frozen at open (null), user reads → live marker 200 → badge clears. - assert.equal(nextThreadBadgeFrontier(null, 200), 200); -}); - -test("nextThreadBadgeFrontier_markerNewerThanStored_advances", () => { - assert.equal(nextThreadBadgeFrontier(100, 250), 250); -}); - -test("nextThreadBadgeFrontier_markerOlderThanStored_keepsStored", () => { - // Monotonic: a stale lower marker never lowers the snapshot. - assert.equal(nextThreadBadgeFrontier(250, 100), 250); -}); - -test("nextThreadBadgeFrontier_markerNullAfterSeed_keepsStored", () => { - // Live marker reads null (never read) but snapshot already advanced — hold. - assert.equal(nextThreadBadgeFrontier(150, null), 150); -}); - -test("nextThreadBadgeFrontier_markerEqualsStored_unchanged", () => { - assert.equal(nextThreadBadgeFrontier(150, 150), 150); -}); - -test("nextThreadBadgeFrontier_storedNullMarkerZero_advancesToZero", () => { - // Zero is a valid frontier (epoch); null is strictly lower than any number. - assert.equal(nextThreadBadgeFrontier(null, 0), 0); -}); - -test("seedThreadBadgeFrontiers_threadWithReplies_seedsToMarker", () => { - const frontiers = new Map(); - const messages = [msg("root", null), msg("r1", "root")]; - seed(frontiers, messages, seedAll, (id) => (id === "root" ? 100 : null)); - assert.equal(frontiers.get("root"), 100); -}); - -test("seedThreadBadgeFrontiers_threadWithoutReplies_skipped", () => { - const frontiers = new Map(); - seed(frontiers, [msg("root", null)], seedAll, () => 100); - assert.equal(frontiers.has("root"), false); -}); - -test("seedThreadBadgeFrontiers_notNotified_skipped", () => { - const frontiers = new Map(); - const messages = [msg("root", null), msg("r1", "root")]; - seed( - frontiers, - messages, - () => false, - () => 100, - ); - assert.equal(frontiers.has("root"), false); -}); - -test("seedThreadBadgeFrontiers_replyEntry_neverSeeded", () => { - // A reply is never a badge root even if its id collides with a notified set. - const frontiers = new Map(); - const messages = [msg("r1", "root"), msg("r2", "root")]; - seed(frontiers, messages, seedAll, () => 100); - assert.equal(frontiers.size, 0); -}); - -test("seedThreadBadgeFrontiers_reseed_advancesMonotonically", () => { - const frontiers = new Map([["root", 100]]); - const messages = [msg("root", null), msg("r1", "root")]; - // Re-render after the live marker advanced to 250 on read. - seed(frontiers, messages, seedAll, () => 250); - assert.equal(frontiers.get("root"), 250); - // A stale lower marker never lowers an already-advanced snapshot. - seed(frontiers, messages, seedAll, () => 100); - assert.equal(frontiers.get("root"), 250); -}); diff --git a/desktop/src/features/channels/lib/threadBadgeFrontier.ts b/desktop/src/features/channels/lib/threadBadgeFrontier.ts deleted file mode 100644 index c739594d1c..0000000000 --- a/desktop/src/features/channels/lib/threadBadgeFrontier.ts +++ /dev/null @@ -1,57 +0,0 @@ -import type { TimelineMessage } from "@/features/messages/types"; - -// Decide the next value for a thread's badge frontier snapshot. The snapshot is -// seeded once at channel-open (reflecting "what was unread on open") and then -// advanced monotonically toward the live thread read marker as the user reads, -// so the badge clears after a read without waiting for channel re-entry. -// -// The advance target is ALWAYS the live marker (what the user actually -// consumed), never "latest reply": a subsequent reply newer than the marker -// re-raises the badge, and a collapsed-branch reply the marker never covered -// stays unread. Monotonic `Math.max` guards against a stale lower marker. -// -// Returns the value the snapshot should hold: -// - `stored === undefined` (unseeded): seed to the live marker. -// - otherwise: the greater of the stored snapshot and the live marker, where -// `null` (never read) is the lowest possible frontier. -export function nextThreadBadgeFrontier( - stored: number | null | undefined, - liveMarker: number | null, -): number | null { - if (stored === undefined) { - return liveMarker; - } - if (liveMarker === null) { - return stored; - } - if (stored === null) { - return liveMarker; - } - return Math.max(stored, liveMarker); -} - -// Seed/advance the per-root badge frontier snapshots for one channel, in place. -// Captures only top-level notified threads that have replies; each entry is -// seeded once at open then advanced toward the live marker on subsequent reads -// (see nextThreadBadgeFrontier). Called during render so snapshots reflect -// "what was unread on open," matching the openFrontierRef pattern. -export function seedThreadBadgeFrontiers( - channelFrontiers: Map, - messages: TimelineMessage[], - directRepliesByParentId: ReadonlyMap, - isNotified: (rootId: string) => boolean, - getReadAt: (rootId: string) => number | null, -): void { - for (const message of messages) { - if (message.parentId) continue; - if (!isNotified(message.id)) continue; - if (!directRepliesByParentId.has(message.id)) continue; - channelFrontiers.set( - message.id, - nextThreadBadgeFrontier( - channelFrontiers.get(message.id), - getReadAt(message.id), - ), - ); - } -} diff --git a/desktop/src/features/channels/lib/threadBadgeInvariants.test.mjs b/desktop/src/features/channels/lib/threadBadgeInvariants.test.mjs new file mode 100644 index 0000000000..c08bbcd472 --- /dev/null +++ b/desktop/src/features/channels/lib/threadBadgeInvariants.test.mjs @@ -0,0 +1,156 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { computeThreadBadgeCounts } from "./threadBadgeCounts.ts"; +import { buildRepliesByRootId } from "./subtreeCreatedAt.ts"; + +// LP4 v3 characterization invariants for thread-unread badges. +// +// Each invariant pins a contract the per-message badge pipeline holds. They are +// the observable behaviors Will depends on: a change that breaks any one of +// (a)-(g) has regressed behavior, not just refactored an internal path. +// +// The model is per-message read markers (`msg:`) read through a resolver, +// not a per-thread-root frontier snapshot. `getReadAt(id)` returns the +// effective read time for a reply (`null` = never read); a reply counts unread +// iff `createdAt > getReadAt(id)`. Reading one reply's marker never touches +// another's — independence is structural, not enforced by a separate seed. +// +// Fixtures carry `rootId` alongside `parentId` so the root-keyed roll-up stays +// falsifiable: a rootId-keyed implementation that ignored parentId, or the +// reverse, must still satisfy the same observable counts here. + +const msg = (id, parentId, rootId, createdAt = 100, pubkey = "author") => ({ + id, + parentId, + rootId: rootId ?? parentId ?? id, + createdAt, + pubkey, +}); + +const notifiedAll = () => true; +const neverRead = () => null; + +// A uniform read-line per thread root, applied to every reply resolving to that +// root by rootId. Translates the legacy "frontier covers part of a subtree" +// intents into the per-message resolver without a single global line. +function readLineByRoot(messages, frontiersByRoot) { + const lineByMessageId = new Map(); + for (const message of messages) { + const root = message.rootId ?? message.parentId ?? message.id; + const line = frontiersByRoot.get(root); + if (line !== undefined) lineByMessageId.set(message.id, line); + } + return (id) => lineByMessageId.get(id) ?? null; +} + +const counts = (messages, getReadAt, isNotified = notifiedAll, currentPubkey) => + computeThreadBadgeCounts( + messages, + buildRepliesByRootId(messages), + getReadAt, + isNotified, + currentPubkey, + ); + +// (a) A root's badge counts EVERY descendant in its subtree, at any depth, not +// just direct replies. The whole connected subtree rolls up to one badge. +test("invariant_a_subtreeRollsUpToOneRootBadge", () => { + const messages = [ + msg("root", null, "root"), + msg("a", "root", "root"), + msg("b", "a", "root"), + msg("c", "b", "root"), + ]; + const result = counts(messages, neverRead); + assert.equal(result.get("root"), 3); + assert.equal(result.size, 1); +}); + +// (b) Only roots the user is notified for produce a badge; an un-notified +// thread with unread replies is silent. +test("invariant_b_onlyNotifiedRootsBadge", () => { + const messages = [ + msg("root1", null, "root1"), + msg("a", "root1", "root1"), + msg("root2", null, "root2"), + msg("b", "root2", "root2"), + ]; + const result = counts(messages, neverRead, (id) => id === "root1"); + assert.equal(result.get("root1"), 1); + assert.equal(result.has("root2"), false); +}); + +// (c) The marker is the read boundary: replies at or below it are read and do +// NOT count; only replies strictly newer than the marker raise the badge. +test("invariant_c_readMarkerExcludesReadReplies", () => { + const messages = [ + msg("root", null, "root", 50), + msg("read", "root", "root", 100), + msg("unread", "root", "root", 200), + ]; + const readAt = readLineByRoot(messages, new Map([["root", 100]])); + assert.equal(counts(messages, readAt).get("root"), 1); +}); + +// (d) The current user's own replies never count as unread, at any depth. +test("invariant_d_selfAuthoredRepliesNeverUnread", () => { + const messages = [ + msg("root", null, "root", 50, "other"), + msg("a", "root", "root", 100, "other"), + msg("mine", "a", "root", 200, "me"), + ]; + assert.equal(counts(messages, neverRead, notifiedAll, "me").get("root"), 1); +}); + +// (e) A notified root with no unread content produces NO entry — absence, not a +// zero. (The badge UI keys off presence; a 0 entry would render a phantom dot.) +test("invariant_e_noUnreadMeansNoEntry", () => { + const messages = [ + msg("root", null, "root", 50), + msg("a", "root", "root", 100), + ]; + const readAt = readLineByRoot(messages, new Map([["root", 100]])); + const result = counts(messages, readAt); + assert.equal(result.has("root"), false); +}); + +// (f) PER-MESSAGE INDEPENDENCE — reading one reply's marker never clears +// another's. This is the structural fix for the original Issue 2 (an ancestor +// read covering a descendant): each reply is judged against its OWN marker, so +// reading the older reply leaves the newer one lit. A resolver that folded +// reply→reply, or keyed all replies to one shared line, would fail here. +test("invariant_f_readOneReplyLeavesOthersUnread", () => { + const messages = [ + msg("root", null, "root", 50), + msg("older", "root", "root", 100), + msg("newer", "root", "root", 200), + ]; + // Only `older` is read (marker at its own timestamp); `newer` untouched. + const readAt = (id) => (id === "older" ? 100 : null); + assert.equal(counts(messages, readAt).get("root"), 1); +}); + +// (g) FALSIFIABLE LOCK — two distinct roots keep INDEPENDENT badges; reading +// one never collapses the other (the original Face-2 cross-thread bug). root1 +// read through its newest reply (badge clears), root2 unread. +test("invariant_g_distinctRootsDoNotCollapse", () => { + const messages = [ + msg("root1", null, "root1", 10), + msg("r1reply", "root1", "root1", 100), + msg("root2", null, "root2", 20), + msg("r2reply", "root2", "root2", 200), + ]; + // root1 read through its reply (marker 100); root2 never read. + const readAt = readLineByRoot( + messages, + new Map([ + ["root1", 100], + ["root2", null], + ]), + ); + const result = counts(messages, readAt); + assert.equal(result.has("root1"), false); // root1 fully read — no badge + assert.equal(result.get("root2"), 1); // root2 independently still unread + assert.equal(result.size, 1); +}); diff --git a/desktop/src/features/channels/lib/threadReplyUnreadCounts.test.mjs b/desktop/src/features/channels/lib/threadReplyUnreadCounts.test.mjs index 0c3c762449..0788a584bb 100644 --- a/desktop/src/features/channels/lib/threadReplyUnreadCounts.test.mjs +++ b/desktop/src/features/channels/lib/threadReplyUnreadCounts.test.mjs @@ -23,15 +23,21 @@ function fixture() { const ROOT_SUBTREE = ["a", "b", "a1", "b1", "b2"]; +// LP4 v3: the panel badge reads a per-message resolver, not an open-time +// frontier snapshot, and there is no separate expanded-subtree gate — the +// per-message marker already distinguishes a read parent from its still-unread +// descendant. A uniform read-line at `seconds` (or null = never read) +// reproduces the legacy boundary cases. +const uniformReadAt = (seconds) => () => seconds; + test("computeThreadReplyUnreadCounts_collapsedBranch_countsUnreadDescendants", () => { - // Frontier 350: a1(400), b1(500), b2(600) are unread. + // Read-line 350: a1(400), b1(500), b2(600) are unread. const counts = computeThreadReplyUnreadCounts({ timelineMessages: fixture(), subtreeReplyIds: ROOT_SUBTREE, visibleReplyIds: ["a", "b"], expandedReplyIds: new Set(), - expandedSubtreeReplyIds: new Set(), - frontierSeconds: 350, + getReadAt: uniformReadAt(350), }); assert.equal(counts.get("a"), 1); // a1 assert.equal(counts.get("b"), 2); // b1, b2 @@ -43,66 +49,66 @@ test("computeThreadReplyUnreadCounts_expandedBranch_omitsBadge", () => { subtreeReplyIds: ROOT_SUBTREE, visibleReplyIds: ["a", "b"], expandedReplyIds: new Set(["b"]), - expandedSubtreeReplyIds: new Set(["b1", "b2"]), - frontierSeconds: 350, + getReadAt: uniformReadAt(350), }); assert.equal(counts.get("a"), 1); + // b renders its children inline, so it carries no summary badge. assert.equal(counts.has("b"), false); }); -test("computeThreadReplyUnreadCounts_expandedBranch_revealedChildNoStaleBadge", () => { - // Expand b: mark-read-on-expand reads b's whole subtree, and the panel now - // reveals collapsed child b1 (descendant b2 still unread vs the open-time - // frontier). b1 must carry NO badge — the expanded subtree is excluded. +test("computeThreadReplyUnreadCounts_revealedCollapsedChild_keepsOwnSubtreeBadge", () => { + // v3 open-at-level: expanding b reveals direct child b1 but marks only the + // revealed set read — it does NOT clear b1's still-collapsed descendant b2. + // The per-message marker leaves b2 unread, so the now-visible (collapsed) b1 + // carries a badge of 1. This is the deliberate reversal of the #1118 + // whole-subtree-on-open behavior. const counts = computeThreadReplyUnreadCounts({ timelineMessages: fixture(), subtreeReplyIds: ROOT_SUBTREE, visibleReplyIds: ["a", "b", "b1"], expandedReplyIds: new Set(["b"]), - expandedSubtreeReplyIds: new Set(["b1", "b2"]), - frontierSeconds: 350, + // b and its revealed direct child b1 are read; b2 (collapsed under b1) + // is still unread. + getReadAt: (id) => (id === "b1" || id === "b" ? 1000 : 350), }); assert.equal(counts.get("a"), 1); - assert.equal(counts.has("b"), false); - assert.equal(counts.has("b1"), false); + assert.equal(counts.has("b"), false); // expanded -> no summary badge + assert.equal(counts.get("b1"), 1); // collapsed b1 keeps its b2 badge }); test("computeThreadReplyUnreadCounts_descendantsButNoneUnread_noBadge", () => { - // Frontier 1000: nothing is newer, so no unread descendants anywhere. + // Read-line 1000: nothing is newer, so no unread descendants anywhere. const counts = computeThreadReplyUnreadCounts({ timelineMessages: fixture(), subtreeReplyIds: ROOT_SUBTREE, visibleReplyIds: ["a", "b"], expandedReplyIds: new Set(), - expandedSubtreeReplyIds: new Set(), - frontierSeconds: 1000, + getReadAt: uniformReadAt(1000), }); assert.equal(counts.size, 0); }); -test("computeThreadReplyUnreadCounts_nullFrontier_allDescendantsUnread", () => { +test("computeThreadReplyUnreadCounts_neverRead_allDescendantsUnread", () => { const counts = computeThreadReplyUnreadCounts({ timelineMessages: fixture(), subtreeReplyIds: ROOT_SUBTREE, visibleReplyIds: ["a", "b"], expandedReplyIds: new Set(), - expandedSubtreeReplyIds: new Set(), - frontierSeconds: null, + getReadAt: uniformReadAt(null), }); assert.equal(counts.get("a"), 1); // a1 assert.equal(counts.get("b"), 2); // b1, b2 }); test("computeThreadReplyUnreadCounts_otherThreadReply_notCounted", () => { - // other1(800) is unread by frontier but outside root's subtree — its - // ancestor "other" is not a visible row here and must never be keyed. + // other1(800) is unread but outside root's subtree — its ancestor "other" + // is not in subtreeReplyIds and must never be keyed. const counts = computeThreadReplyUnreadCounts({ timelineMessages: fixture(), subtreeReplyIds: ROOT_SUBTREE, visibleReplyIds: ["a", "b", "other"], expandedReplyIds: new Set(), - expandedSubtreeReplyIds: new Set(), - frontierSeconds: 350, + getReadAt: uniformReadAt(350), }); assert.equal(counts.has("other"), false); }); @@ -114,8 +120,7 @@ test("computeThreadReplyUnreadCounts_onlyVisibleRowsKeyed", () => { subtreeReplyIds: ROOT_SUBTREE, visibleReplyIds: ["a"], expandedReplyIds: new Set(), - expandedSubtreeReplyIds: new Set(), - frontierSeconds: 350, + getReadAt: uniformReadAt(350), }); assert.equal(counts.get("a"), 1); assert.equal(counts.has("b"), false); @@ -137,39 +142,38 @@ test("computeThreadReplyUnreadCounts_selfAuthored_skipsOwnReplies", () => { subtreeReplyIds: ["a", "b", "a1", "b1", "b2"], visibleReplyIds: ["a", "b"], expandedReplyIds: new Set(), - expandedSubtreeReplyIds: new Set(), - frontierSeconds: 350, + getReadAt: uniformReadAt(350), currentPubkey: "me", }); assert.equal(counts.has("a"), false); // a1 is self-authored, so 0 unread assert.equal(counts.get("b"), 2); // b1, b2 are by "other" }); -test("computeThreadReplyUnreadCounts_openTimeSnapshot_survivesChannelMarkRead", () => { - // Regression (Fix 1): the in-panel badge must reflect "what was unread when - // the thread opened", NOT the live root marker. On channel-open - // markChannelRead advances the channel marker to the newest TOP-LEVEL - // message; effective(thread) = max(thread_own, channel_marker), so the live - // value can jump PAST the nested replies and zero every badge. Passing the - // open-time snapshot (frontier 350, captured before the advance) keeps the - // badges; passing the post-advance live value (650, past b2(600)) loses them. - const args = { +test("computeThreadReplyUnreadCounts_perMessageMarkers_readOneDescendantKeepsRest", () => { + // The defining per-message case: marking b1 read leaves sibling-line b2 + // unread independently. b's badge counts only the still-unread b2. + const counts = computeThreadReplyUnreadCounts({ timelineMessages: fixture(), subtreeReplyIds: ROOT_SUBTREE, visibleReplyIds: ["a", "b"], expandedReplyIds: new Set(), - expandedSubtreeReplyIds: new Set(), - }; - const snapshotCounts = computeThreadReplyUnreadCounts({ - ...args, - frontierSeconds: 350, + getReadAt: (id) => (id === "b1" || id === "a1" ? 1000 : 350), }); - assert.equal(snapshotCounts.get("a"), 1); - assert.equal(snapshotCounts.get("b"), 2); + assert.equal(counts.has("a"), false); // a1 read + assert.equal(counts.get("b"), 1); // b1 read, only b2 remains +}); - const liveAdvancedCounts = computeThreadReplyUnreadCounts({ - ...args, - frontierSeconds: 650, +test("computeThreadReplyUnreadCounts_forcedUnread_relightsReadDescendant", () => { + // Session-local mark-unread forces a1 (read by its marker) back to unread, + // so collapsed parent a regains its badge. + const counts = computeThreadReplyUnreadCounts({ + timelineMessages: fixture(), + subtreeReplyIds: ROOT_SUBTREE, + visibleReplyIds: ["a", "b"], + expandedReplyIds: new Set(), + getReadAt: uniformReadAt(1000), // everything read by marker + isForcedUnread: (id) => id === "a1", }); - assert.equal(liveAdvancedCounts.size, 0); + assert.equal(counts.get("a"), 1); // a1 forced unread + assert.equal(counts.has("b"), false); // b subtree still read }); diff --git a/desktop/src/features/channels/lib/threadReplyUnreadCounts.ts b/desktop/src/features/channels/lib/threadReplyUnreadCounts.ts index 2c7083deb5..987d2aacda 100644 --- a/desktop/src/features/channels/lib/threadReplyUnreadCounts.ts +++ b/desktop/src/features/channels/lib/threadReplyUnreadCounts.ts @@ -4,54 +4,56 @@ import type { TimelineMessage } from "@/features/messages/types"; /** * Per-row subtree unread counts for the in-panel thread summary rows. A * collapsed branch's badge counts unread replies anywhere beneath it; the - * count is omitted for expanded branches (suppress-on-expand happens here, - * upstream of the panel, so the panel needs no gate) and for rows with zero - * unread descendants (no "0" badge). + * count is omitted for expanded branches (their children render inline, so no + * summary badge) and for rows with zero unread descendants (no "0" badge). * - * Unread is measured against the open-time frontier snapshot — the same - * boundary the in-thread divider uses — so the mark-read-on-open advance does - * not zero the badges the instant the panel opens. A null frontier (thread - * never read) treats every subtree reply as unread. + * Unread is decided per-reply against `getReadAt` (LP4 v3): each reply counts + * iff `createdAt > effective(msg:)`. Expanding a branch marks only the + * revealed (direct-child) set read, so a collapsed grandchild keeps its badge + * until it too is revealed — no separate expanded-subtree gate is needed, + * because the per-message marker already distinguishes a read parent from its + * still-unread descendant. A `null` marker (reply never read) counts as unread. * * @param subtreeReplyIds Descendant reply ids of the open thread head. Scoping - * the unread set to this subtree keeps one thread's frontier from marking - * replies that belong to a different thread. + * the unread set to this subtree keeps replies in a different thread from + * ever being counted here. * @param visibleReplyIds Ids of the rows actually rendered in the panel; only * these are keyed, keeping the map consistent with row presence. - * @param expandedSubtreeReplyIds Reply ids beneath any expanded row. Expanding - * a branch persistently marks its whole subtree read (mark-read-on-expand), - * so those replies are dropped from the unread set — otherwise a revealed - * child would carry a stale badge for a reply the same gesture just read. + * @param expandedReplyIds Ids of rows whose children are rendered inline; these + * rows carry no summary badge. + * @param getReadAt Per-message read resolver; `null` means never read. + * @param isForcedUnread Session-local OR-overlay: a reply forced unread this + * session counts regardless of its marker (per-message mark-unread). */ export function computeThreadReplyUnreadCounts(params: { timelineMessages: TimelineMessage[]; subtreeReplyIds: Iterable; visibleReplyIds: Iterable; expandedReplyIds: ReadonlySet; - expandedSubtreeReplyIds: ReadonlySet; - frontierSeconds: number | null; + getReadAt: (messageId: string) => number | null; currentPubkey?: string; + isForcedUnread?: (messageId: string) => boolean; }): Map { const { timelineMessages, subtreeReplyIds, visibleReplyIds, expandedReplyIds, - expandedSubtreeReplyIds, - frontierSeconds, + getReadAt, currentPubkey, + isForcedUnread = () => false, } = params; const subtree = new Set(subtreeReplyIds); const unreadReplyIds = new Set( timelineMessages - .filter( - (message) => - subtree.has(message.id) && - !expandedSubtreeReplyIds.has(message.id) && - (!currentPubkey || message.pubkey !== currentPubkey) && - (frontierSeconds === null || message.createdAt > frontierSeconds), - ) + .filter((message) => { + if (!subtree.has(message.id)) return false; + if (currentPubkey && message.pubkey === currentPubkey) return false; + if (isForcedUnread(message.id)) return true; + const readAt = getReadAt(message.id); + return readAt === null || message.createdAt > readAt; + }) .map((message) => message.id), ); diff --git a/desktop/src/features/channels/readState/readStateFormat.test.mjs b/desktop/src/features/channels/readState/readStateFormat.test.mjs new file mode 100644 index 0000000000..248692a91c --- /dev/null +++ b/desktop/src/features/channels/readState/readStateFormat.test.mjs @@ -0,0 +1,33 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { isMsgContextKey, msgContextKey } from "./readStateFormat.ts"; + +test("msgContextKey_prefixesId_returnsMsgKey", () => { + assert.equal(msgContextKey("abc123"), "msg:abc123"); +}); + +test("isMsgContextKey_wellFormedKey_returnsTrue", () => { + assert.equal(isMsgContextKey("msg:abc123"), true); +}); + +test("isMsgContextKey_threadKey_returnsFalse", () => { + assert.equal(isMsgContextKey(`thread:${"a".repeat(64)}`), false); +}); + +test("isMsgContextKey_channelKey_returnsFalse", () => { + assert.equal(isMsgContextKey("channel-1"), false); +}); + +test("isMsgContextKey_emptyId_returnsFalse", () => { + assert.equal(isMsgContextKey("msg:"), false); +}); + +test("isMsgContextKey_msgPrefixWrappingThreadKey_returnsFalse", () => { + // A thread key accidentally re-prefixed must not pass as a message key. + assert.equal(isMsgContextKey(`msg:thread:${"a".repeat(64)}`), false); +}); + +test("msgContextKey_output_roundTripsThroughValidator", () => { + assert.equal(isMsgContextKey(msgContextKey("event-id")), true); +}); diff --git a/desktop/src/features/channels/readState/readStateFormat.ts b/desktop/src/features/channels/readState/readStateFormat.ts index 6ce0d69c59..4a37d18330 100644 --- a/desktop/src/features/channels/readState/readStateFormat.ts +++ b/desktop/src/features/channels/readState/readStateFormat.ts @@ -8,7 +8,28 @@ export const READ_STATE_D_TAG_PREFIX = "read-state:"; export const READ_STATE_FETCH_LIMIT = 500; export const READ_STATE_HORIZON_SECONDS = 7 * 24 * 60 * 60; -const MAX_CONTEXTS = 10_000; +export const MAX_CONTEXTS = 10_000; + +// Context-key prefix for a per-MESSAGE read marker (LP4 v3). One grow-only +// marker per reply id; the badge predicate reads effective("msg:") live so +// reading an ancestor never covers a descendant (Issue 2 by construction). +// Distinct from THREAD_PREFIX so the parent resolver and eviction can tell the +// two key families apart. +export const MSG_PREFIX = "msg:"; +export const THREAD_PREFIX = "thread:"; + +export function msgContextKey(messageId: string): string { + return `${MSG_PREFIX}${messageId}`; +} + +// A well-formed per-message context key: the msg: prefix with a non-empty id +// that does NOT itself start with thread: (guards against a thread key being +// mistaken for, or double-prefixed into, a message key). +export function isMsgContextKey(value: string): value is `msg:${string}` { + if (!value.startsWith(MSG_PREFIX)) return false; + const id = value.slice(MSG_PREFIX.length); + return id.length > 0 && !id.startsWith(THREAD_PREFIX); +} export function localReadStateKey(pubkey: string): string { return `buzz.channel-read-state.v2:${pubkey}`; diff --git a/desktop/src/features/channels/ui/ChannelPane.tsx b/desktop/src/features/channels/ui/ChannelPane.tsx index d910584e7e..fb611cbc44 100644 --- a/desktop/src/features/channels/ui/ChannelPane.tsx +++ b/desktop/src/features/channels/ui/ChannelPane.tsx @@ -99,6 +99,7 @@ type ChannelPaneProps = { onEdit?: (message: TimelineMessage) => void; onEditSave?: (content: string, mediaTags?: string[][]) => Promise; onMarkUnread?: (message: TimelineMessage) => void; + onMarkRead?: (message: TimelineMessage) => void; onExpandThreadReplies: (message: TimelineMessage) => void; onJoinChannel?: () => Promise; onOpenAgentSession: (pubkey: string) => void; @@ -205,6 +206,7 @@ export const ChannelPane = React.memo(function ChannelPane({ onEditSave, onFollowThread, onMarkUnread, + onMarkRead, onExpandThreadReplies, onJoinChannel, onOpenAgentSession, @@ -678,6 +680,7 @@ export const ChannelPane = React.memo(function ChannelPane({ onDelete={onDelete} onEdit={onEdit} onMarkUnread={onMarkUnread} + onMarkRead={onMarkRead} onReply={activeChannel?.archivedAt ? undefined : onOpenThread} channelName={activeChannel?.name} channelType={activeChannel?.channelType ?? null} @@ -818,6 +821,7 @@ export const ChannelPane = React.memo(function ChannelPane({ onEditSave={onEditSave} onFollowThread={onFollowThread} onMarkUnread={onMarkUnread} + onMarkRead={onMarkRead} onExpandReplies={onExpandThreadReplies} onSelectReplyTarget={onSelectThreadReplyTarget} onSend={onSendThreadReply} diff --git a/desktop/src/features/channels/ui/ChannelScreen.tsx b/desktop/src/features/channels/ui/ChannelScreen.tsx index f63384ff90..1d06ff8a74 100644 --- a/desktop/src/features/channels/ui/ChannelScreen.tsx +++ b/desktop/src/features/channels/ui/ChannelScreen.tsx @@ -6,6 +6,10 @@ import { useChannelMembersQuery, useJoinChannelMutation, } from "@/features/channels/hooks"; +import { + MSG_PREFIX, + THREAD_PREFIX, +} from "@/features/channels/readState/readStateFormat"; import { ChannelScreenEmptyState } from "@/features/channels/ui/ChannelScreenEmptyState"; import { ChannelScreenHeader } from "@/features/channels/ui/ChannelScreenHeader"; import { @@ -42,6 +46,7 @@ import { import { useFetchOlderMessages } from "@/features/messages/useFetchOlderMessages"; import { useLoadMissingAncestors } from "@/features/messages/useLoadMissingAncestors"; import { useChannelTyping } from "@/features/messages/useChannelTyping"; +import type { TimelineMessage } from "@/features/messages/types"; import { useUsersBatchQuery } from "@/features/profile/hooks"; import { mergeCurrentProfileIntoLookup } from "@/features/profile/lib/identity"; import type { RespondToMode } from "@/shared/api/types"; @@ -86,8 +91,8 @@ export function ChannelScreen({ markChannelRead, markChannelUnread, getChannelReadAt, - getThreadReadAt, - markThreadRead, + getMessageReadAt, + markMessageRead, setContextParentResolver, openCreateChannel, openChannelManagement, @@ -204,19 +209,25 @@ export function ChannelScreen({ // thread itself is read. markChannelRead(activeChannelId, activeReadAt, { topLevelOnly: true }); }, [activeChannel?.isMember, activeChannelId, activeReadAt, markChannelRead]); - // Install the NIP-RS parent resolver: every `thread:` context evaluated - // while this channel is active belongs to it (getThreadReadAt is only ever - // called on the active channel's timeline messages), so the parent is always - // the active channel. Non-thread keys (channels) have no parent → null, which - // degrades effective() to the own term. Cleared on channel leave / unmount so - // a stale channel id never becomes the parent of another channel's threads. + // Install the NIP-RS parent resolver: every `thread:` or `msg:` + // context evaluated while this channel is active belongs to it (both are only + // ever read for the active channel's timeline messages), so the parent is + // always the active channel. Folding `msg:` to the channel — never to another + // message — means reading an ancestor never covers a descendant (LP4 Issue 2 + // by construction); a channel-read still clears any message older than the + // top-level channel frontier. Non-thread/non-message keys (channels) have no + // parent → null, which degrades effective() to the own term. Cleared on + // channel leave / unmount so a stale channel id never becomes the parent of + // another channel's contexts. React.useEffect(() => { if (!activeChannelId) { setContextParentResolver(null); return; } setContextParentResolver((contextId) => - contextId.startsWith("thread:") ? activeChannelId : null, + contextId.startsWith(THREAD_PREFIX) || contextId.startsWith(MSG_PREFIX) + ? activeChannelId + : null, ); return () => setContextParentResolver(null); }, [activeChannelId, setContextParentResolver]); @@ -381,8 +392,9 @@ export function ChannelScreen({ firstUnreadMessageId, getFirstReplyIdForMessage, getReplyDescendantIdsForMessage, - getSubtreeMaxCreatedAt, - handleMarkUnread, + handleMarkMessageRead, + handleMarkMessageUnread, + markRevealedRepliesRead, openThreadHeadMessage, threadFirstUnreadReplyId, threadMessages, @@ -398,9 +410,9 @@ export function ChannelScreen({ threadReplyTargetId, expandedThreadReplyIds, getChannelReadAt, - getThreadReadAt, + getMessageReadAt, markChannelUnread, - markThreadRead, + markMessageRead, isThreadMuted, readStateVersion, }); @@ -429,8 +441,7 @@ export function ChannelScreen({ expandedThreadReplyIds, getFirstReplyIdForMessage, getReplyDescendantIdsForMessage, - getSubtreeMaxCreatedAt, - markThreadRead, + markRevealedRepliesRead, openThreadHeadId: effectiveOpenThreadHeadId, onOptimisticOpenThreadHeadIdChange: setOptimisticOpenThreadHeadId, sendMessageMutation, @@ -449,6 +460,17 @@ export function ChannelScreen({ : undefined, [activeChannel, handleToggleReaction], ); + // The menu actions are typed (message) => void; the per-message read-state + // handlers key off the message id (message + subtree). Adapt at the seam so + // the handlers stay id-based and the menu stays message-based. + const handleMessageMarkUnread = React.useCallback( + (message: TimelineMessage) => handleMarkMessageUnread(message.id), + [handleMarkMessageUnread], + ); + const handleMessageMarkRead = React.useCallback( + (message: TimelineMessage) => handleMarkMessageRead(message.id), + [handleMarkMessageRead], + ); const handleSendVideoReviewComment = React.useCallback( async ( message: { id: string }, @@ -746,7 +768,8 @@ export function ChannelScreen({ onEditSave={ activeChannel?.archivedAt ? undefined : handleEditSave } - onMarkUnread={handleMarkUnread} + onMarkUnread={handleMessageMarkUnread} + onMarkRead={handleMessageMarkRead} onExpandThreadReplies={handleExpandThreadReplies} onOpenAgentSession={handleOpenAgentSession} onOpenDm={handleOpenDm} diff --git a/desktop/src/features/channels/ui/useChannelUnreadState.ts b/desktop/src/features/channels/ui/useChannelUnreadState.ts index e1f498f8e0..154c6dfa56 100644 --- a/desktop/src/features/channels/ui/useChannelUnreadState.ts +++ b/desktop/src/features/channels/ui/useChannelUnreadState.ts @@ -2,14 +2,12 @@ import * as React from "react"; import { buildCreatedAtByMessageId, - buildDirectRepliesByParentId, buildDirectReplyIdsByParentId, + buildRepliesByRootId, collectReplyDescendantIds, - subtreeMaxCreatedAt, } from "@/features/channels/lib/subtreeCreatedAt"; import { computeThreadReplyUnreadCounts } from "@/features/channels/lib/threadReplyUnreadCounts"; import { computeThreadBadgeCounts } from "@/features/channels/lib/threadBadgeCounts"; -import { seedThreadBadgeFrontiers } from "@/features/channels/lib/threadBadgeFrontier"; import { buildThreadPanelDataFromIndex, buildThreadPanelIndex, @@ -31,9 +29,9 @@ type UseChannelUnreadStateOptions = { threadReplyTargetId: string | null; expandedThreadReplyIds: ReadonlySet; getChannelReadAt: (channelId: string) => number | null; - getThreadReadAt: (rootId: string, channelId?: string | null) => number | null; + getMessageReadAt: (messageId: string) => number | null; markChannelUnread: (channelId: string) => void; - markThreadRead: (rootId: string, timestamp: number) => void; + markMessageRead: (messageId: string, timestamp: number) => void; isThreadMuted: (rootId: string) => boolean; readStateVersion: number; }; @@ -58,9 +56,9 @@ export function useChannelUnreadState({ threadReplyTargetId, expandedThreadReplyIds, getChannelReadAt, - getThreadReadAt, + getMessageReadAt, markChannelUnread, - markThreadRead, + markMessageRead, isThreadMuted, readStateVersion, }: UseChannelUnreadStateOptions) { @@ -89,6 +87,17 @@ export function useChannelUnreadState({ // is cleared on re-open (a fresh snapshot is recomputed for the channel). const forcedUnreadRef = React.useRef(new Set()); const [, forceUnreadRender] = React.useReducer((n: number) => n + 1, 0); + // Per-message analog of forcedUnreadRef (LP4 v3 mark-unread). A monotonic + // grow-only msg: marker cannot move the read-line backward, so a + // deliberate mark-unread lives in this session-local set, read ONLY as an + // OR-overlay by the badge predicates below — never written to the marker + // store. Cleared on channel-leave (same lifecycle as the channel set), so + // it does not survive reload, exactly like channel mark-unread today. + const forcedUnreadMsgRef = React.useRef(new Set()); + const isMsgForcedUnread = React.useCallback( + (messageId: string) => forcedUnreadMsgRef.current.has(messageId), + [], + ); const isActiveChannelForcedUnread = !!activeChannelId && forcedUnreadRef.current.has(activeChannelId); const isActiveWelcomeInitialUnreadSuppressed = @@ -100,6 +109,9 @@ export function useChannelUnreadState({ if (!channelId) return; return () => { forcedUnreadRef.current.delete(channelId); + // Clear per-message forced-unread too: switching channels ends the + // session window for both the channel-level and message-level overlays. + forcedUnreadMsgRef.current.clear(); }; }, [activeChannelId]); // Clear the open-time frontier on channel leave so re-visiting captures a @@ -118,8 +130,8 @@ export function useChannelUnreadState({ () => buildDirectReplyIdsByParentId(timelineMessages), [timelineMessages], ); - const directRepliesByParentId = React.useMemo( - () => buildDirectRepliesByParentId(timelineMessages), + const repliesByRootId = React.useMemo( + () => buildRepliesByRootId(timelineMessages), [timelineMessages], ); const getFirstReplyIdForMessage = React.useCallback( @@ -135,19 +147,6 @@ export function useChannelUnreadState({ () => buildCreatedAtByMessageId(timelineMessages), [timelineMessages], ); - // Newest createdAt across an expanded branch (the message itself plus every - // descendant). Drilling into a branch advances the thread frontier to this, - // consuming everything chronologically up to the deepest reply read. Returns - // null when the message is absent so the caller skips the read-state write. - const getSubtreeMaxCreatedAt = React.useCallback( - (messageId: string) => - subtreeMaxCreatedAt( - messageId, - directReplyIdsByParentId, - createdAtByMessageId, - ), - [createdAtByMessageId, directReplyIdsByParentId], - ); const threadPanelIndex = React.useMemo( () => buildThreadPanelIndex(timelineMessages), [timelineMessages], @@ -195,75 +194,104 @@ export function useChannelUnreadState({ ); // --- Thread unread state --- - // Capture the thread read frontier on open (same pattern as channel frontier). - // Keyed per thread root so switching threads captures a fresh frontier. - const threadOpenFrontierRef = React.useRef(new Map()); - if ( - openThreadHeadId && - !threadOpenFrontierRef.current.has(openThreadHeadId) - ) { - threadOpenFrontierRef.current.set( - openThreadHeadId, - getThreadReadAt(openThreadHeadId, activeChannelId), - ); + // Snapshot the per-message read state for the open thread's visible replies + // the instant the thread opens, BEFORE the on-open mark-read effect advances + // those markers. This anchors the in-thread "New" divider to "what was unread + // when I opened this thread" — the exact thread-level analog of the channel + // divider's openFrontierRef. Read ONLY by the divider below; the badge + // predicates read effective(msg:) live, so this snapshot is a separate + // concern (divider position) from the badge read-line — not a second source + // of truth for the same read-line. Keyed per thread root so switching threads + // captures a fresh snapshot; cleared on close so re-opening re-snapshots. + const threadOpenReadSnapshotRef = React.useRef( + new Map>(), + ); + // Record a reply's read state into the open thread's divider snapshot the + // first time we observe it, before any marker advance. Idempotent per reply + // (the first capture wins), so a value taken before a mark-read is never + // overwritten by the post-mark value. Keyed to the current open thread so a + // stale entry from a previous open cannot leak across a close→reopen cycle + // (the snapshot is dropped on close by the effect below). + const captureDividerReadState = React.useCallback( + (replyId: string) => { + if (!openThreadHeadId) return; + let snapshot = threadOpenReadSnapshotRef.current.get(openThreadHeadId); + if (!snapshot) { + snapshot = new Map(); + threadOpenReadSnapshotRef.current.set(openThreadHeadId, snapshot); + } + if (!snapshot.has(replyId)) { + snapshot.set(replyId, getMessageReadAt(replyId)); + } + }, + [getMessageReadAt, openThreadHeadId], + ); + if (openThreadHeadId) { + // Capture each visible reply's read state the first render it appears — + // before the on-open mark-read effect advances its marker. Replies revealed + // by expanding a branch are captured eagerly in markRevealedRepliesRead + // (before that path's synchronous mark-read), so this render-time pass + // covers replies present at open and acts as the fallback for any reply + // that reaches render without being pre-captured. + for (const entry of threadMessages) { + captureDividerReadState(entry.message.id); + } } - const threadOpenFrontierSeconds = openThreadHeadId - ? (threadOpenFrontierRef.current.get(openThreadHeadId) ?? null) - : null; - // Clear the thread frontier when the thread closes so re-opening captures fresh. React.useEffect(() => { const rootId = openThreadHeadId; if (!rootId) return; return () => { - threadOpenFrontierRef.current.delete(rootId); + threadOpenReadSnapshotRef.current.delete(rootId); }; }, [openThreadHeadId]); - // Mark thread read when the panel opens, advancing the frontier to the max - // createdAt over the head and its ENTIRE subtree — every reply, including - // ones nested in collapsed branches. Opening a badge-eligible thread means - // engaging with it, so the badge must collapse the instant the panel opens - // (not wait for a channel change or for each branch to be expanded). The - // badge counts the whole subtree (computeThreadBadgeCounts), so marking only - // the visible direct replies would leave it lit whenever the unread lives in - // a nested reply — the reported bug. Consuming collapsed branches here is not - // lossy: a NEWER reply re-raises the badge, because the unread comparison is - // strictly `createdAt > frontier` (computeThreadUnreadMarker) and the badge - // snapshot advances toward the live marker (nextThreadBadgeFrontier). + // Mark the revealed set read when the thread opens (LP4 v3): only the replies + // visible on open are read, never the whole subtree. A reply nested in a + // still-collapsed branch keeps its badge until it too is revealed (the + // deliberate reversal of #1118's whole-subtree-on-open). Each revealed reply + // gets its own msg: marker advanced to its createdAt; a NEWER reply + // re-raises the badge because the predicate is strictly createdAt > read. React.useEffect(() => { if (!openThreadHeadId) return; if (isThreadMuted(openThreadHeadId)) return; - const openReadCeiling = getSubtreeMaxCreatedAt(openThreadHeadId); - if (openReadCeiling === null) return; - markThreadRead(openThreadHeadId, openReadCeiling); - }, [openThreadHeadId, getSubtreeMaxCreatedAt, markThreadRead, isThreadMuted]); - // Compute the in-thread "New" divider position from the open-time frontier. + for (const entry of threadMessages) { + markMessageRead(entry.message.id, entry.message.createdAt); + } + }, [openThreadHeadId, threadMessages, markMessageRead, isThreadMuted]); + // In-thread "New" divider position. Reads the open-time snapshot (frozen + // before the mark-read effect above), so the divider does not collapse the + // instant open marks the revealed replies read. A reply absent from the + // snapshot (loaded after open) falls back to its live marker. const { firstUnreadReplyId: threadFirstUnreadReplyId } = React.useMemo(() => { if (!openThreadHeadId || threadMessages.length === 0) { return { firstUnreadReplyId: null, unreadCount: 0 }; } + const snapshot = threadOpenReadSnapshotRef.current.get(openThreadHeadId); const replies = threadMessages.map((entry) => entry.message); return computeThreadUnreadMarker( replies, - threadOpenFrontierSeconds, + // Use the snapshot value when the reply was captured — even when it is + // null (never read on open). Distinguish "captured null" from "never + // captured" with `has`, not `??`: a never-read reply snapshots to null, + // and a nullish-coalescing fallthrough would discard that and re-read the + // now-advanced live marker, collapsing the divider over the very replies + // that should anchor it. + (replyId) => + snapshot?.has(replyId) + ? (snapshot.get(replyId) ?? null) + : getMessageReadAt(replyId), currentPubkey, ); - }, [ - currentPubkey, - openThreadHeadId, - threadMessages, - threadOpenFrontierSeconds, - ]); + }, [currentPubkey, getMessageReadAt, openThreadHeadId, threadMessages]); // Per-row subtree unread counts for the in-panel thread summary rows. Scoped - // to the open thread's subtree and measured against the open-time frontier - // snapshot (threadOpenFrontierSeconds) — the same boundary the in-thread - // divider uses (above). The LIVE root marker can't be used here: on - // channel-open markChannelRead advances the channel marker to the newest - // top-level message, and effective(thread) = max(thread_own, channel_marker), - // so a channel marker past the nested replies would zero every badge the - // instant the panel opens. The snapshot reflects "what was unread on open." - // Expand-clears-badge is preserved independently: it's driven by the - // expandedSubtreeReplyIds gate inside computeThreadReplyUnreadCounts, not by - // the frontier. + // to the open thread's subtree and decided per-reply against the live + // per-message read state (getMessageReadAt): each collapsed row's badge + // counts unread replies anywhere beneath it. Expanding a branch marks only + // its revealed direct children read, so a collapsed grandchild keeps its + // badge — the per-message marker distinguishes the read parent from the + // unread descendant with no separate expanded-subtree gate. readStateVersion + // is an intentional recompute trigger so the counts re-read after any marker + // advances. + // biome-ignore lint/correctness/useExhaustiveDependencies: readStateVersion is the intentional recompute trigger const threadReplyUnreadCounts = React.useMemo( () => openThreadHeadId @@ -272,77 +300,47 @@ export function useChannelUnreadState({ subtreeReplyIds: getReplyDescendantIdsForMessage(openThreadHeadId), visibleReplyIds: threadMessages.map((entry) => entry.message.id), expandedReplyIds: expandedThreadReplyIds, - expandedSubtreeReplyIds: new Set( - [...expandedThreadReplyIds].flatMap((id) => - getReplyDescendantIdsForMessage(id), - ), - ), - frontierSeconds: threadOpenFrontierSeconds, + getReadAt: getMessageReadAt, currentPubkey, + isForcedUnread: isMsgForcedUnread, }) : new Map(), [ openThreadHeadId, threadMessages, timelineMessages, - threadOpenFrontierSeconds, + getMessageReadAt, expandedThreadReplyIds, getReplyDescendantIdsForMessage, currentPubkey, + isMsgForcedUnread, + readStateVersion, ], ); - // Snapshot per-thread read frontiers at channel-open time. Same pattern as - // openFrontierRef: captured during render (before the mark-read effect) so - // the badge reflects "what was unread on open" rather than the post-advance - // frontier. Keyed by activeChannelId → rootId → frontier value. - const threadBadgeFrontiersRef = React.useRef( - new Map>(), - ); - if (activeChannelId) { - let channelFrontiers = threadBadgeFrontiersRef.current.get(activeChannelId); - if (!channelFrontiers) { - channelFrontiers = new Map(); - threadBadgeFrontiersRef.current.set(activeChannelId, channelFrontiers); - } - seedThreadBadgeFrontiers( - channelFrontiers, - timelineMessages, - directRepliesByParentId, - (rootId) => !isThreadMuted(rootId), - (rootId) => getThreadReadAt(rootId, activeChannelId), - ); - } - // Clear the thread badge frontiers on channel leave (same cleanup as - // openFrontierRef) so re-visiting captures fresh snapshots. - React.useEffect(() => { - const channelId = activeChannelId; - if (!channelId) return; - return () => { - threadBadgeFrontiersRef.current.delete(channelId); - }; - }, [activeChannelId]); - // Per-thread unread counts for the main-timeline summary rows. Pure logic - // lives in computeThreadBadgeCounts; readStateVersion is an intentional - // recompute trigger so the badge re-reads the snapshot the seed block above - // advanced toward the live marker on mark-read. + // Per-thread unread counts for the main-timeline summary rows. Unread is + // decided per-reply against the live per-message read state: each reply + // lights iff createdAt > effective(msg:), folded channel→message only by + // the parent resolver, so reading an ancestor never clears a descendant + // (LP4 Issue 2 by construction). readStateVersion is an intentional recompute + // trigger so the badge re-reads after any marker advances. // biome-ignore lint/correctness/useExhaustiveDependencies: readStateVersion is the intentional recompute trigger const threadUnreadCounts = React.useMemo( () => computeThreadBadgeCounts( timelineMessages, - directRepliesByParentId, - activeChannelId - ? threadBadgeFrontiersRef.current.get(activeChannelId) - : undefined, + repliesByRootId, + getMessageReadAt, (rootId) => !isThreadMuted(rootId), currentPubkey, + isMsgForcedUnread, ), [ - activeChannelId, currentPubkey, timelineMessages, - directRepliesByParentId, + repliesByRootId, + getMessageReadAt, isThreadMuted, + isMsgForcedUnread, readStateVersion, ], ); @@ -356,14 +354,80 @@ export function useChannelUnreadState({ markChannelUnread(activeChannelId); }, [activeChannelId, markChannelUnread]); + // Mark a message's directly-revealed children read (LP4 v3 open-at-level): + // expanding a branch reveals only its direct replies, so only those get a + // msg: marker advanced to their createdAt. A reply still nested in a + // collapsed grandchild branch keeps its badge until it too is revealed. + // + // Capture each child's pre-read state into the divider snapshot BEFORE + // advancing its marker. This path runs synchronously in the expand event + // handler, before React re-renders with the child visible — so without the + // pre-capture the render-time pass above would snapshot the child as already + // read (this mark-read having won the race) and the "New" divider would never + // anchor to a reply first revealed by expansion. + const markRevealedRepliesRead = React.useCallback( + (messageId: string) => { + for (const replyId of directReplyIdsByParentId.get(messageId) ?? []) { + const createdAt = createdAtByMessageId.get(replyId); + if (createdAt !== undefined) { + captureDividerReadState(replyId); + markMessageRead(replyId, createdAt); + } + } + }, + [ + captureDividerReadState, + createdAtByMessageId, + directReplyIdsByParentId, + markMessageRead, + ], + ); + + // Mark a message and its whole subtree READ (LP4 v3 menu action). Writes a + // msg: marker at each message's createdAt — a real, persisted advance — + // and clears those same ids from the forced-unread overlay, so mark-read is + // the exact inverse of mark-unread over the same id set. + const handleMarkMessageRead = React.useCallback( + (messageId: string) => { + const ids = [messageId, ...getReplyDescendantIdsForMessage(messageId)]; + for (const id of ids) { + forcedUnreadMsgRef.current.delete(id); + const createdAt = createdAtByMessageId.get(id); + if (createdAt !== undefined) markMessageRead(id, createdAt); + } + forceUnreadRender(); + }, + [createdAtByMessageId, getReplyDescendantIdsForMessage, markMessageRead], + ); + + // Mark a message and its whole subtree UNREAD (LP4 v3 menu action). Markers + // are monotonic and cannot move backward, so this writes NO marker: it adds + // the ids to the session-local forced-unread overlay the badge predicates OR + // in. Cleared on channel-leave; does not survive reload (symmetric with the + // shipped channel mark-unread). + const handleMarkMessageUnread = React.useCallback( + (messageId: string) => { + for (const id of [ + messageId, + ...getReplyDescendantIdsForMessage(messageId), + ]) { + forcedUnreadMsgRef.current.add(id); + } + forceUnreadRender(); + }, + [getReplyDescendantIdsForMessage], + ); + return { createdAtByMessageId, directReplyIdsByParentId, firstUnreadMessageId, getFirstReplyIdForMessage, getReplyDescendantIdsForMessage, - getSubtreeMaxCreatedAt, + handleMarkMessageRead, + handleMarkMessageUnread, handleMarkUnread, + markRevealedRepliesRead, openThreadHeadMessage, threadFirstUnreadReplyId, threadMessages, diff --git a/desktop/src/features/channels/useChannelPaneHandlers.ts b/desktop/src/features/channels/useChannelPaneHandlers.ts index 5b4cdeb026..e753f4cbb9 100644 --- a/desktop/src/features/channels/useChannelPaneHandlers.ts +++ b/desktop/src/features/channels/useChannelPaneHandlers.ts @@ -24,8 +24,7 @@ export function useChannelPaneHandlers({ expandedThreadReplyIds, getFirstReplyIdForMessage, getReplyDescendantIdsForMessage, - getSubtreeMaxCreatedAt, - markThreadRead, + markRevealedRepliesRead, onOptimisticOpenThreadHeadIdChange, openThreadHeadId, sendMessageMutation, @@ -43,8 +42,7 @@ export function useChannelPaneHandlers({ expandedThreadReplyIds: ReadonlySet; getFirstReplyIdForMessage: (messageId: string) => string | null; getReplyDescendantIdsForMessage: (messageId: string) => string[]; - getSubtreeMaxCreatedAt: (messageId: string) => number | null; - markThreadRead: (rootId: string, timestamp: number) => void; + markRevealedRepliesRead: (messageId: string) => void; onOptimisticOpenThreadHeadIdChange: React.Dispatch< React.SetStateAction >; @@ -201,16 +199,12 @@ export function useChannelPaneHandlers({ return next; }); - // Drilling into a branch consumes its unread, persistently: advance the - // thread frontier to the branch's newest reply. Monotonic Math.max means - // this marks read everything chronologically up to it (channel-open - // parity). The open-time snapshot pins the session divider, so it never - // moves mid-session. - const rootId = openThreadHeadIdRef.current; - const subtreeMaxCreatedAt = getSubtreeMaxCreatedAt(message.id); - if (rootId && subtreeMaxCreatedAt !== null) { - markThreadRead(rootId, subtreeMaxCreatedAt); - } + // Drilling into a branch reveals only its direct replies (LP4 v3 + // open-at-level): mark exactly those read, never the whole subtree. A + // reply still nested in a collapsed grandchild branch keeps its badge + // until it too is revealed — the deliberate reversal of #1118's + // whole-subtree-on-open collapse. + markRevealedRepliesRead(message.id); if (firstReplyId) { setThreadScrollTargetId(firstReplyId); @@ -219,8 +213,7 @@ export function useChannelPaneHandlers({ [ getFirstReplyIdForMessage, getReplyDescendantIdsForMessage, - getSubtreeMaxCreatedAt, - markThreadRead, + markRevealedRepliesRead, setExpandedThreadReplyIds, setThreadScrollTargetId, ], diff --git a/desktop/src/features/messages/lib/unreadMarker.test.mjs b/desktop/src/features/messages/lib/unreadMarker.test.mjs index dcf2c2c05e..6c5bd046b5 100644 --- a/desktop/src/features/messages/lib/unreadMarker.test.mjs +++ b/desktop/src/features/messages/lib/unreadMarker.test.mjs @@ -14,6 +14,13 @@ function reply(id, createdAt, parentId) { return { id, createdAt, author: "a", time: "", body: "", depth: 1, parentId }; } +// LP4 v3: the thread marker now reads a per-message resolver instead of a +// single frontier. A uniform read-line at `seconds` (or null = never read) +// reproduces the old frontier semantics for the shared-boundary cases. +function uniformReadAt(seconds) { + return () => seconds; +} + test("computeChannelUnreadMarker_emptyTimeline_returnsNoUnread", () => { const marker = computeChannelUnreadMarker([], 100); assert.equal(marker.firstUnreadMessageId, null); @@ -98,60 +105,74 @@ test("computeChannelUnreadMarker_suppressedNeverReadChannel_returnsNoMarker", () // --- computeThreadUnreadMarker tests --- test("computeThreadUnreadMarker_emptyReplies_returnsNoUnread", () => { - const marker = computeThreadUnreadMarker([], 100); + const marker = computeThreadUnreadMarker([], uniformReadAt(100)); assert.equal(marker.firstUnreadReplyId, null); assert.equal(marker.unreadCount, 0); }); -test("computeThreadUnreadMarker_nullFrontier_marksAllRepliesUnread", () => { +test("computeThreadUnreadMarker_neverRead_marksAllRepliesUnread", () => { const replies = [ { id: "r1", createdAt: 10 }, { id: "r2", createdAt: 20 }, { id: "r3", createdAt: 30 }, ]; - const marker = computeThreadUnreadMarker(replies, null); + const marker = computeThreadUnreadMarker(replies, uniformReadAt(null)); assert.equal(marker.firstUnreadReplyId, "r1"); assert.equal(marker.unreadCount, 3); }); -test("computeThreadUnreadMarker_frontierBetweenReplies_countsAfterFrontier", () => { +test("computeThreadUnreadMarker_readLineBetweenReplies_countsAfterLine", () => { const replies = [ { id: "r1", createdAt: 10 }, { id: "r2", createdAt: 20 }, { id: "r3", createdAt: 30 }, ]; - const marker = computeThreadUnreadMarker(replies, 15); + const marker = computeThreadUnreadMarker(replies, uniformReadAt(15)); assert.equal(marker.firstUnreadReplyId, "r2"); assert.equal(marker.unreadCount, 2); }); -test("computeThreadUnreadMarker_frontierAtReplyTimestamp_isRead", () => { - // A reply whose createdAt equals the frontier is considered read (strictly >). +test("computeThreadUnreadMarker_readAtEqualsReplyTimestamp_isRead", () => { + // A reply whose createdAt equals its read marker is read (strictly >). const replies = [ { id: "r1", createdAt: 10 }, { id: "r2", createdAt: 20 }, ]; - const marker = computeThreadUnreadMarker(replies, 20); + const marker = computeThreadUnreadMarker(replies, uniformReadAt(20)); assert.equal(marker.firstUnreadReplyId, null); assert.equal(marker.unreadCount, 0); }); -test("computeThreadUnreadMarker_frontierAboveAll_returnsNoUnread", () => { +test("computeThreadUnreadMarker_readLineAboveAll_returnsNoUnread", () => { const replies = [ { id: "r1", createdAt: 10 }, { id: "r2", createdAt: 20 }, ]; - const marker = computeThreadUnreadMarker(replies, 100); + const marker = computeThreadUnreadMarker(replies, uniformReadAt(100)); assert.equal(marker.firstUnreadReplyId, null); assert.equal(marker.unreadCount, 0); }); -test("computeThreadUnreadMarker_frontierBelowAll_allUnread", () => { +test("computeThreadUnreadMarker_readLineBelowAll_allUnread", () => { const replies = [ { id: "r1", createdAt: 10 }, { id: "r2", createdAt: 20 }, ]; - const marker = computeThreadUnreadMarker(replies, 5); + const marker = computeThreadUnreadMarker(replies, uniformReadAt(5)); + assert.equal(marker.firstUnreadReplyId, "r1"); + assert.equal(marker.unreadCount, 2); +}); + +test("computeThreadUnreadMarker_perMessageMarkers_countOnlyUnreadReply", () => { + // The point of the per-message resolver: reading r2 leaves r1 and r3 + // unread independently — no single frontier could express this. + const replies = [ + { id: "r1", createdAt: 10 }, + { id: "r2", createdAt: 20 }, + { id: "r3", createdAt: 30 }, + ]; + const readAt = (id) => (id === "r2" ? 20 : null); + const marker = computeThreadUnreadMarker(replies, readAt); assert.equal(marker.firstUnreadReplyId, "r1"); assert.equal(marker.unreadCount, 2); }); @@ -162,17 +183,34 @@ test("computeThreadUnreadMarker_singleReplyUnread_countsOne", () => { { id: "r2", createdAt: 20 }, { id: "r3", createdAt: 30 }, ]; - const marker = computeThreadUnreadMarker(replies, 25); + const marker = computeThreadUnreadMarker(replies, uniformReadAt(25)); assert.equal(marker.firstUnreadReplyId, "r3"); assert.equal(marker.unreadCount, 1); }); -test("computeThreadUnreadMarker_emptyRepliesNullFrontier_returnsNoUnread", () => { - const marker = computeThreadUnreadMarker([], null); +test("computeThreadUnreadMarker_emptyRepliesNeverRead_returnsNoUnread", () => { + const marker = computeThreadUnreadMarker([], uniformReadAt(null)); assert.equal(marker.firstUnreadReplyId, null); assert.equal(marker.unreadCount, 0); }); +test("computeThreadUnreadMarker_forcedUnread_overridesReadMarker", () => { + // Session-local mark-unread: r1 is read by its marker but forced unread, + // so it counts; the OR-overlay never clears an otherwise-unread reply. + const replies = [ + { id: "r1", createdAt: 10 }, + { id: "r2", createdAt: 20 }, + ]; + const marker = computeThreadUnreadMarker( + replies, + uniformReadAt(100), + undefined, + (id) => id === "r1", + ); + assert.equal(marker.firstUnreadReplyId, "r1"); + assert.equal(marker.unreadCount, 1); +}); + // --- Self-authored skip tests --- test("computeChannelUnreadMarker_selfAuthored_skipsOwnMessages", () => { @@ -213,7 +251,7 @@ test("computeThreadUnreadMarker_selfAuthored_skipsOwnReplies", () => { { id: "r2", createdAt: 20, pubkey: "other" }, { id: "r3", createdAt: 30, pubkey: "me" }, ]; - const marker = computeThreadUnreadMarker(replies, 5, "me"); + const marker = computeThreadUnreadMarker(replies, uniformReadAt(5), "me"); assert.equal(marker.firstUnreadReplyId, "r2"); assert.equal(marker.unreadCount, 1); }); @@ -223,7 +261,7 @@ test("computeThreadUnreadMarker_allSelfAuthored_returnsNoUnread", () => { { id: "r1", createdAt: 10, pubkey: "me" }, { id: "r2", createdAt: 20, pubkey: "me" }, ]; - const marker = computeThreadUnreadMarker(replies, 5, "me"); + const marker = computeThreadUnreadMarker(replies, uniformReadAt(5), "me"); assert.equal(marker.firstUnreadReplyId, null); assert.equal(marker.unreadCount, 0); }); @@ -233,7 +271,7 @@ test("computeThreadUnreadMarker_noPubkey_countsNormally", () => { { id: "r1", createdAt: 10, pubkey: "me" }, { id: "r2", createdAt: 20, pubkey: "other" }, ]; - const marker = computeThreadUnreadMarker(replies, 5); + const marker = computeThreadUnreadMarker(replies, uniformReadAt(5)); assert.equal(marker.firstUnreadReplyId, "r1"); assert.equal(marker.unreadCount, 2); }); @@ -254,7 +292,7 @@ test("computeThreadUnreadMarker_selfAuthoredMixedCase_skipsOwnReplies", () => { { id: "r1", createdAt: 10, pubkey: "ABCDEF" }, { id: "r2", createdAt: 20, pubkey: "other" }, ]; - const marker = computeThreadUnreadMarker(replies, 5, "abcdef"); + const marker = computeThreadUnreadMarker(replies, uniformReadAt(5), "abcdef"); assert.equal(marker.firstUnreadReplyId, "r2"); assert.equal(marker.unreadCount, 1); }); diff --git a/desktop/src/features/messages/lib/unreadMarker.ts b/desktop/src/features/messages/lib/unreadMarker.ts index 1feae13e87..6e47f119c5 100644 --- a/desktop/src/features/messages/lib/unreadMarker.ts +++ b/desktop/src/features/messages/lib/unreadMarker.ts @@ -97,16 +97,24 @@ const EMPTY_THREAD_MARKER: ThreadUnreadMarker = { /** * @param replies Thread replies in chronological order. - * @param frontierSeconds Read frontier in unix seconds captured at thread - * open. `null` means the thread was never read, so every reply counts as - * unread. + * @param getReadAt Per-message read resolver (LP4 v3). A reply is unread when + * its `createdAt` is strictly newer than `getReadAt(reply.id)`; a `null` + * marker means the reply was never read, so it counts as unread. Folding the + * channel term into each marker happens upstream in the resolver, never + * reply→reply, so reading one reply never clears another. * @param currentPubkey When provided, replies authored by this pubkey are * never counted as unread (the user knows about their own posts). + * @param isForcedUnread Session-local OR-overlay (LP4 v3). When it returns + * true for a reply, the reply counts as unread regardless of its marker — + * the per-message analog of channel mark-unread. Markers are monotonic and + * cannot move backward, so a deliberate mark-unread lives in this transient + * overlay, never in the read-line. Defaults to never-forced. */ export function computeThreadUnreadMarker( replies: Pick[], - frontierSeconds: number | null, + getReadAt: (messageId: string) => number | null, currentPubkey?: string, + isForcedUnread: (messageId: string) => boolean = () => false, ): ThreadUnreadMarker { // Normalize once: see computeChannelUnreadMarker for the case-mismatch guard. const normalizedPubkey = currentPubkey?.toLowerCase(); @@ -118,8 +126,9 @@ export function computeThreadUnreadMarker( if (normalizedPubkey && reply.pubkey?.toLowerCase() === normalizedPubkey) { continue; } + const readAt = getReadAt(reply.id); const isUnread = - frontierSeconds === null || reply.createdAt > frontierSeconds; + isForcedUnread(reply.id) || readAt === null || reply.createdAt > readAt; if (!isUnread) { continue; } diff --git a/desktop/src/features/messages/ui/MessageActionBar.tsx b/desktop/src/features/messages/ui/MessageActionBar.tsx index 2360173c69..5813eb4de3 100644 --- a/desktop/src/features/messages/ui/MessageActionBar.tsx +++ b/desktop/src/features/messages/ui/MessageActionBar.tsx @@ -6,6 +6,7 @@ import { CornerUpLeft, EllipsisVertical, Link2, + MailCheck, MailOpen, Pencil, SmilePlus, @@ -77,6 +78,7 @@ function MoreActionsMenu({ onEdit, onFollowThread, onMarkUnread, + onMarkRead, onOpenChange, onRemindLater, onUnfollowThread, @@ -91,6 +93,7 @@ function MoreActionsMenu({ onEdit?: (message: TimelineMessage) => void; onFollowThread?: (message: TimelineMessage) => void; onMarkUnread?: (message: TimelineMessage) => void; + onMarkRead?: (message: TimelineMessage) => void; onOpenChange: (open: boolean) => void; onRemindLater?: (message: TimelineMessage) => void; onUnfollowThread?: (message: TimelineMessage) => void; @@ -165,6 +168,17 @@ function MoreActionsMenu({ ) : null} + {onMarkRead ? ( + { + onMarkRead(message); + }} + > + + Mark read + + ) : null} + {onFollowThread || onUnfollowThread ? ( { @@ -333,6 +347,7 @@ export function MessageActionBar({ onEdit, onFollowThread, onMarkUnread, + onMarkRead, onReactionBadgeBurstRequest, onReactionSelect, onRemindLater, @@ -350,6 +365,7 @@ export function MessageActionBar({ onEdit?: (message: TimelineMessage) => void; onFollowThread?: (message: TimelineMessage) => void; onMarkUnread?: (message: TimelineMessage) => void; + onMarkRead?: (message: TimelineMessage) => void; onReactionBadgeBurstRequest?: (emoji: string) => void; onReactionSelect?: (emoji: string) => Promise; onRemindLater?: (message: TimelineMessage) => void; @@ -382,6 +398,7 @@ export function MessageActionBar({ Boolean(onEdit) || Boolean(onDelete) || Boolean(onMarkUnread) || + Boolean(onMarkRead) || Boolean(onFollowThread) || Boolean(onUnfollowThread) || Boolean(onRemindLater) || @@ -529,6 +546,7 @@ export function MessageActionBar({ onEdit={onEdit} onFollowThread={onFollowThread} onMarkUnread={onMarkUnread} + onMarkRead={onMarkRead} onOpenChange={setIsDropdownOpen} onRemindLater={onRemindLater} onUnfollowThread={onUnfollowThread} diff --git a/desktop/src/features/messages/ui/MessageRow.tsx b/desktop/src/features/messages/ui/MessageRow.tsx index efbacc6815..0d5081e0e9 100644 --- a/desktop/src/features/messages/ui/MessageRow.tsx +++ b/desktop/src/features/messages/ui/MessageRow.tsx @@ -66,6 +66,7 @@ export const MessageRow = React.memo( onEdit, onFollowThread, onMarkUnread, + onMarkRead, onToggleReaction, onReply, onUnfollowThread, @@ -104,6 +105,7 @@ export const MessageRow = React.memo( onEdit?: (message: TimelineMessage) => void; onFollowThread?: (message: TimelineMessage) => void; onMarkUnread?: (message: TimelineMessage) => void; + onMarkRead?: (message: TimelineMessage) => void; onToggleReaction?: ( message: TimelineMessage, emoji: string, @@ -349,6 +351,7 @@ export const MessageRow = React.memo( onEdit={onEdit} onFollowThread={onFollowThread} onMarkUnread={onMarkUnread} + onMarkRead={onMarkRead} onReactionBadgeBurstRequest={ reactionPending ? undefined : setBadgeBurstEmoji } diff --git a/desktop/src/features/messages/ui/MessageThreadPanel.tsx b/desktop/src/features/messages/ui/MessageThreadPanel.tsx index cc8329ace4..b28aa8cf83 100644 --- a/desktop/src/features/messages/ui/MessageThreadPanel.tsx +++ b/desktop/src/features/messages/ui/MessageThreadPanel.tsx @@ -63,6 +63,7 @@ type MessageThreadPanelProps = { onEditLastOwnMessage?: () => boolean; onEditSave?: (content: string, mediaTags?: string[][]) => Promise; onMarkUnread?: (message: TimelineMessage) => void; + onMarkRead?: (message: TimelineMessage) => void; onExpandReplies: (message: TimelineMessage) => void; onScrollTargetResolved: () => void; onSelectReplyTarget: (message: TimelineMessage) => void; @@ -356,6 +357,7 @@ export function MessageThreadPanel({ onEditSave, onFollowThread, onMarkUnread, + onMarkRead, onExpandReplies, onScrollTargetResolved, onSelectReplyTarget, @@ -664,6 +666,7 @@ export function MessageThreadPanel({ onFollowThread ? (_msg) => onFollowThread() : undefined } onMarkUnread={onMarkUnread} + onMarkRead={onMarkRead} onToggleReaction={onToggleReaction} onUnfollowThread={ onUnfollowThread ? (_msg) => onUnfollowThread() : undefined @@ -790,6 +793,7 @@ export function MessageThreadPanel({ : undefined } onMarkUnread={onMarkUnread} + onMarkRead={onMarkRead} onReply={onSelectReplyTarget} onToggleReaction={onToggleReaction} profiles={profiles} diff --git a/desktop/src/features/messages/ui/MessageTimeline.tsx b/desktop/src/features/messages/ui/MessageTimeline.tsx index deaf3f5d8a..d6865c8675 100644 --- a/desktop/src/features/messages/ui/MessageTimeline.tsx +++ b/desktop/src/features/messages/ui/MessageTimeline.tsx @@ -57,6 +57,7 @@ type MessageTimelineProps = { onDelete?: (message: TimelineMessage) => void; onEdit?: (message: TimelineMessage) => void; onMarkUnread?: (message: TimelineMessage) => void; + onMarkRead?: (message: TimelineMessage) => void; onReply?: (message: TimelineMessage) => void; isSendingVideoReviewComment?: boolean; onSendVideoReviewComment?: ( @@ -151,6 +152,7 @@ const MessageTimelineBase = React.forwardRef< onDelete, onEdit, onMarkUnread, + onMarkRead, onReply, channelName, channelType, @@ -538,6 +540,7 @@ const MessageTimelineBase = React.forwardRef< onDelete={onDelete} onEdit={onEdit} onMarkUnread={onMarkUnread} + onMarkRead={onMarkRead} onReply={onReply} isSendingVideoReviewComment={isSendingVideoReviewComment} onSendVideoReviewComment={onSendVideoReviewComment} diff --git a/desktop/src/features/messages/ui/TimelineMessageList.tsx b/desktop/src/features/messages/ui/TimelineMessageList.tsx index 8fcee1c8c9..2d226328d9 100644 --- a/desktop/src/features/messages/ui/TimelineMessageList.tsx +++ b/desktop/src/features/messages/ui/TimelineMessageList.tsx @@ -41,6 +41,7 @@ type TimelineMessageListProps = { onDelete?: (message: TimelineMessage) => void; onEdit?: (message: TimelineMessage) => void; onMarkUnread?: (message: TimelineMessage) => void; + onMarkRead?: (message: TimelineMessage) => void; onReply?: (message: TimelineMessage) => void; isSendingVideoReviewComment?: boolean; onSendVideoReviewComment?: ( @@ -192,6 +193,7 @@ export const TimelineMessageList = React.memo(function TimelineMessageList({ onDelete, onEdit, onMarkUnread, + onMarkRead, onReply, isSendingVideoReviewComment = false, onSendVideoReviewComment, @@ -240,6 +242,7 @@ export const TimelineMessageList = React.memo(function TimelineMessageList({ onDelete={onDelete} onEdit={onEdit} onMarkUnread={onMarkUnread} + onMarkRead={onMarkRead} onReply={onReply} onSendVideoReviewComment={onSendVideoReviewComment} onToggleReaction={onToggleReaction} @@ -281,6 +284,7 @@ const TimelineRenderRowView = React.memo(function TimelineRenderRowView({ onDelete, onEdit, onMarkUnread, + onMarkRead, onReply, onSendVideoReviewComment, onToggleReaction, @@ -385,6 +389,7 @@ const TimelineRenderRowView = React.memo(function TimelineRenderRowView({ followThreadById ? () => followThreadById(message.id) : undefined } onMarkUnread={onMarkUnread} + onMarkRead={onMarkRead} onToggleReaction={onToggleReaction} onReply={onReply} onUnfollowThread={ @@ -431,6 +436,7 @@ const TimelineRenderRowView = React.memo(function TimelineRenderRowView({ : undefined } onMarkUnread={onMarkUnread} + onMarkRead={onMarkRead} onToggleReaction={onToggleReaction} onReply={onReply} profiles={profiles} diff --git a/desktop/tests/e2e/thread-unread-screenshots.spec.ts b/desktop/tests/e2e/thread-unread-screenshots.spec.ts index 1775c7c36b..af2f6e1637 100644 --- a/desktop/tests/e2e/thread-unread-screenshots.spec.ts +++ b/desktop/tests/e2e/thread-unread-screenshots.spec.ts @@ -514,14 +514,23 @@ test.describe("thread unread indicator screenshots", () => { path: `${SHOTS}/05-thread-in-panel-subtree-badge.png`, }); - // Expanding p marks its whole subtree read; the descendant-inclusive gate - // (Phase 2.5) drops the badge from p and every revealed row beneath it. + // v3 contract: expanding a branch marks only its REVEALED direct children + // read, never the whole subtree. The unread replies sit two levels under p + // (p -> c -> c2 -> c2-child), so a single expand of p only reveals c — the + // deeper unread stays collapsed and the badge survives. The badge clears + // only as each level is individually revealed: expand p (reveals c, badge + // still counts c2 + c2-child), expand c (reveals c2, read), expand c2 + // (reveals c2-child, read) -> badge clears to 0. await expandReply(page, p.id); - await expect(inPanelBadge).toHaveCount(0); + await expect(inPanelBadge).toBeVisible(); await page.screenshot({ path: `${SHOTS}/06-thread-expand-clears-subtree-badge.png`, }); + + await expandReply(page, c.id); + await expandReply(page, c2.id); + await expect(inPanelBadge).toHaveCount(0); }); test("06-in-panel-badge-bumps-on-live-reply", async ({ page }) => { @@ -635,11 +644,13 @@ test.describe("thread unread indicator screenshots", () => { await page.getByTestId("channel-random").click(); await expect(page.getByTestId("chat-title")).toHaveText("random"); - // Each branch gains its own unread reply. Badges are computed against the - // open-time frozen frontier snapshot, and expand-clear is driven by the - // per-branch `expandedSubtreeReplyIds` gate — NOT a cross-branch live - // marker sweep. So expanding one branch clears only its OWN badge; the - // sibling's badge survives until that branch is expanded too. + // Each branch gains its own unread reply, nested one level under the + // branch's child (branchNew -> newChild -> unread; branchOld -> oldChild -> + // unread). Under the v3 per-message contract, expanding a branch marks only + // its REVEALED direct children read — so revealing newChild does NOT reach + // the unread reply beneath it. Clearing a branch's badge requires expanding + // down to the level the unread actually sits at; the sibling branch is + // never touched, so its badge survives independently. const base = unreadTimestamp(); await emitMockMessage(page, "general", "Unread in older branch", { parentEventId: oldChild.id, @@ -667,19 +678,23 @@ test.describe("thread unread indicator screenshots", () => { path: `${SHOTS}/08-two-sibling-badges-before-expand.png`, }); - // Expand the LATER branch. Only its OWN badge clears, via the - // `expandedSubtreeReplyIds` gate against the frozen open-time frontier. - // The older sibling's badge SURVIVES — the design does not sweep across - // branches off a live marker. + // Expand the LATER branch down to where its unread sits: revealing + // branchNew shows newChild (still collapsed over the unread reply, so the + // badge survives), then revealing newChild marks the unread reply read and + // clears branchNew's badge. The older sibling is never expanded, so its + // badge survives — per-message markers isolate each branch. await expandReply(page, branchNew.id); + await expect(inPanelBadges).toHaveCount(2); + await expandReply(page, newChild.id); await expect(inPanelBadges).toHaveCount(1); await page.screenshot({ path: `${SHOTS}/09-expand-clears-own-branch-sibling-survives.png`, }); - // Expanding the older branch clears the last remaining badge. + // Expanding the older branch to its unread depth clears the last badge. await expandReply(page, branchOld.id); + await expandReply(page, oldChild.id); await expect(inPanelBadges).toHaveCount(0); await page.screenshot({ @@ -966,11 +981,17 @@ test.describe("thread unread indicator screenshots", () => { await expect(badge).toBeVisible(); await expect(badge).toContainText("2"); - // Opening a notified thread advances the frontier to the full subtree max, - // so it consumes the direct reply A AND the nested mention B at once — the - // badge clears to 0 in place without drilling into A's collapsed branch. + // v3 contract: opening a thread marks only its REVEALED direct children + // read, never the whole subtree. Opening Alice's thread reveals direct + // child A (read), but nested mention B stays collapsed under A — so the + // root badge drops to 1, not 0. Expanding A reveals B, marks it read, and + // clears the badge. The badge predicate reads the live per-message marker, + // not a subtree-max open ceiling. await aliceSummary.click(); await expect(page.getByTestId("message-thread-panel")).toBeVisible(); + await expect(badge).toContainText("1"); + + await expandReply(page, replyA?.id ?? ""); await expect(badge).toHaveCount(0); await page.getByTestId("message-thread-close").click();