diff --git a/desktop/src/features/onboarding/ui/RuntimeErrorTooltip.tsx b/desktop/src/features/onboarding/ui/RuntimeErrorTooltip.tsx index 15708d3087..6a3d8ef319 100644 --- a/desktop/src/features/onboarding/ui/RuntimeErrorTooltip.tsx +++ b/desktop/src/features/onboarding/ui/RuntimeErrorTooltip.tsx @@ -42,7 +42,9 @@ export function RuntimeErrorTooltip({ side="bottom" sideOffset={12} > - {detail} + + {detail} + ); diff --git a/desktop/src/features/onboarding/ui/SetupStep.tsx b/desktop/src/features/onboarding/ui/SetupStep.tsx index 96e7322d14..aa02486706 100644 --- a/desktop/src/features/onboarding/ui/SetupStep.tsx +++ b/desktop/src/features/onboarding/ui/SetupStep.tsx @@ -463,19 +463,56 @@ function RuntimeAuthError({ runtime }: { runtime: AcpRuntimeCatalogEntry }) { } function RuntimeCard({ - installError, - isInstalling, - onInstall, + installResults, + onInstallResultsChange, runtime, }: { - installError: string | null; - isInstalling: boolean; - onInstall: () => void; + installResults: InstallResultsState; + onInstallResultsChange: React.Dispatch< + React.SetStateAction + >; runtime: AcpRuntimeCatalogEntry; }) { + // Each card owns its own mutation instance so concurrent installs on + // different cards each track their own isPending state and callbacks + // independently (react-query v5 per-mutate callbacks only fire for the + // latest mutate() call on a shared instance, silently dropping earlier ones). + const installMutation = useInstallAcpRuntimeMutation(); + const installError = installResults[runtime.id]?.error ?? null; + const isInstalling = installMutation.isPending; const isAvailable = runtime.availability === "available"; const isReady = runtimeIsReadyForOnboarding(runtime); + function handleInstall() { + onInstallResultsChange((current) => ({ + ...current, + [runtime.id]: { error: null, success: false }, + })); + + installMutation.mutate(runtime.id, { + onSuccess: (result) => { + onInstallResultsChange((current) => ({ + ...current, + [runtime.id]: result.success + ? { error: null, success: true } + : { + error: getInstallErrorMessage(result.steps), + success: false, + }, + })); + }, + onError: (error) => { + onInstallResultsChange((current) => ({ + ...current, + [runtime.id]: { + error: error instanceof Error ? error.message : "Install failed.", + success: false, + }, + })); + }, + }); + } + return ( {!isAvailable && runtimeDetailText(runtime) ? ( @@ -517,7 +554,7 @@ function RuntimeCard({ {installError ? ( ({ - ...current, - [runtimeId]: { error: null, success: false }, - })); - - installMutation.mutate(runtimeId, { - onSuccess: (result) => { - onInstallResultsChange((current) => ({ - ...current, - [runtimeId]: result.success - ? { error: null, success: true } - : { error: getInstallErrorMessage(result.steps), success: false }, - })); - }, - onError: (error) => { - onInstallResultsChange((current) => ({ - ...current, - [runtimeId]: { - error: error instanceof Error ? error.message : "Install failed.", - success: false, - }, - })); - }, - }); - } return (
@@ -606,13 +615,9 @@ function RuntimeProvidersSection({
{orderedItems.map((runtime) => ( handleInstall(runtime.id)} + onInstallResultsChange={onInstallResultsChange} runtime={runtime} /> ))} diff --git a/desktop/src/features/settings/ui/DoctorSettingsPanel.tsx b/desktop/src/features/settings/ui/DoctorSettingsPanel.tsx index cee156ece7..7a0186a7ba 100644 --- a/desktop/src/features/settings/ui/DoctorSettingsPanel.tsx +++ b/desktop/src/features/settings/ui/DoctorSettingsPanel.tsx @@ -279,22 +279,56 @@ function RuntimeHeader({ } function RuntimeRow({ - installError, - installSuccess, - isInstalling, - onInstall, + resetEpoch, runtime, }: { - installError: string | null; - installSuccess: boolean; - isInstalling: boolean; - onInstall: () => void; + resetEpoch: number; runtime: AcpRuntimeCatalogEntry; }) { const [terminalLaunchMethodId, setTerminalLaunchMethodId] = React.useState< string | null >(null); const [isUpdateWarningOpen, setIsUpdateWarningOpen] = React.useState(false); + // Each row owns its mutation instance so concurrent installs each track + // their own isPending / result state independently. + const installMutation = useInstallAcpRuntimeMutation(); + const [installResult, setInstallResult] = React.useState<{ + success: boolean; + error: string | null; + } | null>(null); + // Clear stale install results when the parent triggers a catalog refresh + // (Check again) — the runtime may now be healthy and stale failure state + // would linger because keyed rows don't remount on refetch. + // biome-ignore lint/correctness/useExhaustiveDependencies: resetEpoch is an intentional trigger only; its value is not consumed in the effect body + React.useEffect(() => { + setInstallResult(null); + }, [resetEpoch]); + const isInstalling = installMutation.isPending; + const installError = installResult?.error ?? null; + const installSuccess = installResult?.success ?? false; + + function handleInstall() { + setInstallResult(null); + installMutation.mutate(runtime.id, { + onSuccess: (result) => { + if (result.success) { + setInstallResult({ success: true, error: null }); + } else { + setInstallResult({ + success: false, + error: getInstallErrorMessage(result.steps), + }); + } + }, + onError: (error) => { + setInstallResult({ + success: false, + error: error instanceof Error ? error.message : "Install failed.", + }); + }, + }); + } + const canConnectAccount = runtime.availability === "available" && runtime.authStatus.status === "logged_out"; @@ -352,7 +386,7 @@ function RuntimeRow({ setIsUpdateWarningOpen(true); return; } - onInstall(); + handleInstall(); }} runtime={runtime} /> @@ -372,7 +406,10 @@ function RuntimeRow({

) : null} {installError ? ( -

+

{installError}

) : null} @@ -410,7 +447,7 @@ function RuntimeRow({ Cancel Update @@ -491,59 +528,9 @@ export function DoctorSettingsPanel() { [runtimesQuery.data], ); const isRefreshing = runtimesQuery.isFetching; - const installMutation = useInstallAcpRuntimeMutation(); - const [installResults, setInstallResults] = React.useState< - Record - >({}); - // Per-runtime installing state: tracks which runtime IDs have an in-flight - // install so concurrent installs each show their own spinner correctly. - const [installingIds, setInstallingIds] = React.useState>( - new Set(), - ); - - function handleInstall(runtimeId: string) { - // Clear any previous result for this runtime before retrying. - setInstallResults((prev) => ({ - ...prev, - [runtimeId]: { success: false, error: null }, - })); - setInstallingIds((prev) => new Set(prev).add(runtimeId)); - - installMutation.mutate(runtimeId, { - onSuccess: (result) => { - if (result.success) { - setInstallResults((prev) => ({ - ...prev, - [runtimeId]: { success: true, error: null }, - })); - } else { - setInstallResults((prev) => ({ - ...prev, - [runtimeId]: { - success: false, - error: getInstallErrorMessage(result.steps), - }, - })); - } - }, - onError: (error) => { - setInstallResults((prev) => ({ - ...prev, - [runtimeId]: { - success: false, - error: error instanceof Error ? error.message : "Install failed.", - }, - })); - }, - onSettled: () => { - setInstallingIds((prev) => { - const next = new Set(prev); - next.delete(runtimeId); - return next; - }); - }, - }); - } + // Incremented each time the user clicks "Check again" so RuntimeRow + // useEffect clears stale install results from before the refresh. + const [resetEpoch, setResetEpoch] = React.useState(0); return (
{ - setInstallResults({}); + setResetEpoch((e) => e + 1); void runtimesQuery.refetch(); void gitBashQuery.refetch(); }} @@ -598,11 +585,8 @@ export function DoctorSettingsPanel() {
{runtimes.map((runtime) => ( handleInstall(runtime.id)} + resetEpoch={resetEpoch} runtime={runtime} /> ))} diff --git a/desktop/src/shared/lib/installError.test.mjs b/desktop/src/shared/lib/installError.test.mjs index 232ac178cc..c6b51186a8 100644 --- a/desktop/src/shared/lib/installError.test.mjs +++ b/desktop/src/shared/lib/installError.test.mjs @@ -67,3 +67,47 @@ test("getInstallErrorMessage: failed step with empty stderr falls back to stdout ]); assert.match(message, /some stdout output/); }); + +test("getInstallErrorMessage: hint and step detail are separated by double newline for whitespace-pre-line rendering", () => { + const hint = "Git Bash is required. Install it from git-scm.com."; + const message = getInstallErrorMessage([ + { + step: "shell", + command: "bash -l -c 'npm install'", + success: false, + stdout: "", + stderr: "bash: command not found", + exitCode: 127, + hint, + }, + ]); + assert.ok( + message.includes("\n\n"), + "hint and step detail should be separated by a blank line", + ); + assert.ok(message.startsWith(hint)); +}); + +test("getInstallErrorMessage: only reports the last (failing) step when multiple steps present", () => { + const message = getInstallErrorMessage([ + { + step: "node", + command: "node --version", + success: true, + stdout: "v20.0.0", + stderr: "", + exitCode: 0, + }, + { + step: "adapter", + command: "npm install -g @agentclientprotocol/claude-code-acp", + success: false, + stdout: "", + stderr: "npm ERR! code E404", + exitCode: 1, + }, + ]); + assert.match(message, /Step "adapter" failed:/); + assert.match(message, /npm ERR! code E404/); + assert.doesNotMatch(message, /Step "node"/); +}); diff --git a/desktop/src/testing/e2eBridge.ts b/desktop/src/testing/e2eBridge.ts index 9ad4059a05..aa841d2765 100644 --- a/desktop/src/testing/e2eBridge.ts +++ b/desktop/src/testing/e2eBridge.ts @@ -185,6 +185,18 @@ type E2eConfig = { * Call N returns results[N]; when exhausted the last entry repeats. * Takes precedence over `installAcpRuntimeResult`. */ installAcpRuntimeResults?: RawInstallRuntimeResult[]; + /** Per-runtime install configuration keyed by runtimeId. + * When a runtimeId matches, its entry overrides the global + * installAcpRuntime* fields for that specific runtime. */ + installAcpRuntimeByRuntime?: Record< + string, + { + delayMs?: number; + result?: RawInstallRuntimeResult; + /** Call-order sequence — same semantics as installAcpRuntimeResults. */ + results?: RawInstallRuntimeResult[]; + } + >; managedAgentPrereqs?: { acp?: MockCommandAvailability; mcp?: MockCommandAvailability; @@ -7011,6 +7023,8 @@ async function handleConnectAcpRuntime( // Per-page install call counter. Reset each test run because this module is // re-evaluated via addInitScript, so the counter starts at 0 for every test. let installCallCount = 0; +/** Per-runtime call counters for `installAcpRuntimeByRuntime` sequences. */ +const installCallCountByRuntime: Record = {}; let addChannelMembersCallCount = 0; let mockGlobalAgentConfig: { env_vars: Record; @@ -7031,6 +7045,31 @@ async function handleInstallAcpRuntime( }, config: E2eConfig | undefined, ): Promise { + const runtimeId = args.runtimeId ?? ""; + const perRuntime = config?.mock?.installAcpRuntimeByRuntime?.[runtimeId]; + + if (perRuntime) { + const delayMs = perRuntime.delayMs ?? 0; + if (delayMs > 0) { + await new Promise((resolve) => window.setTimeout(resolve, delayMs)); + } + const seq = perRuntime.results; + if (seq && seq.length > 0) { + const idx = Math.min( + installCallCountByRuntime[runtimeId] ?? 0, + seq.length - 1, + ); + installCallCountByRuntime[runtimeId] = idx + 1; + const result = seq[idx]; + if (result.success) mockInstallCompleted = true; + return result; + } + if (perRuntime.result) { + if (perRuntime.result.success) mockInstallCompleted = true; + return perRuntime.result; + } + } + const delayMs = config?.mock?.installAcpRuntimeDelayMs ?? 0; if (delayMs > 0) { await new Promise((resolve) => window.setTimeout(resolve, delayMs)); diff --git a/desktop/tests/e2e/doctor-states.spec.ts b/desktop/tests/e2e/doctor-states.spec.ts index 1a724f719a..dd2cf83286 100644 --- a/desktop/tests/e2e/doctor-states.spec.ts +++ b/desktop/tests/e2e/doctor-states.spec.ts @@ -735,4 +735,133 @@ test.describe("Doctor panel state screenshots", () => { await expect(loading).toBeVisible(); await expect(loading).toContainText("Codex installing"); }); + + /** + * 08 — concurrent installs each keep their own spinner/result state; + * stale install failure is cleared when Check again fires (F1 fix). + * + * Flow: + * - Claude (400ms delay) → failure + * - Codex (100ms delay) → success + * Both started before either settles. + * After both settle: claude shows failure, codex shows success banner. + * Click Check again → both rows lose stale state (claude error gone). + */ + test("08-concurrent-installs-and-stale-clear", async ({ page }) => { + await installMockBridge(page, { + acpRuntimesCatalog: [ + { + ...CLAUDE_AVAILABLE_LOGGED_IN, + availability: "adapter_missing", + command: null, + binary_path: null, + can_auto_install: true, + auth_status: { status: "unknown" }, + }, + { + ...CODEX_NOT_INSTALLED, + can_auto_install: true, + node_required: false, + }, + GOOSE_AVAILABLE, + BUZZ_AGENT_AVAILABLE, + ], + installAcpRuntimeByRuntime: { + claude: { + delayMs: 400, + result: { + success: false, + steps: [ + { + step: "adapter", + command: "npm install -g @agentclientprotocol/claude-agent-acp", + success: false, + stdout: "", + stderr: + "npm ERR! code EACCES\nnpm ERR! syscall mkdir\nnpm ERR! path /usr/local\n\nHint: Check prefix permissions.", + exit_code: 1, + }, + ], + }, + }, + codex: { + delayMs: 100, + result: { + success: true, + steps: [ + { + step: "adapter", + command: "npm install -g @zed-industries/codex-acp", + success: true, + stdout: "added 1 package", + stderr: "", + exit_code: 0, + }, + ], + }, + }, + }, + // After the catalog refresh (triggered by a successful install or Check + // again), all runtimes report healthy so stale errors must clear. + acpRuntimesCatalogAfterInstall: [ + { + ...CLAUDE_AVAILABLE_LOGGED_IN, + availability: "available", + }, + { + ...CODEX_NOT_INSTALLED, + availability: "available", + command: "codex-acp", + binary_path: "/usr/local/bin/codex-acp", + auth_status: { status: "logged_in" }, + }, + GOOSE_AVAILABLE, + BUZZ_AGENT_AVAILABLE, + ], + }); + + await page.goto("/", { waitUntil: "domcontentloaded" }); + await openSettings(page, "agents"); + + const claudeRow = page.getByTestId("doctor-runtime-claude"); + const codexRow = page.getByTestId("doctor-runtime-codex"); + await expect(claudeRow).toBeVisible({ timeout: 10_000 }); + await expect(codexRow).toBeVisible(); + + const claudeToggle = page.getByTestId("doctor-runtime-toggle-claude"); + const codexToggle = page.getByTestId("doctor-runtime-toggle-codex"); + + // Start both installs before either settles. + await claudeToggle.click(); + await codexToggle.click(); + + // Codex settles first (shorter delay): toggle flips on, no error on codex. + // The catalog refresh triggered by codex's success immediately returns + // availability === "available", so the transient "installed. Checking..." + // banner is replaced by the stable isOn state — assert the toggle instead. + await expect(codexToggle).toBeChecked({ timeout: 3_000 }); + await expect( + page.getByTestId("doctor-runtime-install-error-codex"), + ).toHaveCount(0); + + // Claude settles (after its longer delay): failure error visible with + // multiline stderr. Codex toggle must still be on — unaffected by claude. + const claudeError = page.getByTestId("doctor-runtime-install-error-claude"); + await expect(claudeError).toBeVisible({ timeout: 3_000 }); + await expect(claudeError).toContainText("npm ERR!"); + await expect(codexToggle).toBeChecked(); + + // Click Check again — epoch increments, RuntimeRow useEffect clears + // local installResult state, so the stale claude error disappears. + await page.getByRole("button", { name: "Check again" }).click(); + await expect(claudeError).toHaveCount(0, { timeout: 5_000 }); + // Codex toggle stays on (catalog still reports available after refresh). + await expect(codexToggle).toBeChecked({ timeout: 5_000 }); + + await claudeRow.scrollIntoViewIfNeeded(); + await waitForAnimations(page); + await claudeRow.screenshot({ + path: `${SHOTS}/08-concurrent-installs-and-stale-clear.png`, + }); + }); }); diff --git a/desktop/tests/e2e/onboarding-agent-defaults.spec.ts b/desktop/tests/e2e/onboarding-agent-defaults.spec.ts index f8e6322ed3..e1d8b11b6f 100644 --- a/desktop/tests/e2e/onboarding-agent-defaults.spec.ts +++ b/desktop/tests/e2e/onboarding-agent-defaults.spec.ts @@ -635,3 +635,170 @@ test("defaults requires a choice when multiple visible harnesses are ready", asy await expect(page.getByTestId("onboarding-finish")).toBeEnabled(); await expect.poll(() => readSavedRuntime(page)).toBe("codex"); }); + +/** + * Two installs started concurrently — claude fails with a multiline error + * (rich hint+stderr in the tooltip) while codex succeeds. Each card must + * keep its own independent spinner and its own terminal result; neither card + * may show the other's outcome. + * + * This is the behavioral regression test for the per-card mutation fix + * (Bug B) and the multiline tooltip fix (Bug A / F3 from Thufir pass 1). + */ +test("concurrent installs each keep their own state — one fails, one succeeds", async ({ + page, +}) => { + // Realistic 512-head + 1024-tail shape: many short lines followed by one + // long unbroken Windows path. This exercises both overflow axes: + // • vertical: enough lines to exceed max-h-48 (192px at ~16px/line) + // • horizontal: the long path has no spaces, so only break-words prevents + // scrollWidth > clientWidth. + const longWindowsPath = + "C:\\Users\\willp\\AppData\\Roaming\\npm\\node_modules\\@agentclientprotocol\\claude-agent-acp\\dist\\bin\\claude-agent-acp.exe"; + const multilineError = [ + "npm ERR! code EACCES", + "npm ERR! syscall mkdir", + "npm ERR! path C:\\Users\\willp\\AppData\\Roaming\\npm", + "npm ERR! errno -4048", + "npm ERR! Error: EACCES: permission denied, mkdir 'C:\\Users\\willp\\AppData\\Roaming\\npm'", + "npm ERR! { [Error: EACCES: permission denied, mkdir 'C:\\Users\\willp\\AppData\\Roaming\\npm']", + "npm ERR! errno: -4048,", + "npm ERR! code: 'EACCES',", + "npm ERR! syscall: 'mkdir',", + "npm ERR! path: 'C:\\\\Users\\\\willp\\\\AppData\\\\Roaming\\\\npm' }", + "npm ERR!", + "npm ERR! The operation was rejected by your operating system.", + "npm ERR! It is likely you do not have the permissions to access this file as the current user", + "npm ERR!", + `npm ERR! If you believe this might be a permissions issue, please double-check the`, + `npm ERR! permissions of the file and its containing directories, or try running`, + `npm ERR! the command again as root/Administrator.`, + "", + `Hint: Run as Administrator or change npm prefix: npm config set prefix ${longWindowsPath}`, + ].join("\n"); + const claudeNotInstalled = runtime("claude", "adapter_missing", { + status: "unknown", + }); + const codexNotInstalled = runtime("codex", "adapter_missing", { + status: "unknown", + }); + await installMockBridge( + page, + { + acpRuntimesCatalog: [claudeNotInstalled, codexNotInstalled], + // Claude: long delay then failure with multiline stderr + hint. + // Codex: short delay then success. + // Per-runtime config lets both be in flight simultaneously. + installAcpRuntimeByRuntime: { + claude: { + delayMs: 600, + result: { + success: false, + steps: [ + { + step: "adapter", + command: "npm install -g @agentclientprotocol/claude-agent-acp", + success: false, + stdout: "", + stderr: multilineError, + exit_code: 1, + }, + ], + }, + }, + codex: { + delayMs: 200, + result: { + success: true, + steps: [ + { + step: "adapter", + command: "npm install -g @zed-industries/codex-acp", + success: true, + stdout: "added 1 package", + stderr: "", + exit_code: 0, + }, + ], + }, + }, + }, + acpRuntimesCatalogAfterInstall: [ + runtime("claude", "adapter_missing", { status: "unknown" }), + runtime("codex", "available", { status: "logged_in" }), + ], + }, + { skipCommunitySeed: true, skipOnboardingSeed: true }, + ); + await page.goto("/"); + await navigateToSetupPage(page); + + const claudeInstall = page.getByTestId("onboarding-runtime-install-claude"); + const codexInstall = page.getByTestId("onboarding-runtime-install-codex"); + + // Start both installs before either settles. + await claudeInstall.click(); + await codexInstall.click(); + + // While in flight: both install buttons must be absent (no duplicate clicks). + await expect(claudeInstall).toHaveCount(0); + await expect(codexInstall).toHaveCount(0); + + // Codex settles first (shorter delay): success indicator, no error. + await expect(page.getByTestId("onboarding-runtime-ready-codex")).toBeVisible({ + timeout: 3_000, + }); + await expect(page.getByTestId("onboarding-runtime-error-codex")).toHaveCount( + 0, + ); + + // Claude still in flight: its install button must still be absent. + await expect(claudeInstall).toHaveCount(0); + + // Claude settles: failure error visible; codex still shows ready (not reset). + const claudeError = page.getByTestId("onboarding-runtime-error-claude"); + await expect(claudeError).toBeVisible({ timeout: 3_000 }); + await expect( + page.getByTestId("onboarding-runtime-ready-codex"), + ).toBeVisible(); + await expect(page.getByTestId("onboarding-runtime-error-codex")).toHaveCount( + 0, + ); + + // The error trigger has the full aria-label (label + detail). + await expect(claudeError).toHaveAttribute("aria-label", /npm ERR!/); + // Open the tooltip and verify the detail span handles overflow correctly: + // • vertical overflow exists and is scrollable (max-h-48 + overflow-y-auto) + // • no horizontal overflow (break-words forces the long unbroken path to wrap) + await claudeError.focus(); + const tooltip = page.getByRole("tooltip"); + await expect(tooltip).toBeVisible({ timeout: 2_000 }); + await expect(tooltip).toContainText("npm ERR! code EACCES"); + await expect(tooltip).toContainText("Hint: Run as Administrator"); + + // Locate the scroll container using page-level locator since Radix portals + // can place content outside the tooltip role element's subtree in the DOM. + // Use .first() because Radix keeps a hidden duplicate in the light DOM. + const detailSpan = page.locator("span.overflow-y-auto").first(); + await expect(detailSpan).toBeVisible(); + + // Vertical: scrollHeight must exceed clientHeight (content taller than max-h-48). + // Scroll position must advance when set, proving scrollability. + const isVerticallyScrollable = await detailSpan.evaluate((el) => { + return el.scrollHeight > el.clientHeight; + }); + expect(isVerticallyScrollable).toBe(true); + + // Confirm scroll position can actually advance. + await detailSpan.evaluate((el) => { + el.scrollTop = 9999; + }); + const scrolledDown = await detailSpan.evaluate((el) => el.scrollTop > 0); + expect(scrolledDown).toBe(true); + + // Horizontal: break-words must prevent horizontal overflow. + const hasHorizontalOverflow = await detailSpan.evaluate((el) => { + return el.scrollWidth > el.clientWidth; + }); + expect(hasHorizontalOverflow).toBe(false); +});