Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion desktop/src/features/channels/ui/ChannelScreen.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -317,15 +320,27 @@ 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(
timelineMessages,
openThreadHeadId,
threadReplyTargetId,
expandedThreadReplyIds,
descendantStatsByMessageId,
),
[
descendantStatsByMessageId,
expandedThreadReplyIds,
openThreadHeadId,
threadReplyTargetId,
Expand Down
201 changes: 201 additions & 0 deletions desktop/src/features/messages/lib/threadPanel.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import assert from "node:assert/strict";
import test from "node:test";

import {
buildDescendantStatsByMessageId,
buildMainTimelineEntries,
buildThreadPanelData,
} from "./threadPanel.ts";
Expand Down Expand Up @@ -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");
});
54 changes: 50 additions & 4 deletions desktop/src/features/messages/lib/threadPanel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<number, TimelineMessage>
>();

function normalizeInlineReplyMessage(
message: TimelineMessage,
depth: number,
): TimelineMessage {
return {
let byDepth = normalizedInlineReplyCache.get(message);
if (!byDepth) {
byDepth = new Map<number, TimelineMessage>();
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[]) {
Expand All @@ -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<string, ThreadDescendantStats> {
const messageById = new Map(messages.map((message) => [message.id, message]));
Expand Down Expand Up @@ -220,8 +258,11 @@ function buildVisibleThreadReplies(params: {

export function buildMainTimelineEntries(
messages: TimelineMessage[],
precomputedDescendantStatsByMessageId?: Map<string, ThreadDescendantStats>,
): MainTimelineEntry[] {
const descendantStatsByMessageId = buildDescendantStatsByMessageId(messages);
const descendantStatsByMessageId =
precomputedDescendantStatsByMessageId ??
buildDescendantStatsByMessageId(messages);

return messages
.filter(
Expand All @@ -244,6 +285,7 @@ export function buildThreadPanelData(
openThreadHeadId: string | null,
threadReplyTargetId: string | null,
expandedReplyIds: ReadonlySet<string>,
precomputedDescendantStatsByMessageId?: Map<string, ThreadDescendantStats>,
): ThreadPanelData {
if (!openThreadHeadId) {
return {
Expand All @@ -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,
Expand Down
Loading