Skip to content

Commit 2cff652

Browse files
adamleithpclaude
andcommitted
fix(channels): address review — hydration race, double-submit, seen depth
Seven findings from a review of this stack. - The seen store's storage is async (IPC), so both sides raced hydration. Reads: an empty map is indistinguishable from "nothing ever read", so every channel with activity bolded for the first frames of each boot. Writes: a channel opened during boot stamped itself seen, then zustand's default merge replaced that stamp with what was on disk and lost it — the channel you just read stayed bold. Gate reads on `hasHydrated` and merge the two maps (later visit per channel wins) instead of replacing. - CreateChannelModal relied on `disabled={busy}`, which only lands a render after the mutation starts, so a double-click (or a held ⌘Enter) fired two creates — and folder creation isn't idempotent by path, so that's two channels of the same name. Latch synchronously. - ensurePersonalChannel's in-flight guard settled when the POST returned, but callers pass the `channels` from their last render — a click in that gap saw no existing "me" and no in-flight create, and made a second. Remember what was created until the list catches up. - Marking a channel read only happened on its feed, so reading it via Artifacts/Recents/CONTEXT.md left it bold. Moved into ChannelHeader, which every channel surface renders — a new surface now gets it free. - ChannelGroup's onOpenChange ignored the value Base UI emits and blind toggled, so a redundant event would invert the section. - Unread was resolved two ways (by name for shared rows, by id for #me). One predicate now mirrors useBackendChannel's mapping for both. - latestActivityForChannel built a map of every channel to read one key. Covered by 10 new tests: the store's hydration merge (the clobber case fails without the fix) and ensurePersonalChannel's races. Not addressed, deliberately: the seen store stays in @posthog/ui rather than moving to core per the layering rule. Every persisted store in the app lives in ui because persistence goes through electronStorage, a ui/shell adapter; core has no persisted store to follow, and its sibling (activitySeenStore, the same concept for the Activity page) sits in ui. Moving it needs a platform storage interface — worth doing, but as its own change rather than smuggled into this one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019G63f4afY9vsbKsvK654Wj
1 parent 1009a4d commit 2cff652

11 files changed

Lines changed: 349 additions & 65 deletions

packages/core/src/canvas/channelUnread.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,11 +47,20 @@ export function unreadChannelIds(
4747
return unread;
4848
}
4949

50-
/** The newest activity in one channel, for stamping it seen while it's open. */
50+
/**
51+
* The newest activity in one channel, for stamping it seen while it's open.
52+
* Scans for the one channel rather than reusing `latestActivityByChannel`,
53+
* which would build (and throw away) a map of every other channel to answer.
54+
*/
5155
export function latestActivityForChannel(
5256
items: readonly MentionActivityItem[],
5357
channelId: string | undefined,
5458
): string | undefined {
5559
if (!channelId) return undefined;
56-
return latestActivityByChannel(items).get(channelId);
60+
let latest: string | undefined;
61+
for (const item of items) {
62+
if (item.channelId !== channelId) continue;
63+
if (!latest || item.createdAt > latest) latest = item.createdAt;
64+
}
65+
return latest;
5766
}

packages/ui/src/features/canvas/components/ChannelHeader.tsx

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { HashIcon } from "@phosphor-icons/react";
22
import { Button, cn } from "@posthog/quill";
33
import { ChannelTabs } from "@posthog/ui/features/canvas/components/ChannelTabs";
44
import { useChannels } from "@posthog/ui/features/canvas/hooks/useChannels";
5+
import { useMarkChannelSeen } from "@posthog/ui/features/canvas/hooks/useMarkChannelSeen";
56
import { Text } from "@radix-ui/themes";
67
import { useNavigate, useRouterState } from "@tanstack/react-router";
78

@@ -17,6 +18,9 @@ export function ChannelHeader({ channelId }: { channelId: string }) {
1718
const channelName = channels.find((c) => c.id === channelId)?.name;
1819
const pathname = useRouterState({ select: (s) => s.location.pathname });
1920
const isHome = pathname === `/website/${channelId}`;
21+
// Every channel surface renders this header, so it is where "the viewer is
22+
// in this channel" is known — and therefore where the channel is marked read.
23+
useMarkChannelSeen(channelName);
2024

2125
return (
2226
<div className="flex min-w-0 items-center gap-2">

packages/ui/src/features/canvas/components/ChannelsList.tsx

Lines changed: 13 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -57,25 +57,17 @@ import {
5757
} from "@posthog/ui/features/canvas/hooks/useChannels";
5858
import { useCreateAndOpenDashboard } from "@posthog/ui/features/canvas/hooks/useDashboards";
5959
import {
60-
normalizeChannelName,
6160
PERSONAL_CHANNEL_NAME,
6261
useTaskChannels,
6362
} from "@posthog/ui/features/canvas/hooks/useTaskChannels";
64-
import { useUnreadChannelIds } from "@posthog/ui/features/canvas/hooks/useUnreadChannels";
63+
import { useIsChannelUnread } from "@posthog/ui/features/canvas/hooks/useUnreadChannels";
6564
import { copyChannelLink } from "@posthog/ui/features/canvas/utils/copyChannelLink";
6665
import { useSidebarStore } from "@posthog/ui/features/sidebar/sidebarStore";
6766
import { toast } from "@posthog/ui/primitives/toast";
6867
import { track } from "@posthog/ui/shell/analytics";
6968
import { Box, Flex } from "@radix-ui/themes";
7069
import { useNavigate, useRouterState } from "@tanstack/react-router";
71-
import {
72-
Fragment,
73-
type ReactNode,
74-
useEffect,
75-
useMemo,
76-
useRef,
77-
useState,
78-
} from "react";
70+
import { Fragment, type ReactNode, useEffect, useRef, useState } from "react";
7971
import { hostClient } from "../hostClient";
8072

8173
// One actionable entry in a channel's menu, rendered the same whether it
@@ -527,12 +519,10 @@ function PersonalChannelRow() {
527519
const { channels } = useChannels();
528520
const { createChannel, isCreating } = useChannelMutations();
529521
// Listing backend channels lazily provisions the personal channel server-side.
530-
const { personalChannel } = useTaskChannels();
522+
useTaskChannels();
531523
// The "+" dropdown (New task / New canvas), mirroring a shared channel row.
532524
const [newMenuOpen, setNewMenuOpen] = useState(false);
533-
const unreadChannelIds = useUnreadChannelIds();
534-
const isUnread =
535-
!!personalChannel && unreadChannelIds.has(personalChannel.id);
525+
const isUnread = useIsChannelUnread()(PERSONAL_CHANNEL_NAME);
536526

537527
const meFolder = channels.find((c) => c.name === PERSONAL_CHANNEL_NAME);
538528
const createAndOpenCanvas = useCreateAndOpenDashboard(meFolder?.id);
@@ -707,7 +697,12 @@ function ChannelGroup({
707697
return (
708698
<Collapsible.Root
709699
open={isOpen}
710-
onOpenChange={() => toggleSection(sectionId)}
700+
// The store only exposes a toggle, so drive it from the requested value:
701+
// an event for the state we're already in is then a no-op rather than an
702+
// inversion.
703+
onOpenChange={(open) => {
704+
if (open !== isOpen) toggleSection(sectionId);
705+
}}
711706
className={className}
712707
>
713708
{/* MenuLabel carries the sidebar's label styling; `render` keeps it a
@@ -755,20 +750,7 @@ export function ChannelsList() {
755750
const { channels: allChannels, isLoading } = useChannels();
756751
const { starredRefToShortcutId } = useChannelStars();
757752

758-
// Unread activity is keyed by backend channel id, while these rows are folder
759-
// channels — joined by name, the same bridge useBackendChannel walks. Resolved
760-
// once here rather than per row, so the list mounts one lookup, not 46.
761-
const { channels: backendChannels } = useTaskChannels();
762-
const unreadChannelIds = useUnreadChannelIds();
763-
const unreadNames = useMemo(() => {
764-
const names = new Set<string>();
765-
for (const channel of backendChannels) {
766-
if (unreadChannelIds.has(channel.id)) names.add(channel.name);
767-
}
768-
return names;
769-
}, [backendChannels, unreadChannelIds]);
770-
const isUnread = (channel: Channel) =>
771-
unreadNames.has(normalizeChannelName(channel.name));
753+
const isUnread = useIsChannelUnread();
772754

773755
// The "me" folder renders as the pinned personal row, not a shared channel.
774756
const channels = allChannels.filter((c) => c.name !== PERSONAL_CHANNEL_NAME);
@@ -803,7 +785,7 @@ export function ChannelsList() {
803785
<ChannelSection
804786
key={channel.id}
805787
channel={channel}
806-
isUnread={isUnread(channel)}
788+
isUnread={isUnread(channel.name)}
807789
/>
808790
))}
809791
</ChannelGroup>
@@ -819,7 +801,7 @@ export function ChannelsList() {
819801
<ChannelSection
820802
key={channel.id}
821803
channel={channel}
822-
isUnread={isUnread(channel)}
804+
isUnread={isUnread(channel.name)}
823805
/>
824806
))}
825807
</ChannelGroup>

packages/ui/src/features/canvas/components/CreateChannelModal.tsx

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ import { useGenerateContext } from "@posthog/ui/features/canvas/hooks/useGenerat
2020
import { toast } from "@posthog/ui/primitives/toast";
2121
import { track } from "@posthog/ui/shell/analytics";
2222
import { useNavigate } from "@tanstack/react-router";
23-
import { type CSSProperties, useState } from "react";
23+
import { type CSSProperties, useRef, useState } from "react";
2424

2525
// Matches Slack's "Create a channel" naming constraint.
2626
const MAX_CONTEXT_NAME_LENGTH = 80;
@@ -84,6 +84,21 @@ export function CreateChannelModal({
8484
// way through without one.
8585
const canDescribe = !busy && !!trimmedDescription;
8686

87+
// `busy` only disables the buttons a render after the mutation starts, so a
88+
// double-click lands two creates before it applies — and folder creation is
89+
// not idempotent by path, so that is two channels of the same name. Latch
90+
// synchronously; the buttons stay the user-visible half of this.
91+
const submittingRef = useRef(false);
92+
const submitOnce = async (submit: () => Promise<void>) => {
93+
if (submittingRef.current) return;
94+
submittingRef.current = true;
95+
try {
96+
await submit();
97+
} finally {
98+
submittingRef.current = false;
99+
}
100+
};
101+
87102
// Create the channel and land in its feed — the intro (name, creation line,
88103
// context.md card) and "joined" row there are derived from the channel row.
89104
// With a description, also launch the plan session that builds context.md.
@@ -186,10 +201,11 @@ export function CreateChannelModal({
186201
disabled={busy}
187202
onChange={(e) => setDescription(e.target.value)}
188203
onKeyDown={(e) => {
189-
// ⌘/Ctrl+Enter submits; a bare Enter stays a newline.
204+
// ⌘/Ctrl+Enter submits; a bare Enter stays a newline. Held down it
205+
// repeats, so it goes through the same latch as the buttons.
190206
if (e.key === "Enter" && (e.metaKey || e.ctrlKey)) {
191207
e.preventDefault();
192-
void submitDescribeStep();
208+
void submitOnce(submitDescribeStep);
193209
}
194210
}}
195211
/>
@@ -225,7 +241,7 @@ export function CreateChannelModal({
225241
variant="primary"
226242
disabled={!canDescribe}
227243
loading={busy}
228-
onClick={submitDescribeStep}
244+
onClick={() => void submitOnce(submitDescribeStep)}
229245
>
230246
Create
231247
</Button>
@@ -335,15 +351,15 @@ export function CreateChannelModal({
335351
<Button
336352
variant="default"
337353
disabled={busy}
338-
onClick={() => void submitCreate(false)}
354+
onClick={() => void submitOnce(() => submitCreate(false))}
339355
>
340356
Skip
341357
</Button>
342358
<Button
343359
variant="primary"
344360
disabled={!canDescribe}
345361
loading={busy}
346-
onClick={submitDescribeStep}
362+
onClick={() => void submitOnce(submitDescribeStep)}
347363
>
348364
Create
349365
</Button>

packages/ui/src/features/canvas/components/WebsiteChannelHome.tsx

Lines changed: 2 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
import { latestActivityForChannel } from "@posthog/core/canvas/channelUnread";
21
import { insertTaskDedup } from "@posthog/core/tasks/taskDelete";
32
import { ANALYTICS_EVENTS } from "@posthog/shared/analytics-events";
43
import type { Task } from "@posthog/shared/domain-types";
@@ -31,12 +30,10 @@ import {
3130
import { useChannels } from "@posthog/ui/features/canvas/hooks/useChannels";
3231
import { useChannelTaskMutations } from "@posthog/ui/features/canvas/hooks/useChannelTasks";
3332
import { useFolderInstructions } from "@posthog/ui/features/canvas/hooks/useFolderInstructions";
34-
import { useMentionActivity } from "@posthog/ui/features/canvas/hooks/useMentionActivity";
3533
import {
3634
PERSONAL_CHANNEL_NAME,
3735
useBackendChannel,
3836
} from "@posthog/ui/features/canvas/hooks/useTaskChannels";
39-
import { useChannelSeenStore } from "@posthog/ui/features/canvas/stores/channelSeenStore";
4037
import { useThreadPanelStore } from "@posthog/ui/features/canvas/stores/threadPanelStore";
4138
import { SuggestedPromptCard } from "@posthog/ui/features/task-detail/components/SuggestedPromptCard";
4239
import { taskDetailQuery } from "@posthog/ui/features/tasks/queries";
@@ -78,19 +75,8 @@ export function WebsiteChannelHome({ channelId }: { channelId: string }) {
7875
// identity-resolution window (settling if the resolve fails), so fold it in:
7976
// we can't call a channel empty until we know which channel it is.
8077
const isLoading = isLoadingChannels || isResolvingChannel || isLoadingFeed;
81-
// Viewing a channel reads it: stamp it seen so the sidebar drops its bold.
82-
// Keyed on the newest activity rather than "now", so a mention landing while
83-
// the channel is open re-stamps it (and so remounts don't churn the store).
84-
const { items: mentionItems } = useMentionActivity();
85-
const latestActivityAt = useMemo(
86-
() => latestActivityForChannel(mentionItems, backendChannel?.id),
87-
[mentionItems, backendChannel?.id],
88-
);
89-
const markChannelSeen = useChannelSeenStore((s) => s.markChannelSeen);
90-
useEffect(() => {
91-
if (!backendChannel?.id || !latestActivityAt) return;
92-
markChannelSeen(backendChannel.id, latestActivityAt);
93-
}, [backendChannel?.id, latestActivityAt, markChannelSeen]);
78+
// Marking this channel read lives in ChannelHeader (rendered by every channel
79+
// surface), so opening Artifacts or CONTEXT.md counts as reading it too.
9480

9581
// Durable "PostHog agent" rows (CONTEXT.md being built, …) live on the
9682
// backend channel — the same id the feed tasks use, not the folder id.
Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
import type { Channel } from "@posthog/ui/features/canvas/hooks/useChannels";
2+
import { beforeEach, expect, it, vi } from "vitest";
3+
import { ensurePersonalChannel } from "./ensurePersonalChannel";
4+
5+
function channel(id: string, name = "me"): Channel {
6+
return { id, name, path: `/${name}`, type: "folder" } as Channel;
7+
}
8+
9+
// The module memoises the created folder, so each test needs a fresh copy.
10+
beforeEach(() => {
11+
vi.resetModules();
12+
});
13+
14+
it("returns the existing folder without creating", async () => {
15+
const create = vi.fn();
16+
const existing = channel("1");
17+
await expect(ensurePersonalChannel([existing], create)).resolves.toBe(
18+
existing,
19+
);
20+
expect(create).not.toHaveBeenCalled();
21+
});
22+
23+
it("shares one create between callers racing before it settles", async () => {
24+
const { ensurePersonalChannel: ensure } = await import(
25+
"./ensurePersonalChannel"
26+
);
27+
const create = vi.fn(
28+
() => new Promise<Channel>((r) => setTimeout(() => r(channel("1")), 5)),
29+
);
30+
31+
const [a, b] = await Promise.all([ensure([], create), ensure([], create)]);
32+
33+
expect(create).toHaveBeenCalledTimes(1);
34+
expect(a).toBe(b);
35+
});
36+
37+
it("doesn't create a second folder for a caller still holding the pre-create list", async () => {
38+
const { ensurePersonalChannel: ensure } = await import(
39+
"./ensurePersonalChannel"
40+
);
41+
const create = vi.fn(async () => channel("1"));
42+
43+
// First caller creates it. The cache is seeded, but a component that hasn't
44+
// re-rendered yet still passes the empty list it captured last render.
45+
await ensure([], create);
46+
const second = await ensure([], create);
47+
48+
expect(create).toHaveBeenCalledTimes(1);
49+
expect(second.id).toBe("1");
50+
});
51+
52+
it("prefers the list once it carries the folder, so a recreated me isn't stale", async () => {
53+
const { ensurePersonalChannel: ensure } = await import(
54+
"./ensurePersonalChannel"
55+
);
56+
const create = vi.fn(async () => channel("1"));
57+
await ensure([], create);
58+
59+
// "me" was deleted and remade elsewhere; the list is authoritative.
60+
const fresh = channel("2");
61+
await expect(ensure([fresh], create)).resolves.toBe(fresh);
62+
// …and the stale memo is dropped rather than resurfacing afterwards.
63+
await expect(ensure([fresh], create)).resolves.toBe(fresh);
64+
expect(create).toHaveBeenCalledTimes(1);
65+
});
66+
67+
it("lets a later caller retry after a failed create", async () => {
68+
const { ensurePersonalChannel: ensure } = await import(
69+
"./ensurePersonalChannel"
70+
);
71+
const create = vi
72+
.fn<() => Promise<Channel>>()
73+
.mockRejectedValueOnce(new Error("offline"))
74+
.mockResolvedValueOnce(channel("1"));
75+
76+
await expect(ensure([], create)).rejects.toThrow("offline");
77+
await expect(ensure([], create)).resolves.toEqual(channel("1"));
78+
expect(create).toHaveBeenCalledTimes(2);
79+
});

packages/ui/src/features/canvas/ensurePersonalChannel.ts

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,12 @@ import { PERSONAL_CHANNEL_NAME } from "@posthog/ui/features/canvas/hooks/useTask
88
// menu), so they share one in-flight create rather than guarding separately:
99
// per-caller guards would still race each other.
1010
let inFlight: Promise<Channel> | null = null;
11+
// The in-flight promise alone isn't enough: it settles the moment the POST
12+
// returns, but callers pass the `channels` from their last render, which hasn't
13+
// re-rendered with the seeded cache yet. A click landing in that gap sees
14+
// neither an existing "me" nor an in-flight create, and makes a second one.
15+
// Remember what was created until the list catches up.
16+
let created: Channel | null = null;
1117

1218
/**
1319
* The user's "me" folder, creating it once if it doesn't exist yet. Concurrent
@@ -19,11 +25,22 @@ export async function ensurePersonalChannel(
1925
createChannel: (name: string) => Promise<Channel>,
2026
): Promise<Channel> {
2127
const existing = channels.find((c) => c.name === PERSONAL_CHANNEL_NAME);
22-
if (existing) return existing;
28+
if (existing) {
29+
// The list is authoritative once it carries the folder: drop the memo, so a
30+
// deleted-then-recreated "me" resolves fresh rather than to a dead id.
31+
created = null;
32+
return existing;
33+
}
34+
if (created) return created;
2335
if (!inFlight) {
24-
inFlight = createChannel(PERSONAL_CHANNEL_NAME).finally(() => {
25-
inFlight = null;
26-
});
36+
inFlight = createChannel(PERSONAL_CHANNEL_NAME)
37+
.then((channel) => {
38+
created = channel;
39+
return channel;
40+
})
41+
.finally(() => {
42+
inFlight = null;
43+
});
2744
}
2845
return inFlight;
2946
}

0 commit comments

Comments
 (0)