diff --git a/desktop/src/features/channels/lib/subtreeCreatedAt.ts b/desktop/src/features/channels/lib/subtreeCreatedAt.ts index d115962bde..1240987760 100644 --- a/desktop/src/features/channels/lib/subtreeCreatedAt.ts +++ b/desktop/src/features/channels/lib/subtreeCreatedAt.ts @@ -9,6 +9,7 @@ export function subtreeMaxCreatedAt( messageId: string, directReplyIdsByParentId: ReadonlyMap, createdAtByMessageId: ReadonlyMap, + repliesByRootId?: ReadonlyMap, ): number | null { const ownCreatedAt = createdAtByMessageId.get(messageId); if (ownCreatedAt === undefined) return null; @@ -24,6 +25,18 @@ export function subtreeMaxCreatedAt( } pendingIds.push(...(directReplyIdsByParentId.get(currentId) ?? [])); } + // Orphan-immune ceiling: also fold in replies that resolve to this id by + // rootId. When the timeline window drops a middle ancestor, a deep reply + // keys under its absent parent and the adjacency walk above can't reach it, + // so the root-started ceiling stops short and the channel-root badge can + // never clear. rootId travels with the event (getThreadReference), so a root + // reaches its severed orphans here. A BRANCH node is no reply's rootId, so + // its rootId-bucket is empty and the branch-scoped ceiling is unchanged. + for (const reply of repliesByRootId?.get(messageId) ?? []) { + if (reply.createdAt > maxCreatedAt) { + maxCreatedAt = reply.createdAt; + } + } return maxCreatedAt; } @@ -31,6 +44,7 @@ export function subtreeMaxCreatedAt( interface ReplyGraphMessage { id: string; parentId?: string | null; + rootId?: string | null; createdAt: number; } @@ -66,6 +80,29 @@ export function buildDirectRepliesByParentId( return map; } +/** + * 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 buildRepliesByRootId( + messages: readonly T[], +): Map { + const map = new Map(); + for (const message of messages) { + const rootId = message.rootId; + if (!rootId || rootId === message.id) continue; + const currentReplies = map.get(rootId) ?? []; + currentReplies.push(message); + map.set(rootId, currentReplies); + } + return map; +} + /** Maps each message id to its `createdAt`. */ export function buildCreatedAtByMessageId( messages: readonly ReplyGraphMessage[], diff --git a/desktop/src/features/channels/lib/threadBadgeCollapseOnOpen.test.mjs b/desktop/src/features/channels/lib/threadBadgeCollapseOnOpen.test.mjs index d88a49f3b3..0de93a8bd8 100644 --- a/desktop/src/features/channels/lib/threadBadgeCollapseOnOpen.test.mjs +++ b/desktop/src/features/channels/lib/threadBadgeCollapseOnOpen.test.mjs @@ -5,8 +5,8 @@ import { computeThreadBadgeCounts } from "./threadBadgeCounts.ts"; import { nextThreadBadgeFrontier } from "./threadBadgeFrontier.ts"; import { buildCreatedAtByMessageId, - buildDirectRepliesByParentId, buildDirectReplyIdsByParentId, + buildRepliesByRootId, subtreeMaxCreatedAt, } from "./subtreeCreatedAt.ts"; @@ -21,9 +21,13 @@ import { // (subtreeMaxCreatedAt); these tests pin that the badge collapses to 0 on open // whether or not the OWN marker actually advances. +// rootId is "root" on every reply: these threads are all rooted at "root", and +// the badge roll-up groups by rootId (getThreadReference's `root` e-tag), so the +// nested b under a still tallies at root. Top-level "root" carries its own id. const msg = (id, parentId, createdAt, pubkey = "author") => ({ id, parentId, + rootId: parentId === null ? id : "root", createdAt, pubkey, }); @@ -53,7 +57,7 @@ const badgeAfterOpen = (rootId, messages, priorOwnMarker, currentPubkey) => { const frontier = nextThreadBadgeFrontier(undefined, liveMarker); return computeThreadBadgeCounts( messages, - buildDirectRepliesByParentId(messages), + buildRepliesByRootId(messages), new Map([[rootId, frontier]]), () => true, currentPubkey, @@ -102,7 +106,7 @@ test("openThreadWithUnreadNestedReply_oldDirectCeilingLeftBadgeLit", () => { const frontier = nextThreadBadgeFrontier(undefined, oldDirectCeiling); const count = computeThreadBadgeCounts( messages, - buildDirectRepliesByParentId(messages), + buildRepliesByRootId(messages), new Map([["root", frontier]]), () => true, ).get("root"); @@ -135,7 +139,7 @@ test("openThreadWhereOnlyUnreadIsOwnReply_neverShowsBadge", () => { // Frontier below every reply (never read) — only "other"'s reply a counts. const beforeOpen = computeThreadBadgeCounts( messages, - buildDirectRepliesByParentId(messages), + buildRepliesByRootId(messages), new Map([["root", null]]), () => true, "me", @@ -154,7 +158,7 @@ test("openThreadWhereEveryUnreadIsOwnReply_inertNoBadgeEver", () => { ]; const before = computeThreadBadgeCounts( messages, - buildDirectRepliesByParentId(messages), + buildRepliesByRootId(messages), new Map([["root", null]]), () => true, "me", diff --git a/desktop/src/features/channels/lib/threadBadgeCounts.test.mjs b/desktop/src/features/channels/lib/threadBadgeCounts.test.mjs index e38ea8e078..7144696ff2 100644 --- a/desktop/src/features/channels/lib/threadBadgeCounts.test.mjs +++ b/desktop/src/features/channels/lib/threadBadgeCounts.test.mjs @@ -2,15 +2,17 @@ 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, rootId, // createdAt, pubkey. createdAt defaults high so replies count unread against a // null frontier unless a test sets it lower. `rootId` defaults to the parent -// (mirroring getThreadReference's `rootTag?.[1] ?? parentId` fallback) so the -// broken-chain fixtures can pin the orphan-immune root explicitly: the orphan -// carries its true rootId even when the middle ancestor is unloaded, which is -// what keeps these tests falsifiable once the roll-up re-keys on rootId. +// (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, @@ -23,7 +25,7 @@ const countAll = () => true; const counts = (messages, frontiers, isNotified = countAll, currentPubkey) => computeThreadBadgeCounts( messages, - buildDirectRepliesByParentId(messages), + buildRepliesByRootId(messages), frontiers, isNotified, currentPubkey, @@ -35,31 +37,37 @@ test("computeThreadBadgeCounts_directRepliesOnly_countsEach", () => { }); 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")]; + // 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, undefined).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); }); 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); @@ -71,7 +79,11 @@ test("computeThreadBadgeCounts_rootWithNoReplies_omitted", () => { }); test("computeThreadBadgeCounts_notNotified_omitted", () => { - const messages = [msg("root", null), msg("a", "root"), msg("b", "a")]; + const messages = [ + msg("root", null), + msg("a", "root"), + msg("b", "a", 100, "author", "root"), + ]; assert.equal(counts(messages, undefined, () => false).size, 0); }); @@ -80,7 +92,7 @@ test("computeThreadBadgeCounts_frontierCoversNestedReplies_excludesRead", () => 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); @@ -90,7 +102,7 @@ test("computeThreadBadgeCounts_frontierCoversWholeSubtree_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); @@ -101,7 +113,7 @@ 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); }); @@ -110,7 +122,7 @@ 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"), ]; @@ -119,67 +131,55 @@ test("computeThreadBadgeCounts_multipleRoots_eachCountsOwnSubtree", () => { assert.equal(result.get("root2"), 1); }); -// --- LP4 Case 1 demonstration: orphaned subtree from a broken parent chain --- +// --- LP4 Case 1: orphaned subtree from a broken parent chain rolls up --- // -// The roll-up keys each reply under its immediate `parentId` and walks the -// adjacency map down from true roots (collectSubtreeReplies). A descendant is -// only reached if its FULL parent chain is present in the loaded timeline. -// Pagination / load windows can drop an intermediate ancestor, which severs the -// chain: the deep reply still sits under its (absent) parent's key, but that key -// is never visited from any root's bucket, so it is never tallied at the root. +// 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 pass -// against TODAY'S behavior. The first DOCUMENTS THE DEFECT (root undercounts / -// shows no badge); the second is the contrasting full-chain control. +// 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_orphanedReplyMissesRoot_DEFECT", () => { +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. - // DEFECT: `c` is keyed under "b", and "b" is never reached from root's bucket - // (root -> [a], a -> [] because b is absent), so `c` is orphaned. The root - // badge counts only `a` (1), NOT the 2 it would show with the chain intact. - // With the bug, opening the thread shows `c` unread at its level while the - // channel-root summary undercounts it. `c.rootId === "root"` is set - // explicitly: today's parentId-keyed walk ignores it and still orphans `c` - // (this assertion holds), but once the roll-up re-keys on rootId this fixture - // flips — `c` will roll up and the expected count becomes 2. + // 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, undefined).get("root"), 1); + assert.equal(counts(loaded, undefined).get("root"), 2); }); -test("computeThreadBadgeCounts_brokenParentChain_orphanedSoleReply_noBadge_DEFECT", () => { - // Sharper form: root's ONLY unread content is the deep reply `c`, and its - // intermediate ancestor `b` is unloaded. DEFECT: root shows NO badge at all - // (count absent) even though `c` is genuinely unread, because the orphaned - // `c` is unreachable from root and root then has zero tallied replies. - // `c.rootId === "root"` is set explicitly so the fixture stays falsifiable: - // today's parentId walk still orphans it (root absent from the count map), - // but once re-keyed on rootId the badge appears with count 1. +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, undefined).has("root"), false); + assert.equal(counts(loaded, undefined).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, so every descendant rolls up and the - // root badge correctly counts 3. This is the behavior the broken-chain cases - // above SHOULD produce once the deep reply's ancestors are guaranteed loaded. - // rootId is set on `c` for parity with the broken-chain fixtures; with the - // chain intact the parentId walk already reaches it, so this stays green - // whether the roll-up keys on parentId or rootId. + // 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"), + msg("b", "a", 100, "author", "root"), msg("c", "b", 100, "author", "root"), ]; assert.equal(counts(loaded, undefined).get("root"), 3); diff --git a/desktop/src/features/channels/lib/threadBadgeCounts.ts b/desktop/src/features/channels/lib/threadBadgeCounts.ts index fb78c84267..3179211a61 100644 --- a/desktop/src/features/channels/lib/threadBadgeCounts.ts +++ b/desktop/src/features/channels/lib/threadBadgeCounts.ts @@ -1,34 +1,6 @@ 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. * @@ -39,9 +11,16 @@ function collectSubtreeReplies( * 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. + * * @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 repliesByRootId Replies grouped by their resolved thread root id. * @param frontiers Per-root read frontier in unix seconds, or null/undefined * when the thread was never read (every reply counts unread). * @param isNotified Whether a thread root is one the user is notified for. @@ -49,7 +28,7 @@ function collectSubtreeReplies( */ export function computeThreadBadgeCounts( messages: TimelineMessage[], - directRepliesByParentId: ReadonlyMap, + repliesByRootId: ReadonlyMap, frontiers: ReadonlyMap | undefined, isNotified: (rootId: string) => boolean, currentPubkey?: string, @@ -58,11 +37,8 @@ export function computeThreadBadgeCounts( 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, diff --git a/desktop/src/features/channels/lib/threadBadgeFrontier.test.mjs b/desktop/src/features/channels/lib/threadBadgeFrontier.test.mjs index 20a3c73cf6..80961b470d 100644 --- a/desktop/src/features/channels/lib/threadBadgeFrontier.test.mjs +++ b/desktop/src/features/channels/lib/threadBadgeFrontier.test.mjs @@ -3,16 +3,16 @@ import test from "node:test"; import { nextThreadBadgeFrontier } from "./threadBadgeFrontier.ts"; import { seedThreadBadgeFrontiers } from "./threadBadgeFrontier.ts"; -import { buildDirectRepliesByParentId } from "./subtreeCreatedAt.ts"; +import { buildRepliesByRootId } from "./subtreeCreatedAt.ts"; import { computeThreadBadgeCounts } from "./threadBadgeCounts.ts"; -const msg = (id, parentId) => ({ id, parentId }); +const msg = (id, parentId) => ({ id, parentId, rootId: parentId ?? id }); const seedAll = () => true; const seed = (frontiers, messages, isNotified, getReadAt) => seedThreadBadgeFrontiers( frontiers, messages, - buildDirectRepliesByParentId(messages), + buildRepliesByRootId(messages), isNotified, getReadAt, ); @@ -131,14 +131,14 @@ test("seedThreadBadgeFrontiers_channelMarkerFoldedIntoSeed_badgeVanishes_DEFECT" // already advanced the channel marker to 250, so the EFFECTIVE marker // getReadAt returns is max(100, 250) = 250. const messages = [reply("root", null, 50), reply("r1", "root", 200)]; - const directReplies = buildDirectRepliesByParentId(messages); + const repliesByRootId = buildRepliesByRootId(messages); const foldedEffectiveMarker = Math.max(100, 250); // thread_own vs channel const frontiers = new Map(); seedThreadBadgeFrontiers( frontiers, messages, - directReplies, + repliesByRootId, seedAll, () => foldedEffectiveMarker, ); @@ -147,7 +147,7 @@ test("seedThreadBadgeFrontiers_channelMarkerFoldedIntoSeed_badgeVanishes_DEFECT" const result = computeThreadBadgeCounts( messages, - directReplies, + repliesByRootId, frontiers, seedAll, ); @@ -159,14 +159,14 @@ test("seedThreadBadgeFrontiers_preMarkReadMarkerSeeded_badgeSurvives_DESIRED", ( // Identical thread, but the seed reads the PRE-mark-read marker: the thread's // own marker (100), captured before the channel-open fold advanced it to 250. const messages = [reply("root", null, 50), reply("r1", "root", 200)]; - const directReplies = buildDirectRepliesByParentId(messages); + const repliesByRootId = buildRepliesByRootId(messages); const preMarkReadMarker = 100; // thread_own only, channel fold not applied const frontiers = new Map(); seedThreadBadgeFrontiers( frontiers, messages, - directReplies, + repliesByRootId, seedAll, () => preMarkReadMarker, ); @@ -175,7 +175,7 @@ test("seedThreadBadgeFrontiers_preMarkReadMarkerSeeded_badgeSurvives_DESIRED", ( const result = computeThreadBadgeCounts( messages, - directReplies, + repliesByRootId, frontiers, seedAll, ); @@ -183,31 +183,23 @@ test("seedThreadBadgeFrontiers_preMarkReadMarkerSeeded_badgeSurvives_DESIRED", ( assert.equal(result.get("root"), 1); }); -// --- LP4 Case 3, second face: orphan-only root is never seeded at all --- +// --- LP4 Case 3, second face: orphan-only root IS seed-eligible --- // -// seedThreadBadgeFrontiers gates seed-eligibility on a DIRECT reply existing: -// `if (!directRepliesByParentId.has(message.id)) continue;` (threadBadgeFrontier -// .ts). A root whose ONLY reply is a deep orphan — the middle ancestor unloaded, -// so the orphan keys under its absent parent, not the root — has NO direct-reply -// entry. So the root is skipped: its frontier is never created. This is a -// distinct failure from a wrong COUNT (the count tests above) — here the -// frontier snapshot itself never exists, so even a corrected count path has -// nothing to measure against. -// -// The orphan carries rootId === "root" (getThreadReference resolves it from the -// event's own `root` e-tag regardless of ancestor load state), which is exactly -// the key the redesign will use to make the root seed-eligible. This test is -// expected-RED on current code (root is skipped, so it asserts the DESIRED -// seeded state and fails today) and flips green once seeding keys eligibility -// on rootId-reachability rather than a direct-reply entry. +// seedThreadBadgeFrontiers gates seed-eligibility on a reply existing under the +// root by rootId: `if (!repliesByRootId.has(message.id)) continue;`. A root +// whose ONLY reply is a deep orphan — the middle ancestor unloaded, so the +// orphan keys under its absent parent in the direct-reply map — still owns that +// reply by rootId (getThreadReference resolves rootId from the event's own +// `root` e-tag regardless of ancestor load state). The old direct-reply gate +// skipped such a root entirely, so its frontier never existed and its badge +// could never clear; keying on rootId makes it seed-eligible. This is a face of +// Case 3 distinct from a wrong COUNT — here the frontier snapshot itself was +// missing, so even a corrected count path had nothing to measure against. -test("seedThreadBadgeFrontiers_orphanOnlyRoot_seedEligible_DEFECT", { - todo: "Case 3 second face: direct-reply seed gate skips orphan-only roots; P2 rootId re-key fixes it", -}, () => { +test("seedThreadBadgeFrontiers_orphanOnlyRoot_seedEligible", () => { // root's only reply is `c`, whose middle ancestor `b` is unloaded. `c` keys - // under "b" (absent), so directRepliesByParentId has NO entry for "root" and - // today's seed skips it. DESIRED: the root IS seed-eligible (orphan is part of - // its thread by rootId) and seeds to the read marker 100. + // under "b" (absent) in the direct-reply map but carries rootId === "root", + // so repliesByRootId has an entry for "root" and the root seeds to marker 100. const messages = [ reply("root", null, 50), // reply("b", "root", ...) — intentionally absent: unloaded ancestor. @@ -215,14 +207,12 @@ test("seedThreadBadgeFrontiers_orphanOnlyRoot_seedEligible_DEFECT", { ]; const frontiers = new Map(); seed(frontiers, messages, seedAll, () => 100); - // EXPECTED-RED on current code: root is skipped (frontier absent). Post-fix - // the root is seeded to 100 and this passes. assert.equal(frontiers.get("root"), 100); }); test("seedThreadBadgeFrontiers_directReplyRoot_seedEligible_DESIRED", () => { - // Control: the SAME root with a DIRECT reply present. directRepliesByParentId - // has an entry for "root", so it is seed-eligible today and after the fix. + // Control: the SAME root with a DIRECT reply present. repliesByRootId has an + // entry for "root", so it is seed-eligible — the intact-chain baseline. const messages = [reply("root", null, 50), reply("r1", "root", 200)]; const frontiers = new Map(); seed(frontiers, messages, seedAll, () => 100); diff --git a/desktop/src/features/channels/lib/threadBadgeFrontier.ts b/desktop/src/features/channels/lib/threadBadgeFrontier.ts index c739594d1c..754173764a 100644 --- a/desktop/src/features/channels/lib/threadBadgeFrontier.ts +++ b/desktop/src/features/channels/lib/threadBadgeFrontier.ts @@ -35,17 +35,24 @@ export function nextThreadBadgeFrontier( // 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. +// +// Reply-presence is keyed on `repliesByRootId`, not the direct-parent map: a +// root whose only reply is a deep orphan (intermediate ancestor absent from the +// loaded window) has no direct child but still owns that reply by rootId. Gating +// on direct children alone skipped such a root entirely, so its frontier never +// existed and its badge could never clear — the seed-side face of the orphan +// defect that the count rollup fixes on the tally side. export function seedThreadBadgeFrontiers( channelFrontiers: Map, messages: TimelineMessage[], - directRepliesByParentId: ReadonlyMap, + repliesByRootId: 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; + if (!repliesByRootId.has(message.id)) continue; channelFrontiers.set( message.id, nextThreadBadgeFrontier( diff --git a/desktop/src/features/channels/lib/threadBadgeInvariants.test.mjs b/desktop/src/features/channels/lib/threadBadgeInvariants.test.mjs index d361d8e849..3b53223c5a 100644 --- a/desktop/src/features/channels/lib/threadBadgeInvariants.test.mjs +++ b/desktop/src/features/channels/lib/threadBadgeInvariants.test.mjs @@ -6,7 +6,7 @@ import { nextThreadBadgeFrontier, seedThreadBadgeFrontiers, } from "./threadBadgeFrontier.ts"; -import { buildDirectRepliesByParentId } from "./subtreeCreatedAt.ts"; +import { buildRepliesByRootId } from "./subtreeCreatedAt.ts"; // LP4 characterization invariants for thread-unread badges. // @@ -35,7 +35,7 @@ const notifiedAll = () => true; const counts = (messages, frontiers, isNotified = notifiedAll, currentPubkey) => computeThreadBadgeCounts( messages, - buildDirectRepliesByParentId(messages), + buildRepliesByRootId(messages), frontiers, isNotified, currentPubkey, @@ -46,7 +46,7 @@ const seed = (frontiers, messages, getReadAt, isNotified = seedAll) => seedThreadBadgeFrontiers( frontiers, messages, - buildDirectRepliesByParentId(messages), + buildRepliesByRootId(messages), isNotified, getReadAt, ); diff --git a/desktop/src/features/channels/lib/threadOpenCeiling.test.mjs b/desktop/src/features/channels/lib/threadOpenCeiling.test.mjs index 47b58936d9..34255c4c95 100644 --- a/desktop/src/features/channels/lib/threadOpenCeiling.test.mjs +++ b/desktop/src/features/channels/lib/threadOpenCeiling.test.mjs @@ -5,14 +5,16 @@ import { subtreeMaxCreatedAt, buildDirectReplyIdsByParentId, buildCreatedAtByMessageId, + buildRepliesByRootId, } from "./subtreeCreatedAt.ts"; // The thread-open read ceiling: opening a thread advances its read frontier to // subtreeMaxCreatedAt(headId), the newest createdAt anywhere in the head's // subtree. The thread-open caller starts the walk at the ROOT (consume the -// whole thread); the expand caller starts at a BRANCH node (consume only that -// branch). These tests pin both — Case 1's orphan-misses-ceiling defect and the -// call-site split that must NOT let expand cross into a sibling branch. +// whole thread) and folds in rootId-matched orphans; the expand caller starts +// at a BRANCH node (consume only that branch) with no rootId folding. These +// tests pin both — Case 1's orphan-misses-ceiling defect and the call-site +// split that must NOT let expand cross into a sibling branch. const msg = (id, parentId, createdAt, rootId) => ({ id, @@ -21,6 +23,7 @@ const msg = (id, parentId, createdAt, rootId) => ({ createdAt, }); +// Branch-scoped ceiling — the expand caller. Parent-chain walk only. const ceiling = (headId, messages) => subtreeMaxCreatedAt( headId, @@ -28,28 +31,29 @@ const ceiling = (headId, messages) => buildCreatedAtByMessageId(messages), ); +// Root-scoped ceiling — the thread-open caller. Folds in replies that resolve +// to the head by rootId, so a severed orphan still raises the ceiling. +const rootCeiling = (headId, messages) => + subtreeMaxCreatedAt( + headId, + buildDirectReplyIdsByParentId(messages), + buildCreatedAtByMessageId(messages), + buildRepliesByRootId(messages), + ); + // (3) Case 1 — orphan misses the thread-open ceiling. The deep reply `c` is the // newest content (300), but its middle ancestor `b` is unloaded, so `c` keys -// under absent "b" and the root-started walk never reaches it. The ceiling -// stops at the newest REACHABLE node (`a` at 200), leaving `c` permanently -// above the frontier — the channel-root badge can never clear via thread-open. -// -// EXPECTED-RED on current code: the parentId-only walk yields 200, not 300. -// `c.rootId === "root"` is set explicitly — the redesign keys the ceiling walk -// on rootId-reachability so the orphan is included and the assertion flips to -// 300. Asserting the DESIRED value (300) makes this a failing characterization -// of the live defect, not a pin of the bug. -test("openThreadCeiling_deepOrphanMissingAncestor_includedInCeiling_DEFECT", { - todo: "Case 1: parentId walk can't reach the orphan; P2 rootId re-key fixes it", -}, () => { +// under absent "b" and the parentId-only walk never reaches it. The root-scoped +// ceiling folds in `c` by its rootId ("root", which travels with the event via +// getThreadReference), so it reaches 300 and the channel-root badge can clear. +test("openThreadCeiling_deepOrphanMissingAncestor_includedInCeiling", () => { const loaded = [ msg("root", null, 50, "root"), msg("a", "root", 200, "root"), // msg("b", "a", ...) — intentionally absent: unloaded middle ancestor. msg("c", "b", 300, "root"), ]; - // DESIRED: ceiling reaches the orphan's 300. Current code returns 200 (RED). - assert.equal(ceiling("root", loaded), 300); + assert.equal(rootCeiling("root", loaded), 300); }); // (3) control — with the middle ancestor present the chain is intact and the diff --git a/desktop/src/features/channels/ui/useChannelUnreadState.ts b/desktop/src/features/channels/ui/useChannelUnreadState.ts index e1f498f8e0..e3a6ef999b 100644 --- a/desktop/src/features/channels/ui/useChannelUnreadState.ts +++ b/desktop/src/features/channels/ui/useChannelUnreadState.ts @@ -2,8 +2,8 @@ import * as React from "react"; import { buildCreatedAtByMessageId, - buildDirectRepliesByParentId, buildDirectReplyIdsByParentId, + buildRepliesByRootId, collectReplyDescendantIds, subtreeMaxCreatedAt, } from "@/features/channels/lib/subtreeCreatedAt"; @@ -118,8 +118,8 @@ export function useChannelUnreadState({ () => buildDirectReplyIdsByParentId(timelineMessages), [timelineMessages], ); - const directRepliesByParentId = React.useMemo( - () => buildDirectRepliesByParentId(timelineMessages), + const repliesByRootId = React.useMemo( + () => buildRepliesByRootId(timelineMessages), [timelineMessages], ); const getFirstReplyIdForMessage = React.useCallback( @@ -148,6 +148,24 @@ export function useChannelUnreadState({ ), [createdAtByMessageId, directReplyIdsByParentId], ); + // Root-scoped variant of the ceiling, used only by the thread-open mark-read + // effect below. Folds in replies that resolve to the root by rootId, so a + // severed orphan (intermediate ancestor outside the loaded window) still + // raises the ceiling and the channel-root badge can clear on open. The + // branch-scoped getSubtreeMaxCreatedAt above stays as-is for the expand + // caller, which must advance only its own branch — a branch node owns no + // rootId bucket, so passing repliesByRootId there would be a no-op anyway, + // but keeping the two callbacks distinct makes the scope intent explicit. + const getRootSubtreeMaxCreatedAt = React.useCallback( + (rootId: string) => + subtreeMaxCreatedAt( + rootId, + directReplyIdsByParentId, + createdAtByMessageId, + repliesByRootId, + ), + [createdAtByMessageId, directReplyIdsByParentId, repliesByRootId], + ); const threadPanelIndex = React.useMemo( () => buildThreadPanelIndex(timelineMessages), [timelineMessages], @@ -232,10 +250,15 @@ export function useChannelUnreadState({ React.useEffect(() => { if (!openThreadHeadId) return; if (isThreadMuted(openThreadHeadId)) return; - const openReadCeiling = getSubtreeMaxCreatedAt(openThreadHeadId); + const openReadCeiling = getRootSubtreeMaxCreatedAt(openThreadHeadId); if (openReadCeiling === null) return; markThreadRead(openThreadHeadId, openReadCeiling); - }, [openThreadHeadId, getSubtreeMaxCreatedAt, markThreadRead, isThreadMuted]); + }, [ + openThreadHeadId, + getRootSubtreeMaxCreatedAt, + markThreadRead, + isThreadMuted, + ]); // Compute the in-thread "New" divider position from the open-time frontier. const { firstUnreadReplyId: threadFirstUnreadReplyId } = React.useMemo(() => { if (!openThreadHeadId || threadMessages.length === 0) { @@ -307,7 +330,7 @@ export function useChannelUnreadState({ seedThreadBadgeFrontiers( channelFrontiers, timelineMessages, - directRepliesByParentId, + repliesByRootId, (rootId) => !isThreadMuted(rootId), (rootId) => getThreadReadAt(rootId, activeChannelId), ); @@ -330,7 +353,7 @@ export function useChannelUnreadState({ () => computeThreadBadgeCounts( timelineMessages, - directRepliesByParentId, + repliesByRootId, activeChannelId ? threadBadgeFrontiersRef.current.get(activeChannelId) : undefined, @@ -341,7 +364,7 @@ export function useChannelUnreadState({ activeChannelId, currentPubkey, timelineMessages, - directRepliesByParentId, + repliesByRootId, isThreadMuted, readStateVersion, ],