diff --git a/desktop/src/features/channels/ui/ChannelScreen.tsx b/desktop/src/features/channels/ui/ChannelScreen.tsx index 9e4737957..18c47c00f 100644 --- a/desktop/src/features/channels/ui/ChannelScreen.tsx +++ b/desktop/src/features/channels/ui/ChannelScreen.tsx @@ -33,7 +33,10 @@ import { collectMessageMentionPubkeys, formatTimelineMessages, } from "@/features/messages/lib/formatTimelineMessages"; -import { buildThreadPanelData } from "@/features/messages/lib/threadPanel"; +import { + buildDescendantStatsByMessageId, + buildThreadPanelData, +} from "@/features/messages/lib/threadPanel"; import { imetaMediaFromTags } from "@/features/messages/lib/imetaMediaMarkdown"; import { useFetchOlderMessages } from "@/features/messages/useFetchOlderMessages"; import { useLoadMissingAncestors } from "@/features/messages/useLoadMissingAncestors"; @@ -317,6 +320,16 @@ export function ChannelScreen({ }, [directReplyIdsByParentId], ); + // A.3.1: the channel-wide descendant walk is the expensive O(N x avg-depth) + // pass. Keying it on `timelineMessages` ALONE means a thread-open or expand + // (which flip `openThreadHeadId` / `expandedThreadReplyIds`) no longer + // re-fires the whole-channel walk — only the cheap per-thread slice below + // re-runs. The same map is shared with `buildThreadPanelData` so the channel + // is walked once, not twice, per `timelineMessages` change. + const descendantStatsByMessageId = React.useMemo( + () => buildDescendantStatsByMessageId(timelineMessages), + [timelineMessages], + ); const threadPanelData = React.useMemo( () => buildThreadPanelData( @@ -324,8 +337,10 @@ export function ChannelScreen({ openThreadHeadId, threadReplyTargetId, expandedThreadReplyIds, + descendantStatsByMessageId, ), [ + descendantStatsByMessageId, expandedThreadReplyIds, openThreadHeadId, threadReplyTargetId, diff --git a/desktop/src/features/messages/lib/threadPanel.test.mjs b/desktop/src/features/messages/lib/threadPanel.test.mjs index 5129ec871..c9e719b82 100644 --- a/desktop/src/features/messages/lib/threadPanel.test.mjs +++ b/desktop/src/features/messages/lib/threadPanel.test.mjs @@ -2,6 +2,7 @@ import assert from "node:assert/strict"; import test from "node:test"; import { + buildDescendantStatsByMessageId, buildMainTimelineEntries, buildThreadPanelData, } from "./threadPanel.ts"; @@ -100,3 +101,203 @@ test("buildThreadPanelData keeps direct comments unindented", () => { ], ); }); + +// A.3.1 regression microbench: the channel-wide descendant walk must run ONCE +// per `timelineMessages` change, not once per thread-open. The ChannelScreen +// memo computes `buildDescendantStatsByMessageId(timelineMessages)` keyed on the +// message set alone, then shares the result with every `buildThreadPanelData` +// call. This test mirrors that contract and counts the heavy walks. +function buildChannel(messageCount, branchingDepth) { + const messages = [message({ id: "root", createdAt: 1 })]; + let parentId = "root"; + for (let index = 1; index < messageCount; index += 1) { + const id = `m${index}`; + messages.push( + message({ + id, + createdAt: index + 1, + parentId, + rootId: "root", + depth: 1, + tags: [["e", parentId, "", "reply"]], + }), + ); + // Re-anchor to root every `branchingDepth` to vary the tree shape. + parentId = index % branchingDepth === 0 ? "root" : id; + } + return messages; +} + +test("A.3.1: channel-wide walk runs once per timelineMessages change, not per thread-open", () => { + const messages = buildChannel(200, 5); + + // Count how many times the expensive whole-channel walk actually fires. + let walkCount = 0; + const countingBuildStats = (msgs) => { + walkCount += 1; + return buildDescendantStatsByMessageId(msgs); + }; + + // The ChannelScreen seam: compute the channel-wide stats ONCE for this + // `timelineMessages` identity... + const sharedStats = countingBuildStats(messages); + + // ...then drive many thread-opens / expands reusing the shared map. None of + // these should re-walk the whole channel. + const threadOpenIds = ["root", "m5", "m10", "m25", "m50", "m100"]; + const results = threadOpenIds.map((openThreadHeadId) => + buildThreadPanelData( + messages, + openThreadHeadId, + openThreadHeadId, + new Set(), + sharedStats, + ), + ); + + // Exactly one whole-channel walk despite 6 thread-opens. + assert.equal( + walkCount, + 1, + `expected 1 channel-wide walk for ${threadOpenIds.length} thread-opens, got ${walkCount}`, + ); + + // The shared-stats path must produce identical output to the + // build-it-internally path (back-compat: omitting the arg recomputes). + for (let index = 0; index < threadOpenIds.length; index += 1) { + const openThreadHeadId = threadOpenIds[index]; + const recomputed = buildThreadPanelData( + messages, + openThreadHeadId, + openThreadHeadId, + new Set(), + ); + assert.equal( + results[index].totalReplyCount, + recomputed.totalReplyCount, + `totalReplyCount mismatch for thread ${openThreadHeadId}`, + ); + assert.deepEqual( + results[index].visibleReplies.map((entry) => entry.message.id), + recomputed.visibleReplies.map((entry) => entry.message.id), + `visibleReplies mismatch for thread ${openThreadHeadId}`, + ); + } + + // The main-timeline path shares the same map too — still one walk total. + const mainEntries = buildMainTimelineEntries(messages, sharedStats); + assert.equal( + walkCount, + 1, + "buildMainTimelineEntries must reuse the shared stats, not re-walk", + ); + assert.ok(mainEntries.length > 0); +}); + +// Per-id stabilization: thread rows feed `MessageRow` a depth-normalized copy +// of each reply. When `timelineMessages` churns (typing/presence) but the +// reply objects survive by reference, rebuilding the thread panel must hand +// `MessageRow` the SAME normalized object reference so the row/markdown memo +// hits — instead of a fresh `{ ...reply, depth }` spread every render. +test("thread reply objects keep identity across unrelated timelineMessages churn", () => { + const root = message({ id: "root", createdAt: 1 }); + const replyA = message({ + id: "a", + createdAt: 2, + parentId: "root", + rootId: "root", + depth: 1, + tags: [["e", "root", "", "reply"]], + }); + const replyB = message({ + id: "b", + createdAt: 3, + parentId: "a", + rootId: "root", + depth: 2, + tags: [["e", "a", "", "reply"]], + }); + + // First render of the thread. + const first = buildThreadPanelData( + [root, replyA, replyB], + "root", + "root", + new Set(["a"]), + ); + + // An unrelated channel churn produces a NEW `timelineMessages` array, but the + // reply objects themselves are reused by reference (only their position in + // the surrounding array changed — e.g. a presence ping or typing indicator + // that the snapshot layer leaves the reply identities intact for). + const churned = [ + message({ id: "noise", createdAt: 99 }), + root, + replyA, + replyB, + ]; + const second = buildThreadPanelData(churned, "root", "root", new Set(["a"])); + + const firstById = new Map( + first.visibleReplies.map((entry) => [entry.message.id, entry.message]), + ); + const secondById = new Map( + second.visibleReplies.map((entry) => [entry.message.id, entry.message]), + ); + + assert.ok(firstById.size > 0, "expected at least one visible reply"); + for (const [id, normalized] of firstById) { + assert.strictEqual( + secondById.get(id), + normalized, + `normalized reply ${id} must be the SAME object reference across an unrelated churn (memo hit)`, + ); + // Depth must still reach the row correctly via the cached object. + assert.equal( + typeof normalized.depth, + "number", + `normalized reply ${id} must carry a numeric depth`, + ); + } +}); + +test("thread reply objects recompute when the source reply object is replaced", () => { + const root = message({ id: "root", createdAt: 1 }); + const reply = message({ + id: "a", + createdAt: 2, + parentId: "root", + rootId: "root", + depth: 1, + tags: [["e", "root", "", "reply"]], + }); + + const first = buildThreadPanelData([root, reply], "root", "root", new Set()); + + // A genuine edit/refresh: the reply is a brand-new object (new identity). + const editedReply = message({ + id: "a", + createdAt: 2, + parentId: "root", + rootId: "root", + depth: 1, + body: "edited body", + tags: [["e", "root", "", "reply"]], + }); + const second = buildThreadPanelData( + [root, editedReply], + "root", + "root", + new Set(), + ); + + const firstA = first.visibleReplies.find((e) => e.message.id === "a"); + const secondA = second.visibleReplies.find((e) => e.message.id === "a"); + assert.ok(firstA && secondA, "expected reply 'a' in both renders"); + assert.notStrictEqual( + secondA.message, + firstA.message, + "a replaced source reply must produce a fresh normalized object", + ); + assert.equal(secondA.message.body, "edited body"); +}); diff --git a/desktop/src/features/messages/lib/threadPanel.ts b/desktop/src/features/messages/lib/threadPanel.ts index 03baa1dd5..f598eae9a 100644 --- a/desktop/src/features/messages/lib/threadPanel.ts +++ b/desktop/src/features/messages/lib/threadPanel.ts @@ -41,14 +41,47 @@ function normalizeHeadMessage(message: TimelineMessage): TimelineMessage { }; } +// Thread rows feed `MessageRow` a depth-normalized copy of each reply. Building +// that copy fresh (`{ ...message, depth }`) on every render hands `MessageRow` a +// new object identity every time `timelineMessages` churns (typing/presence), +// even when the reply and its depth are byte-identical — which defeats the +// row/markdown memo and forces a ~1.4ms/row re-parse on threads where the main +// timeline (which passes the raw stable ref) stays cheap. +// +// Mirror the main list's per-id context memoization (`videoReviewContextById`): +// cache the normalized object keyed on the source reply identity + depth, so an +// unrelated channel churn that leaves a reply (and its tree position) intact +// reuses the exact same object reference and the memo hits. +// +// Keyed on the source `reply` reference via a WeakMap: a new `timelineMessages` +// set produces new reply objects (genuine recompute), and stale entries are +// collected automatically when the old message set is dropped. +const normalizedInlineReplyCache = new WeakMap< + TimelineMessage, + Map +>(); + function normalizeInlineReplyMessage( message: TimelineMessage, depth: number, ): TimelineMessage { - return { + let byDepth = normalizedInlineReplyCache.get(message); + if (!byDepth) { + byDepth = new Map(); + normalizedInlineReplyCache.set(message, byDepth); + } + + const cached = byDepth.get(depth); + if (cached) { + return cached; + } + + const normalized: TimelineMessage = { ...message, depth, }; + byDepth.set(depth, normalized); + return normalized; } function buildDirectChildrenByParentId(messages: TimelineMessage[]) { @@ -67,7 +100,12 @@ function buildDirectChildrenByParentId(messages: TimelineMessage[]) { return childrenByParentId; } -function buildDescendantStatsByMessageId( +// A.3.1: the channel-wide descendant walk is O(N x avg-depth) and depends ONLY +// on the timeline message set. Both render paths (main timeline + thread panel) +// need it, so it is exported to be computed once per `timelineMessages` change +// and shared, instead of re-walking the whole channel on every thread-open / +// expand. Memoize this on `messages` identity at the call site. +export function buildDescendantStatsByMessageId( messages: TimelineMessage[], ): Map { const messageById = new Map(messages.map((message) => [message.id, message])); @@ -220,8 +258,11 @@ function buildVisibleThreadReplies(params: { export function buildMainTimelineEntries( messages: TimelineMessage[], + precomputedDescendantStatsByMessageId?: Map, ): MainTimelineEntry[] { - const descendantStatsByMessageId = buildDescendantStatsByMessageId(messages); + const descendantStatsByMessageId = + precomputedDescendantStatsByMessageId ?? + buildDescendantStatsByMessageId(messages); return messages .filter( @@ -244,6 +285,7 @@ export function buildThreadPanelData( openThreadHeadId: string | null, threadReplyTargetId: string | null, expandedReplyIds: ReadonlySet, + precomputedDescendantStatsByMessageId?: Map, ): ThreadPanelData { if (!openThreadHeadId) { return { @@ -267,7 +309,11 @@ export function buildThreadPanelData( } const directChildrenByParentId = buildDirectChildrenByParentId(messages); - const descendantStatsByMessageId = buildDescendantStatsByMessageId(messages); + + const descendantStatsByMessageId = + precomputedDescendantStatsByMessageId ?? + buildDescendantStatsByMessageId(messages); + const normalizedThreadHead = normalizeHeadMessage(threadHead); const visibleReplies = buildVisibleThreadReplies({ openThreadHeadId, diff --git a/desktop/src/features/messages/lib/timelineSnapshot.test.mjs b/desktop/src/features/messages/lib/timelineSnapshot.test.mjs new file mode 100644 index 000000000..f16c6e7a9 --- /dev/null +++ b/desktop/src/features/messages/lib/timelineSnapshot.test.mjs @@ -0,0 +1,289 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { + BOTTOM_THRESHOLD_PX, + buildDayGroupBoundaries, + isNearBottomMetrics, + resolveDeepLinkTarget, + selectDeferredListRenderState, + selectLatestMessageKey, +} from "./timelineSnapshot.ts"; + +// Local-midnight unix-second timestamps so isSameDay (local time) is stable +// regardless of the machine's timezone. +function dayAt(year, month, day, hour = 12) { + return Math.floor( + new Date(year, month - 1, day, hour, 0, 0).getTime() / 1_000, + ); +} + +function message(overrides) { + return { + id: "message", + renderKey: undefined, + createdAt: dayAt(2026, 6, 14), + pubkey: "author", + author: "Author", + avatarUrl: null, + role: undefined, + personaDisplayName: undefined, + time: "12:00 PM", + body: "body", + parentId: null, + rootId: null, + depth: 0, + accent: false, + pending: undefined, + edited: false, + kind: 9, + tags: [], + reactions: undefined, + ...overrides, + }; +} + +// --- sticky-bottom autoscroll ------------------------------------------------- + +test("isNearBottomMetrics: true when within threshold of the bottom", () => { + // distance = scrollHeight - clientHeight - scrollTop = 1000 - 600 - 380 = 20 + assert.equal( + isNearBottomMetrics({ + scrollHeight: 1_000, + clientHeight: 600, + scrollTop: 380, + }), + true, + ); +}); + +test("isNearBottomMetrics: true exactly at the threshold boundary", () => { + const scrollTop = 1_000 - 600 - BOTTOM_THRESHOLD_PX; // distance === threshold + assert.equal( + isNearBottomMetrics({ scrollHeight: 1_000, clientHeight: 600, scrollTop }), + true, + ); +}); + +test("isNearBottomMetrics: false when scrolled up beyond the threshold", () => { + // distance = 1000 - 600 - 100 = 300 > 72 + assert.equal( + isNearBottomMetrics({ + scrollHeight: 1_000, + clientHeight: 600, + scrollTop: 100, + }), + false, + ); +}); + +test("selectLatestMessageKey: prefers renderKey, falls back to id, undefined when empty", () => { + assert.equal(selectLatestMessageKey([]), undefined); + assert.equal( + selectLatestMessageKey([message({ id: "a" }), message({ id: "b" })]), + "b", + ); + assert.equal( + selectLatestMessageKey([message({ id: "b", renderKey: "local-b" })]), + "local-b", + ); +}); + +test("selectLatestMessageKey: detects a newly arrived latest message", () => { + const before = [message({ id: "a" }), message({ id: "b" })]; + const after = [ + ...before, + message({ id: "c", createdAt: dayAt(2026, 6, 14, 13) }), + ]; + assert.notEqual( + selectLatestMessageKey(before), + selectLatestMessageKey(after), + ); +}); + +// --- day dividers ------------------------------------------------------------- + +test("buildDayGroupBoundaries: empty snapshot has no groups", () => { + assert.deepEqual(buildDayGroupBoundaries([]), []); +}); + +test("buildDayGroupBoundaries: messages on one day form a single group", () => { + const messages = [ + message({ id: "a", createdAt: dayAt(2026, 6, 14, 9) }), + message({ id: "b", createdAt: dayAt(2026, 6, 14, 10) }), + message({ id: "c", createdAt: dayAt(2026, 6, 14, 23) }), + ]; + const groups = buildDayGroupBoundaries(messages); + assert.equal(groups.length, 1); + assert.deepEqual( + { startIndex: groups[0].startIndex, count: groups[0].count }, + { startIndex: 0, count: 3 }, + ); +}); + +test("buildDayGroupBoundaries: a day boundary opens a new group", () => { + const messages = [ + message({ id: "a", createdAt: dayAt(2026, 6, 13, 22) }), + message({ id: "b", createdAt: dayAt(2026, 6, 14, 1) }), + message({ id: "c", createdAt: dayAt(2026, 6, 14, 2) }), + message({ id: "d", createdAt: dayAt(2026, 6, 15, 8) }), + ]; + const groups = buildDayGroupBoundaries(messages); + assert.deepEqual( + groups.map((g) => ({ startIndex: g.startIndex, count: g.count })), + [ + { startIndex: 0, count: 1 }, + { startIndex: 1, count: 2 }, + { startIndex: 3, count: 1 }, + ], + ); +}); + +test("buildDayGroupBoundaries: group counts always sum to message count", () => { + const messages = [ + message({ id: "a", createdAt: dayAt(2026, 6, 13) }), + message({ id: "b", createdAt: dayAt(2026, 6, 14) }), + message({ id: "c", createdAt: dayAt(2026, 6, 14) }), + ]; + const total = buildDayGroupBoundaries(messages).reduce( + (n, g) => n + g.count, + 0, + ); + assert.equal(total, messages.length); +}); + +// --- jump-to-message deep links ---------------------------------------------- + +test("resolveDeepLinkTarget: unresolved with no target", () => { + const messages = [message({ id: "a" })]; + assert.deepEqual(resolveDeepLinkTarget(messages, null), { + resolved: false, + index: -1, + }); + assert.deepEqual(resolveDeepLinkTarget(messages, undefined), { + resolved: false, + index: -1, + }); +}); + +test("resolveDeepLinkTarget: resolves a present target to its index", () => { + const messages = [ + message({ id: "a" }), + message({ id: "b" }), + message({ id: "c" }), + ]; + assert.deepEqual(resolveDeepLinkTarget(messages, "b"), { + resolved: true, + index: 1, + }); +}); + +test("resolveDeepLinkTarget: unresolved when the target is not in the snapshot", () => { + const messages = [message({ id: "a" }), message({ id: "b" })]; + assert.deepEqual(resolveDeepLinkTarget(messages, "missing"), { + resolved: false, + index: -1, + }); +}); + +// --- shared-snapshot / no-tearing guarantee ---------------------------------- +// +// All three must-keep decisions must read off the SAME snapshot. If the deep-link +// decision reads a fresh snapshot while the rendered list / scroll math still +// read a stale one, the jump fires against a row that hasn't committed and +// silently fails. + +test("no-tearing: a target only in the fresh snapshot does NOT resolve against the stale one", () => { + const stale = [message({ id: "a" }), message({ id: "b" })]; + const fresh = [ + ...stale, + message({ id: "target", createdAt: dayAt(2026, 6, 15) }), + ]; + + // Reading the deep link against the stale snapshot (what the painted DOM + // still shows) must report unresolved — you can't scroll to an uncommitted row. + assert.equal(resolveDeepLinkTarget(stale, "target").resolved, false); + // Against the fresh snapshot it resolves — proving the gate is which snapshot + // you feed, not the function. + assert.equal(resolveDeepLinkTarget(fresh, "target").resolved, true); +}); + +test("no-tearing: all three decisions agree when fed one shared snapshot", () => { + const snapshot = [ + message({ id: "a", createdAt: dayAt(2026, 6, 13) }), + message({ id: "b", createdAt: dayAt(2026, 6, 14) }), + message({ + id: "target", + renderKey: "rk-target", + createdAt: dayAt(2026, 6, 14, 18), + }), + ]; + + // Deep link resolves to the last index... + const link = resolveDeepLinkTarget(snapshot, "target"); + // ...the day grouping covers that same index... + const groups = buildDayGroupBoundaries(snapshot); + const coveredCount = groups.reduce((n, g) => n + g.count, 0); + // ...and the sticky-bottom latest-key points at that same final message. + const latestKey = selectLatestMessageKey(snapshot); + + assert.equal(link.index, snapshot.length - 1); + assert.equal(coveredCount, snapshot.length); + assert.equal(latestKey, snapshot[snapshot.length - 1].renderKey); +}); + +test("no-tearing: stale snapshot keeps all three decisions internally consistent", () => { + // Feeding the stale list everywhere keeps the decisions consistent with each + // other — none of them see the uncommitted row. + const stale = [ + message({ id: "a", createdAt: dayAt(2026, 6, 14, 9) }), + message({ id: "b", createdAt: dayAt(2026, 6, 14, 10) }), + ]; + + const link = resolveDeepLinkTarget(stale, "target"); + const coveredCount = buildDayGroupBoundaries(stale).reduce( + (n, g) => n + g.count, + 0, + ); + const latestKey = selectLatestMessageKey(stale); + + assert.equal(link.resolved, false); + assert.equal(coveredCount, stale.length); + assert.equal(latestKey, "b"); +}); + +// --- deferred reply-list render state (thread side pane) -------------------- +// +// When MessageThreadPanel gates its reply render behind useDeferredValue, the +// painted (deferred) snapshot lags the live one for a frame. selectDeferredList +// RenderState picks which of three states the reply region paints so we never +// flash "No replies" over a list that's streaming in on the deferred commit. + +test("deferred-render: paints the list whenever the deferred snapshot has rows", () => { + // deferred caught up — normal steady state. + assert.equal(selectDeferredListRenderState(3, 3), "list"); + // deferred still showing the OLD non-empty list while a new one streams in; + // we keep painting rows (no flash) — the dim-pending styling handles the lag. + assert.equal(selectDeferredListRenderState(2, 5), "list"); +}); + +test("deferred-render: empty state only when the LIVE list is genuinely empty", () => { + // Both empty — the thread truly has no replies. + assert.equal(selectDeferredListRenderState(0, 0), "empty"); +}); + +test("deferred-render: pending when deferred is empty but live has content", () => { + // Deferred snapshot hasn't committed the rows yet but the live list is + // non-empty. Must NOT report "empty" — that would flash the "No replies" + // affordance for a frame on thread-open. + assert.equal(selectDeferredListRenderState(0, 4), "pending"); + assert.notEqual(selectDeferredListRenderState(0, 4), "empty"); +}); + +test("deferred-render: keys the empty decision off the live count, not deferred", () => { + // Same deferred count (0), opposite verdicts — proving the live count is the + // tie-breaker. This is the no-tearing guarantee for the empty affordance: + // the empty state is a function of the LIVE list, never the lagging one. + assert.equal(selectDeferredListRenderState(0, 0), "empty"); + assert.equal(selectDeferredListRenderState(0, 1), "pending"); +}); diff --git a/desktop/src/features/messages/lib/timelineSnapshot.ts b/desktop/src/features/messages/lib/timelineSnapshot.ts new file mode 100644 index 000000000..c0fdff16d --- /dev/null +++ b/desktop/src/features/messages/lib/timelineSnapshot.ts @@ -0,0 +1,150 @@ +/** + * Pure helpers that read a timeline message snapshot to compute the values the + * timeline render needs: sticky-bottom autoscroll, day dividers, jump-to-message + * deep links, and the deferred reply-list render state. + * + * Keeping these out of the component render body / scroll-manager effects lets + * them be covered by the lib-level `*.test.mjs` suite. It also enforces the key + * correctness property: every decision must read off the SAME snapshot. If the + * deep-link lookup reads a fresher list than the rows the DOM has actually + * committed, a jump fires against a row that isn't there yet and silently fails. + */ + +import type { TimelineMessage } from "@/features/messages/types"; +import { isSameDay } from "./dateFormatters"; + +/** Distance (px) from the bottom within which the timeline counts as "at bottom". */ +export const BOTTOM_THRESHOLD_PX = 72; + +/** Minimal scroll geometry the sticky-bottom decision needs — a pure subset of the DOM element. */ +export type ScrollMetrics = { + scrollHeight: number; + clientHeight: number; + scrollTop: number; +}; + +/** + * Is the timeline scrolled close enough to the bottom to count as "at bottom"? + * Pure over geometry so the threshold math is testable without a DOM. + */ +export function isNearBottomMetrics(metrics: ScrollMetrics): boolean { + return ( + metrics.scrollHeight - metrics.clientHeight - metrics.scrollTop <= + BOTTOM_THRESHOLD_PX + ); +} + +/** Reads live scroll geometry off a container and applies the bottom-threshold rule. */ +export function isNearBottom(container: HTMLDivElement): boolean { + return isNearBottomMetrics({ + scrollHeight: container.scrollHeight, + clientHeight: container.clientHeight, + scrollTop: container.scrollTop, + }); +} + +/** + * Identity of the last message in a snapshot, used to detect "a new latest + * message arrived" for autoscroll. Prefers `renderKey` (stable across optimistic + * send-ack) and falls back to `id`. Returns `undefined` for an empty snapshot. + */ +export function selectLatestMessageKey( + messages: readonly TimelineMessage[], +): string | undefined { + if (messages.length === 0) { + return undefined; + } + const latest = messages[messages.length - 1]; + return latest.renderKey ?? latest.id; +} + +/** A single day boundary in the timeline: where it starts and how many messages it covers. */ +export type DayGroupBoundary = { + /** Stable key for the day section. */ + key: string; + /** Index into `messages` of the first message in this day. */ + startIndex: number; + /** Number of messages in this day group. */ + count: number; + /** The `createdAt` (unix seconds) used to render the heading label. */ + headingTimestamp: number; +}; + +/** + * Walks a snapshot in order and produces the day-group boundaries. A new group + * starts at index 0 and whenever a message falls on a different calendar day + * than the one before it. + */ +export function buildDayGroupBoundaries( + messages: readonly TimelineMessage[], +): DayGroupBoundary[] { + const boundaries: DayGroupBoundary[] = []; + + for (let i = 0; i < messages.length; i++) { + const message = messages[i]; + const prev = i > 0 ? messages[i - 1] : null; + + if (!prev || !isSameDay(prev.createdAt, message.createdAt)) { + boundaries.push({ + key: `day-${message.createdAt}`, + startIndex: i, + count: 1, + headingTimestamp: message.createdAt, + }); + } else { + boundaries[boundaries.length - 1].count += 1; + } + } + + return boundaries; +} + +/** Outcome of resolving a deep-link target against the current snapshot. */ +export type DeepLinkResolution = { + /** Whether the target message exists in this snapshot (i.e. a row would be committed). */ + resolved: boolean; + /** Index of the target in `messages`, or -1 when unresolved. */ + index: number; +}; + +/** + * Does a jump-to-message target resolve against THIS snapshot? The scroll-manager + * effect only does `querySelector` + `scrollIntoView` once a target row is + * actually committed, so the jump must read the same snapshot the list rendered + * — otherwise it scrolls to a row that isn't there yet. + */ +export function resolveDeepLinkTarget( + messages: readonly TimelineMessage[], + targetMessageId: string | null | undefined, +): DeepLinkResolution { + if (!targetMessageId) { + return { resolved: false, index: -1 }; + } + const index = messages.findIndex((message) => message.id === targetMessageId); + return { resolved: index !== -1, index }; +} + +/** + * Which of three states a deferred list should paint. A list gated behind + * `useDeferredValue` lags the live one for a frame, so the deferred snapshot can + * be empty while the live list is not. Keying the empty state off the LIVE count + * stops us flashing an "empty" affordance over a list that's streaming in: + * + * - "list" → the deferred snapshot has rows; paint them + * - "empty" → the LIVE list is genuinely empty; paint the empty state + * - "pending" → deferred is empty but live has content; paint nothing yet + */ +export type DeferredListRenderState = "list" | "empty" | "pending"; + +export function selectDeferredListRenderState( + deferredCount: number, + liveCount: number, +): DeferredListRenderState { + if (deferredCount > 0) { + return "list"; + } + if (liveCount === 0) { + return "empty"; + } + return "pending"; +} diff --git a/desktop/src/features/messages/ui/MessageThreadPanel.tsx b/desktop/src/features/messages/ui/MessageThreadPanel.tsx index 5e8bad43d..1cc199a24 100644 --- a/desktop/src/features/messages/ui/MessageThreadPanel.tsx +++ b/desktop/src/features/messages/ui/MessageThreadPanel.tsx @@ -29,6 +29,7 @@ import { MessageThreadSummaryRow } from "./MessageThreadSummaryRow"; import { TypingIndicatorRow } from "./TypingIndicatorRow"; import { useComposerHeightPadding } from "./useComposerHeightPadding"; import { useTimelineScrollManager } from "./useTimelineScrollManager"; +import { selectDeferredListRenderState } from "@/features/messages/lib/timelineSnapshot"; type MessageThreadPanelProps = { agentPubkeys?: ReadonlySet; @@ -80,6 +81,12 @@ type MessageThreadPanelProps = { onUnfollowThread?: () => void; }; +/** Stable empty reference used as the `useDeferredValue` initial value so the + * first render when a thread opens stays light instead of blocking on the full + * reply list. Must be module-level so its identity never changes. Mirrors + * `EMPTY_MESSAGES` in MessageTimeline. */ +const EMPTY_THREAD_REPLIES: MainTimelineEntry[] = []; + function canManageMessage( message: TimelineMessage, currentPubkey: string | undefined, @@ -150,9 +157,35 @@ export function MessageThreadPanel({ } : null; + // The thread side pane renders its reply list straight into heavy + // `react-markdown` rows (`MessageRow`), so opening a deep thread would block + // the main thread and the OS would show the busy cursor. Gate the reply render + // behind `useDeferredValue`. `initialValue: []` keeps even the FIRST render on + // thread-open light; the heavy list streams in on a deferred, interruptible + // commit. We deliberately drive BOTH the scroll manager and the rendered list + // off the SAME deferred value — sticky-bottom / deep-link logic reads the DOM + // (`scrollIntoView`), so it must stay consistent with what's actually painted. + // You can't scroll to a reply that hasn't committed yet. The thread pane gets + // this no-tearing guarantee for free by routing through the same + // `useTimelineScrollManager` (and its `timelineSnapshot` helpers) as the main + // timeline. + const deferredThreadReplies = React.useDeferredValue( + threadReplies, + EMPTY_THREAD_REPLIES, + ); + const isRepliesPending = deferredThreadReplies !== threadReplies; + + // Which of the three states the reply region paints this frame. Delegated to + // a pure helper so the "don't flash empty over an incoming list" rule is + // covered in the lib test suite (see selectDeferredListRenderState). + const repliesRenderState = selectDeferredListRenderState( + deferredThreadReplies.length, + threadReplies.length, + ); + const threadMessages = React.useMemo( - () => threadReplies.map((entry) => entry.message), - [threadReplies], + () => deferredThreadReplies.map((entry) => entry.message), + [deferredThreadReplies], ); const { @@ -219,9 +252,19 @@ export function MessageThreadPanel({
- {threadReplies.length > 0 ? ( -
- {threadReplies.map((entry) => { + {repliesRenderState === "list" ? ( +
+ {deferredThreadReplies.map((entry) => { return (
- ) : ( + ) : repliesRenderState === "empty" ? ( + // Only show the empty state when the thread is GENUINELY empty. + // Keying off `deferredThreadReplies` would flash "No replies" for a + // frame while a non-empty list streams in on the deferred commit.

No replies in this branch yet @@ -274,7 +320,10 @@ export function MessageThreadPanel({ Reply in the thread to continue this branch.

- )} + ) : // "pending": deferred list is empty but the live list has content — + // rows are streaming in on the deferred commit. Paint nothing rather + // than flashing the empty state. + null}
diff --git a/desktop/src/features/messages/ui/MessageTimeline.tsx b/desktop/src/features/messages/ui/MessageTimeline.tsx index 1fa347a20..e93ec1ace 100644 --- a/desktop/src/features/messages/ui/MessageTimeline.tsx +++ b/desktop/src/features/messages/ui/MessageTimeline.tsx @@ -88,6 +88,11 @@ type ChannelIntro = { icon?: React.ReactNode; }; +/** Stable empty reference used as the `useDeferredValue` initial value so the + * first render on channel entry stays light instead of blocking on the full + * message list. Must be module-level so its identity never changes. */ +const EMPTY_MESSAGES: TimelineMessage[] = []; + export const MessageTimeline = React.memo(function MessageTimeline({ agentPubkeys, channelId, @@ -127,6 +132,20 @@ export const MessageTimeline = React.memo(function MessageTimeline({ const internalScrollRef = React.useRef(null); const scrollContainerRef = externalScrollRef ?? internalScrollRef; const topSentinelRef = React.useRef(null); + + // Gate the heavy timeline render (each row runs a synchronous + // react-markdown parse) behind React concurrency. `useDeferredValue` lets the + // commit that rebuilds the message list yield to higher-priority work, so the + // main thread stops freezing and the OS no longer shows the busy cursor when + // entering a channel. We pass `initialValue: []` so even the FIRST render on + // channel entry stays light — the heavy list streams in on a deferred commit + // rather than blocking the initial paint. We deliberately drive BOTH the + // scroll manager and the rendered list off the same deferred value — + // scroll/autoscroll/deep-link logic reads the DOM (`scrollIntoView`, + // ResizeObserver on the content), so it must stay consistent with what's + // actually painted. You can't scroll to a row that hasn't committed yet. + const deferredMessages = React.useDeferredValue(messages, EMPTY_MESSAGES); + const isRenderPending = deferredMessages !== messages; const scrollRestorationId = targetMessageId ? `message-timeline:${channelId ?? "none"}:target:${targetMessageId}` : `message-timeline:${channelId ?? "none"}`; @@ -143,7 +162,7 @@ export const MessageTimeline = React.memo(function MessageTimeline({ } = useTimelineScrollManager({ channelId, isLoading, - messages, + messages: deferredMessages, onTargetReached, scrollContainerRef, targetMessageId, @@ -188,10 +207,10 @@ export const MessageTimeline = React.memo(function MessageTimeline({ const showIntro = showDirectMessageIntro || showChannelIntro; const showGenericEmpty = !isLoading && - messages.length === 0 && + deferredMessages.length === 0 && directMessageIntro === null && channelIntro === null; - const showMessageList = !isLoading && messages.length > 0; + const showMessageList = !isLoading && deferredMessages.length > 0; return ( @@ -358,7 +377,15 @@ export const MessageTimeline = React.memo(function MessageTimeline({ {showMessageList ? (
buildMainTimelineEntries(messages), + const descendantStatsByMessageId = React.useMemo( + () => buildDescendantStatsByMessageId(messages), [messages], ); + const entries = React.useMemo( + () => buildMainTimelineEntries(messages, descendantStatsByMessageId), + [descendantStatsByMessageId, messages], + ); const reviewCommentsByRootId = React.useMemo( () => buildReviewCommentsByRootId(messages), [messages], @@ -191,12 +196,21 @@ export const TimelineMessageList = React.memo(function TimelineMessageList({ }> = []; let currentDayGroup: (typeof dayGroups)[number] | null = null; + // Day-divider decision delegated to a pure, lib-tested helper: a new group + // starts at index 0 and whenever a message falls on a different calendar day + // than the one before it. We index the boundary start positions so the render + // loop below stays a straight walk while the grouping logic lives in `lib/`. + const dayGroupStartIndices = new Set( + buildDayGroupBoundaries(entries.map((entry) => entry.message)).map( + (boundary) => boundary.startIndex, + ), + ); + for (let i = 0; i < entries.length; i++) { const { message, summary } = entries[i]; - const prev = i > 0 ? entries[i - 1]?.message : null; const messageRenderKey = message.renderKey ?? message.id; - if (!prev || !isSameDay(prev.createdAt, message.createdAt)) { + if (dayGroupStartIndices.has(i)) { currentDayGroup = { key: `day-${message.createdAt}`, label: formatDayHeading(message.createdAt), diff --git a/desktop/src/features/messages/ui/messageTimelineUtils.ts b/desktop/src/features/messages/ui/messageTimelineUtils.ts deleted file mode 100644 index 34cc79246..000000000 --- a/desktop/src/features/messages/ui/messageTimelineUtils.ts +++ /dev/null @@ -1,8 +0,0 @@ -const BOTTOM_THRESHOLD_PX = 72; - -export function isNearBottom(container: HTMLDivElement) { - return ( - container.scrollHeight - container.clientHeight - container.scrollTop <= - BOTTOM_THRESHOLD_PX - ); -} diff --git a/desktop/src/features/messages/ui/useTimelineScrollManager.ts b/desktop/src/features/messages/ui/useTimelineScrollManager.ts index da2fd7ff6..a9ee2485a 100644 --- a/desktop/src/features/messages/ui/useTimelineScrollManager.ts +++ b/desktop/src/features/messages/ui/useTimelineScrollManager.ts @@ -1,7 +1,11 @@ import * as React from "react"; +import { + isNearBottom, + resolveDeepLinkTarget, + selectLatestMessageKey, +} from "@/features/messages/lib/timelineSnapshot"; import type { TimelineMessage } from "@/features/messages/types"; -import { isNearBottom } from "./messageTimelineUtils"; type UseTimelineScrollManagerOptions = { channelId?: string | null; @@ -97,9 +101,7 @@ export function useTimelineScrollManager({ const latestMessage = messages.length > 0 ? messages[messages.length - 1] : undefined; - const latestMessageKey = latestMessage - ? (latestMessage.renderKey ?? latestMessage.id) - : undefined; + const latestMessageKey = selectLatestMessageKey(messages); // biome-ignore lint/correctness/useExhaustiveDependencies: timelineRef is a stable React ref passed from the parent — its identity never changes const syncScrollState = React.useCallback(() => { @@ -356,6 +358,15 @@ export function useTimelineScrollManager({ return; } + // Deep-link decision delegated to a pure, lib-tested helper: only attempt the + // jump once the target actually exists in THIS (deferred) snapshot. If it + // doesn't, the row hasn't committed yet — bail and let the next snapshot that + // includes it drive the jump. This reads the same `messages` snapshot the + // list rendered, which closes the tearing race. + if (!resolveDeepLinkTarget(messages, targetMessageId).resolved) { + return; + } + const timeline = timelineRef.current; if (!timeline) { return;