From 2e05b82aa6adc5b1bce941e43ccd607aaf12320b Mon Sep 17 00:00:00 2001 From: Alessandro Pogliaghi Date: Tue, 7 Jul 2026 18:03:33 +0100 Subject: [PATCH 1/2] fix(cloud-task): make question answered state durable across chat switches and restarts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since questions relay over the durable event stream (#3199, #3215), users report that switching chats resets answered questions. Two gaps cause it: 1. The question card held its in-progress step answers in component state. Switching chats unmounts the card, so every answer selected so far was wiped and the card restarted at question 1 on return. The answers now live in a view store keyed by toolCallId, and the card restores them (including free-text input, via a new ActionSelector initialCustomInput seed) on remount. Submit and cancel clear the draft. 2. A question answered after its run terminalized (the resume-as-prompt path) never got a `_posthog/permission_resolved` record: the sandbox that would write one is gone, so the request stays pending in the persisted log forever and any log derivation that escapes the in-memory guards (app restart, another device, a session rebuild) re-surfaces the answered question as a fresh card. The client now appends the resolved marker to the owning run's log itself, tracks responded requestIds so stale snapshots (a marker not yet flushed to storage) cannot re-add them, and the watcher dedups replayed permission_request frames by stream id — they previously bypassed the seenEventIds guard entirely. Generated-By: PostHog Code Task-Id: 9c723d92-b8ef-4c9f-9317-2f53b4644f33 --- .../core/src/cloud-task/cloud-task.test.ts | 56 +++++++ packages/core/src/cloud-task/cloud-task.ts | 30 ++-- packages/core/src/sessions/sessionService.ts | 118 ++++++++++++-- .../permissions/QuestionPermission.test.tsx | 95 +++++++++++ .../permissions/QuestionPermission.tsx | 123 +++++++++----- .../permissions/questionDraftStore.ts | 73 +++++++++ .../sessions/sessionServiceHost.test.ts | 153 ++++++++++++++++++ .../action-selector/ActionSelector.tsx | 2 + .../src/primitives/action-selector/types.ts | 1 + .../action-selector/useActionSelectorState.ts | 12 +- 10 files changed, 592 insertions(+), 71 deletions(-) create mode 100644 packages/ui/src/features/permissions/QuestionPermission.test.tsx create mode 100644 packages/ui/src/features/permissions/questionDraftStore.ts diff --git a/packages/core/src/cloud-task/cloud-task.test.ts b/packages/core/src/cloud-task/cloud-task.test.ts index 7cbd763302..48c7dac251 100644 --- a/packages/core/src/cloud-task/cloud-task.test.ts +++ b/packages/core/src/cloud-task/cloud-task.test.ts @@ -135,6 +135,62 @@ describe("CloudTaskService", () => { vi.unstubAllGlobals(); }); + it("emits a replayed permission_request frame only once", async () => { + const updates: unknown[] = []; + service.on(CloudTaskEvent.Update, (payload) => updates.push(payload)); + + mockNetFetch + .mockResolvedValueOnce( + createJsonResponse({ + id: "run-1", + status: "in_progress", + stage: "build", + output: null, + error_message: null, + branch: "main", + updated_at: "2026-01-01T00:00:00Z", + }), + ) + .mockResolvedValue( + createJsonResponse([], 200, { "X-Has-More": "false" }), + ); + + const frame = + 'id: 5\ndata: {"type":"permission_request","requestId":"req-1","toolCall":{"toolCallId":"tool-1"},"options":[]}\n\n'; + const trailingEntry = + 'id: 6\ndata: {"type":"notification","timestamp":"2026-01-01T00:00:02Z","notification":{"jsonrpc":"2.0","method":"_posthog/console","params":{"sessionId":"run-1","level":"info","message":"after replay"}}}\n\n'; + // The durable stream re-sends the tail on reconnect/replay — the same + // frame arriving twice must not re-surface the question a second time. + mockStreamFetch.mockResolvedValueOnce( + createOpenSseResponse(frame + frame + trailingEntry), + ); + + service.watch({ + taskId: "task-1", + runId: "run-1", + apiHost: "https://app.example.com", + teamId: 2, + }); + + await waitFor(() => + updates.some((u) => (u as { kind?: string }).kind === "logs"), + ); + + const permissionUpdates = updates.filter( + (u) => (u as { kind?: string }).kind === "permission_request", + ); + expect(permissionUpdates).toEqual([ + { + taskId: "task-1", + runId: "run-1", + kind: "permission_request", + requestId: "req-1", + toolCall: { toolCallId: "tool-1" }, + options: [], + }, + ]); + }); + it("bootstraps paged backlog for active runs and drains deduped live SSE entries", async () => { const updates: unknown[] = []; service.on(CloudTaskEvent.Update, (payload) => updates.push(payload)); diff --git a/packages/core/src/cloud-task/cloud-task.ts b/packages/core/src/cloud-task/cloud-task.ts index 576e53e8e1..10cbfc416f 100644 --- a/packages/core/src/cloud-task/cloud-task.ts +++ b/packages/core/src/cloud-task/cloud-task.ts @@ -1138,6 +1138,22 @@ export class CloudTaskService extends TypedEventEmitter { return null; } + // Drop a re-delivered event by its stream id. The durable stream re-sends + // the tail on reconnect/replay: each re-sent log entry would otherwise be + // counted as a new entry (advancing totalEntryCount past the renderer's + // processedLineCount guard) and emitted again — the root cause of duplicate + // transcript entries and back-to-back completion notifications — and a + // re-sent permission_request frame would re-surface an already-answered + // question as a fresh pending card. Events without an id (legacy servers) + // fall through and are handled downstream. + const eventId = event.id; + if (eventId !== undefined) { + if (watcher.seenEventIds.has(eventId)) { + return null; + } + watcher.seenEventIds.add(eventId); + } + if (isPermissionRequestEvent(event.data)) { this.emit(CloudTaskEvent.Update, { taskId: watcher.taskId, @@ -1150,20 +1166,6 @@ export class CloudTaskService extends TypedEventEmitter { return null; } - // Drop a re-delivered log entry by its stream id. The durable stream - // re-sends the tail on reconnect/replay, and each resend would otherwise be - // counted as a new entry (advancing totalEntryCount past the renderer's - // processedLineCount guard) and emitted again — the root cause of duplicate - // transcript entries and back-to-back completion notifications. Entries - // without an id (legacy servers) fall through and are handled downstream. - const eventId = event.id; - if (eventId !== undefined) { - if (watcher.seenEventIds.has(eventId)) { - return null; - } - watcher.seenEventIds.add(eventId); - } - watcher.pendingLogEntries.push(event.data as StoredLogEntry); if (watcher.pendingLogEntries.length >= EVENT_BATCH_MAX_SIZE) { this.flushLogBatch(key); diff --git a/packages/core/src/sessions/sessionService.ts b/packages/core/src/sessions/sessionService.ts index d0d3bc8720..5a775ad38a 100644 --- a/packages/core/src/sessions/sessionService.ts +++ b/packages/core/src/sessions/sessionService.ts @@ -98,6 +98,7 @@ const AUTO_RETRY_MAX_ATTEMPTS = 2; const AUTO_RETRY_DELAY_MS = 10_000; const AUTH_RESTORE_MAX_RETRY_WAITS = 6; const MAX_SUPERSEDED_RUN_IDS = 100; +const MAX_RESPONDED_PERMISSION_REQUEST_IDS = 500; /** * Streamed events are buffered and flushed on this cadence so a burst of tokens * coalesces into one processing pass (and roughly one render) instead of one @@ -607,6 +608,13 @@ export class SessionService { private cloudLogGapReconciler: CloudLogGapReconciler; /** Maps toolCallId → cloud requestId for routing permission responses */ private cloudPermissionRequestIds = new Map(); + /** + * Cloud permission requestIds the user has already responded to this app + * session. A stale snapshot (a resolved marker not yet flushed to storage) + * or a replayed stream frame can re-deliver an answered request; without + * this guard it would re-surface as a fresh pending card. + */ + private respondedCloudPermissionRequestIds = new Set(); private liveTurnContent = new Map< string, { startedAtTs: number; agentTextChunks: number; agentOutputEvents: number } @@ -2167,6 +2175,15 @@ export class SessionService { return; } + if (this.respondedCloudPermissionRequestIds.has(update.requestId)) { + this.d.log.debug("Skipping already-answered cloud permission request", { + taskRunId, + requestId: update.requestId, + toolCallId: update.toolCall.toolCallId, + }); + return; + } + if ( isPermissionRequestAlreadySurfaced( session.pendingPermissions, @@ -3179,6 +3196,7 @@ export class SessionService { session: AgentSession, permission: PermissionRequest | undefined, toolCallId: string, + requestId: string | undefined, optionId: string, customInput?: string, answers?: Record, @@ -3191,23 +3209,88 @@ export class SessionService { customInput, answers, ); - if (!answerPrompt) { + if (answerPrompt) { + await this.sendCloudPrompt( + { ...session, cloudStatus: cloudStatus ?? session.cloudStatus }, + answerPrompt, + ); + this.d.log.info("Permission answer resumed terminal cloud run", { + taskId: session.taskId, + toolCallId, + optionId, + }); + } else { this.d.log.info("Dropped permission response for terminal cloud run", { taskId: session.taskId, toolCallId, optionId, }); - return; } - await this.sendCloudPrompt( - { ...session, cloudStatus: cloudStatus ?? session.cloudStatus }, - answerPrompt, - ); - this.d.log.info("Permission answer resumed terminal cloud run", { - taskId: session.taskId, + await this.persistCloudPermissionResolution( + session.taskId, + permission?.taskRunId ?? session.taskRunId, toolCallId, + requestId, optionId, - }); + ); + } + + /** + * Record a response to a permission request whose sandbox is gone. A live + * sandbox writes `_posthog/permission_resolved` to the run log when it + * resolves a request, but a request answered after its run terminalized has + * no sandbox left to do that — without this record the request stays + * pending in the persisted log forever, and every future derivation (app + * restart, another device, a session rebuild) re-surfaces the + * already-answered question as a fresh card. + */ + private async persistCloudPermissionResolution( + taskId: string, + taskRunId: string, + toolCallId: string, + requestId: string | undefined, + optionId: string, + ): Promise { + if (!requestId) return; + this.markCloudPermissionResponded(requestId); + + const client = await this.d.getAuthenticatedClient(); + if (!client) return; + try { + await client.appendTaskRunLog(taskId, taskRunId, [ + { + type: "notification", + timestamp: new Date().toISOString(), + notification: { + method: POSTHOG_NOTIFICATIONS.PERMISSION_RESOLVED, + params: { requestId, toolCallId, optionId }, + }, + }, + ]); + } catch (error) { + this.d.log.warn("Failed to persist permission resolution to run log", { + taskId, + taskRunId, + toolCallId, + error, + }); + } + } + + private markCloudPermissionResponded(requestId: string): void { + this.respondedCloudPermissionRequestIds.add(requestId); + // add() grows the set by at most one, so one eviction restores the cap. + if ( + this.respondedCloudPermissionRequestIds.size > + MAX_RESPONDED_PERMISSION_REQUEST_IDS + ) { + const oldest = this.respondedCloudPermissionRequestIds + .values() + .next().value; + if (oldest !== undefined) { + this.respondedCloudPermissionRequestIds.delete(oldest); + } + } } // --- Permissions --- @@ -3269,6 +3352,7 @@ export class SessionService { session, permission, toolCallId, + cloudRequestId, optionId, customInput, answers, @@ -3292,6 +3376,7 @@ export class SessionService { session, permission, toolCallId, + cloudRequestId, optionId, customInput, answers, @@ -3301,6 +3386,10 @@ export class SessionService { } throw error; } + // The live sandbox persists its own resolved marker; remember the + // response locally so a snapshot fetched before that marker flushes + // to storage cannot re-surface the question. + this.markCloudPermissionResponded(cloudRequestId); } else { await this.d.trpc.agent.respondToPermission.mutate({ taskRunId: session.taskRunId, @@ -3352,8 +3441,16 @@ export class SessionService { try { if (session.isCloud && isTerminalStatus(session.cloudStatus)) { // The run is over — the card was resolved locally above and there is no - // live permission promise left to reject. + // live permission promise left to reject. Persist the dismissal so the + // request is not re-derived as pending from the run log later. this.cloudPermissionRequestIds.delete(toolCallId); + await this.persistCloudPermissionResolution( + session.taskId, + permission?.taskRunId ?? session.taskRunId, + toolCallId, + cloudRequestId, + "cancelled", + ); return; } if (session.isCloud && cloudRequestId) { @@ -3363,6 +3460,7 @@ export class SessionService { optionId: "reject_with_feedback", customInput: "User cancelled the permission request.", }); + this.markCloudPermissionResponded(cloudRequestId); } else { await this.d.trpc.agent.cancelPermission.mutate({ taskRunId: session.taskRunId, diff --git a/packages/ui/src/features/permissions/QuestionPermission.test.tsx b/packages/ui/src/features/permissions/QuestionPermission.test.tsx new file mode 100644 index 0000000000..c930d98802 --- /dev/null +++ b/packages/ui/src/features/permissions/QuestionPermission.test.tsx @@ -0,0 +1,95 @@ +import { Theme } from "@radix-ui/themes"; +import { render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { QuestionPermission } from "./QuestionPermission"; +import { useQuestionDraftStore } from "./questionDraftStore"; +import type { PermissionToolCall } from "./types"; + +const toolCall = { + toolCallId: "question-1", + title: "Questions", + _meta: { + codeToolKind: "question", + questions: [ + { + question: "Which framework?", + header: "Framework", + options: [{ label: "React" }, { label: "Vue" }], + }, + { + question: "Which color?", + header: "Color", + options: [{ label: "Red" }, { label: "Blue" }], + }, + ], + }, +} as unknown as PermissionToolCall; + +function renderQuestion() { + const onSelect = vi.fn(); + const onCancel = vi.fn(); + const view = render( + + + , + ); + return { onSelect, onCancel, view }; +} + +describe("QuestionPermission", () => { + beforeEach(() => { + useQuestionDraftStore.setState({ drafts: new Map() }); + }); + + it("restores in-progress answers when the card remounts", async () => { + const user = userEvent.setup(); + const first = renderQuestion(); + + await user.click(screen.getByText("React")); + await user.click(screen.getByText("Next")); + expect(screen.getByText("Which color?")).toBeDefined(); + + // Switching chats unmounts the card; the store keeps the draft. + first.view.unmount(); + + const second = renderQuestion(); + expect(screen.getByText("Which color?")).toBeDefined(); + + await user.click(screen.getByText("Blue")); + await user.click(screen.getByText("Next")); + + // The review step summarizes the answer given before the remount. + expect(screen.getByText("Ready to submit your answers?")).toBeDefined(); + expect(screen.getByText("React")).toBeDefined(); + + const submitOptions = screen.getAllByText("Submit"); + await user.click(submitOptions[submitOptions.length - 1] as HTMLElement); + + expect(second.onSelect).toHaveBeenCalledWith("_submit", undefined, { + "Which framework?": "React", + "Which color?": "Blue", + }); + expect(useQuestionDraftStore.getState().drafts.size).toBe(0); + }); + + it("clears the draft when the card is cancelled", async () => { + const user = userEvent.setup(); + const { onCancel } = renderQuestion(); + + await user.click(screen.getByText("React")); + await user.click(screen.getByText("Next")); + await user.click(screen.getByText("Red")); + await user.click(screen.getByText("Next")); + + await user.click(screen.getByText("Cancel")); + + expect(onCancel).toHaveBeenCalledTimes(1); + expect(useQuestionDraftStore.getState().drafts.size).toBe(0); + }); +}); diff --git a/packages/ui/src/features/permissions/QuestionPermission.tsx b/packages/ui/src/features/permissions/QuestionPermission.tsx index e6d1f89506..7df2490e6f 100644 --- a/packages/ui/src/features/permissions/QuestionPermission.tsx +++ b/packages/ui/src/features/permissions/QuestionPermission.tsx @@ -14,7 +14,8 @@ import { SUBMIT_OPTION_ID, } from "@posthog/ui/primitives/ActionSelector"; import { Box, Flex, Text } from "@radix-ui/themes"; -import { useCallback, useMemo, useState } from "react"; +import { useCallback, useMemo } from "react"; +import { useQuestionDraftStore } from "./questionDraftStore"; import { type BasePermissionProps, toSelectorOptions } from "./types"; function parseQuestionMeta(raw: unknown): QuestionMeta | undefined { @@ -77,6 +78,17 @@ function buildAnswersRecord( return result; } +function buildSingleQuestionAnswers( + selectedIds: string[], + customInput: string | undefined, + allQuestions: QuestionItem[], +): Record { + return buildAnswersRecord( + new Map([[0, { selectedIds, customInput: customInput ?? "" }]]), + allQuestions, + ); +} + function isQuestionAnswered( stepAnswers: Map, questionIndex: number, @@ -88,6 +100,8 @@ function isQuestionAnswered( return hasOptions || hasCustomInput; } +const EMPTY_STEP_ANSWERS: Map = new Map(); + export function QuestionPermission({ toolCall, options, @@ -97,11 +111,17 @@ export function QuestionPermission({ const meta = parseQuestionMeta(toolCall._meta); const allQuestions = meta?.questions ?? []; const totalQuestions = allQuestions.length; + const toolCallId = toolCall.toolCallId; - const [activeStep, setActiveStep] = useState(0); - const [stepAnswers, setStepAnswers] = useState>( - () => new Map(), - ); + // Chat switches unmount this card. The draft store preserves the + // in-progress answers so a remount restores them instead of resetting to + // the first question. + const draft = useQuestionDraftStore((s) => s.drafts.get(toolCallId)); + const setDraftActiveStep = useQuestionDraftStore((s) => s.setActiveStep); + const setDraftStepAnswer = useQuestionDraftStore((s) => s.setStepAnswer); + const clearDraft = useQuestionDraftStore((s) => s.clearDraft); + const activeStep = draft?.activeStep ?? 0; + const stepAnswers = draft?.stepAnswers ?? EMPTY_STEP_ANSWERS; const isOnSubmitStep = activeStep >= totalQuestions; @@ -116,59 +136,76 @@ export function QuestionPermission({ const currentStepAnswer = stepAnswers.get(activeStep); + const handleCancel = useCallback(() => { + clearDraft(toolCallId); + onCancel(); + }, [toolCallId, clearDraft, onCancel]); + const advanceStep = useCallback( (optionIds: string[], customInput?: string) => { - setStepAnswers((prev) => { - const next = new Map(prev); - next.set(activeStep, { - selectedIds: optionIds, - customInput: customInput ?? "", - }); - return next; + setDraftStepAnswer(toolCallId, activeStep, { + selectedIds: optionIds, + customInput: customInput ?? "", }); - - if (activeStep < totalQuestions - 1) { - setActiveStep(activeStep + 1); - } else { - setActiveStep(totalQuestions); - } + setDraftActiveStep( + toolCallId, + activeStep < totalQuestions - 1 ? activeStep + 1 : totalQuestions, + ); }, - [activeStep, totalQuestions], + [ + toolCallId, + activeStep, + totalQuestions, + setDraftStepAnswer, + setDraftActiveStep, + ], ); const handleMultiSelect = useCallback( (optionIds: string[], customInput?: string) => { if (totalQuestions === 1) { const filteredIds = filterOtherOptions(optionIds); - const singleAnswer: Map = new Map([ - [0, { selectedIds: optionIds, customInput: customInput ?? "" }], - ]); - const answers = buildAnswersRecord(singleAnswer, allQuestions); + const answers = buildSingleQuestionAnswers( + optionIds, + customInput, + allQuestions, + ); + clearDraft(toolCallId); onSelect(filteredIds[0] ?? "other", customInput, answers); return; } advanceStep(optionIds, customInput); }, - [totalQuestions, onSelect, advanceStep, allQuestions], + [ + totalQuestions, + onSelect, + advanceStep, + allQuestions, + clearDraft, + toolCallId, + ], ); const handleSelect = useCallback( (optionId: string, customInput?: string) => { if (isOnSubmitStep) { if (optionId === CANCEL_OPTION_ID) { - onCancel(); + handleCancel(); return; } const answers = buildAnswersRecord(stepAnswers, allQuestions); + clearDraft(toolCallId); onSelect(SUBMIT_OPTION_ID, undefined, answers); return; } if (totalQuestions === 1) { - const singleAnswer: Map = new Map([ - [0, { selectedIds: [optionId], customInput: customInput ?? "" }], - ]); - const answers = buildAnswersRecord(singleAnswer, allQuestions); + const answers = buildSingleQuestionAnswers( + [optionId], + customInput, + allQuestions, + ); + clearDraft(toolCallId); onSelect(optionId, customInput, answers); return; } @@ -181,28 +218,29 @@ export function QuestionPermission({ allQuestions, totalQuestions, onSelect, - onCancel, + handleCancel, advanceStep, + clearDraft, + toolCallId, ], ); const handleStepAnswer = useCallback( (stepIndex: number, optionIds: string[], customInput?: string) => { - setStepAnswers((prev) => { - const next = new Map(prev); - next.set(stepIndex, { - selectedIds: optionIds, - customInput: customInput ?? "", - }); - return next; + setDraftStepAnswer(toolCallId, stepIndex, { + selectedIds: optionIds, + customInput: customInput ?? "", }); }, - [], + [toolCallId, setDraftStepAnswer], ); - const handleStepChange = useCallback((stepIndex: number) => { - setActiveStep(stepIndex); - }, []); + const handleStepChange = useCallback( + (stepIndex: number) => { + setDraftActiveStep(toolCallId, stepIndex); + }, + [toolCallId, setDraftActiveStep], + ); const hasUnanswered = useMemo(() => { for (let i = 0; i < totalQuestions; i++) { @@ -272,9 +310,10 @@ export function QuestionPermission({ currentStep={activeStep} steps={steps} initialSelections={currentStepAnswer?.selectedIds} + initialCustomInput={currentStepAnswer?.customInput} onSelect={handleSelect} onMultiSelect={handleMultiSelect} - onCancel={onCancel} + onCancel={handleCancel} onStepChange={handleStepChange} onStepAnswer={handleStepAnswer} /> diff --git a/packages/ui/src/features/permissions/questionDraftStore.ts b/packages/ui/src/features/permissions/questionDraftStore.ts new file mode 100644 index 0000000000..301043d6c2 --- /dev/null +++ b/packages/ui/src/features/permissions/questionDraftStore.ts @@ -0,0 +1,73 @@ +import type { StepAnswer } from "@posthog/ui/primitives/ActionSelector"; +import { create } from "zustand"; + +export interface QuestionDraft { + activeStep: number; + stepAnswers: Map; +} + +interface QuestionDraftState { + drafts: Map; + setActiveStep: (toolCallId: string, activeStep: number) => void; + setStepAnswer: ( + toolCallId: string, + stepIndex: number, + answer: StepAnswer, + ) => void; + clearDraft: (toolCallId: string) => void; +} + +/** Bounds memory for drafts of cards that resolved without a submit/cancel. */ +const MAX_DRAFTS = 50; + +function upsertDraft( + drafts: Map, + toolCallId: string, + update: (existing: QuestionDraft) => QuestionDraft, +): Map { + const next = new Map(drafts); + const existing = next.get(toolCallId) ?? { + activeStep: 0, + stepAnswers: new Map(), + }; + // Re-insert so insertion order tracks recency and eviction drops the stalest. + next.delete(toolCallId); + next.set(toolCallId, update(existing)); + while (next.size > MAX_DRAFTS) { + const oldest = next.keys().next().value; + if (oldest === undefined) break; + next.delete(oldest); + } + return next; +} + +/** + * In-progress AskUserQuestion answers, keyed by toolCallId. The question card + * unmounts whenever the user switches chats; holding the step answers here + * instead of component state lets a remounted card restore them rather than + * resetting to the first question. + */ +export const useQuestionDraftStore = create((set) => ({ + drafts: new Map(), + setActiveStep: (toolCallId, activeStep) => + set((state) => ({ + drafts: upsertDraft(state.drafts, toolCallId, (draft) => ({ + ...draft, + activeStep, + })), + })), + setStepAnswer: (toolCallId, stepIndex, answer) => + set((state) => ({ + drafts: upsertDraft(state.drafts, toolCallId, (draft) => ({ + ...draft, + stepAnswers: new Map(draft.stepAnswers).set(stepIndex, answer), + })), + })), + clearDraft: (toolCallId) => + set((state) => { + if (!state.drafts.has(toolCallId)) return state; + const drafts = new Map(state.drafts); + drafts.delete(toolCallId); + return { drafts }; + }), +})); diff --git a/packages/ui/src/features/sessions/sessionServiceHost.test.ts b/packages/ui/src/features/sessions/sessionServiceHost.test.ts index be96ef8179..a61454e9fb 100644 --- a/packages/ui/src/features/sessions/sessionServiceHost.test.ts +++ b/packages/ui/src/features/sessions/sessionServiceHost.test.ts @@ -5076,6 +5076,71 @@ describe("SessionService", () => { }); }); + // Surfaces a cloud question through the live watcher update path so the + // service tracks its cloud requestId, mirroring how real question cards + // arrive. Returns the session (with the surfaced permission attached) and + // the update feeder for replay scenarios. + const surfaceCloudQuestion = ( + service: ReturnType, + ) => { + const session = createMockSession({ + isCloud: true, + cloudStatus: "in_progress", + events: [ + { + type: "acp_message", + ts: 1700000000, + message: { + jsonrpc: "2.0", + method: "session/update", + params: { update: { sessionUpdate: "tool_call" } }, + }, + } as AcpMessage, + ], + processedLineCount: 3, + }); + mockSessionStoreSetters.getSessionByTaskId.mockReturnValue(session); + mockSessionStoreSetters.getSessions.mockReturnValue({ "run-123": session }); + + service.watchCloudTask( + "task-123", + "run-123", + "https://api.example.com", + 123, + undefined, + "https://logs.example.com/run-123", + undefined, + "claude", + ); + + const onData = mockTrpcCloudTask.onUpdate.subscribe.mock.calls[0]?.[1] + ?.onData as (update: unknown) => void; + const requestUpdate = { + kind: "permission_request", + taskId: "task-123", + runId: "run-123", + requestId: "request-1", + toolCall: { + toolCallId: "tool-1", + title: "Which license should I use?", + kind: "other", + _meta: { + codeToolKind: "question", + questions: [{ question: "Which license should I use?" }], + }, + }, + options: [{ optionId: "option_0", name: "MIT", kind: "allow_once" }], + }; + onData(requestUpdate); + + const surfaced = + mockSessionStoreSetters.setPendingPermissions.mock.calls.at( + -1, + )?.[1] as AgentSession["pendingPermissions"]; + session.pendingPermissions = surfaced; + return { session, onData, requestUpdate }; + }; + describe("respondToPermission", () => { it("does nothing if no session exists", async () => { const service = getSessionService(); @@ -5286,6 +5351,65 @@ describe("SessionService", () => { expect(mockTrpcAgent.respondToPermission.mutate).not.toHaveBeenCalled(); expect(mockAuthenticatedClient.runTaskInCloud).not.toHaveBeenCalled(); }); + + it("persists a durable resolved marker when answering a question on a terminal cloud run", async () => { + const service = getSessionService(); + surfaceCloudQuestion(service); + mockTerminalCloudRun(); + + await service.respondToPermission( + "task-123", + "tool-1", + "option_0", + undefined, + { "Which license should I use?": "MIT" }, + ); + + // The answer resumed the run as a prompt; without a resolved marker the + // request would be re-derived as pending from the log forever. + expect(mockAuthenticatedClient.runTaskInCloud).toHaveBeenCalled(); + expect(mockAuthenticatedClient.appendTaskRunLog).toHaveBeenCalledWith( + "task-123", + "run-123", + [ + expect.objectContaining({ + type: "notification", + notification: expect.objectContaining({ + method: "_posthog/permission_resolved", + params: { + requestId: "request-1", + toolCallId: "tool-1", + optionId: "option_0", + }, + }), + }), + ], + ); + }); + + it("does not re-surface a question the user already answered when the stream re-delivers it", async () => { + const service = getSessionService(); + const { session, onData, requestUpdate } = surfaceCloudQuestion(service); + mockTerminalCloudRun(); + + await service.respondToPermission( + "task-123", + "tool-1", + "option_0", + undefined, + { "Which license should I use?": "MIT" }, + ); + + session.pendingPermissions = new Map(); + mockSessionStoreSetters.setPendingPermissions.mockClear(); + + // The durable stream re-sends the tail on reconnect/replay. + onData(requestUpdate); + + expect( + mockSessionStoreSetters.setPendingPermissions, + ).not.toHaveBeenCalled(); + }); }); describe("cancelPermission", () => { @@ -5298,6 +5422,35 @@ describe("SessionService", () => { expect(mockTrpcAgent.cancelPermission.mutate).not.toHaveBeenCalled(); }); + it("persists a dismissal marker when cancelling a question on a terminal cloud run", async () => { + const service = getSessionService(); + const { session } = surfaceCloudQuestion(service); + session.cloudStatus = "completed"; + + await service.cancelPermission("task-123", "tool-1"); + + // The dead run can't receive a permission_response; record the + // dismissal so the request is not re-derived as pending from the log. + expect(mockTrpcCloudTask.sendCommand.mutate).not.toHaveBeenCalled(); + expect(mockAuthenticatedClient.appendTaskRunLog).toHaveBeenCalledWith( + "task-123", + "run-123", + [ + expect.objectContaining({ + type: "notification", + notification: expect.objectContaining({ + method: "_posthog/permission_resolved", + params: { + requestId: "request-1", + toolCallId: "tool-1", + optionId: "cancelled", + }, + }), + }), + ], + ); + }); + it("removes permission from UI and cancels", async () => { const service = getSessionService(); mockSessionStoreSetters.getSessionByTaskId.mockReturnValue( diff --git a/packages/ui/src/primitives/action-selector/ActionSelector.tsx b/packages/ui/src/primitives/action-selector/ActionSelector.tsx index 63d5fdd8d3..49699617f3 100644 --- a/packages/ui/src/primitives/action-selector/ActionSelector.tsx +++ b/packages/ui/src/primitives/action-selector/ActionSelector.tsx @@ -18,6 +18,7 @@ export function ActionSelector({ currentStep = 0, steps, initialSelections, + initialCustomInput, hideSubmitButton = false, onSelect, onMultiSelect, @@ -33,6 +34,7 @@ export function ActionSelector({ currentStep, steps, initialSelections, + initialCustomInput, onSelect, onMultiSelect, onStepChange, diff --git a/packages/ui/src/primitives/action-selector/types.ts b/packages/ui/src/primitives/action-selector/types.ts index b4b7acfcfa..7b93dc2190 100644 --- a/packages/ui/src/primitives/action-selector/types.ts +++ b/packages/ui/src/primitives/action-selector/types.ts @@ -28,6 +28,7 @@ export interface ActionSelectorProps { currentStep?: number; steps?: StepInfo[]; initialSelections?: string[]; + initialCustomInput?: string; hideSubmitButton?: boolean; onSelect: (optionId: string, customInput?: string) => void; onMultiSelect?: (optionIds: string[], customInput?: string) => void; diff --git a/packages/ui/src/primitives/action-selector/useActionSelectorState.ts b/packages/ui/src/primitives/action-selector/useActionSelectorState.ts index bdde8edb0a..42c7a21239 100644 --- a/packages/ui/src/primitives/action-selector/useActionSelectorState.ts +++ b/packages/ui/src/primitives/action-selector/useActionSelectorState.ts @@ -44,6 +44,7 @@ interface UseActionSelectorStateProps { currentStep: number; steps: ActionSelectorProps["steps"]; initialSelections?: string[]; + initialCustomInput?: string; onSelect: ActionSelectorProps["onSelect"]; onMultiSelect: ActionSelectorProps["onMultiSelect"]; onStepChange: ActionSelectorProps["onStepChange"]; @@ -58,6 +59,7 @@ export function useActionSelectorState({ currentStep, steps, initialSelections, + initialCustomInput, onSelect, onMultiSelect, onStepChange, @@ -68,7 +70,7 @@ export function useActionSelectorState({ const [checkedOptions, setCheckedOptions] = useState>(() => initialSelections?.length ? new Set(initialSelections) : new Set(), ); - const [customInput, setCustomInput] = useState(""); + const [customInput, setCustomInput] = useState(initialCustomInput ?? ""); const [isEditing, setIsEditing] = useState(false); const [internalStep, setInternalStep] = useState(currentStep); const [stepAnswers, setStepAnswers] = useState>( @@ -148,9 +150,9 @@ export function useActionSelectorState({ if (saved) { setCheckedOptions(new Set(saved.selectedIds)); setCustomInput(saved.customInput); - } else if (initialSelections?.length) { - setCheckedOptions(new Set(initialSelections)); - setCustomInput(""); + } else if (initialSelections?.length || initialCustomInput) { + setCheckedOptions(new Set(initialSelections ?? [])); + setCustomInput(initialCustomInput ?? ""); } else { setCheckedOptions(new Set()); setCustomInput(""); @@ -161,7 +163,7 @@ export function useActionSelectorState({ containerRef.current?.focus(); } }, - [initialSelections, stepAnswers], + [initialSelections, initialCustomInput, stepAnswers], ); useEffect(() => { From fb6d4b727981913affb8ce8029d57c5abb0eb138 Mon Sep 17 00:00:00 2001 From: Alessandro Pogliaghi Date: Tue, 7 Jul 2026 18:33:32 +0100 Subject: [PATCH 2/2] fix(ui): memoize question meta parsing for stable hook dependencies React Doctor flagged exhaustive-deps on the question card: allQuestions was re-parsed (Zod safeParse) and re-allocated on every render, so every useCallback depending on it re-created each render. Wrap the parse in useMemo keyed on toolCall._meta. Generated-By: PostHog Code Task-Id: 9c723d92-b8ef-4c9f-9317-2f53b4644f33 --- .../ui/src/features/permissions/QuestionPermission.tsx | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/ui/src/features/permissions/QuestionPermission.tsx b/packages/ui/src/features/permissions/QuestionPermission.tsx index 7df2490e6f..2f25065b31 100644 --- a/packages/ui/src/features/permissions/QuestionPermission.tsx +++ b/packages/ui/src/features/permissions/QuestionPermission.tsx @@ -108,8 +108,12 @@ export function QuestionPermission({ onSelect, onCancel, }: BasePermissionProps) { - const meta = parseQuestionMeta(toolCall._meta); - const allQuestions = meta?.questions ?? []; + // Memoized so the hooks depending on allQuestions keep a stable identity + // instead of re-parsing the meta (and re-allocating) on every render. + const allQuestions = useMemo( + () => parseQuestionMeta(toolCall._meta)?.questions ?? [], + [toolCall._meta], + ); const totalQuestions = allQuestions.length; const toolCallId = toolCall.toolCallId;