diff --git a/packages/app/e2e/browser/question-prompt-pagination.spec.ts b/packages/app/e2e/browser/question-prompt-pagination.spec.ts index 90a87433ed..35002ac2dd 100644 --- a/packages/app/e2e/browser/question-prompt-pagination.spec.ts +++ b/packages/app/e2e/browser/question-prompt-pagination.spec.ts @@ -108,10 +108,7 @@ test.describe("Question option text", () => { } }); - test("selecting description text never answers, and the label still does", async ({ - page, - context, - }) => { + test("selecting description text keeps the question open", async ({ page, context }) => { test.setTimeout(180_000); const { session, card } = await openLinkedQuestions(page, context); try { @@ -120,16 +117,13 @@ test.describe("Question option text", () => { await dragAcross(page, description); await expect.poll(() => selectedText(page)).toBe(DONE_DESCRIPTION); - await expectChecked(done, false); - await description.getByText("Logs are at", { exact: false }).dblclick({ position: { x: 4, y: 4 }, }); await expect.poll(() => selectedText(page)).toBe("Logs"); await expectCurrentQuestion(page, { index: 1, total: 2, question: LINKED_QUESTION }); - await expectChecked(done, false); - - await done.getByText("Done", { exact: true }).click(); + await expectChecked(done, true); + await continueToNextQuestion(page); await expectCurrentQuestion(page, { index: 2, total: 2, question: CHECKS_QUESTION }); await openQuestion(page, { index: 1, total: 2 }); await expectChecked(done, true); @@ -138,11 +132,17 @@ test.describe("Question option text", () => { } }); - test("double-clicking a single-select label answers once", async ({ page, context }) => { + test("double-clicking a single-select label selects once without advancing", async ({ + page, + context, + }) => { test.setTimeout(180_000); const { session, card } = await openLinkedQuestions(page, context); try { await card.getByRole("radio", { name: "Done" }).getByText("Done", { exact: true }).dblclick(); + await expectCurrentQuestion(page, { index: 1, total: 2, question: LINKED_QUESTION }); + await expectChecked(card.getByRole("radio", { name: "Done" }), true); + await continueToNextQuestion(page); await expectCurrentQuestion(page, { index: 2, total: 2, question: CHECKS_QUESTION }); await expectChecked(card.getByRole("checkbox", { name: "Lint" }), false); @@ -154,14 +154,12 @@ test.describe("Question option text", () => { } }); - test("multi-select description links and selection leave the option unchanged", async ({ - page, - context, - }) => { + test("multi-select descriptions support links and text selection", async ({ page, context }) => { test.setTimeout(180_000); const { session, card } = await openLinkedQuestions(page, context); try { await card.getByRole("radio", { name: "Done" }).getByText("Done", { exact: true }).click(); + await continueToNextQuestion(page); await expectCurrentQuestion(page, { index: 2, total: 2, question: CHECKS_QUESTION }); const lint = card.getByRole("checkbox", { name: "Lint" }); @@ -177,18 +175,16 @@ test.describe("Question option text", () => { await dragAcross(page, description); await expect.poll(() => selectedText(page)).toBe(LINT_DESCRIPTION); - await expectChecked(lint, false); - await description.getByText("Rules:", { exact: false }).dblclick({ position: { x: 4, y: 4 }, }); await expect.poll(() => selectedText(page)).toBe("Rules"); - await expectChecked(lint, false); + await expectChecked(lint, true); await typecheck.getByText("Typecheck", { exact: true }).dblclick(); await expectChecked(typecheck, true); await lint.getByText("Lint", { exact: true }).click(); - await expectChecked(lint, true); + await expectChecked(lint, false); } finally { await session.cleanup(); } @@ -268,6 +264,7 @@ test.describe("Question prompt pagination", () => { await expectQuestionHidden(page, SUCCESS_QUESTION); await chooseQuestionOption(page, "App"); + await continueToNextQuestion(page); await expectCurrentQuestion(page, { index: 2, total: TOTAL_QUESTIONS, @@ -288,6 +285,7 @@ test.describe("Question prompt pagination", () => { preview: 'flags.enabled("new-surface")', }); await chooseQuestionOption(page, "Behind feature flag"); + await continueToNextQuestion(page); await expectCurrentQuestion(page, { index: 3, total: TOTAL_QUESTIONS, @@ -466,7 +464,9 @@ retryTest( retryTest.setTimeout(180_000); const card = page.getByTestId("question-form-card"); await chooseQuestionOption(page, "App"); + await continueToNextQuestion(page); await chooseQuestionOption(page, "Behind feature flag"); + await continueToNextQuestion(page); await fillQuestionAnswer(page, { question: SUCCESS_QUESTION, answer: "Preserve my answers after a failed submission.", @@ -520,7 +520,9 @@ retryTest( const card = page.getByTestId("question-form-card"); questionClient.releaseFirstResponse(); await chooseQuestionOption(page, "App"); + await continueToNextQuestion(page); await chooseQuestionOption(page, "Behind feature flag"); + await continueToNextQuestion(page); await fillQuestionAnswer(page, { question: SUCCESS_QUESTION, answer: "Start with the internal team.", diff --git a/packages/app/src/components/question-form-card.browser.test.tsx b/packages/app/src/components/question-form-card.browser.test.tsx index 0b2a5df25d..a91ba41215 100644 --- a/packages/app/src/components/question-form-card.browser.test.tsx +++ b/packages/app/src/components/question-form-card.browser.test.tsx @@ -39,7 +39,10 @@ afterEach(() => { } }); -function buildPermission(question: Record): PendingPermission { +function buildPermission( + question: Record, + following: Record[] = [], +): PendingPermission { return { key: "perm-1", agentId: "agent-1", @@ -48,12 +51,12 @@ function buildPermission(question: Record): PendingPermission { provider: "claude", name: "AskUserQuestion", kind: "question", - input: { questions: [question] }, + input: { questions: [question, ...following] }, }, }; } -function mountCard(question: Record) { +function mountCard(question: Record, following: Record[] = []) { const onRespond = vi.fn<(response: AgentPermissionResponse) => void>(); const container = document.createElement("div"); document.body.appendChild(container); @@ -61,7 +64,7 @@ function mountCard(question: Record) { act(() => root.render( ) { mounted.push({ root, container }); const view = within(container); - const optionRole = question.multiSelect ? "checkbox" : "radio"; const otherInput = () => view.getByRole("textbox", { name: String(question.question) }); - const check = (label: string) => act(() => view.getByRole(optionRole, { name: label }).click()); + const check = (label: string) => + act(() => + ( + view.queryByRole("radio", { name: label }) ?? view.getByRole("checkbox", { name: label }) + ).click(), + ); const type = (text: string) => { const input = otherInput(); const valueSetter = Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, "value")?.set; @@ -90,7 +97,7 @@ function mountCard(question: Record) { if (!response || response.behavior !== "allow") throw new Error("card did not submit"); return (response.updatedInput as { answers: Record }).answers; }; - return { check, type, otherInput, submit, submittedAnswers }; + return { check, type, otherInput, submit, submittedAnswers, view, onRespond }; } const multiSelectQuestion = { @@ -154,3 +161,31 @@ describe("QuestionFormCard other answers", () => { expect(card.submittedAnswers()).toEqual({ Provider: "Codex" }); }); }); + +describe("QuestionFormCard explicit navigation", () => { + it("keeps a picked option visible until Next and submits only on Submit", () => { + const card = mountCard(singleSelectQuestion, [multiSelectQuestion]); + card.check("Codex"); + expect(card.view.getByRole("radio", { name: "Codex" }).getAttribute("aria-checked")).toBe( + "true", + ); + expect(card.onRespond).not.toHaveBeenCalled(); + act(() => card.view.getByRole("button", { name: "Next" }).click()); + card.check("Apple"); + expect(card.onRespond).not.toHaveBeenCalled(); + card.submit(); + expect(card.submittedAnswers()).toEqual({ Provider: "Codex", Fruits: "Apple" }); + }); + + it("selects an option when its description is clicked without submitting", () => { + const card = mountCard({ + ...singleSelectQuestion, + options: [{ label: "Codex", description: "Use this provider" }], + }); + act(() => card.view.getByText("Use this provider").click()); + expect(card.view.getByRole("radio", { name: "Codex" }).getAttribute("aria-checked")).toBe( + "true", + ); + expect(card.onRespond).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/app/src/components/question-form-card.tsx b/packages/app/src/components/question-form-card.tsx index 8ff81963d1..7417cf64c4 100644 --- a/packages/app/src/components/question-form-card.tsx +++ b/packages/app/src/components/question-form-card.tsx @@ -43,20 +43,16 @@ const IS_WEB = isWeb; // drag to the preview's nested scroll, so lines past a cap would be unreachable. const OPTION_PREVIEW_MAX_HEIGHT = IS_WEB ? 240 : undefined; -// Web clicks that must not answer: a link in the label, anything in the description, and the -// second click of a double-click, which would otherwise land on the next question's options. -function isNonAnsweringClick(event: GestureResponderEvent, description: View | null): boolean { +// Links are independent actions. Ignore repeat clicks so double-click text selection +// toggles an option only once. +function isNonAnsweringClick(event: GestureResponderEvent): boolean { if (!IS_WEB) return false; const click = event.nativeEvent as unknown as MouseEvent; if (click.detail > 1) return true; const target = click.target as Element | null; - const descriptionElement = description as unknown as Element | null; - return Boolean(target?.closest("a") || descriptionElement?.contains(target)); + return Boolean(target?.closest("a")); } -// The description is selectable text, so a touch on it belongs to the text, not the option. -const claimResponder = () => true; - function getQuestionInputPlaceholder({ question, answerPlaceholder, @@ -92,10 +88,9 @@ function QuestionOptionRow({ }: QuestionOptionRowProps) { const { theme } = useUnistyles(); - const descriptionRef = useRef(null); const handlePress = useCallback( (event: GestureResponderEvent) => { - if (isNonAnsweringClick(event, descriptionRef.current)) return; + if (isNonAnsweringClick(event)) return; onToggle(qIndex, optIndex, multiSelect); }, [onToggle, qIndex, optIndex, multiSelect], @@ -152,11 +147,7 @@ function QuestionOptionRow({ {option.description ? ( - + ) : null} @@ -413,12 +404,8 @@ export function QuestionFormCard({ }); otherInputRef.current?.replaceText(""); } - - if (!multiSelect && next.size > 0 && qIndex === activeQuestionIndex && questions) { - setActiveQuestionIndex(Math.min(qIndex + 1, questions.length - 1)); - } }, - [activeQuestionIndex, otherTexts, questions, selections], + [otherTexts, selections], ); const setOtherText = useCallback(