Skip to content

Commit 0a8bdca

Browse files
pauldambracharlesvien
authored andcommitted
refactor: simplify the stale-conversation gate changes
- clamp selectedIndex in useActionSelectorState when options change, replacing the consumer-level remount key (fixes the primitive for every consumer with live-changing options) - extract ComposerSlot/ComposerWidth and ConnectingToAgent in SessionView, collapsing three copies of the composer-slot wrapper and two copies of the connecting spinner - reuse formatUsd instead of hand-rolled toFixed(2) - plain functions instead of useCallbacks for the gate handlers (the notice isn't memoized and the closed-over usage values change per streamed event anyway); single guard on onNewSession - flatten the gate store's engagement Map to number | null and keep the Map reference stable when acknowledge has nothing to release Generated-By: PostHog Code Task-Id: 1ec8e82e-b126-48b8-8fd4-54edcaf041d5
1 parent 0887efc commit 0a8bdca

6 files changed

Lines changed: 128 additions & 101 deletions

File tree

packages/ui/src/features/sessions/components/SessionView.tsx

Lines changed: 92 additions & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,54 @@ interface SessionViewProps {
9999
const DEFAULT_ERROR_MESSAGE =
100100
"Failed to resume this session. The working directory may have been deleted. Please start a new session.";
101101

102+
function ConnectingToAgent() {
103+
return (
104+
<>
105+
<Spinner size={28} className="animate-spin text-gray-9" />
106+
<Text color="gray" className="text-base">
107+
Connecting to agent...
108+
</Text>
109+
</>
110+
);
111+
}
112+
113+
/** Centers composer-slot content at the chat width (or compact padding). */
114+
function ComposerWidth({
115+
compact,
116+
children,
117+
}: {
118+
compact: boolean;
119+
children: React.ReactNode;
120+
}) {
121+
return (
122+
<Box
123+
className={compact ? "p-1" : "mx-auto px-2 pb-3"}
124+
style={compact ? undefined : { maxWidth: CHAT_CONTENT_MAX_WIDTH }}
125+
>
126+
{children}
127+
</Box>
128+
);
129+
}
130+
131+
/**
132+
* Input region replacing the composer: `shrink-0` keeps it from being
133+
* compressed by the scroller above, and `min-h-0 overflow-y-auto` lets tall
134+
* content scroll inside itself.
135+
*/
136+
function ComposerSlot({
137+
compact,
138+
children,
139+
}: {
140+
compact: boolean;
141+
children: React.ReactNode;
142+
}) {
143+
return (
144+
<Box className="min-h-0 shrink-0 overflow-y-auto">
145+
<ComposerWidth compact={compact}>{children}</ComposerWidth>
146+
</Box>
147+
);
148+
}
149+
102150
interface CloudStreamDisconnectedBannerProps {
103151
errorTitle?: string;
104152
errorMessage?: string;
@@ -276,35 +324,38 @@ export function SessionView({
276324
// prompt cache has likely expired (see useStaleConversationGate).
277325
const staleGate = useStaleConversationGate(sessionId, events);
278326

279-
const trackStaleGateChoice = useCallback(
280-
(choice: StaleConversationGateChoiceProperties["choice"]) =>
281-
track(ANALYTICS_EVENTS.STALE_CONVERSATION_GATE_CHOICE, {
282-
choice,
283-
used_tokens: staleGate.usedTokens,
284-
cost_usd: staleGate.costUsd,
285-
}),
286-
[staleGate.usedTokens, staleGate.costUsd],
287-
);
327+
// Plain functions, not useCallbacks: the notice isn't memoized, and the
328+
// values they close over (usage, cost) change with every streamed event.
329+
const trackStaleGateChoice = (
330+
choice: StaleConversationGateChoiceProperties["choice"],
331+
) =>
332+
track(ANALYTICS_EVENTS.STALE_CONVERSATION_GATE_CHOICE, {
333+
choice,
334+
used_tokens: staleGate.usedTokens,
335+
cost_usd: staleGate.costUsd,
336+
});
288337

289-
const handleStaleCompact = useCallback(() => {
338+
const handleStaleCompact = () => {
290339
if (!isOnline) {
291340
showOfflineToast();
292341
return;
293342
}
294343
trackStaleGateChoice("compact");
295344
staleGate.onContinue();
296345
onSendPrompt("/compact");
297-
}, [isOnline, trackStaleGateChoice, staleGate.onContinue, onSendPrompt]);
346+
};
298347

299-
const handleStaleContinue = useCallback(() => {
348+
const handleStaleContinue = () => {
300349
trackStaleGateChoice("continue");
301350
staleGate.onContinue();
302-
}, [trackStaleGateChoice, staleGate.onContinue]);
351+
};
303352

304-
const handleStaleNewSession = useCallback(() => {
305-
trackStaleGateChoice("new_session");
306-
onNewSession?.();
307-
}, [trackStaleGateChoice, onNewSession]);
353+
const handleStaleNewSession = onNewSession
354+
? () => {
355+
trackStaleGateChoice("new_session");
356+
onNewSession();
357+
}
358+
: undefined;
308359

309360
const [isDraggingFile, setIsDraggingFile] = useState(false);
310361
const editorRef = useRef<PromptInputHandle>(null);
@@ -611,42 +662,20 @@ export function SessionView({
611662
// Replaces the composer (and any pending permission — answering
612663
// one also resumes the costly turn) until the user chooses.
613664
isRunning ? (
614-
<Box className="min-h-0 shrink-0 overflow-y-auto">
615-
<Box
616-
className={compact ? "p-1" : "mx-auto px-2 pb-3"}
617-
style={
618-
compact
665+
<ComposerSlot compact={compact}>
666+
<StaleConversationCostNotice
667+
usedTokens={staleGate.usedTokens}
668+
lastActivityAt={staleGate.lastActivityAt}
669+
costUsd={staleGate.costUsd}
670+
onContinue={handleStaleContinue}
671+
onCompact={
672+
firstPendingPermission
619673
? undefined
620-
: { maxWidth: CHAT_CONTENT_MAX_WIDTH }
674+
: handleStaleCompact
621675
}
622-
>
623-
<StaleConversationCostNotice
624-
// Remount when the option set changes so the
625-
// selector's highlighted index can't dereference or
626-
// fire a different option than the one shown.
627-
key={
628-
firstPendingPermission
629-
? "no-compact"
630-
: "with-compact"
631-
}
632-
usedTokens={staleGate.usedTokens}
633-
lastActivityAt={staleGate.lastActivityAt}
634-
costUsd={staleGate.costUsd}
635-
onContinue={handleStaleContinue}
636-
// A queued /compact would land after the pending
637-
// permission resumes the costly turn — paying the
638-
// reload twice — so the option hides until then.
639-
onCompact={
640-
firstPendingPermission
641-
? undefined
642-
: handleStaleCompact
643-
}
644-
onNewSession={
645-
onNewSession ? handleStaleNewSession : undefined
646-
}
647-
/>
648-
</Box>
649-
</Box>
676+
onNewSession={handleStaleNewSession}
677+
/>
678+
</ComposerSlot>
650679
) : (
651680
// While reconnecting the gate still covers the composer
652681
// slot: handoff can leave pendingPermissions set, and the
@@ -657,33 +686,18 @@ export function SessionView({
657686
gap="2"
658687
className="min-h-[66px]"
659688
>
660-
<Spinner size={28} className="animate-spin text-gray-9" />
661-
<Text color="gray" className="text-base">
662-
Connecting to agent...
663-
</Text>
689+
<ConnectingToAgent />
664690
</Flex>
665691
)
666692
) : firstPendingPermission ? (
667-
// This box replaces the composer while a permission is pending, so it's an input
668-
// region: `shrink-0` keeps it from being compressed by the scroller above, and
669-
// `min-h-0 overflow-y-auto` lets a tall permission prompt scroll inside itself.
670-
<Box className="min-h-0 shrink-0 overflow-y-auto">
671-
<Box
672-
className={compact ? "p-1" : "mx-auto px-2 pb-3"}
673-
style={
674-
compact
675-
? undefined
676-
: { maxWidth: CHAT_CONTENT_MAX_WIDTH }
677-
}
678-
>
679-
<PermissionSelector
680-
toolCall={firstPendingPermission.toolCall}
681-
options={firstPendingPermission.options}
682-
onSelect={handlePermissionSelect}
683-
onCancel={handlePermissionCancel}
684-
/>
685-
</Box>
686-
</Box>
693+
<ComposerSlot compact={compact}>
694+
<PermissionSelector
695+
toolCall={firstPendingPermission.toolCall}
696+
options={firstPendingPermission.options}
697+
onSelect={handlePermissionSelect}
698+
onCancel={handlePermissionCancel}
699+
/>
700+
</ComposerSlot>
687701
) : (
688702
<Box className="relative">
689703
<Box
@@ -693,10 +707,7 @@ export function SessionView({
693707
: "opacity-100"
694708
}`}
695709
>
696-
<Spinner size={28} className="animate-spin text-gray-9" />
697-
<Text color="gray" className="text-base">
698-
Connecting to agent...
699-
</Text>
710+
<ConnectingToAgent />
700711
</Box>
701712
<Box
702713
className={`transition-all duration-300 ease-out ${
@@ -705,14 +716,7 @@ export function SessionView({
705716
: "pointer-events-none translate-y-4 opacity-0"
706717
}`}
707718
>
708-
<Box
709-
className={compact ? "p-1" : "mx-auto px-2 pb-3"}
710-
style={
711-
compact
712-
? undefined
713-
: { maxWidth: CHAT_CONTENT_MAX_WIDTH }
714-
}
715-
>
719+
<ComposerWidth compact={compact}>
716720
{taskId && <QueuedMessagesDock taskId={taskId} />}
717721
<PromptInput
718722
ref={editorRef}
@@ -764,7 +768,7 @@ export function SessionView({
764768
onBashCommand={onBashCommand}
765769
onCancel={onCancelPrompt}
766770
/>
767-
</Box>
771+
</ComposerWidth>
768772
</Box>
769773
</Box>
770774
)}

packages/ui/src/features/sessions/components/StaleConversationCostNotice.tsx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { formatUsd } from "@posthog/core/billing/spendAnalysisFormat";
12
import { formatRelativeTimeLong } from "@posthog/shared";
23
import { formatTokensCompact } from "@posthog/ui/features/sessions/contextColors";
34
import { ActionSelector } from "@posthog/ui/primitives/ActionSelector";
@@ -40,7 +41,7 @@ export function StaleConversationCostNotice({
4041
? `was last active ${formatRelativeTimeLong(lastActivityAt)}`
4142
: "has been idle";
4243
const spent =
43-
costUsd !== null ? ` (≈$${costUsd.toFixed(2)} spent so far)` : "";
44+
costUsd !== null ? ` (≈${formatUsd(costUsd)} spent so far)` : "";
4445
return (
4546
<ActionSelector
4647
title="Continue this large, idle conversation?"

packages/ui/src/features/sessions/staleConversationGateStore.test.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,14 +20,14 @@ describe("useStaleConversationGateStore", () => {
2020

2121
it("engages a single session with its last-activity snapshot", () => {
2222
state().engage("s1", 1000);
23-
expect(state().engagedSessions.get("s1")).toEqual({ lastActivityAt: 1000 });
23+
expect(state().engagedSessions.get("s1")).toBe(1000);
2424
expect(engaged("s2")).toBe(false);
2525
});
2626

2727
it("keeps the first engagement snapshot when engaged again", () => {
2828
state().engage("s1", 1000);
2929
state().engage("s1", 2000);
30-
expect(state().engagedSessions.get("s1")).toEqual({ lastActivityAt: 1000 });
30+
expect(state().engagedSessions.get("s1")).toBe(1000);
3131
});
3232

3333
it("does not re-engage an acknowledged session", () => {
@@ -62,4 +62,11 @@ describe("useStaleConversationGateStore", () => {
6262
const second = state().acknowledgedSessions;
6363
expect(second).toBe(first);
6464
});
65+
66+
it("acknowledging a never-engaged session keeps the engaged Map reference", () => {
67+
state().engage("s2", 1000);
68+
const before = state().engagedSessions;
69+
state().acknowledge("s1");
70+
expect(state().engagedSessions).toBe(before);
71+
});
6572
});

packages/ui/src/features/sessions/staleConversationGateStore.ts

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,13 @@ import { create } from "zustand";
33
interface StaleConversationGateState {
44
/**
55
* Sessions where the gate engaged, keyed to the last-activity time observed
6-
* at that moment. Latched because reconnecting to a stale session
7-
* immediately appends freshly-stamped events (usage updates, handshakes)
8-
* that would otherwise make the conversation look active again and dismiss
9-
* the warning before the user has chosen.
6+
* at that moment (null when no activity was observed). Latched because
7+
* reconnecting to a stale session immediately appends freshly-stamped
8+
* events (usage updates, handshakes) that would otherwise make the
9+
* conversation look active again and dismiss the warning before the user
10+
* has chosen.
1011
*/
11-
engagedSessions: Map<string, { lastActivityAt: number | null }>;
12+
engagedSessions: Map<string, number | null>;
1213
/** Sessions where the user accepted the "large + idle = costly" warning. */
1314
acknowledgedSessions: Set<string>;
1415
}
@@ -40,7 +41,7 @@ export const useStaleConversationGateStore =
4041
return state;
4142
}
4243
const next = new Map(state.engagedSessions);
43-
next.set(sessionId, { lastActivityAt });
44+
next.set(sessionId, lastActivityAt);
4445
return { engagedSessions: next };
4546
}),
4647

@@ -49,6 +50,9 @@ export const useStaleConversationGateStore =
4950
if (state.acknowledgedSessions.has(sessionId)) return state;
5051
const nextAcknowledged = new Set(state.acknowledgedSessions);
5152
nextAcknowledged.add(sessionId);
53+
if (!state.engagedSessions.has(sessionId)) {
54+
return { acknowledgedSessions: nextAcknowledged };
55+
}
5256
const nextEngaged = new Map(state.engagedSessions);
5357
nextEngaged.delete(sessionId);
5458
return {

packages/ui/src/features/sessions/useStaleConversationGate.ts

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,10 @@ export function useStaleConversationGate(
3636
const contextUsage = useContextUsage(events);
3737
const { data: currentUser } = useMeQuery();
3838
const isStaff = currentUser?.is_staff === true;
39-
const engagement = useStaleConversationGateStore((s) =>
39+
const engaged = useStaleConversationGateStore((s) =>
40+
s.engagedSessions.has(sessionId),
41+
);
42+
const engagedLastActivityAt = useStaleConversationGateStore((s) =>
4043
s.engagedSessions.get(sessionId),
4144
);
4245
const acknowledged = useStaleConversationGateStore((s) =>
@@ -67,11 +70,13 @@ export function useStaleConversationGate(
6770

6871
return {
6972
// shouldEngage covers the first paint before the effect latches;
70-
// engagement covers every render after (reconnect events flip
73+
// engaged covers every render after (reconnect events flip
7174
// shouldEngage back off — see the hook doc).
72-
active: shouldEngage || engagement !== undefined,
75+
active: shouldEngage || engaged,
7376
usedTokens,
74-
lastActivityAt: engagement ? engagement.lastActivityAt : liveLastActivityAt,
77+
lastActivityAt: engaged
78+
? (engagedLastActivityAt ?? null)
79+
: liveLastActivityAt,
7580
costUsd: contextUsage?.cost?.amount ?? null,
7681
onContinue,
7782
};

packages/ui/src/primitives/action-selector/useActionSelectorState.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,12 @@ export function useActionSelectorState({
103103
isEditing && selectedOption && needsCustomInput(selectedOption);
104104
const canSubmitOrAdvance = checkedOptions.size > 0;
105105

106+
// Options can change while mounted (a consumer adding/removing a choice);
107+
// clamp so the highlight and Enter never reference past the end of the list.
108+
useEffect(() => {
109+
setSelectedIndex((i) => Math.min(i, Math.max(0, numOptions - 1)));
110+
}, [numOptions]);
111+
106112
useEffect(() => {
107113
if (!isInteractiveElementInDifferentCell(containerRef)) {
108114
containerRef.current?.focus();

0 commit comments

Comments
 (0)