From 42a991e563a2ca52bc3364ada3bd68c53a0e993f Mon Sep 17 00:00:00 2001 From: Sawyer Date: Thu, 3 Sep 2026 11:15:10 -0700 Subject: [PATCH 1/3] Add tests for question-block retry after a failed notify (CL-7245) --- packages/chat-ui/src/strings.ts | 3 + packages/chat-ui/test/question-block.test.tsx | 64 +++++++++++++++++-- 2 files changed, 61 insertions(+), 6 deletions(-) diff --git a/packages/chat-ui/src/strings.ts b/packages/chat-ui/src/strings.ts index 4a9790311..70237a9ff 100644 --- a/packages/chat-ui/src/strings.ts +++ b/packages/chat-ui/src/strings.ts @@ -216,6 +216,9 @@ export const CHAT_STRINGS = { blockQuestionSubmit: "Send", blockQuestionSubmitting: "Sending…", blockQuestionAnswerError: "Couldn't send your answer — try again.", + blockQuestionNotifyFailed: + "Your answer was saved, but the agent couldn't be notified — try again.", + blockQuestionRetry: "Try again", blockQuestionAnsweredLabel: "Your answer", blockConnectGithubPickHeadline: "Choose what gets reviewed", blockConnectGithubStepDone: "Done", diff --git a/packages/chat-ui/test/question-block.test.tsx b/packages/chat-ui/test/question-block.test.tsx index 04091ee50..4d68f3924 100644 --- a/packages/chat-ui/test/question-block.test.tsx +++ b/packages/chat-ui/test/question-block.test.tsx @@ -1,10 +1,11 @@ // DOM tests for the interview-question card: open state (lettered options, // optional free-text field), answered state (collapsed with a checkmark, -// controls disabled), and keyboard navigation over the option buttons -// (native tab order — no custom key handling needed since each option is -// a plain ` + + + )} ); } From a6492c7eac6be9aed994d625e527627337d6879a Mon Sep 17 00:00:00 2001 From: Sawyer Date: Thu, 3 Sep 2026 15:00:52 -0700 Subject: [PATCH 3/3] Keep question retry after remount via notifiedAt (CL-7245) Retry lived in React state, so a reload of an answered card whose notify never landed looked finished. GET own now carries notifiedAt; the card derives retry from that, omits the success checkmark while notify failed, and guards a double-click on Try again. --- packages/chat-ui/src/api.ts | 1 + .../chat-ui/src/blocks/block-responses.ts | 7 + .../chat-ui/src/blocks/question-block.tsx | 76 +++++---- packages/chat-ui/test/question-block.test.tsx | 154 +++++++++++++++--- packages/chat/src/routes.ts | 38 ++++- .../chat/test/block-responses-routes.test.ts | 14 +- 6 files changed, 233 insertions(+), 57 deletions(-) diff --git a/packages/chat-ui/src/api.ts b/packages/chat-ui/src/api.ts index 2e7c5b21a..6bbe2c074 100644 --- a/packages/chat-ui/src/api.ts +++ b/packages/chat-ui/src/api.ts @@ -1056,6 +1056,7 @@ const BlockResponsePayloadWire = type({ kind: "'question'", answer: "string", "optionIndex?": "number", + notifiedAt: "string | null", }), ); export type BlockResponsePayload = typeof BlockResponsePayloadWire.infer; diff --git a/packages/chat-ui/src/blocks/block-responses.ts b/packages/chat-ui/src/blocks/block-responses.ts index b8c015c0c..422ffe2c6 100644 --- a/packages/chat-ui/src/blocks/block-responses.ts +++ b/packages/chat-ui/src/blocks/block-responses.ts @@ -25,6 +25,13 @@ export type QuestionResponsePayload = { readonly kind: "question"; readonly answer: string; readonly optionIndex?: number; + /** + * ISO timestamp once the asking agent was notified, or `null` when + * the answer is on file but notify never landed. Omitted only in + * tests that are not exercising the retry path — treat missing as + * already notified. + */ + readonly notifiedAt?: string | null; }; export type BlockResponsePayload = diff --git a/packages/chat-ui/src/blocks/question-block.tsx b/packages/chat-ui/src/blocks/question-block.tsx index 012d10c0d..c39131b46 100644 --- a/packages/chat-ui/src/blocks/question-block.tsx +++ b/packages/chat-ui/src/blocks/question-block.tsx @@ -6,11 +6,12 @@ // route additionally relays it into the workbench as this principal's own // message (see `packages/chat/src/routes.ts`) -- the card itself never // asserts a resolved answer; it always re-reads `own` from the server, -// same anti-spoof rule every other block card follows. A notify_failed -// submit still leaves `own` on that read, so the card keeps a retry until -// the agent is actually notified rather than collapsing as if it were. +// same anti-spoof rule every other block card follows. A question `own` +// carries `notifiedAt`: null means the answer is on file but notify never +// landed, so the card keeps a retry (including after remount) rather than +// collapsing as if the agent had already been reached. -import { useEffect, useState, type FormEvent } from "react"; +import { useEffect, useRef, useState, type FormEvent } from "react"; import type { QuestionBlockData } from "@corbits/chat/blocks"; import { Check } from "@corbits/icons"; @@ -21,14 +22,26 @@ import type { BlockResponseQuery, } from "./block-responses"; -function ownAnswer( - query: BlockResponseQuery, -): { readonly answer: string; readonly optionIndex?: number } | null { +function ownAnswer(query: BlockResponseQuery): { + readonly answer: string; + readonly optionIndex?: number; + readonly notifiedAt?: string | null; +} | null { if (query.kind !== "ready" || query.own === null) return null; if (query.own.kind !== "question") return null; - return query.own.optionIndex !== undefined - ? { answer: query.own.answer, optionIndex: query.own.optionIndex } - : { answer: query.own.answer }; + return { + answer: query.own.answer, + ...(query.own.optionIndex !== undefined + ? { optionIndex: query.own.optionIndex } + : {}), + ...(query.own.notifiedAt !== undefined + ? { notifiedAt: query.own.notifiedAt } + : {}), + }; +} + +function notifyNeverLanded(answered: ReturnType): boolean { + return answered !== null && answered.notifiedAt === null; } function answeredValue( @@ -41,14 +54,18 @@ function answeredValue( function AnsweredSummary({ answered, + notifyFailed, }: { readonly answered: NonNullable>; + readonly notifyFailed: boolean; }) { return (
- + {!notifyFailed && ( + + )}

{CHAT_STRINGS.blockQuestionAnsweredLabel} @@ -92,15 +109,14 @@ export function QuestionBlockView({ }) { const [query, setQuery] = useState({ kind: "loading" }); const [submitting, setSubmitting] = useState(false); + const submittingRef = useRef(false); const [error, setError] = useState(null); - const [notifyFailed, setNotifyFailed] = useState(false); const [freeText, setFreeText] = useState(""); useEffect(() => { if (actions === undefined || messageId === undefined) return; let cancelled = false; setQuery({ kind: "loading" }); - setNotifyFailed(false); setError(null); actions.getResponses(messageId, data.questionId).then((result) => { if (!cancelled) setQuery(result); @@ -125,6 +141,8 @@ export function QuestionBlockView({ function submitAnswer(answer: string, optionIndex?: number) { if (actions === undefined || messageId === undefined) return; + if (submitting || submittingRef.current) return; + submittingRef.current = true; setSubmitting(true); setError(null); actions @@ -132,21 +150,20 @@ export function QuestionBlockView({ .then(async (result) => { const next = await actions.getResponses(messageId, data.questionId); setQuery(next); - if (result.kind === "submitted") { - setNotifyFailed(false); - return; - } - // The route persists the answer before notify. A failed submit that - // still left `own` on the GET is notify_failed: keep a retry so the - // agent can still be reached. Collapse only after a successful notify. - setNotifyFailed(true); + if (result.kind === "submitted") return; + // Persist failure leaves no `own`; notify_failed leaves `own` with + // `notifiedAt: null`. The card derives retry from that GET, not from + // this submit result — the result only picks the error copy. setError( ownAnswer(next) !== null ? CHAT_STRINGS.blockQuestionNotifyFailed : CHAT_STRINGS.blockQuestionAnswerError, ); }) - .finally(() => setSubmitting(false)); + .finally(() => { + submittingRef.current = false; + setSubmitting(false); + }); } function onFreeTextSubmit(event: FormEvent) { @@ -158,18 +175,17 @@ export function QuestionBlockView({ const loading = query.kind === "loading"; const answered = ownAnswer(query); + const notifyFailed = notifyNeverLanded(answered); if (answered !== null) { return ( - + {notifyFailed && ( <> - {error !== null && ( -

- {error} -

- )} +

+ {error ?? CHAT_STRINGS.blockQuestionNotifyFailed} +