Fix/pi ask user submit (#1188)

* Fix Pi ask_user optional input handling * Clarify Pi ask_user optional comment prompt * Combine Pi ask_user optional comment UI
2026-05-28 19:08:38 +08:00
parent 3176f844e7
commit 8262fb42af
6 changed files with 646 additions and 110 deletions
@@ -0,0 +1,79 @@
+import { describe, expect, test } from "vitest";
+import {
+  areQuestionsAnswered,
+  buildQuestionFormAnswers,
+  parseQuestionFormQuestions,
+  questionShowsTextInput,
+  resolveDismissLabel,
+  shouldSubmitEmptyOnDismiss,
+} from "./question-form-card-core";
+
+describe("question form card core", () => {
+  test("treats optional input prompts as skippable empty answers", () => {
+    const questions = parseQuestionFormQuestions({
+      questions: [
+        {
+          question: "Optional comment?",
+          header: "Response",
+          options: [],
+          multiSelect: false,
+          placeholder: "Optional comment (press Enter to skip)...",
+          allowEmpty: true,
+          dismissLabel: "Skip",
+        },
+      ],
+    });
+
+    if (!questions) throw new Error("questions did not parse");
+    expect(areQuestionsAnswered(questions, {}, {})).toBe(true);
+    expect(buildQuestionFormAnswers(questions, {}, {})).toEqual({ Response: "" });
+    expect(shouldSubmitEmptyOnDismiss(questions)).toBe(true);
+    expect(resolveDismissLabel(questions)).toBe("Skip");
+  });
+
+  test("requires a selection for option-only questions", () => {
+    const questions = parseQuestionFormQuestions({
+      questions: [
+        {
+          question: "Pick one",
+          header: "Response",
+          options: [{ label: "A" }, { label: "B" }],
+          multiSelect: false,
+        },
+      ],
+    });
+
+    if (!questions) throw new Error("questions did not parse");
+    const [question] = questions;
+    if (!question) throw new Error("question missing");
+    expect(questionShowsTextInput(question)).toBe(false);
+    expect(areQuestionsAnswered(questions, {}, { 0: "freeform" })).toBe(false);
+    expect(areQuestionsAnswered(questions, { 0: new Set([1]) }, {})).toBe(true);
+    expect(buildQuestionFormAnswers(questions, { 0: new Set([1]) }, {})).toEqual({
+      Response: "B",
+    });
+  });
+
+  test("shows text input for explicit other questions", () => {
+    const questions = parseQuestionFormQuestions({
+      questions: [
+        {
+          question: "Pick or type",
+          header: "Response",
+          options: [{ label: "A" }],
+          isOther: true,
+          multiSelect: false,
+        },
+      ],
+    });
+
+    if (!questions) throw new Error("questions did not parse");
+    const [question] = questions;
+    if (!question) throw new Error("question missing");
+    expect(questionShowsTextInput(question)).toBe(true);
+    expect(areQuestionsAnswered(questions, {}, { 0: "custom" })).toBe(true);
+    expect(buildQuestionFormAnswers(questions, {}, { 0: "custom" })).toEqual({
+      Response: "custom",
+    });
+  });
+});
@@ -0,0 +1,143 @@
+export interface QuestionOption {
+  label: string;
+  description?: string;
+}
+
+export interface QuestionFormQuestion {
+  question: string;
+  header: string;
+  options: QuestionOption[];
+  multiSelect: boolean;
+  allowOther: boolean;
+  allowEmpty: boolean;
+  placeholder?: string;
+  dismissLabel?: string;
+}
+
+export type QuestionSelections = Record<number, ReadonlySet<number>>;
+export type QuestionOtherTexts = Record<number, string>;
+
+function readOptionalString(record: Record<string, unknown>, key: string): string | undefined {
+  const value = record[key];
+  return typeof value === "string" ? value : undefined;
+}
+
+export function parseQuestionFormQuestions(input: unknown): QuestionFormQuestion[] | null {
+  if (
+    typeof input !== "object" ||
+    input === null ||
+    !("questions" in input) ||
+    !Array.isArray((input as Record<string, unknown>).questions)
+  ) {
+    return null;
+  }
+  const raw = (input as Record<string, unknown>).questions as unknown[];
+  const questions: QuestionFormQuestion[] = [];
+  for (const item of raw) {
+    if (typeof item !== "object" || item === null) return null;
+    const q = item as Record<string, unknown>;
+    if (typeof q.question !== "string" || typeof q.header !== "string") return null;
+    if (!Array.isArray(q.options)) return null;
+    const options: QuestionOption[] = [];
+    for (const opt of q.options as unknown[]) {
+      if (typeof opt !== "object" || opt === null) return null;
+      const o = opt as Record<string, unknown>;
+      if (typeof o.label !== "string") return null;
+      options.push({
+        label: o.label,
+        description: typeof o.description === "string" ? o.description : undefined,
+      });
+    }
+    questions.push({
+      question: q.question,
+      header: q.header,
+      options,
+      multiSelect: q.multiSelect === true,
+      allowOther: q.allowOther === true || q.isOther === true,
+      allowEmpty: q.allowEmpty === true,
+      placeholder: readOptionalString(q, "placeholder"),
+      dismissLabel: readOptionalString(q, "dismissLabel"),
+    });
+  }
+  return questions.length > 0 ? questions : null;
+}
+
+export function questionShowsTextInput(question: QuestionFormQuestion): boolean {
+  return question.options.length === 0 || question.allowOther;
+}
+
+export function isQuestionAnswered(
+  question: QuestionFormQuestion,
+  qIndex: number,
+  selections: QuestionSelections,
+  otherTexts: QuestionOtherTexts,
+): boolean {
+  const selected = selections[qIndex];
+  if (selected && selected.size > 0) {
+    return true;
+  }
+
+  if (!questionShowsTextInput(question)) {
+    return false;
+  }
+
+  const otherText = otherTexts[qIndex]?.trim();
+  if (otherText && otherText.length > 0) {
+    return true;
+  }
+
+  return question.allowEmpty;
+}
+
+export function areQuestionsAnswered(
+  questions: QuestionFormQuestion[] | null,
+  selections: QuestionSelections,
+  otherTexts: QuestionOtherTexts,
+): boolean {
+  return (
+    questions?.every((question, qIndex) =>
+      isQuestionAnswered(question, qIndex, selections, otherTexts),
+    ) ?? false
+  );
+}
+
+export function buildQuestionFormAnswers(
+  questions: QuestionFormQuestion[],
+  selections: QuestionSelections,
+  otherTexts: QuestionOtherTexts,
+): Record<string, string> {
+  const answers: Record<string, string> = {};
+  for (let i = 0; i < questions.length; i++) {
+    const q = questions[i];
+    const selected = selections[i];
+    const otherText = otherTexts[i]?.trim();
+
+    if (questionShowsTextInput(q)) {
+      if (otherText && otherText.length > 0) {
+        answers[q.header] = otherText;
+        continue;
+      }
+      if (q.allowEmpty && q.options.length === 0) {
+        answers[q.header] = "";
+        continue;
+      }
+    }
+
+    if (selected && selected.size > 0) {
+      const labels = Array.from(selected).map((idx) => q.options[idx].label);
+      answers[q.header] = labels.join(", ");
+    }
+  }
+  return answers;
+}
+
+export function shouldSubmitEmptyOnDismiss(questions: QuestionFormQuestion[]): boolean {
+  return (
+    questions.length > 0 &&
+    questions.every((question) => question.allowEmpty && question.options.length === 0)
+  );
+}
+
+export function resolveDismissLabel(questions: QuestionFormQuestion[]): string {
+  return questions.find((question) => question.dismissLabel)?.dismissLabel ?? "Dismiss";
+}
@@ -13,54 +13,16 @@ import { Check, CircleHelp, X } from "lucide-react-native";
 import type { PendingPermission } from "@/types/shared";
 import type { AgentPermissionResponse } from "@getpaseo/protocol/agent-types";
 import { isWeb } from "@/constants/platform";
-
-interface QuestionOption {
-  label: string;
-  description?: string;
-}
-
-interface Question {
-  question: string;
-  header: string;
-  options: QuestionOption[];
-  multiSelect: boolean;
-}
-
-function parseQuestions(input: unknown): Question[] | null {
-  if (
-    typeof input !== "object" ||
-    input === null ||
-    !("questions" in input) ||
-    !Array.isArray((input as Record<string, unknown>).questions)
-  ) {
-    return null;
-  }
-  const raw = (input as Record<string, unknown>).questions as unknown[];
-  const questions: Question[] = [];
-  for (const item of raw) {
-    if (typeof item !== "object" || item === null) return null;
-    const q = item as Record<string, unknown>;
-    if (typeof q.question !== "string" || typeof q.header !== "string") return null;
-    if (!Array.isArray(q.options)) return null;
-    const options: QuestionOption[] = [];
-    for (const opt of q.options as unknown[]) {
-      if (typeof opt !== "object" || opt === null) return null;
-      const o = opt as Record<string, unknown>;
-      if (typeof o.label !== "string") return null;
-      options.push({
-        label: o.label,
-        description: typeof o.description === "string" ? o.description : undefined,
-      });
-    }
-    questions.push({
-      question: q.question,
-      header: q.header,
-      options,
-      multiSelect: q.multiSelect === true,
-    });
-  }
-  return questions.length > 0 ? questions : null;
-}
+import {
+  areQuestionsAnswered,
+  buildQuestionFormAnswers,
+  parseQuestionFormQuestions,
+  questionShowsTextInput,
+  resolveDismissLabel,
+  shouldSubmitEmptyOnDismiss,
+  type QuestionFormQuestion,
+  type QuestionOption,
+} from "./question-form-card-core";

 interface QuestionFormCardProps {
  permission: PendingPermission;
@@ -70,6 +32,12 @@ interface QuestionFormCardProps {

 const IS_WEB = isWeb;

+function getQuestionInputPlaceholder(question: QuestionFormQuestion): string {
+  return (
+    question.placeholder ?? (question.options.length === 0 ? "Type your answer..." : "Other...")
+  );
+}
+
 interface QuestionOptionRowProps {
  qIndex: number;
  optIndex: number;
@@ -137,6 +105,7 @@ function QuestionOptionRow({
 interface QuestionOtherInputProps {
  qIndex: number;
  value: string;
+  placeholder: string;
  isResponding: boolean;
  onChange: (qIndex: number, text: string) => void;
  onSubmit: () => void;
@@ -145,6 +114,7 @@ interface QuestionOtherInputProps {
 function QuestionOtherInput({
  qIndex,
  value,
+  placeholder,
  isResponding,
  onChange,
  onSubmit,
@@ -179,7 +149,7 @@ function QuestionOtherInput({
    <TextInput
      // @ts-expect-error - outlineStyle is web-only
      style={otherInputStyle}
-      placeholder="Other..."
+      placeholder={placeholder}
      placeholderTextColor={theme.colors.foregroundMuted}
      value={value}
      onChangeText={handleChange}
@@ -193,7 +163,7 @@ function QuestionOtherInput({
 export function QuestionFormCard({ permission, onRespond, isResponding }: QuestionFormCardProps) {
  const { theme } = useUnistyles();
  const isMobile = useIsCompactFormFactor();
-  const questions = parseQuestions(permission.request.input);
+  const questions = parseQuestionFormQuestions(permission.request.input);

  const [selections, setSelections] = useState<Record<number, Set<number>>>({});
  const [otherTexts, setOtherTexts] = useState<Record<number, string>>({});
@@ -237,33 +207,17 @@ export function QuestionFormCard({ permission, onRespond, isResponding }: Questi
    }
  }, []);

-  const allAnswered =
-    questions?.every((_, qIndex) => {
-      const selected = selections[qIndex];
-      const otherText = otherTexts[qIndex]?.trim();
-      return (selected && selected.size > 0) || (otherText && otherText.length > 0);
-    }) ?? false;
+  const allAnswered = areQuestionsAnswered(questions, selections, otherTexts);

  const handleSubmit = useCallback(() => {
    if (!questions || !allAnswered || isResponding) return;
    setRespondingAction("submit");
-    const answers: Record<string, string> = {};
-    for (let i = 0; i < questions.length; i++) {
-      const q = questions[i];
-      const selected = selections[i];
-      const otherText = otherTexts[i]?.trim();
-
-      if (otherText && otherText.length > 0) {
-        answers[q.header] = otherText;
-      } else if (selected && selected.size > 0) {
-        const labels = Array.from(selected).map((idx) => q.options[idx].label);
-        answers[q.header] = labels.join(", ");
-      }
-    }
-
    onRespond({
      behavior: "allow",
-      updatedInput: { ...permission.request.input, answers },
+      updatedInput: {
+        ...permission.request.input,
+        answers: buildQuestionFormAnswers(questions, selections, otherTexts),
+      },
    });
  }, [
    questions,
@@ -276,12 +230,23 @@ export function QuestionFormCard({ permission, onRespond, isResponding }: Questi
  ]);

  const handleDeny = useCallback(() => {
+    if (!questions) return;
    setRespondingAction("dismiss");
+    if (shouldSubmitEmptyOnDismiss(questions)) {
+      onRespond({
+        behavior: "allow",
+        updatedInput: {
+          ...permission.request.input,
+          answers: buildQuestionFormAnswers(questions, selections, otherTexts),
+        },
+      });
+      return;
+    }
    onRespond({
      behavior: "deny",
      message: "Dismissed by user",
    });
-  }, [onRespond]);
+  }, [questions, onRespond, otherTexts, permission.request.input, selections]);

  const dismissButtonStyle = useCallback(
    ({ pressed, hovered }: PressableStateCallbackType & { hovered?: boolean }) => [
@@ -349,11 +314,14 @@ export function QuestionFormCard({ permission, onRespond, isResponding }: Questi
    return null;
  }

+  const dismissLabel = resolveDismissLabel(questions);
+
  return (
    <View style={containerStyle}>
      {questions.map((q, qIndex) => {
        const selected = selections[qIndex] ?? new Set<number>();
        const otherText = otherTexts[qIndex] ?? "";
+        const showTextInput = questionShowsTextInput(q);

        return (
          <View key={q.question} style={styles.questionBlock}>
@@ -361,27 +329,32 @@ export function QuestionFormCard({ permission, onRespond, isResponding }: Questi
              <Text style={questionTextStyle}>{q.question}</Text>
              <CircleHelp size={14} color={theme.colors.foregroundMuted} />
            </View>
-            <View style={styles.optionsWrap}>
-              {q.options.map((opt, optIndex) => (
-                <QuestionOptionRow
-                  key={opt.label}
-                  qIndex={qIndex}
-                  optIndex={optIndex}
-                  option={opt}
-                  isSelected={selected.has(optIndex)}
-                  multiSelect={q.multiSelect}
-                  isResponding={isResponding}
-                  onToggle={toggleOption}
-                />
-              ))}
-            </View>
-            <QuestionOtherInput
-              qIndex={qIndex}
-              value={otherText}
-              isResponding={isResponding}
-              onChange={setOtherText}
-              onSubmit={handleSubmit}
-            />
+            {q.options.length > 0 ? (
+              <View style={styles.optionsWrap}>
+                {q.options.map((opt, optIndex) => (
+                  <QuestionOptionRow
+                    key={opt.label}
+                    qIndex={qIndex}
+                    optIndex={optIndex}
+                    option={opt}
+                    isSelected={selected.has(optIndex)}
+                    multiSelect={q.multiSelect}
+                    isResponding={isResponding}
+                    onToggle={toggleOption}
+                  />
+                ))}
+              </View>
+            ) : null}
+            {showTextInput ? (
+              <QuestionOtherInput
+                qIndex={qIndex}
+                value={otherText}
+                placeholder={getQuestionInputPlaceholder(q)}
+                isResponding={isResponding}
+                onChange={setOtherText}
+                onSubmit={handleSubmit}
+              />
+            ) : null}
          </View>
        );
      })}
@@ -393,7 +366,7 @@ export function QuestionFormCard({ permission, onRespond, isResponding }: Questi
          ) : (
            <View style={styles.actionContent}>
              <X size={14} color={theme.colors.foregroundMuted} />
-              <Text style={dismissActionTextStyle}>Dismiss</Text>
+              <Text style={dismissActionTextStyle}>{dismissLabel}</Text>
            </View>
          )}
        </Pressable>