From 37f3bb7dd9c0fe665051ce26dadab03b090dc1af Mon Sep 17 00:00:00 2001 From: Bohan Jiang <52446949+Bohan-J@users.noreply.github.com> Date: Sat, 1 Aug 2026 11:04:10 +0800 Subject: [PATCH] MUL-5587: fix(autopilots): tell the user which required field blocks Create (#6231) (#6237) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(autopilots): tell the user which required field blocks Create (MUL-5587) The create dialog gated its submit button with native `disabled` on an empty title, an unpicked assignee, or a server-rejected schedule. A natively disabled button is neither hoverable nor focusable, so a user who had not picked an assignee got a dead grey control and no hint about what was missing (GitHub #6231) — the assignee picker looks exactly like the optional Project and Subscribers pickers beside it, and nothing said only that one was required. An unmet requirement now uses `aria-disabled` instead: the button still takes the hover that shows a tooltip naming the missing field, and still takes the click, which reveals an inline error under the field at fault and focuses it. `handleSubmit` is the real gate either way, so nothing can be submitted that could not be before. The assignee section also carries a required marker up front, so the dead end is avoided rather than only explained. The three states come from one `submitBlock` value shared by the button, the tooltip, the inline errors and `handleSubmit`, so a rendered affordance cannot disagree with what submitting actually does. Co-authored-by: multica-agent * refactor(autopilots): stop dimming Create at all — the click is the feedback (MUL-5587) Follow-up on review. The previous commit kept the button greyed via `aria-disabled` and explained the grey with a tooltip. Since a blocked click now intercepts and points at the offending field, the greying earns nothing: drop `aria-disabled`, the dimming classes and the tooltip, and let the button be an ordinary live button whenever a save isn't already in flight. The schedule case no longer scrolls to the editor's inline error and returns. It falls through to `scheduleGate.ensureAccepted`, which re-asks the server and toasts its actual reason — visible feedback where the scroll could be a no-op, and it self-heals when a stored expression the server once rejected is accepted again. That leaves nothing in this dialog reading the gate's `scheduleValid`, so its `onValidityChange` / `clearRejection` wiring goes too rather than sitting inert. The shared hook keeps them for the detail page's add-trigger dialog, which still gates on it. Co-authored-by: multica-agent --------- Co-authored-by: Bohan-J Co-authored-by: multica-agent --- .../components/autopilot-dialog.tsx | 113 +++++++++-- .../autopilot-dialog.validation.test.tsx | 187 ++++++++++++++++++ packages/views/locales/en/autopilots.json | 2 + packages/views/locales/ja/autopilots.json | 2 + packages/views/locales/ko/autopilots.json | 2 + .../views/locales/zh-Hans/autopilots.json | 2 + 6 files changed, 290 insertions(+), 18 deletions(-) create mode 100644 packages/views/autopilots/components/autopilot-dialog.validation.test.tsx diff --git a/packages/views/autopilots/components/autopilot-dialog.tsx b/packages/views/autopilots/components/autopilot-dialog.tsx index c08fa0bdc0..00461c97d0 100644 --- a/packages/views/autopilots/components/autopilot-dialog.tsx +++ b/packages/views/autopilots/components/autopilot-dialog.tsx @@ -1,6 +1,6 @@ "use client"; -import { useMemo, useRef, useState } from "react"; +import { useId, useMemo, useRef, useState } from "react"; import { useQuery } from "@tanstack/react-query"; import { toast } from "sonner"; import { @@ -53,7 +53,7 @@ import type { AutopilotExecutionMode, AutopilotTrigger, } from "@multica/core/types"; -import { TitleEditor, ContentEditor } from "../../editor"; +import { TitleEditor, ContentEditor, type TitleEditorRef } from "../../editor"; import { ActorAvatar } from "../../common/actor-avatar"; import { SegmentedToggle } from "../../common/segmented-toggle"; import { ProjectPicker } from "../../projects/components/project-picker"; @@ -251,14 +251,32 @@ export function AutopilotDialog(props: AutopilotDialogProps) { // edits to the title, prompt or assignee. const scheduleWillBeWritten = triggerKind === "schedule" && !schedulePillDisabled && (isCreate || scheduleDirty); - const canSubmit = - title.trim().length > 0 && - assigneeId.length > 0 && - !submitting && - (!scheduleWillBeWritten || scheduleGate.scheduleValid); + + // The FIRST empty required field in reading order — the user fills one, the + // next surfaces. Only these two are answered here: a rejected schedule is + // re-checked against the server below, which toasts its actual reason. + const missingField: "title" | "assignee" | null = + title.trim().length === 0 ? "title" : assigneeId.length === 0 ? "assignee" : null; + + // Inline errors appear only after a submit attempt: a form that opens already + // shouting at the user for fields they have not reached yet is worse than the + // silence this replaces. Rendered as `showErrors && `, + // so filling the field clears its error without a second submit. + const [showErrors, setShowErrors] = useState(false); + const titleEditorRef = useRef(null); + const assigneeTriggerRef = useRef(null); + const assigneeErrorId = useId(); const handleSubmit = async () => { - if (!canSubmit) return; + if (submitting) return; + if (missingField !== null) { + // Reveal the inline errors and take the user to the field at fault; + // focusing scrolls the config column to it on its own. + setShowErrors(true); + if (missingField === "title") titleEditorRef.current?.focus(); + else assigneeTriggerRef.current?.focus(); + return; + } setSubmitting(true); try { if (scheduleWillBeWritten && !(await scheduleGate.ensureAccepted(schedule))) { @@ -519,6 +537,7 @@ export function AutopilotDialog(props: AutopilotDialogProps) {
$.dialog.title_placeholder)} @@ -526,6 +545,15 @@ export function AutopilotDialog(props: AutopilotDialogProps) { onChange={setTitle} onSubmit={handleSubmit} /> + {/* role="alert": the title is a contenteditable, so there is no + input to hang aria-describedby off — announcing the error is + the only way a screen-reader user learns why Create did + nothing. */} + {showErrors && title.trim().length === 0 && ( +

+ {t(($) => $.dialog.error_title_required)} +

+ )}
@@ -553,11 +581,14 @@ export function AutopilotDialog(props: AutopilotDialogProps) { {/* Right: Configuration */}
@@ -646,42 +689,71 @@ export function AutopilotDialog(props: AutopilotDialogProps) { // Right column sections // --------------------------------------------------------------------------- -function SectionLabel({ children }: { children: React.ReactNode }) { +function SectionLabel({ + children, + required, +}: { + children: React.ReactNode; + // Purely the sighted user's advance warning. `aria-required` is not supported + // on `role="button"`, so the picker cannot carry it; what a screen reader + // gets instead is the blocked submit's error, wired to the trigger through + // aria-describedby and announced by its own role="alert". + required?: boolean; +}) { return (
{children} + {required === true && ( + + * + + )}
); } function AgentSection({ + ref, selectedType, selectedId, onChange, selectedName, selectedDescription, + invalid, + errorId, }: { + ref: React.Ref; selectedType: AutopilotAssigneeType; selectedId: string; onChange: (next: AssigneeSelection) => void; selectedName?: string; selectedDescription?: string; + /** A submit was attempted with no assignee picked. */ + invalid: boolean; + errorId: string; }) { const { t } = useT("autopilots"); const hasSelection = selectedId.length > 0; return (
- {t(($) => $.dialog.section_assignee)} + {/* Marked required, unlike the Project and Subscribers pickers below it: + the three look identical, and nothing else told the user that only + this one blocks Create (#6231). */} + {t(($) => $.dialog.section_assignee)} {hasSelection ? ( @@ -710,6 +782,11 @@ function AgentSection({ } /> + {invalid && ( + + )}
); } diff --git a/packages/views/autopilots/components/autopilot-dialog.validation.test.tsx b/packages/views/autopilots/components/autopilot-dialog.validation.test.tsx new file mode 100644 index 0000000000..a55c66a75a --- /dev/null +++ b/packages/views/autopilots/components/autopilot-dialog.validation.test.tsx @@ -0,0 +1,187 @@ +import { useImperativeHandle, useRef, useState } from "react"; +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { renderWithI18n } from "../../test/i18n"; + +// Regression cover for GitHub #6231: "Create Autopilot" was disabled whenever +// a required field was empty, so a user who had not picked an assignee saw a +// dead control and no reason for it. The button is now live whenever a save +// isn't already in flight, and the click it accepts is what surfaces an inline +// error on the field at fault. + +const mockCreateAutopilot = vi.hoisted(() => vi.fn()); +const mockCreateTrigger = vi.hoisted(() => vi.fn()); + +vi.mock("@multica/core/hooks", () => ({ useWorkspaceId: () => "ws-test" })); +vi.mock("@multica/core/paths", () => ({ useCurrentWorkspace: () => ({ name: "Acme" }) })); + +vi.mock("@multica/core/workspace/queries", () => ({ + agentListOptions: (wsId: string) => ({ + queryKey: ["agents", wsId], + queryFn: async () => [ + { id: "agent-1", name: "Scout", description: "Researches things", archived_at: null }, + ], + }), + squadListOptions: (wsId: string) => ({ + queryKey: ["squads", wsId], + queryFn: async () => [], + }), +})); + +vi.mock("@multica/core/projects/queries", () => ({ + projectListOptions: (wsId: string) => ({ + queryKey: ["projects", wsId], + queryFn: async () => [], + }), +})); + +vi.mock("@multica/core/autopilots/queries", () => ({ + cronPreviewOptions: (wsId: string, expr: string, tz: string) => ({ + queryKey: ["cron-preview", wsId, expr, tz], + queryFn: async () => ({ next_runs: ["2126-07-14T01:00:00Z"] }), + retry: false, + }), +})); + +vi.mock("@multica/core/autopilots/mutations", () => ({ + useCreateAutopilot: () => ({ mutateAsync: mockCreateAutopilot }), + useCreateAutopilotTrigger: () => ({ mutateAsync: mockCreateTrigger }), + useUpdateAutopilot: () => ({ mutateAsync: vi.fn() }), + useUpdateAutopilotTrigger: () => ({ mutateAsync: vi.fn() }), +})); + +vi.mock("sonner", () => ({ toast: { success: vi.fn(), error: vi.fn() } })); + +// Tiptap in jsdom is neither cheap nor the subject here: the title is a plain +// input whose ref honours focus(), which is all the dialog asks of it. +vi.mock("../../editor", () => ({ + TitleEditor: ({ ref, defaultValue, placeholder, onChange, onSubmit }: any) => { + const [value, setValue] = useState(defaultValue ?? ""); + const inputRef = useRef(null); + useImperativeHandle(ref, () => ({ + getText: () => value, + focus: () => inputRef.current?.focus(), + focusAtCoords: () => inputRef.current?.focus(), + })); + return ( + { + setValue(e.target.value); + onChange?.(e.target.value); + }} + onKeyDown={(e) => { + if (e.key === "Enter") onSubmit?.(); + }} + /> + ); + }, + ContentEditor: ({ placeholder }: any) =>