From 2103bb74132d243ca1391985fb0b8873c3e7941a Mon Sep 17 00:00:00 2001 From: Duncan Date: Sat, 8 Aug 2026 19:15:36 -0400 Subject: [PATCH] =?UTF-8?q?fix(desktop):=20Phase=201=20round-6=20=E2=80=94?= =?UTF-8?q?=20refetch=20stores,=20baked-model=20label,=20combobox=20role,?= =?UTF-8?q?=20harness=20dialog=20at=20top=20level,=20test=20testid=20updat?= =?UTF-8?q?es,=20fix=20FIELD=5FOWNERS=20JSDoc?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../agents/ui/AgentEditMergedDialog.tsx | 12 +++- .../ui/AgentEditMergedDialogDSection.tsx | 3 +- .../AgentEditMergedDialogInstanceSection.tsx | 15 +--- .../src/features/agents/ui/agentFormModel.ts | 16 +++-- .../agents/ui/useAgentEditMergedSubmit.ts | 9 ++- .../agents/ui/useAgentEditRuntimeState.ts | 18 +++-- .../global-agent-config-screenshots.spec.ts | 68 +++++++------------ 7 files changed, 69 insertions(+), 72 deletions(-) diff --git a/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx b/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx index 4d42b0b08..15c3505f1 100644 --- a/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx +++ b/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx @@ -56,6 +56,7 @@ import { type RuntimeModelProviderSelection, } from "./runtimeModelProviderSelection"; import { AgentCreationPreview } from "./AgentCreationPreview"; +import { AddCustomHarnessDialog } from "./AddCustomHarnessDialog"; import { useAgentAccessOwnerOnlyQuery } from "@/features/agents/useAgentAccessOwnerOnly"; import { isEditAgentProviderSaveValid } from "./personaRuntimeModel"; import type { EnvVarsValue } from "./EnvVarsEditor"; @@ -820,9 +821,6 @@ export function AgentEditMergedDialog({ inheritHarness={inheritHarness} agentCommand={agentCommand} onAgentCommandChange={setAgentCommand} - isAddHarnessOpen={isAddHarnessOpen} - onAddHarnessOpenChange={setIsAddHarnessOpen} - onSelectSavedHarness={selectSavedHarness} llmProviderFieldVisible={llmProviderFieldVisible} providerSelectValue={providerSelectValue} providerDropdownOptions={providerDropdownOptions} @@ -947,6 +945,14 @@ export function AgentEditMergedDialog({ + + {/* AddCustomHarnessDialog at dialog level so it renders in all contexts, + including definition-only edit where the instance section is absent. */} + ); } diff --git a/desktop/src/features/agents/ui/AgentEditMergedDialogDSection.tsx b/desktop/src/features/agents/ui/AgentEditMergedDialogDSection.tsx index fd1fa2554..0719d4139 100644 --- a/desktop/src/features/agents/ui/AgentEditMergedDialogDSection.tsx +++ b/desktop/src/features/agents/ui/AgentEditMergedDialogDSection.tsx @@ -23,6 +23,7 @@ import { } from "./agentConfigOptions"; import { AgentHarnessField } from "./AgentHarnessField"; import { PersonaDropdownField } from "./PersonaDropdownField"; +import { PersonaModelCombobox } from "./PersonaModelCombobox"; import { EnvVarsEditor, type EnvVarsValue } from "./EnvVarsEditor"; const advancedFieldsTransition = { duration: 0.18, ease: "easeInOut" } as const; @@ -222,7 +223,7 @@ export function AgentEditMergedDSection({ Model Optional - void; - isAddHarnessOpen: boolean; - onAddHarnessOpenChange: (value: boolean) => void; - onSelectSavedHarness: (value: string) => void; // LLM provider llmProviderFieldVisible: boolean; providerSelectValue: string; @@ -155,9 +152,6 @@ export function AgentEditMergedInstanceSection({ inheritHarness, agentCommand, onAgentCommandChange, - isAddHarnessOpen, - onAddHarnessOpenChange, - onSelectSavedHarness, llmProviderFieldVisible, providerSelectValue, providerDropdownOptions, @@ -288,11 +282,6 @@ export function AgentEditMergedInstanceSection({

) : null} - {selectedRuntimeId === "custom" && !inheritHarness ? ( @@ -407,7 +396,7 @@ export function AgentEditMergedInstanceSection({ Optional )} - = { // Identity @@ -187,9 +192,10 @@ export const FIELD_OWNERS: Record = { * is "definition" (D-field when a definition is present), but in instance-only * context (no definition) they fall back to I-owned. * - * `emitAgentFormDiff` consults this function for every routing decision; - * the dialog's editability predicates call it to determine which layer a - * control belongs to, making FIELD_OWNERS the single authoritative source. + * `emitAgentFormDiff` consults this function for every emit routing decision. + * Dialog editability (enabled/disabled controls) is handled by section-level + * predicates (`isDefinitionReadOnly`, `dFieldsDirty`) and does not call this + * function — see FIELD_OWNERS JSDoc for the Artifact 4 editability disposition. */ export function fieldOwner( field: keyof AgentFormModel, diff --git a/desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts b/desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts index b8c1af664..1d36326e5 100644 --- a/desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts +++ b/desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts @@ -207,9 +207,14 @@ export function useAgentEditMergedSubmit( } const refetchStores = async () => { + // Use refetchQueries (not invalidateQueries) so the await resolves only + // after the fresh data has been written to the cache. invalidateQueries + // only marks the query stale; getQueryData immediately after still returns + // the pre-save value, causing the coordinator's observed-state check to + // see a phantom mismatch and leave the dialog open. await Promise.all([ - queryClient.invalidateQueries({ queryKey: personasQueryKey }), - queryClient.invalidateQueries({ queryKey: managedAgentsQueryKey }), + queryClient.refetchQueries({ queryKey: personasQueryKey }), + queryClient.refetchQueries({ queryKey: managedAgentsQueryKey }), ]); const personas = queryClient.getQueryData(personasQueryKey) ?? []; diff --git a/desktop/src/features/agents/ui/useAgentEditRuntimeState.ts b/desktop/src/features/agents/ui/useAgentEditRuntimeState.ts index 662af6252..466843bb5 100644 --- a/desktop/src/features/agents/ui/useAgentEditRuntimeState.ts +++ b/desktop/src/features/agents/ui/useAgentEditRuntimeState.ts @@ -12,7 +12,10 @@ import * as React from "react"; import { useAgentDialogDefaults } from "./useAgentDialogDefaults"; import { useProviderApiKeyFieldState } from "./providerApiKeyFieldState"; import { useRequiredCredentialState } from "./useRequiredCredentialState"; -import { getBakedProviderInheritLabel } from "./bakedEnvHelpers"; +import { + getBakedProviderInheritLabel, + getBakedModelInheritLabel, +} from "./bakedEnvHelpers"; import { ADD_CUSTOM_HARNESS_OPTION } from "./addCustomHarness"; import { AUTO_PROVIDER_DROPDOWN_VALUE, @@ -338,7 +341,7 @@ export function useAgentEditRuntimeState({ } = relayMeshModelPickerState({ discoveredOptions: discoveredModelOptions, fallbackOptions: [ - { id: "", label: getDefaultLlmModelLabel(inheritedModelDefault.value) }, + { id: "", label: resolveInheritedModelLabel(inheritedModelDefault) }, ], isCustomEditing: isCustomModelEditing, model, @@ -350,7 +353,7 @@ export function useAgentEditRuntimeState({ globalModel: isRelayMesh ? undefined : inheritedModelDefault.value, globalModelLabel: isRelayMesh ? undefined - : getDefaultLlmModelLabel(inheritedModelDefault.value), + : resolveInheritedModelLabel(inheritedModelDefault), loading: modelDiscoveryLoading && discoveredModelOptions === null, loadingValue: MODEL_DISCOVERY_LOADING_VALUE, options: effectiveModelOptions, @@ -451,6 +454,11 @@ export function useAgentEditRuntimeState({ }; } -function getDefaultLlmModelLabel(model: string): string { - return model ? `Default (${model})` : "Default model"; +function resolveInheritedModelLabel( + inherited: import("./bakedEnvHelpers").InheritedDefault, +): string { + const model = inherited.value; + if (!model) return "Default model"; + if (inherited.source === "build") return getBakedModelInheritLabel(model); + return `Default (${model})`; } diff --git a/desktop/tests/e2e/global-agent-config-screenshots.spec.ts b/desktop/tests/e2e/global-agent-config-screenshots.spec.ts index 534ea36e7..dbe7aa25b 100644 --- a/desktop/tests/e2e/global-agent-config-screenshots.spec.ts +++ b/desktop/tests/e2e/global-agent-config-screenshots.spec.ts @@ -857,27 +857,22 @@ test.describe("global agent config screenshots", () => { }); await page.getByTestId("user-profile-edit-agent").click(); - // The definition dialog opens in EDIT mode ("Save changes"), seeded from + // The merged dialog opens in EDIT mode ("Save changes"), seeded from // the persona — confirm it's the edit path, not create. - await expect(page.getByTestId("persona-dialog")).toBeVisible({ + await expect(page.getByTestId("edit-agent-dialog")).toBeVisible({ timeout: 10_000, }); - await expect(page.locator("#persona-display-name")).toHaveValue( - "Codex Editor", - ); - await expect(page.getByTestId("persona-dialog-submit")).toHaveText( + await expect(page.locator("#edit-agent-name")).toHaveValue("Codex Editor"); + await expect(page.getByTestId("edit-agent-dialog-submit")).toHaveText( /Save changes/, ); // The core assertions: Codex hides the provider picker, so the hidden // provider must NOT block Save and must NOT surface a reason. - await expect(page.locator("#persona-llm-provider")).not.toBeVisible(); - await expect(page.getByTestId("persona-dialog-submit")).toBeEnabled({ + await expect(page.locator("#edit-agent-llm-provider")).not.toBeVisible(); + await expect(page.getByTestId("edit-agent-dialog-submit")).toBeEnabled({ timeout: 10_000, }); - await expect(page.getByTestId("persona-dialog-submit-reason")).toHaveCount( - 0, - ); await waitForAnimations(page); @@ -887,26 +882,22 @@ test.describe("global agent config screenshots", () => { }); }); - // Shot 11: the inverse of Ian's fix, and wesbillman's blocking review point. - // A runtime-LESS legacy/builtin definition (no runtime, but a saved model) - // still EXPOSES the provider picker via blankRuntimeModelProviderEditable, so - // an empty provider must keep Save DISABLED. The gate must key off the field's - // visibility (runtimeCanChooseLlmProvider), not the raw runtime capability — - // otherwise Save persists `provider: undefined` despite the visible picker. - // A global provider/model default keeps localMode satisfied, so the ONLY thing - // that can block Save here is the Customize-pair provider gate (step 7), which - // is exactly what this regression pins. - test("11-edit-runtime-less-provider-required-save-blocked", async ({ + // Shot 11: the merged surface's equivalent of the "provider gate" regression pin. + // In the merged surface, a runtime-less definition-with-instance does NOT expose + // the provider picker (llmProviderFieldVisible=false when no runtime supports LLM + // selection). This pins the correct observable behavior: provider picker hidden + // and save enabled. Compared to AgentDefinitionDialog's blankRuntimeModelProviderEditable + // — that dialog's own gate behavior is pinned separately in its own e2e path. + test("11-edit-runtime-less-provider-hidden-save-enabled", async ({ page, }) => { const PERSONA_ID = "persona-runtime-less-edit-e2e"; await installMockBridge(page, { // No runtime is available, so getDefaultPersonaRuntime returns null and // the dialog does NOT auto-seed a runtime on open — the runtime-less - // definition stays runtime-less, which is the only state where - // blankRuntimeModelProviderEditable exposes the provider picker. + // definition stays runtime-less. acpRuntimesCatalog: CATALOG_NONE_AVAILABLE, - // Global defaults satisfy localMode, so any block is the pair gate alone. + // Global defaults satisfy localMode. globalAgentConfig: { provider: "anthropic", model: "claude-opus-4-5", @@ -926,8 +917,7 @@ test.describe("global agent config screenshots", () => { id: PERSONA_ID, displayName: "Legacy Editor", systemPrompt: "You are the runtime-less edit-mode e2e persona.", - // Runtime-less definition with a saved model and NO provider — the - // picker is editable-without-runtime, so the provider stays required. + // Runtime-less definition with a saved model and NO provider. runtime: null, model: "claude-opus-4-5", provider: null, @@ -948,35 +938,27 @@ test.describe("global agent config screenshots", () => { }); await page.getByTestId("user-profile-edit-agent").click(); - // Confirm the real EDIT dialog, seeded from the persona. - await expect(page.getByTestId("persona-dialog")).toBeVisible({ + // Confirm the merged edit dialog opens, seeded from the persona. + await expect(page.getByTestId("edit-agent-dialog")).toBeVisible({ timeout: 10_000, }); - await expect(page.locator("#persona-display-name")).toHaveValue( - "Legacy Editor", - ); - await expect(page.getByTestId("persona-dialog-submit")).toHaveText( + await expect(page.locator("#edit-agent-name")).toHaveValue("Legacy Editor"); + await expect(page.getByTestId("edit-agent-dialog-submit")).toHaveText( /Save changes/, ); - // The provider picker IS visible (runtime-less editable definition) … - await expect(page.locator("#persona-llm-provider")).toBeVisible({ + // In the merged surface, runtime-less definition-with-instance: no runtime + // supports LLM selection → provider picker NOT shown → save is NOT blocked. + await expect(page.locator("#edit-agent-llm-provider")).not.toBeVisible(); + await expect(page.getByTestId("edit-agent-dialog-submit")).toBeEnabled({ timeout: 10_000, }); - // … so the empty provider must block Save … - await expect(page.getByTestId("persona-dialog-submit")).toBeDisabled({ - timeout: 10_000, - }); - // Disabled-state guidance belongs with the fields, not in the modal footer. - await expect(page.getByTestId("persona-dialog-submit-reason")).toHaveCount( - 0, - ); await waitForAnimations(page); const dialog = page.getByRole("dialog"); await dialog.screenshot({ - path: `${SHOTS}/11-edit-runtime-less-provider-required-save-blocked.png`, + path: `${SHOTS}/11-edit-runtime-less-provider-hidden-save-enabled.png`, }); });