fix(onboarding): show real install errors and fix concurrent install state (#2658)

Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Co-authored-by: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
This commit is contained in:
Will Pfleger
2026-07-24 09:12:08 -07:00
committed by GitHub
parent 596386ee55
commit 9731cd818b
7 changed files with 482 additions and 112 deletions

View File

@@ -42,7 +42,9 @@ export function RuntimeErrorTooltip({
side="bottom"
sideOffset={12}
>
<span className="leading-4">{detail}</span>
<span className="block max-h-48 overflow-y-auto overflow-x-hidden break-words whitespace-pre-line leading-4">
{detail}
</span>
</TooltipContent>
</Tooltip>
);

View File

@@ -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<InstallResultsState>
>;
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 (
<Card
className={cn(
@@ -499,7 +536,7 @@ function RuntimeCard({
<RuntimeStatus
installError={installError}
isInstalling={isInstalling}
onInstall={onInstall}
onInstall={handleInstall}
runtime={runtime}
/>
{!isAvailable && runtimeDetailText(runtime) ? (
@@ -517,7 +554,7 @@ function RuntimeCard({
{installError ? (
<RuntimeErrorTooltip
className="absolute inset-x-3 bottom-2 flex min-w-0 items-center justify-center gap-1.5 overflow-hidden whitespace-nowrap text-xs leading-4 text-destructive"
detail="Installation couldn’t be completed. Try again."
detail={installError}
label="Installation failed"
showIcon
testId={`onboarding-runtime-error-${runtime.id}`}
@@ -560,34 +597,6 @@ function RuntimeProvidersSection({
}) {
const { errorMessage, isChecking, items } = runtimeProviders;
const orderedItems = getVisibleOnboardingRuntimes(items);
const installMutation = useInstallAcpRuntimeMutation();
function handleInstall(runtimeId: string) {
onInstallResultsChange((current) => ({
...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 (
<section className="flex min-h-full w-full flex-col items-center">
@@ -606,13 +615,9 @@ function RuntimeProvidersSection({
<div className="grid min-w-0 w-full max-w-[592px] grid-cols-1 gap-4 md:grid-cols-2">
{orderedItems.map((runtime) => (
<RuntimeCard
installError={installResults[runtime.id]?.error ?? null}
isInstalling={
installMutation.isPending &&
installMutation.variables === runtime.id
}
installResults={installResults}
key={runtime.id}
onInstall={() => handleInstall(runtime.id)}
onInstallResultsChange={onInstallResultsChange}
runtime={runtime}
/>
))}

View File

@@ -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({
</p>
) : null}
{installError ? (
<p className="mt-2 whitespace-pre-line rounded-lg border border-destructive/30 bg-destructive/10 px-3 py-1.5 text-sm text-destructive">
<p
className="mt-2 whitespace-pre-line rounded-lg border border-destructive/30 bg-destructive/10 px-3 py-1.5 text-sm text-destructive"
data-testid={`doctor-runtime-install-error-${runtime.id}`}
>
{installError}
</p>
) : null}
@@ -410,7 +447,7 @@ function RuntimeRow({
<AlertDialogFooter>
<AlertDialogCancel>Cancel</AlertDialogCancel>
<AlertDialogAction
onClick={onInstall}
onClick={handleInstall}
data-testid={`doctor-runtime-confirm-update-${runtime.id}`}
>
Update
@@ -491,59 +528,9 @@ export function DoctorSettingsPanel() {
[runtimesQuery.data],
);
const isRefreshing = runtimesQuery.isFetching;
const installMutation = useInstallAcpRuntimeMutation();
const [installResults, setInstallResults] = React.useState<
Record<string, { success: boolean; error: string | null }>
>({});
// 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<Set<string>>(
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 (
<section
@@ -558,7 +545,7 @@ export function DoctorSettingsPanel() {
<Button
disabled={isRefreshing}
onClick={() => {
setInstallResults({});
setResetEpoch((e) => e + 1);
void runtimesQuery.refetch();
void gitBashQuery.refetch();
}}
@@ -598,11 +585,8 @@ export function DoctorSettingsPanel() {
<div className="space-y-3" data-testid="doctor-runtime-list">
{runtimes.map((runtime) => (
<RuntimeRow
installError={installResults[runtime.id]?.error ?? null}
installSuccess={installResults[runtime.id]?.success ?? false}
isInstalling={installingIds.has(runtime.id)}
key={runtime.id}
onInstall={() => handleInstall(runtime.id)}
resetEpoch={resetEpoch}
runtime={runtime}
/>
))}

View File

@@ -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"/);
});

View File

@@ -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;
@@ -7060,6 +7072,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<string, number> = {};
let addChannelMembersCallCount = 0;
let mockGlobalAgentConfig: {
env_vars: Record<string, string>;
@@ -7080,6 +7094,31 @@ async function handleInstallAcpRuntime(
},
config: E2eConfig | undefined,
): Promise<RawInstallRuntimeResult> {
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));

View File

@@ -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`,
});
});
});

View File

@@ -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);
});