Skip to content

Commit 228c2ee

Browse files
charlesviencharlesvien
committed
fix builder session pruning and row style
1 parent 0bf2a9e commit 228c2ee

5 files changed

Lines changed: 199 additions & 46 deletions

File tree

‎packages/ui/src/features/loops/components/LoopsListView.tsx‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import { useOrgMembers } from "@posthog/ui/features/canvas/hooks/useOrgMembers";
1111
import { StopCloudRunDialog } from "@posthog/ui/features/sessions/components/StopCloudRunDialog";
1212
import { useSetHeaderContent } from "@posthog/ui/hooks/useSetHeaderContent";
1313
import { Button } from "@posthog/ui/primitives/Button";
14+
import { toast } from "@posthog/ui/primitives/toast";
1415
import {
1516
navigateToNewLoop,
1617
navigateToTaskDetail,
@@ -264,11 +265,11 @@ function BuilderSessionRow({
264265
<Flex
265266
align="center"
266267
gap="3"
267-
className="rounded-(--radius-3) border border-border bg-(--color-panel-solid) px-3 py-2"
268+
className="rounded-(--radius-2) border border-border bg-(--color-panel-solid) px-3 py-2"
268269
>
269270
<ChatCircleDotsIcon size={16} className="shrink-0 text-(--accent-11)" />
270271
<Flex direction="column" className="min-w-0 flex-1">
271-
<Text className="font-medium text-[11px] text-gray-9 uppercase tracking-wide">
272+
<Text className="font-medium text-[12px] text-gray-10 uppercase tracking-wide">
272273
Builder in progress
273274
</Text>
274275
<Text className="truncate text-[13px] text-gray-12">
@@ -283,12 +284,12 @@ function BuilderSessionRow({
283284
Resume
284285
</Button>
285286
<Button
286-
variant="ghost"
287-
color="gray"
287+
variant="soft"
288+
color="red"
288289
size="1"
289290
onClick={() => setConfirmStop(true)}
290291
>
291-
<StopIcon size={12} />
292+
<StopIcon size={12} weight="bold" />
292293
Stop
293294
</Button>
294295
{confirmStop ? (
@@ -298,7 +299,10 @@ function BuilderSessionRow({
298299
title="Stop loop builder"
299300
buttonLabel="Stop builder"
300301
onOpenChange={setConfirmStop}
301-
onStopped={() => onStopped?.(session.taskId)}
302+
onStopped={() => {
303+
toast.success("Builder stopped");
304+
onStopped?.(session.taskId);
305+
}}
302306
/>
303307
) : null}
304308
</Flex>

‎packages/ui/src/features/loops/hooks/useLoopBuilderSessions.ts‎

Lines changed: 50 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,22 @@
1-
import { isTerminalStatus } from "@posthog/shared/domain-types";
21
import { useArchivedTaskIds } from "@posthog/ui/features/archive/useArchivedTaskIds";
32
import { useTaskSummaries } from "@posthog/ui/features/tasks/useTasks";
4-
import { useEffect, useMemo } from "react";
3+
import { useEffect, useMemo, useState } from "react";
4+
import {
5+
type BuilderRunSummaries,
6+
FRESH_SESSION_GRACE_MS,
7+
isBuilderSessionEnded,
8+
} from "../loopBuilderLiveness";
59
import {
610
type LoopBuilderSession,
711
useLoopBuilderSessionStore,
812
} from "../loopBuilderSessionStore";
913

10-
// A fresh task can briefly report no run (or a stale summary via
11-
// keepPreviousData) before the cloud run registers; don't treat that as ended.
12-
const FRESH_SESSION_GRACE_MS = 60_000;
13-
1414
/**
1515
* The recorded builder sessions whose cloud run is still alive. Sessions whose
1616
* sandbox has shut down (run completed, failed, cancelled, or task archived or
1717
* deleted) are pruned from the persisted store as their status comes in, so the
18-
* "in progress" list never offers a resume into a dead session.
18+
* "in progress" list never offers a resume into a dead session. The liveness
19+
* decision itself is the pure `isBuilderSessionEnded`.
1920
*/
2021
export function useLoopBuilderSessions(): LoopBuilderSession[] {
2122
const sessions = useLoopBuilderSessionStore((state) => state.sessions);
@@ -24,45 +25,55 @@ export function useLoopBuilderSessions(): LoopBuilderSession[] {
2425
() => sessions.map((session) => session.taskId),
2526
[sessions],
2627
);
27-
const {
28-
data: summaries,
29-
isSuccess,
30-
isPlaceholderData,
31-
} = useTaskSummaries(taskIds);
28+
const { data, isSuccess, isPlaceholderData } = useTaskSummaries(taskIds);
3229

33-
const liveTaskIds = useMemo(() => {
34-
if (!isSuccess || isPlaceholderData) return null;
35-
const live = new Set<string>();
36-
for (const summary of summaries ?? []) {
37-
const run = summary.latest_run;
38-
if (run?.environment === "cloud" && !isTerminalStatus(run.status)) {
39-
live.add(summary.id);
40-
}
41-
}
42-
return live;
43-
}, [isSuccess, isPlaceholderData, summaries]);
30+
const summaries = useMemo<BuilderRunSummaries | null>(() => {
31+
// Placeholder data is the previous id set's response; judging liveness on
32+
// it would prune a just-added session that isn't in that response yet.
33+
if (!isSuccess || isPlaceholderData || !data) return null;
34+
return new Map(
35+
data.map((summary) => [
36+
summary.id,
37+
summary.latest_run
38+
? {
39+
environment: summary.latest_run.environment,
40+
status: summary.latest_run.status,
41+
}
42+
: null,
43+
]),
44+
);
45+
}, [isSuccess, isPlaceholderData, data]);
4446

47+
// Grace expiry doesn't produce a re-render by itself (polled summaries keep
48+
// their identity when nothing changed), so schedule one for the soonest
49+
// boundary; `now` is otherwise only refreshed by real data changes.
50+
const [now, setNow] = useState(() => Date.now());
4551
useEffect(() => {
46-
if (!liveTaskIds) return;
52+
const waits = sessions
53+
.map((session) => session.startedAt + FRESH_SESSION_GRACE_MS - now)
54+
.filter((wait) => wait > 0);
55+
if (waits.length === 0) return;
56+
const timer = setTimeout(() => setNow(Date.now()), Math.min(...waits) + 50);
57+
return () => clearTimeout(timer);
58+
}, [sessions, now]);
59+
60+
useEffect(() => {
61+
if (!summaries) return;
4762
const store = useLoopBuilderSessionStore.getState();
4863
for (const session of store.sessions) {
49-
const dead =
50-
!liveTaskIds.has(session.taskId) &&
51-
Date.now() - session.startedAt >= FRESH_SESSION_GRACE_MS;
52-
if (dead || archivedTaskIds.has(session.taskId)) {
64+
if (isBuilderSessionEnded(session, summaries, archivedTaskIds, now)) {
5365
store.removeSession(session.taskId);
5466
}
5567
}
56-
}, [liveTaskIds, archivedTaskIds]);
68+
}, [summaries, archivedTaskIds, now]);
5769

58-
return useMemo(
59-
() =>
60-
sessions.filter((session) => {
61-
if (archivedTaskIds.has(session.taskId)) return false;
62-
if (!liveTaskIds) return true;
63-
if (liveTaskIds.has(session.taskId)) return true;
64-
return Date.now() - session.startedAt < FRESH_SESSION_GRACE_MS;
65-
}),
66-
[sessions, archivedTaskIds, liveTaskIds],
67-
);
70+
return useMemo(() => {
71+
if (!summaries) {
72+
return sessions.filter((session) => !archivedTaskIds.has(session.taskId));
73+
}
74+
return sessions.filter(
75+
(session) =>
76+
!isBuilderSessionEnded(session, summaries, archivedTaskIds, now),
77+
);
78+
}, [sessions, summaries, archivedTaskIds, now]);
6879
}

‎packages/ui/src/features/loops/hooks/useLoopBuilderTask.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ export function useLoopBuilderTask(context?: {
4343
return {
4444
content: taskContent,
4545
// Divergent on purpose: the description becomes the task's title, so
46-
// the sidebar row reads as the builder instead of the raw prompt.
46+
// the "Loop builder:" prefix marks the sidebar row as a builder session.
4747
taskDescription: hasSeed
4848
? `Loop builder: ${userPrompt}`
4949
: "Loop builder",

‎packages/ui/src/features/loops/loopBuilderLiveness.test.ts‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
import { expect, it } from "vitest";
2+
import {
3+
type BuilderRunSummary,
4+
FRESH_SESSION_GRACE_MS,
5+
isBuilderSessionEnded,
6+
} from "./loopBuilderLiveness";
7+
import type { LoopBuilderSession } from "./loopBuilderSessionStore";
8+
9+
const NOW = 1_752_000_000_000;
10+
11+
function session(ageMs: number): LoopBuilderSession {
12+
return { taskId: "task-1", prompt: "prompt", startedAt: NOW - ageMs };
13+
}
14+
15+
function summaries(
16+
run: BuilderRunSummary | null | undefined,
17+
): Map<string, BuilderRunSummary | null> {
18+
const map = new Map<string, BuilderRunSummary | null>();
19+
if (run !== undefined) map.set("task-1", run);
20+
return map;
21+
}
22+
23+
const WITHIN_GRACE = FRESH_SESSION_GRACE_MS - 1_000;
24+
const PAST_GRACE = FRESH_SESSION_GRACE_MS + 1_000;
25+
26+
it.each([
27+
{
28+
name: "unknown task within grace stays",
29+
ageMs: WITHIN_GRACE,
30+
run: undefined,
31+
ended: false,
32+
},
33+
{
34+
name: "unknown task past grace is ended",
35+
ageMs: PAST_GRACE,
36+
run: undefined,
37+
ended: true,
38+
},
39+
{
40+
name: "runless task within grace stays",
41+
ageMs: WITHIN_GRACE,
42+
run: null,
43+
ended: false,
44+
},
45+
{
46+
name: "runless task past grace is ended",
47+
ageMs: PAST_GRACE,
48+
run: null,
49+
ended: true,
50+
},
51+
{
52+
name: "running cloud run stays even past grace",
53+
ageMs: PAST_GRACE,
54+
run: { environment: "cloud", status: "in_progress" },
55+
ended: false,
56+
},
57+
{
58+
name: "queued cloud run stays",
59+
ageMs: PAST_GRACE,
60+
run: { environment: "cloud", status: "queued" },
61+
ended: false,
62+
},
63+
{
64+
name: "statusless cloud run stays",
65+
ageMs: PAST_GRACE,
66+
run: { environment: "cloud", status: null },
67+
ended: false,
68+
},
69+
{
70+
name: "completed run is ended even within grace",
71+
ageMs: WITHIN_GRACE,
72+
run: { environment: "cloud", status: "completed" },
73+
ended: true,
74+
},
75+
{
76+
name: "failed run is ended even within grace",
77+
ageMs: WITHIN_GRACE,
78+
run: { environment: "cloud", status: "failed" },
79+
ended: true,
80+
},
81+
{
82+
name: "cancelled run is ended even within grace",
83+
ageMs: WITHIN_GRACE,
84+
run: { environment: "cloud", status: "cancelled" },
85+
ended: true,
86+
},
87+
{
88+
name: "non-cloud run is ended",
89+
ageMs: WITHIN_GRACE,
90+
run: { environment: "local", status: "in_progress" },
91+
ended: true,
92+
},
93+
])("$name", ({ ageMs, run, ended }) => {
94+
expect(
95+
isBuilderSessionEnded(session(ageMs), summaries(run), new Set(), NOW),
96+
).toBe(ended);
97+
});
98+
99+
it("archived task is ended regardless of a live run", () => {
100+
expect(
101+
isBuilderSessionEnded(
102+
session(WITHIN_GRACE),
103+
summaries({ environment: "cloud", status: "in_progress" }),
104+
new Set(["task-1"]),
105+
NOW,
106+
),
107+
).toBe(true);
108+
});

‎packages/ui/src/features/loops/loopBuilderLiveness.ts‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
import { isTerminalStatus } from "@posthog/shared/domain-types";
2+
import type { LoopBuilderSession } from "./loopBuilderSessionStore";
3+
4+
// A fresh task can briefly report no run (or be absent from the summaries
5+
// response) before its cloud run registers; don't treat that as ended.
6+
export const FRESH_SESSION_GRACE_MS = 60_000;
7+
8+
export interface BuilderRunSummary {
9+
environment: string | null;
10+
status: string | null;
11+
}
12+
13+
/** taskId -> latest run; a null value means the task exists but has no run,
14+
* an absent key means the summaries response doesn't know the task at all. */
15+
export type BuilderRunSummaries = ReadonlyMap<string, BuilderRunSummary | null>;
16+
17+
export function isBuilderSessionEnded(
18+
session: LoopBuilderSession,
19+
summaries: BuilderRunSummaries,
20+
archivedTaskIds: ReadonlySet<string>,
21+
now: number,
22+
): boolean {
23+
if (archivedTaskIds.has(session.taskId)) return true;
24+
const pastGrace = now - session.startedAt >= FRESH_SESSION_GRACE_MS;
25+
if (!summaries.has(session.taskId)) return pastGrace;
26+
const run = summaries.get(session.taskId) ?? null;
27+
if (!run) return pastGrace;
28+
if (run.environment !== "cloud") return true;
29+
return isTerminalStatus(run.status);
30+
}

0 commit comments

Comments
 (0)