diff --git a/ui/src/components/task-chat/QuestionForm.tsx b/ui/src/components/task-chat/QuestionForm.tsx index 2ed848b21c..8474299ad2 100644 --- a/ui/src/components/task-chat/QuestionForm.tsx +++ b/ui/src/components/task-chat/QuestionForm.tsx @@ -24,12 +24,21 @@ import { useTaskChatComposerTakeoverActions, } from "./TaskChatComposerTakeoverContext"; import { TaskChatRichInput } from "./TaskChatRichInput"; -import { parseCssTimeMs } from "./motion-tokens"; import { matchSafeQuestionValidationPattern } from "./question-validation-pattern"; type Question = PaperclipQuestionSet["questions"][number]; type Answer = PaperclipQuestionResponse["answers"][string]; +/** + * A form-level message, tagged with whether answering a question resolves it. + * + * Only a missing-answer complaint is something a selection can settle. A send + * that failed is not: the answers are still unsent, so the message has to + * outlive the next click rather than disappear the moment the reader touches + * an option. + */ +type FormError = { message: string; fromMissingAnswer?: boolean }; + export interface QuestionFormProps { id: string; questionSet: PaperclipQuestionSet; @@ -109,7 +118,6 @@ function SelectOption({ recommended, selected, multiple, - confirming = false, disabled, onClick, }: { @@ -119,7 +127,6 @@ function SelectOption({ recommended?: boolean; selected: boolean; multiple: boolean; - confirming?: boolean; disabled: boolean; onClick: () => void; }) { @@ -147,7 +154,6 @@ function SelectOption({ className={cn( "mt-0.5 flex h-4 w-4 shrink-0 items-center justify-center border", multiple ? "rounded-sm" : "rounded-full", - confirming && "tc-question-choice-confirm", selected ? "border-primary bg-primary text-primary-foreground" : "border-muted-foreground/50", @@ -267,12 +273,7 @@ export function QuestionForm({ ); const [working, setWorking] = useState<"submit" | "cancel" | null>(null); const [inputUploading, setInputUploading] = useState(false); - const [error, setError] = useState(null); - const [pendingAdvance, setPendingAdvance] = useState<{ - page: number; - questionId: string; - optionId: string; - } | null>(null); + const [error, setError] = useState(null); const promptRef = useRef(null); const previousPage = useRef(page); @@ -296,38 +297,6 @@ export function QuestionForm({ }, [answers, customActive, draftKey, page]); const question = questionSet.questions[page]; - useEffect(() => { - if (!pendingAdvance) return; - if ( - disabled || - working || - inputUploading || - pendingAdvance.page !== page || - pendingAdvance.questionId !== question?.id || - page >= questionSet.questions.length - 1 - ) { - setPendingAdvance(null); - return; - } - const duration = getComputedStyle(document.documentElement) - .getPropertyValue("--motion-question-confirm") - .trim(); - const durationMs = parseCssTimeMs(duration) || 0; - const advance = () => { - setPendingAdvance(null); - setPage(page + 1); - }; - // With reduced motion (or without CSS), no visual hold is needed. - if (durationMs <= 0) { - advance(); - return; - } - const timer = window.setTimeout(advance, durationMs); - return () => window.clearTimeout(timer); - }, [ - pendingAdvance, page, question?.id, questionSet.questions.length, - disabled, working, inputUploading, - ]); const validationErrors = useMemo( () => Object.fromEntries( @@ -381,17 +350,18 @@ export function QuestionForm({ setAnswers({ ...answers, [question.id]: nextAnswer }); if (!multiple) { setCustomActive((current) => ({ ...current, [question.id]: false })); - // Briefly confirm the selected choice before moving on. The last page needs an - // explicit submit, and custom answers stay open for typing. - if (page < questionSet.questions.length - 1) { - setError(null); - setPendingAdvance({ page, questionId: question.id, optionId }); - } + // Picking an option answers the question; it does not navigate. Moving on + // stays an explicit act — Next, the pagination arrows, or Submit — so a + // misclick never costs the reader the page they were still reading. + // + // Clear only the complaint this selection actually answers. A failed send + // has to survive it, or the last page quietly loses the one sign that the + // answers never left. + setError((current) => (current?.fromMissingAnswer ? null : current)); } } function toggleCustom() { - setPendingAdvance(null); const active = !isCustomActive; setCustomActive((current) => ({ ...current, [question.id]: active })); updateAnswer({ @@ -414,9 +384,10 @@ export function QuestionForm({ // validating, and a restored draft can land past it. Go back to that // question and say so rather than dropping the send. setPage(invalidIndex); - setError( - `Question ${invalidIndex + 1} needs an answer before you can send.`, - ); + setError({ + message: `Question ${invalidIndex + 1} needs an answer before you can send.`, + fromMissingAnswer: true, + }); return; } setWorking("submit"); @@ -428,11 +399,12 @@ export function QuestionForm({ }); if (draftKey) clearDraft(draftKey); } catch (cause) { - setError( - cause instanceof Error - ? cause.message - : "The answers could not be submitted.", - ); + setError({ + message: + cause instanceof Error + ? cause.message + : "The answers could not be submitted.", + }); } finally { setWorking(null); } @@ -446,11 +418,12 @@ export function QuestionForm({ await onCancel(); if (draftKey) clearDraft(draftKey); } catch (cause) { - setError( - cause instanceof Error - ? cause.message - : "The questions could not be cancelled.", - ); + setError({ + message: + cause instanceof Error + ? cause.message + : "The questions could not be cancelled.", + }); } finally { setWorking(null); } @@ -635,7 +608,6 @@ export function QuestionForm({ description={option.description} recommended={option.recommended} selected={selected.includes(option.id)} - confirming={pendingAdvance?.questionId === question.id && pendingAdvance.optionId === option.id} multiple={multiple} disabled={disabled || working != null} onClick={() => toggleOption(option.id)} @@ -684,7 +656,7 @@ export function QuestionForm({
{error ? (
- {error} + {error.message}
) : null}
diff --git a/ui/src/components/task-chat/TaskChatComposer.test.tsx b/ui/src/components/task-chat/TaskChatComposer.test.tsx index c056e35a9f..05ef7f9955 100644 --- a/ui/src/components/task-chat/TaskChatComposer.test.tsx +++ b/ui/src/components/task-chat/TaskChatComposer.test.tsx @@ -2165,7 +2165,7 @@ describe("TaskChatComposer", () => { }); }); - it("advances single selections, preserves answers when going back, and skips optional answers", async () => { + it("advances on Next, preserves answers when going back, and skips optional answers", async () => { const onSubmit = vi.fn(); render( { const byLabel = (label: string) => buttons().find((button) => button.textContent?.trim() === label); - // Page 1: required, so no Skip; Next waits for an answer. + // Page 1: required, so no Skip; Next waits for an answer. Answering + // enables Next but stays put — only Next moves on. expect(byLabel("Skip")).toBeUndefined(); expect(byLabel("Next")?.disabled).toBe(true); flushSync(() => byLabel("Staging")?.click()); await flushAsync(); + expect(container.textContent).toContain("Where?"); + expect(byLabel("Next")?.disabled).toBe(false); + flushSync(() => byLabel("Next")?.click()); + await flushAsync(); expect(onSubmit).not.toHaveBeenCalled(); expect(container.textContent).toContain("When?"); expect(document.activeElement?.textContent).toBe("When?"); - // Page 2: pick advances. Go back to confirm it is saved, then skip. + // Page 2: answer, then Next. Go back to confirm it is saved, then skip. flushSync(() => byLabel("Today")?.click()); await flushAsync(); + flushSync(() => byLabel("Next")?.click()); + await flushAsync(); expect(container.textContent).toContain("Who?"); flushSync(() => container.querySelector('button[aria-label="Previous question"]')?.click()); await flushAsync(); @@ -2253,7 +2260,7 @@ describe("TaskChatComposer", () => { expect(response.answers.who).toEqual({ selectedOptionIds: ["me"] }); }); - it.each(["click", "keyboard"])("advances a single choice by %s, while Other and multi-select stay put", async (input) => { + it.each(["click", "keyboard"])("records a single choice by %s without leaving the question", async (input) => { const onSubmit = vi.fn(); render( { else byLabel("SQLite").dispatchEvent(new KeyboardEvent("keydown", { key: "1", bubbles: true })); }); await flushAsync(); + // Answering re-enables Next and closes Other, but the reader stays here. + expect(container.textContent).toContain("1 of 3"); + expect(container.querySelector('[data-testid="question-other-answer-composer"]')).toBeNull(); + expect(byLabel("SQLite").getAttribute("aria-checked")).toBe("true"); + expect(byLabel("Next").disabled).toBe(false); + flushSync(() => byLabel("Next").click()); + await flushAsync(); expect(container.textContent).toContain("2 of 3"); expect(document.activeElement?.textContent).toBe("Features?"); flushSync(() => document.activeElement?.dispatchEvent(new KeyboardEvent("keydown", { key: "1", repeat: true, bubbles: true }))); @@ -2305,71 +2319,130 @@ describe("TaskChatComposer", () => { }); }); - describe("single-choice confirmation animation", () => { + describe("single-choice selection stays on the page", () => { const questionSet = { schema: "paperclip.question_set.v1" as const, questions: ["First", "Second", "Third"].map((prompt) => ({ id: prompt, prompt, required: true, answerMode: "single_select" as const, - options: [{ id: "yes", label: "Yes" }], customAnswer: { enabled: true as const }, + options: [{ id: "yes", label: "Yes" }, { id: "no", label: "No" }], + customAnswer: { enabled: true as const }, })), }; - const form = (disabled = false) => ( - + const form = () => ( + ); const click = (selector: string) => act(() => { flushSync(() => container.querySelector(selector)!.click()); }); - beforeEach(() => { - vi.useFakeTimers(); - document.documentElement.style.setProperty("--motion-question-confirm", "160ms"); - }); - afterEach(() => { - render(
); - vi.useRealTimers(); - document.documentElement.style.removeProperty("--motion-question-confirm"); - }); + const buttonByText = (text: string) => + Array.from(container.querySelectorAll("button")).find( + (button) => button.textContent?.trim() === text, + ); - it("shows the selected radio before advancing exactly one page", () => { + it("records the answer without navigating", () => { render(form()); click('[role="radio"]'); expect(container.querySelector('[role="radio"]')?.getAttribute("aria-checked")).toBe("true"); - expect(container.querySelector(".tc-question-choice-confirm")).not.toBeNull(); expect(container.textContent).toContain("1 of 3"); - act(() => vi.advanceTimersByTime(159)); + expect(container.textContent).toContain("First"); + }); + + it("lets the reader change their mind before moving on", () => { + render(form()); + click('[role="radio"]'); + const radios = () => Array.from(container.querySelectorAll('[role="radio"]')); + act(() => { flushSync(() => (radios()[1] as HTMLButtonElement).click()); }); + expect(radios()[0]?.getAttribute("aria-checked")).toBe("false"); + expect(radios()[1]?.getAttribute("aria-checked")).toBe("true"); expect(container.textContent).toContain("1 of 3"); - act(() => vi.advanceTimersByTime(1)); + }); + + it("advances only on an explicit Next", () => { + render(form()); + click('[role="radio"]'); + expect(container.textContent).toContain("1 of 3"); + act(() => { flushSync(() => buttonByText("Next")!.click()); }); expect(container.textContent).toContain("2 of 3"); expect(document.activeElement?.textContent).toBe("Second"); - act(() => vi.advanceTimersByTime(160)); - expect(container.textContent).toContain("2 of 3"); }); - it.each(["navigation", "custom answer", "disabled", "unmount"])("cancels the pending advance on %s", (reason) => { + it("keeps number-key selection on the same question", () => { render(form()); - click('[role="radio"]'); - if (reason === "navigation") { - click('[aria-label="Next question"]'); - click('[aria-label="Next question"]'); - } else if (reason === "custom answer") { - click('#animated-First-custom'); - } else if (reason === "disabled") { - render(form(true)); - render(form(false)); - } else { - render(
Closed
); - } - act(() => vi.advanceTimersByTime(160)); - expect(container.textContent).toContain( - reason === "navigation" ? "3 of 3" : reason === "unmount" ? "Closed" : "1 of 3", + const page = container.querySelector(".tc-question-page")!; + act(() => { + flushSync(() => page.dispatchEvent( + new KeyboardEvent("keydown", { key: "2", bubbles: true }), + )); + }); + expect(Array.from(container.querySelectorAll('[role="radio"]'))[1]?.getAttribute("aria-checked")) + .toBe("true"); + expect(container.textContent).toContain("1 of 3"); + }); + + // Selection now clears the form error, which it did not do on the last + // page before. A failed send has to outlive it: the answers are still + // unsent, so wiping the message would leave no sign of that at all. + it("keeps a failed send visible while the reader changes their answer", async () => { + const onSubmit = vi.fn().mockRejectedValue(new Error("Network unreachable")); + render( + , ); + const radios = () => Array.from(container.querySelectorAll('[role="radio"]')); + act(() => { flushSync(() => radios()[0]!.click()); }); + act(() => { flushSync(() => buttonByText("Submit answers")!.click()); }); + await flushAsync(); + expect(onSubmit).toHaveBeenCalledOnce(); + expect(container.textContent).toContain("Network unreachable"); + act(() => { flushSync(() => radios()[1]!.click()); }); + expect(radios()[1]?.getAttribute("aria-checked")).toBe("true"); + expect(container.textContent).toContain("Network unreachable"); }); - it("advances immediately when the motion token is zero", () => { - document.documentElement.style.setProperty("--motion-question-confirm", "0ms"); - render(form()); - click('[role="radio"]'); - expect(container.textContent).toContain("2 of 3"); - expect(container.querySelector(".tc-question-choice-confirm")).toBeNull(); + // The other half of the same rule: the one complaint a selection does + // answer still goes away when it is answered. + it("clears a missing-answer complaint once that question is answered", async () => { + render( + , + ); + const radios = () => Array.from(container.querySelectorAll('[role="radio"]')); + // The pagination arrow browses without validating, so the reader can + // reach the end with the first question still blank. Skip on the last + // question then sends, which is where the complaint comes from. + click('[aria-label="Next question"]'); + act(() => { flushSync(() => buttonByText("Skip")!.click()); }); + await flushAsync(); + expect(container.textContent).toContain("Question 1 needs an answer"); + act(() => { flushSync(() => radios()[0]!.click()); }); + expect(container.textContent).not.toContain("Question 1 needs an answer"); }); }); @@ -2420,6 +2493,8 @@ describe("TaskChatComposer", () => { ).find((button) => button.textContent?.trim() === label); flushSync(() => byLabel("Staging")?.click()); await flushAsync(); + flushSync(() => byLabel("Next")?.click()); + await flushAsync(); expect(container.textContent).toContain("Anything else?"); flushSync(() => byLabel("Skip")?.click()); await flushAsync(); diff --git a/ui/src/components/task-chat/TaskChatInteractionCard.test.tsx b/ui/src/components/task-chat/TaskChatInteractionCard.test.tsx index 07b7034e81..f720a5ae69 100644 --- a/ui/src/components/task-chat/TaskChatInteractionCard.test.tsx +++ b/ui/src/components/task-chat/TaskChatInteractionCard.test.tsx @@ -438,6 +438,13 @@ describe("TaskChatInteractionCard", () => { ); await act(async () => firstAnswer?.click()); expect(submit).not.toHaveBeenCalled(); + // Answering stays on the question; Next is what moves on. + expect(container.textContent).toContain("1 of 2"); + await act(async () => + Array.from(container.querySelectorAll("button")) + .find((button) => button.textContent?.trim() === "Next") + ?.click(), + ); expect(container.textContent).toContain("2 of 2"); expect(container.textContent).toContain( diff --git a/ui/src/components/task-chat/TaskChatProtocolCard.test.tsx b/ui/src/components/task-chat/TaskChatProtocolCard.test.tsx index 9167dc05c4..5da2a68e45 100644 --- a/ui/src/components/task-chat/TaskChatProtocolCard.test.tsx +++ b/ui/src/components/task-chat/TaskChatProtocolCard.test.tsx @@ -640,11 +640,13 @@ describe("TaskChatProtocolCard", () => { (button) => button.textContent?.includes("Production"), ); await act(async () => production?.click()); - // Single selection advances; multi-selection waits for Next. + // Selecting answers the question; every page waits for Next. const nextButton = () => Array.from(container.querySelectorAll("button")).find( (button) => button.textContent?.trim() === "Next", ); + expect(container.textContent).toContain("1 of 3"); + await act(async () => nextButton()?.click()); expect(container.textContent).toContain("2 of 3"); expect(onDecision).not.toHaveBeenCalled(); expect(container.textContent).toContain( diff --git a/ui/src/components/task-chat/motion-tokens.ts b/ui/src/components/task-chat/motion-tokens.ts index 6e6074441c..b885d99bea 100644 --- a/ui/src/components/task-chat/motion-tokens.ts +++ b/ui/src/components/task-chat/motion-tokens.ts @@ -55,7 +55,6 @@ export const MOTION_TOKENS: MotionTokenDef[] = [ { name: "--motion-approval-pulse", group: "States", kind: "time", min: 0, max: 3000, step: 20 }, { name: "--motion-plan-entry-stagger", group: "States", kind: "time", min: 0, max: 300, step: 5 }, { name: "--motion-plan-check", group: "States", kind: "time", min: 0, max: 1500, step: 10 }, - { name: "--motion-question-confirm", group: "States", kind: "time", min: 0, max: 1000, step: 10 }, { name: "--motion-question-page-enter", group: "States", kind: "time", min: 0, max: 1000, step: 10 }, { name: "--motion-count-tween", group: "States", kind: "time", min: 0, max: 1500, step: 10 }, { name: "--motion-streaming-cursor-blink", group: "States", kind: "time", min: 0, max: 3000, step: 20 }, diff --git a/ui/src/index.css b/ui/src/index.css index 951556f36f..f7b2e97140 100644 --- a/ui/src/index.css +++ b/ui/src/index.css @@ -269,7 +269,6 @@ --motion-approval-pulse: 1.6s; --motion-plan-entry-stagger: 40ms; --motion-plan-check: var(--motion-duration-fast); - --motion-question-confirm: var(--motion-duration-fast); --motion-question-page-enter: var(--motion-duration-instant); --motion-count-tween: var(--motion-duration-slow); --motion-streaming-cursor-blink: 1.1s; @@ -870,11 +869,6 @@ 0%, 100% { box-shadow: 0 0 0 0 color-mix(in oklab, var(--primary) 0%, transparent); } 50% { box-shadow: 0 0 0 3px color-mix(in oklab, var(--primary) 18%, transparent); } } -@keyframes tc-question-choice-confirm { - from { transform: scale(0.65); } - 50%, to { transform: scale(1); } -} -.tc-question-choice-confirm { animation: tc-question-choice-confirm var(--motion-question-confirm) var(--motion-ease-out-expo) both; } .tc-question-option { transition-duration: var(--motion-duration-instant); } .tc-question-page { animation: tc-fade-in var(--motion-question-page-enter) var(--motion-ease-standard) both; }