From 5538199627dfc0cdbf7804e8306993f0f31daea3 Mon Sep 17 00:00:00 2001 From: "Trace (Engineer)" <9d485ff0c62915e08a801162c195c7ef096f6f6143925407255cea2656c4b8a4@buzz.block.builderlab.xyz> Date: Sat, 22 Aug 2026 15:27:55 -0700 Subject: [PATCH] Deep links to top-level messages highlight them in the main timeline Clicking an inbox row, desktop notification, search hit, or buzz:// link that targets a top-level message (channel or DM) used to force-open the reply panel for that root, showing an empty "No replies in this branch yet" pane instead of the message in its own context. The route-target logic is now an exported pure function, getRouteTargetPanelAction: - top-level target without an explicit threadRootId -> main-timeline scroll + highlight only, no thread panel - top-level target with an explicit threadRootId (inbox "Open full thread", channel-activity rows, thread-draft auto-send) -> panel opens at that root, unchanged - reply target -> panel opens at the thread head scrolled to the reply, unchanged targetThreadRootId is plumbed from the route search params through ChannelRouteScreen/ChannelScreen into the hook. Unit tests cover the new function; the two e2e tests from #1092 are rewritten to the new contract and the deep-link reload test now re-issues the link cold since a consumed messageId no longer pins a thread param. Co-authored-by: morgmart <98432065+morgmart@users.noreply.github.com> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com> --- desktop/src/app/routes/ChannelRouteScreen.tsx | 1 + .../features/channels/ui/ChannelScreen.tsx | 2 + .../channels/ui/ChannelScreen.types.ts | 7 ++ .../ui/useChannelRouteTarget.test.mjs | 111 ++++++++++++++++++ .../channels/ui/useChannelRouteTarget.ts | 110 +++++++++++++---- desktop/tests/e2e/navigation.spec.ts | 57 +++++---- 6 files changed, 242 insertions(+), 46 deletions(-) create mode 100644 desktop/src/features/channels/ui/useChannelRouteTarget.test.mjs diff --git a/desktop/src/app/routes/ChannelRouteScreen.tsx b/desktop/src/app/routes/ChannelRouteScreen.tsx index d4626d2c6f..7951f419c3 100644 --- a/desktop/src/app/routes/ChannelRouteScreen.tsx +++ b/desktop/src/app/routes/ChannelRouteScreen.tsx @@ -234,6 +234,7 @@ export function ChannelRouteScreen({ targetForumReplyId={targetReplyId} targetMessageEvents={targetMessageEvents} targetMessageId={targetMessageId} + targetThreadRootId={targetThreadRootId} /> ); } diff --git a/desktop/src/features/channels/ui/ChannelScreen.tsx b/desktop/src/features/channels/ui/ChannelScreen.tsx index 6254afd8c7..8ba518ccc3 100644 --- a/desktop/src/features/channels/ui/ChannelScreen.tsx +++ b/desktop/src/features/channels/ui/ChannelScreen.tsx @@ -94,6 +94,7 @@ export function ChannelScreen({ targetForumReplyId, targetMessageEvents, targetMessageId, + targetThreadRootId, }: ChannelScreenProps) { const { goHome } = useAppNavigation(); const { activeCommunity } = useCommunities(); @@ -651,6 +652,7 @@ export function ChannelScreen({ setThreadReplyTargetId, setThreadScrollTargetId, targetMessageId, + targetThreadRootId, timelineMessages, }); useThreadTargetSync({ diff --git a/desktop/src/features/channels/ui/ChannelScreen.types.ts b/desktop/src/features/channels/ui/ChannelScreen.types.ts index 371af6faf5..766f53eeb7 100644 --- a/desktop/src/features/channels/ui/ChannelScreen.types.ts +++ b/desktop/src/features/channels/ui/ChannelScreen.types.ts @@ -22,4 +22,11 @@ export type ChannelScreenProps = { targetForumReplyId: string | null; targetMessageEvents: RelayEvent[]; targetMessageId: string | null; + /** + * Thread root requested by the navigation source (`?threadRootId`, or the + * `?thread` panel param on deep links). Deciding input for top-level route + * targets: present → open the thread panel at that root; absent → the + * target is shown in the main timeline only. + */ + targetThreadRootId: string | null; }; diff --git a/desktop/src/features/channels/ui/useChannelRouteTarget.test.mjs b/desktop/src/features/channels/ui/useChannelRouteTarget.test.mjs new file mode 100644 index 0000000000..6675f9bf5e --- /dev/null +++ b/desktop/src/features/channels/ui/useChannelRouteTarget.test.mjs @@ -0,0 +1,111 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { getRouteTargetPanelAction } from "./useChannelRouteTarget.ts"; + +function makeMessage(overrides = {}) { + return { + id: "target", + author: "alice", + body: "hello", + createdAt: 1, + depth: 0, + parentId: null, + rootId: null, + tags: [], + time: "now", + ...overrides, + }; +} + +function byId(...messages) { + return new Map(messages.map((message) => [message.id, message])); +} + +test("top-level target without threadRootId stays in the main timeline", () => { + const root = makeMessage(); + assert.deepEqual(getRouteTargetPanelAction(root, null, byId(root)), { + kind: "main-timeline-only", + }); +}); + +test("top-level target with an explicit threadRootId opens its thread panel", () => { + const root = makeMessage(); + assert.deepEqual(getRouteTargetPanelAction(root, root.id, byId(root)), { + kind: "open-thread", + expandedReplyIds: new Set(), + replyTargetId: root.id, + scrollTargetId: null, + threadHeadId: root.id, + }); +}); + +test("reply target opens the thread scrolled to the reply", () => { + const root = makeMessage({ id: "root" }); + const reply = makeMessage({ + id: "reply", + parentId: "root", + rootId: "root", + depth: 1, + }); + assert.deepEqual(getRouteTargetPanelAction(reply, null, byId(root, reply)), { + kind: "open-thread", + expandedReplyIds: new Set(), + replyTargetId: "root", + scrollTargetId: "reply", + threadHeadId: "root", + }); +}); + +test("nested reply target expands its intermediate ancestors", () => { + const root = makeMessage({ id: "root" }); + const mid = makeMessage({ + id: "mid", + parentId: "root", + rootId: "root", + depth: 1, + }); + const leaf = makeMessage({ + id: "leaf", + parentId: "mid", + rootId: "root", + depth: 2, + }); + assert.deepEqual( + getRouteTargetPanelAction(leaf, null, byId(root, mid, leaf)), + { + kind: "open-thread", + expandedReplyIds: new Set(["mid"]), + replyTargetId: "root", + scrollTargetId: "leaf", + threadHeadId: "root", + }, + ); +}); + +test("broadcast reply target is not a panel action", () => { + const root = makeMessage({ id: "root" }); + const broadcast = makeMessage({ + id: "broadcast", + parentId: "root", + rootId: "root", + depth: 1, + tags: [["broadcast", "1"]], + }); + assert.deepEqual( + getRouteTargetPanelAction(broadcast, null, byId(root, broadcast)), + { kind: "none" }, + ); +}); + +test("reply whose thread head is not loaded yet defers", () => { + const reply = makeMessage({ + id: "reply", + parentId: "missing-root", + rootId: "missing-root", + depth: 1, + }); + assert.deepEqual(getRouteTargetPanelAction(reply, null, byId(reply)), { + kind: "none", + }); +}); diff --git a/desktop/src/features/channels/ui/useChannelRouteTarget.ts b/desktop/src/features/channels/ui/useChannelRouteTarget.ts index 0dc4b0e4d6..76eb5c57d2 100644 --- a/desktop/src/features/channels/ui/useChannelRouteTarget.ts +++ b/desktop/src/features/channels/ui/useChannelRouteTarget.ts @@ -37,6 +37,71 @@ function getThreadRouteTarget( return { expandedReplyIds, threadHeadId }; } +export type RouteTargetPanelAction = + | { kind: "none" } + | { kind: "main-timeline-only" } + | { + kind: "open-thread"; + expandedReplyIds: Set; + replyTargetId: string; + scrollTargetId: string | null; + threadHeadId: string; + }; + +/** + * Decides what a message route target does to the thread panel. + * + * - Top-level target without an explicit `threadRootId` (inbox message rows, + * desktop notifications, search hits, `buzz://` root links): the + * main-timeline scroll + highlight is the entire navigation. Opening the + * reply panel here would show an empty "no replies" pane instead of the + * message in its own context — the exact defect this guards against. + * - Top-level target with an explicit `threadRootId` (inbox "Open full + * thread", thread-draft auto-send, channel-activity rows): the surface + * asked for the thread, so the panel opens at that root. + * - Reply target: the panel opens at the thread head, scrolled to the reply. + * - `none`: not actionable yet (broadcast reply, or the thread head is not + * loaded) — the caller retries when more messages arrive. + * + * Exported as a pure function so the routing contract is unit-testable + * without mounting the hook. + */ +export function getRouteTargetPanelAction( + targetMessage: TimelineMessage, + targetThreadRootId: string | null, + messageById: ReadonlyMap, +): RouteTargetPanelAction { + if (!targetMessage.parentId) { + if (!targetThreadRootId) { + return { kind: "main-timeline-only" }; + } + return { + kind: "open-thread", + expandedReplyIds: new Set(), + replyTargetId: targetMessage.id, + scrollTargetId: null, + threadHeadId: targetMessage.id, + }; + } + + if (isBroadcastReply(targetMessage.tags ?? [])) { + return { kind: "none" }; + } + + const routeTarget = getThreadRouteTarget(targetMessage, messageById); + if (!routeTarget) { + return { kind: "none" }; + } + + return { + kind: "open-thread", + expandedReplyIds: routeTarget.expandedReplyIds, + replyTargetId: routeTarget.threadHeadId, + scrollTargetId: targetMessage.id, + threadHeadId: routeTarget.threadHeadId, + }; +} + function getRouteMainTimelineTargetId( targetMessageId: string | null, targetMessage: TimelineMessage | null, @@ -63,6 +128,7 @@ export function useChannelRouteTarget({ setThreadReplyTargetId, setThreadScrollTargetId, targetMessageId, + targetThreadRootId, timelineMessages, }: { activeChannel: Channel | null; @@ -75,6 +141,7 @@ export function useChannelRouteTarget({ setThreadReplyTargetId: React.Dispatch>; setThreadScrollTargetId: React.Dispatch>; targetMessageId: string | null; + targetThreadRootId: string | null; timelineMessages: TimelineMessage[]; }) { const timelineMessageById = React.useMemo( @@ -114,31 +181,21 @@ export function useChannelRouteTarget({ return; } - if (!targetMessage.parentId) { - closeAgentSession(); - // Root message links should open the reply panel for that root. The - // timeline scroll/highlight target alone is not enough: root links have - // no parent/thread metadata, so the reply-only branch below cannot infer - // a thread head. - setProfilePanelPubkey(null, { replace: true }); - setEditTargetId(null); - setOpenThreadHeadId(targetMessage.id, { replace: true }); - setThreadReplyTargetId(targetMessage.id); - setThreadScrollTargetId(null); - setExpandedThreadReplyIds(new Set()); - handledThreadRouteTargetRef.current = targetKey; - return; - } - - if (isBroadcastReply(targetMessage.tags ?? [])) { - return; - } - - const routeTarget = getThreadRouteTarget( + const action = getRouteTargetPanelAction( targetMessage, + targetThreadRootId, timelineMessageById, ); - if (!routeTarget) { + if (action.kind === "none") { + return; + } + + if (action.kind === "main-timeline-only") { + // Top-level target with no requested thread: the main-timeline + // scroll/highlight (mainTimelineTargetMessageId) is the whole + // navigation. Mark handled so a later timeline update cannot + // re-process this target. + handledThreadRouteTargetRef.current = targetKey; return; } @@ -147,10 +204,10 @@ export function useChannelRouteTarget({ // back should leave the deep link, not strip the panel from it. setProfilePanelPubkey(null, { replace: true }); setEditTargetId(null); - setOpenThreadHeadId(routeTarget.threadHeadId, { replace: true }); - setThreadReplyTargetId(routeTarget.threadHeadId); - setThreadScrollTargetId(targetMessageId); - setExpandedThreadReplyIds(routeTarget.expandedReplyIds); + setOpenThreadHeadId(action.threadHeadId, { replace: true }); + setThreadReplyTargetId(action.replyTargetId); + setThreadScrollTargetId(action.scrollTargetId); + setExpandedThreadReplyIds(action.expandedReplyIds); handledThreadRouteTargetRef.current = targetKey; }, [ activeChannel, @@ -163,6 +220,7 @@ export function useChannelRouteTarget({ setThreadReplyTargetId, setThreadScrollTargetId, targetMessageId, + targetThreadRootId, timelineMessageById, ]); diff --git a/desktop/tests/e2e/navigation.spec.ts b/desktop/tests/e2e/navigation.spec.ts index 189db09e0f..a65c7a9198 100644 --- a/desktop/tests/e2e/navigation.spec.ts +++ b/desktop/tests/e2e/navigation.spec.ts @@ -445,7 +445,7 @@ test("mixed Buzz permalinks render as chips in the composer", async ({ await expect(composerInput).not.toContainText("buzz://"); }); -test("message links to visible root messages open the thread panel", async ({ +test("message links to visible root messages highlight them in the main timeline", async ({ page, }) => { await page.goto("/"); @@ -639,12 +639,17 @@ test("message links to visible root messages open the thread panel", async ({ await rootThreadLink.click({ button: "right" }); await linkMenu.getByRole("button", { name: "Open link" }).click(); + // Root-message links resolve in the main timeline: scroll + highlight the + // root, never force-open its (possibly empty) reply panel (block/buzz — + // "inbox deep links open an empty thread for top-level messages"). const threadPanel = page.getByTestId("message-thread-panel"); - await expect(threadPanel).toBeVisible(); - await expect(page).toHaveURL(/thread=mock-general-welcome/); - await expect(threadPanel.getByTestId("message-thread-head")).toContainText( - "Welcome to general", - ); + await expect(threadPanel).not.toBeVisible(); + await expect(page).not.toHaveURL(/thread=/); + const welcomeRow = page + .getByTestId("message-timeline") + .locator('[data-message-id="mock-general-welcome"]'); + await expect(welcomeRow).toBeVisible(); + await expect(welcomeRow).toHaveClass(/route-target-highlight-fade/); }); test("direct-message tooltip metadata stays on one physical line", async ({ @@ -742,7 +747,7 @@ test("message links explain when preview metadata is unavailable", async ({ await expect(unavailableTooltip).toHaveCount(0); }); -test("message links reopen a closed thread when the same messageId is already in the URL", async ({ +test("root message links re-target the main timeline when the same messageId was already in the URL", async ({ page, }) => { await page.goto( @@ -750,14 +755,20 @@ test("message links reopen a closed thread when the same messageId is already in ); await expect(page.getByTestId("chat-title")).toHaveText("general"); + // A top-level deep link never opens the reply panel — the message is shown + // highlighted in its own context instead. const threadPanel = page.getByTestId("message-thread-panel"); - await expect(threadPanel).toBeVisible(); - await expect(threadPanel.getByTestId("message-thread-head")).toContainText( - "Welcome to general", - ); - - await threadPanel.getByRole("button", { name: "Close panel" }).click(); await expect(threadPanel).not.toBeVisible(); + const welcomeRow = page + .getByTestId("message-timeline") + .locator('[data-message-id="mock-general-welcome"]'); + await expect(welcomeRow).toBeVisible(); + await expect(welcomeRow).toHaveClass(/route-target-highlight-fade/); + + // Once the target is reached the messageId param clears; re-activating a + // link to the same root must route again instead of being swallowed + // (regression guard from the original reopen-a-closed-thread test). + await expect(page).not.toHaveURL(/messageId=/); const link = "buzz://message?channel=9a1657ac-f7aa-5db0-b632-d8bbeb6dfb50&id=mock-general-welcome"; @@ -777,24 +788,30 @@ test("message links reopen a closed thread when the same messageId is already in await expect(rootThreadLink).toHaveText("general"); await rootThreadLink.click(); - await expect(threadPanel).toBeVisible(); - await expect(threadPanel.getByTestId("message-thread-head")).toContainText( - "Welcome to general", - ); + await expect(threadPanel).not.toBeVisible(); + await expect(welcomeRow).toHaveClass(/route-target-highlight-fade/); }); test("message deep links survive reload", async ({ page }) => { - await page.goto( - `/#/channels/${ENGINEERING_CHANNEL_ID}?messageId=mock-engineering-shipped`, - ); + const deepLinkUrl = `/#/channels/${ENGINEERING_CHANNEL_ID}?messageId=mock-engineering-shipped`; + await page.goto(deepLinkUrl); await expect(page.getByTestId("chat-title")).toHaveText("engineering"); await expect(page.getByTestId("message-timeline")).toContainText( "Engineering shipped the desktop build.", ); + // Once the target is centered the messageId param is consumed (cleared via + // onTargetReached so re-activating the same link is never swallowed), and a + // top-level target no longer pins a `thread` param either — so a bare + // reload lands on the plain channel. + await expect(page).not.toHaveURL(/messageId=/); await page.reload(); + await expect(page.getByTestId("chat-title")).toHaveText("engineering"); + // The deep-link URL itself stays valid: loading it again cold re-resolves + // and re-splices the target message. + await page.goto(deepLinkUrl); await expect(page.getByTestId("chat-title")).toHaveText("engineering"); await expect(page.getByTestId("message-timeline")).toContainText( "Engineering shipped the desktop build.",