From 2bd370211fbaefb1e1b50c0e9f9f2566181ff434 Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Mon, 27 Jul 2026 21:04:38 -0400 Subject: [PATCH 1/2] feat(desktop): add custom harness inline from agent dialogs Registering a custom ACP harness was reachable only from Settings > Agents, so anyone whose first touchpoint was "New agent" had no way to learn the path existed. The harness dropdowns now carry the entry. Selection is deferred rather than applied on save: saving writes the definition file, and the harness only becomes a catalog entry once the invalidated discovery query refetches. Selecting immediately would pick an id no entry backs, which blocks the create dialog's Save on an unknown availability and leaves the instance dialog no command to pin. The sentinel routes through each dialog's existing dropdown-change handler, so an inline registration resets model/provider exactly as a hand-picked harness does. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/playwright.config.ts | 1 + desktop/scripts/check-file-sizes.mjs | 10 +- .../agents/ui/AddCustomHarnessDialog.tsx | 47 +++ .../agents/ui/AgentDefinitionDialog.tsx | 30 +- .../agents/ui/AgentInstanceEditDialog.tsx | 29 +- .../agents/ui/addCustomHarness.test.mjs | 268 ++++++++++++++++++ .../features/agents/ui/addCustomHarness.ts | 89 ++++++ .../settings/ui/CustomHarnessForm.tsx | 11 +- .../tests/e2e/inline-custom-harness.spec.ts | 191 +++++++++++++ 9 files changed, 664 insertions(+), 12 deletions(-) create mode 100644 desktop/src/features/agents/ui/AddCustomHarnessDialog.tsx create mode 100644 desktop/src/features/agents/ui/addCustomHarness.test.mjs create mode 100644 desktop/src/features/agents/ui/addCustomHarness.ts create mode 100644 desktop/tests/e2e/inline-custom-harness.spec.ts diff --git a/desktop/playwright.config.ts b/desktop/playwright.config.ts index 0d89b8e2d2..459fa75743 100644 --- a/desktop/playwright.config.ts +++ b/desktop/playwright.config.ts @@ -126,6 +126,7 @@ export default defineConfig({ "**/observer-archive-policy.spec.ts", "**/harness-management.spec.ts", "**/harness-catalog-screenshots.spec.ts", + "**/inline-custom-harness.spec.ts", ], use: { ...devices["Desktop Chrome"], diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index f8ee32dfab..a4accdebf7 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -697,12 +697,18 @@ const overrides = new Map([ // hidden-key projection keeps the top-level secret out of Advanced rows. // +6 (1195 -> 1201): rebase onto main — this PR's model-source label wiring // lands on top of main's dialog growth. Queued to split. - ["src/features/agents/ui/AgentInstanceEditDialog.tsx", 1201], + // +25 (1201 -> 1226): inline "Add custom harness…" entry — sentinel option, + // modal state, and the AddCustomHarnessDialog mount. The shared routing and + // deferred-selection logic lives in addCustomHarness.ts to keep this minimal. + ["src/features/agents/ui/AgentInstanceEditDialog.tsx", 1226], // AgentDefinitionDialog grew past 1000 with the following load-bearing fixes: // isRuntimeAutoSeededRef tracking for edit-mode seeding (Fizz shows models); // runtimeSupportsLlmProviderSelection guard on discovery provider (codex fix); // hideProviderIds computation for Databricks v1 gate. Queued to split. - ["src/features/agents/ui/AgentDefinitionDialog.tsx", 1035], + // +26 (1020 -> 1046): inline "Add custom harness…" entry — sentinel option, + // modal state, and the AddCustomHarnessDialog mount. The shared routing and + // deferred-selection logic lives in addCustomHarness.ts to keep this minimal. + ["src/features/agents/ui/AgentDefinitionDialog.tsx", 1046], // #2630 emoji picker search: the shadow-root search-input autofocus effect // (rAF retry loop) took this file 999 -> 1026 and landed without this entry, // so main's Desktop Core went red. Queued to split with the rest of this list. diff --git a/desktop/src/features/agents/ui/AddCustomHarnessDialog.tsx b/desktop/src/features/agents/ui/AddCustomHarnessDialog.tsx new file mode 100644 index 0000000000..0c84e99275 --- /dev/null +++ b/desktop/src/features/agents/ui/AddCustomHarnessDialog.tsx @@ -0,0 +1,47 @@ +import { CustomHarnessForm } from "@/features/settings/ui/CustomHarnessForm"; +import { ChooserDialogContent } from "@/shared/ui/chooser-dialog-content"; +import { Dialog } from "@/shared/ui/dialog"; + +/** + * Registers a custom ACP harness from inside an agent dialog, so "New agent" + * is a complete entry point and not a dead end that sends the user to + * Settings. Hosts the same `CustomHarnessForm` the harness catalog uses. + */ +export function AddCustomHarnessDialog({ + onOpenChange, + onSaved, + open, +}: { + onOpenChange: (open: boolean) => void; + /** Called with the id of the harness that was just registered. */ + onSaved: (id: string) => void; + open: boolean; +}) { + return ( + + + + Register any ACP-speaking agent tool as a selectable harness. +

+ } + onCancel={() => onOpenChange(false)} + onSaved={(id) => { + // Dismiss on save as well as cancel — both exits belong to this + // dialog, so callers only handle the resulting selection. + onOpenChange(false); + onSaved(id); + }} + /> + + + ); +} diff --git a/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx b/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx index 45031e0371..d6641e6394 100644 --- a/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx +++ b/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx @@ -83,6 +83,12 @@ import { import { useProviderApiKeyFieldState } from "./providerApiKeyFieldState"; import { buildRuntimeModelProviderPayload } from "./agentDefinitionSubmitPayload"; import { AgentDefinitionDialogFooter } from "./AgentDefinitionDialogFooter"; +import { AddCustomHarnessDialog } from "./AddCustomHarnessDialog"; +import { + ADD_CUSTOM_HARNESS_OPTION, + runtimeDropdownAction, + usePendingHarnessSelection, +} from "./addCustomHarness"; type AgentDefinitionDialogProps = { open: boolean; @@ -167,6 +173,7 @@ export function AgentDefinitionDialog({ const [isAvatarUploadPending, setIsAvatarUploadPending] = React.useState(false); const [hasUserChanges, setHasUserChanges] = React.useState(false); + const [isAddHarnessOpen, setIsAddHarnessOpen] = React.useState(false); const { globalConfig, inheritedDefaults: { @@ -308,6 +315,7 @@ export function AgentDefinitionDialog({ setShowAdvancedFields(false); setIsAvatarUploadPending(false); setHasUserChanges(false); + setIsAddHarnessOpen(false); // isRuntimeAutoSeededRef and hasSeededForOpenRef are NOT reset here — the // [initialValues, open] effect resets both when the dialog re-opens. } @@ -578,6 +586,7 @@ export function AgentDefinitionDialog({ runtimes, runtimesLoading, }); + runtimeDropdownOptions.push(ADD_CUSTOM_HARNESS_OPTION); const runtimeSummaryLabel = selectedRuntime ? formatRuntimeOptionLabel(selectedRuntime) : runtime.trim() || "Not configured"; @@ -662,9 +671,13 @@ export function AgentDefinitionDialog({ } function handleRuntimeDropdownChange(nextValue: string) { + const action = runtimeDropdownAction(nextValue); + if (action.kind === "add-custom-harness") { + setIsAddHarnessOpen(true); + return; + } setHasUserChanges(true); - const nextRuntime = - nextValue === NO_RUNTIME_DROPDOWN_VALUE ? "" : nextValue; + const nextRuntime = action.runtimeId; // The user made an explicit choice — no longer auto-seeded. isRuntimeAutoSeededRef.current = false; setRuntime(nextRuntime); @@ -680,6 +693,13 @@ export function AgentDefinitionDialog({ ); } + // Routed through the normal change handler so a harness registered inline + // resets model/provider exactly as a hand-picked one would. + const selectSavedHarness = usePendingHarnessSelection( + runtimes, + handleRuntimeDropdownChange, + ); + function handleProviderDropdownChange(nextValue: string) { setHasUserChanges(true); const nextProvider = @@ -944,6 +964,12 @@ export function AgentDefinitionDialog({ returnFocusRef={aiDefaultsTriggerRef} /> + + {isCreateMode ? createRunSection : null}
diff --git a/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx b/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx index 601d57f95d..36ceb1214d 100644 --- a/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx +++ b/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx @@ -83,6 +83,12 @@ import { useProviderApiKeyFieldState } from "./providerApiKeyFieldState"; import { resolveModelFieldStatusMessage } from "./agentConfigControls"; import { AdvancedRequiredBadge } from "./AdvancedRequiredBadge"; import { showAgentProfileSyncWarning } from "./agentProfileSyncWarning"; +import { AddCustomHarnessDialog } from "./AddCustomHarnessDialog"; +import { + ADD_CUSTOM_HARNESS_OPTION, + runtimeDropdownAction, + usePendingHarnessSelection, +} from "./addCustomHarness"; const ADVANCED_FIELDS_MOTION_TRANSITION = { duration: 0.18, @@ -157,6 +163,7 @@ export function AgentInstanceEditDialog({ const [avatarUrl, setAvatarUrl] = React.useState(agent.avatarUrl ?? ""); const [isAvatarUploadPending, setIsAvatarUploadPending] = React.useState(false); + const [isAddHarnessOpen, setIsAddHarnessOpen] = React.useState(false); const shouldReduceMotion = useReducedMotion(); // Runtime selector: defaults to "custom" until the dialog opens and the @@ -191,6 +198,7 @@ export function AgentInstanceEditDialog({ setAvatarUrl(agent.avatarUrl ?? ""); setShowAdvancedFields(false); setIsAvatarUploadPending(false); + setIsAddHarnessOpen(false); runtimeTouched.current = false; const matched = runtimes.find((r) => r.command?.trim() === agent.agentCommand.trim()) ?? @@ -244,6 +252,7 @@ export function AgentInstanceEditDialog({ value: selectedRuntimeId, }); } + options.push(ADD_CUSTOM_HARNESS_OPTION); return options; }, [sortedRuntimes, selectedRuntimeId]); @@ -484,8 +493,12 @@ export function AgentInstanceEditDialog({ } function handleRuntimeDropdownChange(nextValue: string) { - const nextRuntimeId = - nextValue === NO_RUNTIME_DROPDOWN_VALUE ? "" : nextValue; + const action = runtimeDropdownAction(nextValue); + if (action.kind === "add-custom-harness") { + setIsAddHarnessOpen(true); + return; + } + const nextRuntimeId = action.runtimeId; const previousRuntimeId = selectedRuntimeId; const nextRuntime = runtimes.find((r) => r.id === nextRuntimeId); @@ -532,6 +545,13 @@ export function AgentInstanceEditDialog({ ); } + // Routed through the normal change handler so a harness registered inline + // pins its command and resets model/provider like a hand-picked one. + const selectSavedHarness = usePendingHarnessSelection( + runtimes, + handleRuntimeDropdownChange, + ); + function handleProviderDropdownChange(nextValue: string) { const nextProvider = nextValue === AUTO_PROVIDER_DROPDOWN_VALUE ? "" : nextValue; @@ -949,6 +969,11 @@ export function AgentInstanceEditDialog({

) : null} +
{selectedRuntimeId === "custom" && !inheritHarness ? (
diff --git a/desktop/src/features/agents/ui/addCustomHarness.test.mjs b/desktop/src/features/agents/ui/addCustomHarness.test.mjs new file mode 100644 index 0000000000..f01d94bb2c --- /dev/null +++ b/desktop/src/features/agents/ui/addCustomHarness.test.mjs @@ -0,0 +1,268 @@ +/** + * Behavior tests for the inline "Add custom harness…" dropdown entry shared by + * AgentDefinitionDialog and AgentInstanceEditDialog. + * + * Two seams carry the feature, and both are pinned here: + * + * 1. ROUTING (`runtimeDropdownAction`) — the sentinel must resolve to "open + * the form", never to a selection. If it ever resolved to a selection the + * dialogs would write "\u0000add-custom-harness" into `runtime` and try to + * spawn an agent on a harness that does not exist. + * 2. DEFERRED SELECTION (`usePendingHarnessSelection`) — saving only writes + * the definition file; the harness becomes a catalog entry when the + * invalidated discovery query refetches. Selecting on save would pick an + * id no entry backs. The hook must wait for the catalog, fire exactly + * once, and stay silent when the user cancels. + * + * The hook is mounted for real (react-dom/client + act) rather than simulated, + * so its effect wiring — including the guard that survives the dialogs' + * non-memoized change handlers — is what gets tested. + */ + +import assert from "node:assert/strict"; +import test from "node:test"; + +// ── Minimal DOM shim ───────────────────────────────────────────────────────── +// react-dom/client needs a container element and a document; node has neither. +// The harness renders null, so no real node operations are exercised. + +class ElementShim { + constructor() { + this.children = []; + this.childNodes = []; + this.nodeType = 1; + this.nodeName = "DIV"; + this.tagName = "DIV"; + this.namespaceURI = "http://www.w3.org/1999/xhtml"; + } + get ownerDocument() { + return globalThis.document; + } + addEventListener() {} + removeEventListener() {} + appendChild(child) { + this.children.push(child); + this.childNodes.push(child); + return child; + } + removeChild(child) { + this.children = this.children.filter((current) => current !== child); + this.childNodes = this.childNodes.filter((current) => current !== child); + return child; + } + insertBefore(child) { + return this.appendChild(child); + } + contains(target) { + return this === target; + } +} + +globalThis.document = { + activeElement: null, + addEventListener() {}, + createElement: () => new ElementShim(), + get defaultView() { + return globalThis.window; + }, + nodeType: 9, + removeEventListener() {}, +}; +// react-dom derives update priority from window.event and walks iframe +// boundaries via window.HTMLIFrameElement during commit. +Object.defineProperty(globalThis, "window", { + configurable: true, + value: { + addEventListener() {}, + document: globalThis.document, + event: undefined, + HTMLIFrameElement: ElementShim, + removeEventListener() {}, + }, +}); +globalThis.HTMLElement = ElementShim; +globalThis.Node = ElementShim; +globalThis.IS_REACT_ACT_ENVIRONMENT = true; + +import React from "react"; +import { act } from "react"; +import { createRoot } from "react-dom/client"; + +import { NO_RUNTIME_DROPDOWN_VALUE } from "./agentConfigOptions.tsx"; +import { + ADD_CUSTOM_HARNESS_OPTION, + ADD_CUSTOM_HARNESS_VALUE, + readyHarnessId, + runtimeDropdownAction, + usePendingHarnessSelection, +} from "./addCustomHarness.ts"; + +// ── Routing: the sentinel opens the form, it is never a selection ──────────── + +test("selecting the add-custom entry requests the form and yields no runtime id", () => { + const action = runtimeDropdownAction(ADD_CUSTOM_HARNESS_VALUE); + assert.equal(action.kind, "add-custom-harness"); + // The dialogs read `action.runtimeId` on the select branch; the sentinel + // must not carry one, or it could leak into form state. + assert.equal("runtimeId" in action, false); +}); + +test("selecting a harness yields that harness id", () => { + assert.deepEqual(runtimeDropdownAction("my-harness"), { + kind: "select", + runtimeId: "my-harness", + }); +}); + +test("selecting the no-runtime entry yields the empty id", () => { + assert.deepEqual(runtimeDropdownAction(NO_RUNTIME_DROPDOWN_VALUE), { + kind: "select", + runtimeId: "", + }); +}); + +test("the add-custom sentinel cannot collide with a backend-valid harness id", () => { + // Backend ids match [a-z0-9_][a-z0-9_-]* (custom_harnesses.rs), so a + // NUL-prefixed value is unreachable as a real id. + assert.equal(ADD_CUSTOM_HARNESS_VALUE.startsWith("\u0000"), true); + assert.equal(ADD_CUSTOM_HARNESS_OPTION.value, ADD_CUSTOM_HARNESS_VALUE); + assert.equal(ADD_CUSTOM_HARNESS_OPTION.label, "Add custom harness…"); +}); + +// ── Readiness: an id is selectable only once the catalog publishes it ──────── + +test("a pending id absent from the catalog is not ready", () => { + assert.equal(readyHarnessId([{ id: "claude" }], "my-harness"), null); +}); + +test("a pending id present in the catalog is ready", () => { + assert.equal( + readyHarnessId([{ id: "claude" }, { id: "my-harness" }], "my-harness"), + "my-harness", + ); +}); + +test("no pending id is never ready even against a populated catalog", () => { + assert.equal(readyHarnessId([{ id: "claude" }], null), null); +}); + +// ── Deferred selection: mounted hook ───────────────────────────────────────── + +/** + * Mount the real hook over a mutable catalog. Returns the setter the dialogs + * call on save, a `setRuntimes` to simulate the discovery refetch, and the log + * of ids the hook handed back for selection. + */ +async function mountPendingSelection(initialRuntimes = []) { + const selected = []; + const control = {}; + + function Harness() { + const [runtimes, setRuntimes] = React.useState(initialRuntimes); + // Deliberately NOT memoized: both dialogs pass a plain function + // declaration, so `onReady` has a fresh identity on every render. + const onReady = (id) => selected.push(id); + control.save = usePendingHarnessSelection(runtimes, onReady); + control.setRuntimes = setRuntimes; + return null; + } + + const root = createRoot(new ElementShim()); + await act(async () => { + root.render(React.createElement(Harness)); + }); + return { control, root, selected }; +} + +test("saving a harness selects it only once the catalog publishes it", async () => { + const { control, root, selected } = await mountPendingSelection([ + { id: "claude" }, + ]); + + // Save returns before discovery refetches — nothing to select yet. + await act(async () => control.save("my-harness")); + assert.deepEqual(selected, []); + + // The invalidated discovery query resolves with the new entry. + await act(async () => + control.setRuntimes([{ id: "claude" }, { id: "my-harness" }]), + ); + assert.deepEqual(selected, ["my-harness"]); + + await act(async () => root.unmount()); +}); + +test("a published harness is selected exactly once across later catalog updates", async () => { + const { control, root, selected } = await mountPendingSelection([ + { id: "claude" }, + ]); + + await act(async () => control.save("my-harness")); + await act(async () => + control.setRuntimes([{ id: "claude" }, { id: "my-harness" }]), + ); + assert.deepEqual(selected, ["my-harness"]); + + // Any later refetch re-renders with a new array identity and a new onReady + // identity. Re-firing here would clobber a selection the user made in + // between, so the pending id must have been cleared. + await act(async () => + control.setRuntimes([ + { id: "claude" }, + { id: "my-harness" }, + { id: "codex" }, + ]), + ); + assert.deepEqual(selected, ["my-harness"]); + + await act(async () => root.unmount()); +}); + +test("cancelling the form leaves the current selection untouched", async () => { + const { control, root, selected } = await mountPendingSelection([ + { id: "claude" }, + ]); + + // Cancel never reports a saved id, so no selection is ever requested — even + // as the catalog keeps refreshing underneath. + await act(async () => + control.setRuntimes([{ id: "claude" }, { id: "codex" }]), + ); + assert.deepEqual(selected, []); + + await act(async () => root.unmount()); +}); + +test("a saved harness discovery never publishes is never selected", async () => { + const { control, root, selected } = await mountPendingSelection([ + { id: "claude" }, + ]); + + // e.g. the definition file was written but the entry failed to load. The + // hook must stall rather than select an id no catalog entry backs. + await act(async () => control.save("ghost-harness")); + await act(async () => + control.setRuntimes([{ id: "claude" }, { id: "codex" }]), + ); + assert.deepEqual(selected, []); + + await act(async () => root.unmount()); +}); + +test("two harnesses registered in a row are each selected when published", async () => { + const { control, root, selected } = await mountPendingSelection([ + { id: "claude" }, + ]); + + await act(async () => control.save("first")); + await act(async () => + control.setRuntimes([{ id: "claude" }, { id: "first" }]), + ); + await act(async () => control.save("second")); + await act(async () => + control.setRuntimes([{ id: "claude" }, { id: "first" }, { id: "second" }]), + ); + assert.deepEqual(selected, ["first", "second"]); + + await act(async () => root.unmount()); +}); diff --git a/desktop/src/features/agents/ui/addCustomHarness.ts b/desktop/src/features/agents/ui/addCustomHarness.ts new file mode 100644 index 0000000000..465ce47f2a --- /dev/null +++ b/desktop/src/features/agents/ui/addCustomHarness.ts @@ -0,0 +1,89 @@ +/** + * Shared pieces of the inline "Add custom harness…" entry the agent dialogs + * append to their harness dropdown. + * + * Registering a custom harness used to be reachable only from Settings, so + * anyone whose first stop was "New agent" never learned the path existed. + * These helpers keep the entry identical across the dropdowns, keep its + * sentinel value out of form state, and defer selecting a freshly registered + * harness until discovery has actually published it. + */ + +import * as React from "react"; + +import { + NO_RUNTIME_DROPDOWN_VALUE, + type PersonaDropdownOption, +} from "./agentConfigOptions"; + +/** + * Dropdown value for the add-custom-harness entry. NUL-prefixed so it can + * never collide with a harness id (`[a-z0-9_][a-z0-9_-]*`) — same trick as the + * harness catalog's `CUSTOM_ENTRY_ID`. + */ +export const ADD_CUSTOM_HARNESS_VALUE = "\u0000add-custom-harness"; + +export const ADD_CUSTOM_HARNESS_OPTION: PersonaDropdownOption = { + label: "Add custom harness…", + value: ADD_CUSTOM_HARNESS_VALUE, +}; + +export type RuntimeDropdownAction = + | { kind: "add-custom-harness" } + | { kind: "select"; runtimeId: string }; + +/** + * Route a harness-dropdown change. The add-custom entry only opens the + * registration form — it is never a selection, so its sentinel can't reach + * form state. Every other value selects, with the no-runtime sentinel + * normalized to the empty id. + */ +export function runtimeDropdownAction(value: string): RuntimeDropdownAction { + if (value === ADD_CUSTOM_HARNESS_VALUE) { + return { kind: "add-custom-harness" }; + } + return { + kind: "select", + runtimeId: value === NO_RUNTIME_DROPDOWN_VALUE ? "" : value, + }; +} + +/** + * The pending harness id once discovery has published it, else `null`. + * + * Saving only writes the definition file — the harness becomes a catalog entry + * when the invalidated discovery query refetches. Selecting before then would + * pick an id no entry backs: the create dialog would block Save on an unknown + * availability, and the instance dialog could not read the command to pin. + */ +export function readyHarnessId( + runtimes: ReadonlyArray<{ id: string }>, + pendingId: string | null, +): string | null { + return runtimes.some((runtime) => runtime.id === pendingId) + ? pendingId + : null; +} + +/** + * Selects a newly registered custom harness once discovery publishes it. + * + * Returns the setter to hand the saved id; `onReady` then fires with it, so + * callers reuse their normal dropdown-change path instead of growing a second + * selection code path. + */ +export function usePendingHarnessSelection( + runtimes: ReadonlyArray<{ id: string }>, + onReady: (id: string) => void, +): (id: string) => void { + const [pendingId, setPendingId] = React.useState(null); + const readyId = readyHarnessId(runtimes, pendingId); + + React.useEffect(() => { + if (readyId === null) return; + setPendingId(null); + onReady(readyId); + }, [onReady, readyId]); + + return setPendingId; +} diff --git a/desktop/src/features/settings/ui/CustomHarnessForm.tsx b/desktop/src/features/settings/ui/CustomHarnessForm.tsx index 60f3a66af1..52e7906265 100644 --- a/desktop/src/features/settings/ui/CustomHarnessForm.tsx +++ b/desktop/src/features/settings/ui/CustomHarnessForm.tsx @@ -216,7 +216,8 @@ export function CustomHarnessForm({ * delete the old file when the id changes. */ originalId?: string; onCancel: () => void; - onSaved: () => void; + /** Receives the id the harness was saved under (the form may rewrite it). */ + onSaved: (id: string) => void; /** Render without the bordered card chrome (for embedding in the catalog * dialog detail pane). */ chromeless?: boolean; @@ -273,11 +274,9 @@ export function CustomHarnessForm({ return; } try { - await save.mutateAsync({ - definition: definitionFromFormValues(form), - originalId, - }); - onSaved(); + const definition = definitionFromFormValues(form); + await save.mutateAsync({ definition, originalId }); + onSaved(definition.id); } catch (err) { setError(err instanceof Error ? err.message : String(err)); } diff --git a/desktop/tests/e2e/inline-custom-harness.spec.ts b/desktop/tests/e2e/inline-custom-harness.spec.ts new file mode 100644 index 0000000000..b5f7c64a21 --- /dev/null +++ b/desktop/tests/e2e/inline-custom-harness.spec.ts @@ -0,0 +1,191 @@ +/** + * E2E spec for the inline "Add custom harness…" entry in the agent dialogs. + * + * Registering a custom harness used to be reachable only from Settings → + * Agents, so anyone whose first touchpoint was "New agent" never learned the + * path existed. The harness dropdowns now carry the entry directly. + * + * Covers, on all three surfaces (create, edit definition, edit instance): + * - the entry is present in the harness dropdown + * - choosing it opens the registration form without changing the selection + * - saving registers the harness and selects it in the dropdown + * - dismissing the form leaves the previous selection untouched + */ +import { expect, test } from "@playwright/test"; + +import { installMockBridge } from "../helpers/bridge"; + +const ADD_ENTRY = "Add custom harness…"; +const HARNESS_LABEL = "My Weird Agent"; +const HARNESS_COMMAND = "my-weird-acp"; + +type Page = import("@playwright/test").Page; +type Locator = import("@playwright/test").Locator; + +/** Open a PersonaDropdownField (button trigger + menuitemradio options). */ +async function openDropdown(trigger: Locator) { + await expect(trigger).toBeVisible({ timeout: 10_000 }); + await trigger.click(); +} + +/** Register a harness through the inline form and wait for it to close. */ +async function registerHarness(page: Page) { + const form = page.getByTestId("custom-harness-form"); + await expect(form).toBeVisible({ timeout: 8_000 }); + await page.fill("#ch-label", HARNESS_LABEL); + await page.fill("#ch-command", HARNESS_COMMAND); + // The id auto-derives from the label; it is what the dropdown selects on. + await expect(page.locator("#ch-id")).toHaveValue("my-weird-agent"); + await form.getByRole("button", { name: "Save", exact: true }).click(); + await expect(page.getByTestId("add-custom-harness-dialog")).not.toBeVisible({ + timeout: 8_000, + }); +} + +/** Open the create-agent dialog (AgentDefinitionDialog, create mode). */ +async function openCreateDialog(page: Page) { + await page.goto("/", { waitUntil: "domcontentloaded" }); + await page.getByTestId("open-agents-view").click(); + await page.getByTestId("new-agent-card").click(); + await page.getByRole("menuitem", { name: "Create agent" }).click(); + const dialog = page.getByTestId("persona-dialog"); + await expect(dialog).toBeVisible({ timeout: 10_000 }); + await dialog.getByRole("tab", { name: "Customize for this agent" }).click(); + return dialog; +} + +/** Open the edit dialog for a saved definition (same dialog, edit mode). */ +async function openDefinitionEditDialog(page: Page, name: string) { + await page.goto("/", { waitUntil: "domcontentloaded" }); + await page.getByTestId("open-agents-view").click(); + await expect(page.getByTestId("agents-library-personas")).toBeVisible({ + timeout: 10_000, + }); + await page.getByRole("button", { name: `Open actions for ${name}` }).click(); + await page.getByRole("menuitem", { name: "Edit" }).click(); + const dialog = page.getByTestId("persona-dialog"); + await expect(dialog).toBeVisible({ timeout: 10_000 }); + await dialog.getByRole("tab", { name: "Customize for this agent" }).click(); + return dialog; +} + +test.describe("inline add custom harness", () => { + test("create dialog registers a harness inline and selects it", async ({ + page, + }) => { + await installMockBridge(page); + const dialog = await openCreateDialog(page); + + const harness = dialog.locator("#persona-runtime"); + await openDropdown(harness); + await page.getByRole("menuitemradio", { name: ADD_ENTRY }).click(); + + // The sentinel opens the form; it must never become the selection. + await expect(page.getByTestId("add-custom-harness-dialog")).toBeVisible({ + timeout: 8_000, + }); + await expect(harness).not.toContainText(ADD_ENTRY); + + await registerHarness(page); + + // The saved harness is now the selected harness. + await expect(harness).toContainText(HARNESS_LABEL, { timeout: 8_000 }); + }); + + test("dismissing the form leaves the create dialog's harness unchanged", async ({ + page, + }) => { + await installMockBridge(page); + const dialog = await openCreateDialog(page); + + const harness = dialog.locator("#persona-runtime"); + await expect(harness).toBeVisible({ timeout: 10_000 }); + const before = await harness.textContent(); + + await openDropdown(harness); + await page.getByRole("menuitemradio", { name: ADD_ENTRY }).click(); + await expect(page.getByTestId("add-custom-harness-dialog")).toBeVisible({ + timeout: 8_000, + }); + + await page.keyboard.press("Escape"); + await expect( + page.getByTestId("add-custom-harness-dialog"), + ).not.toBeVisible(); + + // No harness was registered, so the prior selection must survive. + await expect(harness).toHaveText(before ?? ""); + }); + + test("definition edit dialog registers a harness inline and selects it", async ({ + page, + }) => { + await installMockBridge(page, { + personas: [ + { + displayName: "Editable Agent", + systemPrompt: "An agent whose harness gets replaced.", + }, + ], + }); + const dialog = await openDefinitionEditDialog(page, "Editable Agent"); + + const harness = dialog.locator("#persona-runtime"); + await openDropdown(harness); + await page.getByRole("menuitemradio", { name: ADD_ENTRY }).click(); + await expect(page.getByTestId("add-custom-harness-dialog")).toBeVisible({ + timeout: 8_000, + }); + + await registerHarness(page); + + await expect(harness).toContainText(HARNESS_LABEL, { timeout: 8_000 }); + }); + + test("instance edit dialog registers a harness inline and keeps Custom command", async ({ + page, + }) => { + await installMockBridge(page, { + managedAgents: [ + { + pubkey: + "npub1e2e00000000000000000000000000000000000000000000000000000000", + name: "Instance Agent", + status: "stopped", + channelNames: ["agents"], + }, + ], + }); + + await page.goto("/", { waitUntil: "domcontentloaded" }); + await page.getByTestId("open-agents-view").click(); + await page + .getByRole("button", { name: "Instance Agent agent profile" }) + .click(); + await expect(page.getByTestId("user-profile-panel")).toBeVisible({ + timeout: 10_000, + }); + await page.getByTestId("user-profile-edit-agent").click(); + await expect(page.getByTestId("edit-agent-dialog")).toBeVisible({ + timeout: 10_000, + }); + + const provider = page.locator("#edit-agent-runtime"); + await openDropdown(provider); + + // "Custom command" is a different feature (ad-hoc command override) and + // must survive alongside the new entry. + await expect( + page.getByRole("menuitemradio", { name: "Custom command" }), + ).toBeVisible(); + await page.getByRole("menuitemradio", { name: ADD_ENTRY }).click(); + await expect(page.getByTestId("add-custom-harness-dialog")).toBeVisible({ + timeout: 8_000, + }); + await expect(provider).not.toContainText(ADD_ENTRY); + + await registerHarness(page); + + await expect(provider).toContainText(HARNESS_LABEL, { timeout: 8_000 }); + }); +}); From 543327262771663074acdabb07ecdaeaf6cc6b1e Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Mon, 27 Jul 2026 21:55:34 -0400 Subject: [PATCH 2/2] fix(desktop): scope pending harness selection to its dialog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit usePendingHarnessSelection held a saved-but-unpublished harness id with no owner lifecycle, and both host dialogs stay mounted across closes (UserProfilePersonaDialogs, UserProfilePanel toggle `open` on retained dialogs). Closing before discovery published the id left it pending, so a later publish selected into form state the close had already reset — or auto-selected an abandoned harness on the next open. A discovery failure kept the id alive indefinitely. The hook now takes the owning dialog's `open` state, gating readiness at render as well as in the effect so a catalog update committed alongside the close cannot slip through. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/scripts/check-file-sizes.mjs | 8 +- .../agents/ui/AgentDefinitionDialog.tsx | 4 +- .../agents/ui/AgentInstanceEditDialog.tsx | 5 +- .../agents/ui/addCustomHarness.test.mjs | 84 ++++++++++++++++++- .../features/agents/ui/addCustomHarness.ts | 19 ++++- .../tests/e2e/inline-custom-harness.spec.ts | 9 +- 6 files changed, 114 insertions(+), 15 deletions(-) diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index a4accdebf7..3983fa591d 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -697,18 +697,18 @@ const overrides = new Map([ // hidden-key projection keeps the top-level secret out of Advanced rows. // +6 (1195 -> 1201): rebase onto main — this PR's model-source label wiring // lands on top of main's dialog growth. Queued to split. - // +25 (1201 -> 1226): inline "Add custom harness…" entry — sentinel option, + // +28 (1201 -> 1229): inline "Add custom harness…" entry — sentinel option, // modal state, and the AddCustomHarnessDialog mount. The shared routing and // deferred-selection logic lives in addCustomHarness.ts to keep this minimal. - ["src/features/agents/ui/AgentInstanceEditDialog.tsx", 1226], + ["src/features/agents/ui/AgentInstanceEditDialog.tsx", 1229], // AgentDefinitionDialog grew past 1000 with the following load-bearing fixes: // isRuntimeAutoSeededRef tracking for edit-mode seeding (Fizz shows models); // runtimeSupportsLlmProviderSelection guard on discovery provider (codex fix); // hideProviderIds computation for Databricks v1 gate. Queued to split. - // +26 (1020 -> 1046): inline "Add custom harness…" entry — sentinel option, + // +28 (1020 -> 1048): inline "Add custom harness…" entry — sentinel option, // modal state, and the AddCustomHarnessDialog mount. The shared routing and // deferred-selection logic lives in addCustomHarness.ts to keep this minimal. - ["src/features/agents/ui/AgentDefinitionDialog.tsx", 1046], + ["src/features/agents/ui/AgentDefinitionDialog.tsx", 1048], // #2630 emoji picker search: the shadow-root search-input autofocus effect // (rAF retry loop) took this file 999 -> 1026 and landed without this entry, // so main's Desktop Core went red. Queued to split with the rest of this list. diff --git a/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx b/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx index d6641e6394..5425131448 100644 --- a/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx +++ b/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx @@ -694,10 +694,12 @@ export function AgentDefinitionDialog({ } // Routed through the normal change handler so a harness registered inline - // resets model/provider exactly as a hand-picked one would. + // resets model/provider exactly as a hand-picked one would. Scoped to `open` + // so a pending id can't outlive the dialog that started the registration. const selectSavedHarness = usePendingHarnessSelection( runtimes, handleRuntimeDropdownChange, + open, ); function handleProviderDropdownChange(nextValue: string) { diff --git a/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx b/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx index 36ceb1214d..f3c410e2ff 100644 --- a/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx +++ b/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx @@ -546,10 +546,13 @@ export function AgentInstanceEditDialog({ } // Routed through the normal change handler so a harness registered inline - // pins its command and resets model/provider like a hand-picked one. + // pins its command and resets model/provider like a hand-picked one. Scoped + // to `open` so a pending id can't outlive the dialog that started the + // registration. const selectSavedHarness = usePendingHarnessSelection( runtimes, handleRuntimeDropdownChange, + open, ); function handleProviderDropdownChange(nextValue: string) { diff --git a/desktop/src/features/agents/ui/addCustomHarness.test.mjs b/desktop/src/features/agents/ui/addCustomHarness.test.mjs index f01d94bb2c..6c0aa32daf 100644 --- a/desktop/src/features/agents/ui/addCustomHarness.test.mjs +++ b/desktop/src/features/agents/ui/addCustomHarness.test.mjs @@ -12,7 +12,9 @@ * the definition file; the harness becomes a catalog entry when the * invalidated discovery query refetches. Selecting on save would pick an * id no entry backs. The hook must wait for the catalog, fire exactly - * once, and stay silent when the user cancels. + * once, stay silent when the user cancels, and drop the pending id when + * its dialog closes — the host dialogs stay mounted, so a stale id would + * otherwise select into reset form state on a later publish. * * The hook is mounted for real (react-dom/client + act) rather than simulated, * so its effect wiring — including the guard that survives the dialogs' @@ -150,8 +152,9 @@ test("no pending id is never ready even against a populated catalog", () => { /** * Mount the real hook over a mutable catalog. Returns the setter the dialogs - * call on save, a `setRuntimes` to simulate the discovery refetch, and the log - * of ids the hook handed back for selection. + * call on save, a `setRuntimes` to simulate the discovery refetch, a `setOpen` + * to simulate the owning dialog closing and reopening, and the log of ids the + * hook handed back for selection. */ async function mountPendingSelection(initialRuntimes = []) { const selected = []; @@ -159,11 +162,13 @@ async function mountPendingSelection(initialRuntimes = []) { function Harness() { const [runtimes, setRuntimes] = React.useState(initialRuntimes); + const [open, setOpen] = React.useState(true); // Deliberately NOT memoized: both dialogs pass a plain function // declaration, so `onReady` has a fresh identity on every render. const onReady = (id) => selected.push(id); - control.save = usePendingHarnessSelection(runtimes, onReady); + control.save = usePendingHarnessSelection(runtimes, onReady, open); control.setRuntimes = setRuntimes; + control.setOpen = setOpen; return null; } @@ -266,3 +271,74 @@ test("two harnesses registered in a row are each selected when published", async await act(async () => root.unmount()); }); + +test("a second save before the first publishes selects only the later harness", async () => { + const { control, root, selected } = await mountPendingSelection([ + { id: "claude" }, + ]); + + // The dropdown holds one harness, so the latest registration wins: the + // first id is dropped rather than queued behind the second. + await act(async () => control.save("first")); + await act(async () => control.save("second")); + await act(async () => + control.setRuntimes([{ id: "claude" }, { id: "first" }, { id: "second" }]), + ); + assert.deepEqual(selected, ["second"]); + + await act(async () => root.unmount()); +}); + +// ── Lifecycle: a pending id never outlives the dialog that created it ──────── + +test("a harness published after its dialog closed is never selected", async () => { + const { control, root, selected } = await mountPendingSelection([ + { id: "claude" }, + ]); + + // Both host dialogs stay mounted when closed, so the hook keeps running. + await act(async () => control.save("my-harness")); + await act(async () => control.setOpen(false)); + await act(async () => + control.setRuntimes([{ id: "claude" }, { id: "my-harness" }]), + ); + + // Selecting here would write into form state the close already reset. + assert.deepEqual(selected, []); + + await act(async () => root.unmount()); +}); + +test("reopening after closing mid-registration does not select the abandoned harness", async () => { + const { control, root, selected } = await mountPendingSelection([ + { id: "claude" }, + ]); + + await act(async () => control.save("my-harness")); + await act(async () => control.setOpen(false)); + await act(async () => + control.setRuntimes([{ id: "claude" }, { id: "my-harness" }]), + ); + // The reopened dialog seeds from its own initial values; a stale pending id + // must not overwrite them. + await act(async () => control.setOpen(true)); + assert.deepEqual(selected, []); + + await act(async () => root.unmount()); +}); + +test("a harness saved after reopening is still selected when published", async () => { + const { control, root, selected } = await mountPendingSelection([ + { id: "claude" }, + ]); + + await act(async () => control.setOpen(false)); + await act(async () => control.setOpen(true)); + await act(async () => control.save("my-harness")); + await act(async () => + control.setRuntimes([{ id: "claude" }, { id: "my-harness" }]), + ); + assert.deepEqual(selected, ["my-harness"]); + + await act(async () => root.unmount()); +}); diff --git a/desktop/src/features/agents/ui/addCustomHarness.ts b/desktop/src/features/agents/ui/addCustomHarness.ts index 465ce47f2a..f9c2143530 100644 --- a/desktop/src/features/agents/ui/addCustomHarness.ts +++ b/desktop/src/features/agents/ui/addCustomHarness.ts @@ -71,19 +71,34 @@ export function readyHarnessId( * Returns the setter to hand the saved id; `onReady` then fires with it, so * callers reuse their normal dropdown-change path instead of growing a second * selection code path. + * + * `active` is the owning dialog's open state. The wait is only meaningful + * while that dialog is open: both host dialogs stay mounted across closes, so + * a pending id would otherwise survive the close and select into reset — or + * hidden — form state whenever discovery caught up. Going inactive both blocks + * `onReady` and drops the pending id, so a later publish is a no-op and + * reopening starts clean. A second save before the first publishes replaces + * it: the field holds one harness, so the latest save wins. */ export function usePendingHarnessSelection( runtimes: ReadonlyArray<{ id: string }>, onReady: (id: string) => void, + active: boolean, ): (id: string) => void { const [pendingId, setPendingId] = React.useState(null); - const readyId = readyHarnessId(runtimes, pendingId); + // Gated at render, not just in the effect, so a catalog update landing in + // the same commit as the close cannot slip a selection through. + const readyId = active ? readyHarnessId(runtimes, pendingId) : null; React.useEffect(() => { + if (!active) { + setPendingId(null); + return; + } if (readyId === null) return; setPendingId(null); onReady(readyId); - }, [onReady, readyId]); + }, [active, onReady, readyId]); return setPendingId; } diff --git a/desktop/tests/e2e/inline-custom-harness.spec.ts b/desktop/tests/e2e/inline-custom-harness.spec.ts index b5f7c64a21..6b3845d65a 100644 --- a/desktop/tests/e2e/inline-custom-harness.spec.ts +++ b/desktop/tests/e2e/inline-custom-harness.spec.ts @@ -6,10 +6,13 @@ * path existed. The harness dropdowns now carry the entry directly. * * Covers, on all three surfaces (create, edit definition, edit instance): - * - the entry is present in the harness dropdown - * - choosing it opens the registration form without changing the selection + * - choosing the entry opens the registration form * - saving registers the harness and selects it in the dropdown - * - dismissing the form leaves the previous selection untouched + * + * Create and instance edit additionally assert the sentinel never becomes the + * selection; create alone covers dismissing the form leaving the previous + * selection untouched. The three surfaces share the same routing, so those + * checks are not repeated on every one. */ import { expect, test } from "@playwright/test";