fix(first-run): découple busy de la détection des CLI (#28)
Même backend réparé, le wizard restait à la merci d'une détection qui ne répond pas : `reload()` et `detect()` awaitaient `detectProfiles` sous le même drapeau `busy` qui grise « Save and continue » et « Detect installed CLIs ». Une promesse IPC jamais résolue laissait donc les deux boutons grisés à vie — sans issue pour l'utilisateur. La détection redevient ce qu'elle est : une étape best-effort, jamais bloquante. - `busy` ne garde plus que ce dont le wizard ne peut pas se passer (`firstRunState`) ou qui mute l'état (`configureProfiles`). Il ne dépend plus jamais de `detectProfiles`. L'invariant est documenté en tête de module. - Un drapeau `detecting` distinct suit la sonde et n'inhibe aucune action. Il est relâché par un timer (`DETECT_TIMEOUT_MS`), jamais par la seule promesse : celle-ci peut rester pendante indéfiniment. - Un identifiant de tour monotone (`detectRun`) invalide les tours périmés et ceux qui survivent au démontage, évitant un `setState` hors montage. - `reload()` rend les lignes immédiatement puis lance la détection en tâche détachée. Si elle échoue, les lignes restent cochables à la main ; seul le bouton explicite remonte l'erreur. Tests: vitest 59 fichiers / 569 tests verts, tsc --noEmit exit 0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@ -6,6 +6,7 @@
|
|||||||
*/
|
*/
|
||||||
import { describe, it, expect, vi } from "vitest";
|
import { describe, it, expect, vi } from "vitest";
|
||||||
import {
|
import {
|
||||||
|
act,
|
||||||
render,
|
render,
|
||||||
screen,
|
screen,
|
||||||
waitFor,
|
waitFor,
|
||||||
@ -13,9 +14,11 @@ import {
|
|||||||
} from "@testing-library/react";
|
} from "@testing-library/react";
|
||||||
|
|
||||||
import { MockProfileGateway } from "@/adapters/mock";
|
import { MockProfileGateway } from "@/adapters/mock";
|
||||||
|
import type { ProfileAvailability } from "@/domain";
|
||||||
import type { Gateways } from "@/ports";
|
import type { Gateways } from "@/ports";
|
||||||
import { DIProvider } from "@/app/di";
|
import { DIProvider } from "@/app/di";
|
||||||
import { FirstRunWizard } from "./FirstRunWizard";
|
import { FirstRunWizard } from "./FirstRunWizard";
|
||||||
|
import { DETECT_TIMEOUT_MS } from "./useFirstRun";
|
||||||
|
|
||||||
function renderWizard(
|
function renderWizard(
|
||||||
profile: MockProfileGateway = new MockProfileGateway(),
|
profile: MockProfileGateway = new MockProfileGateway(),
|
||||||
@ -301,6 +304,69 @@ describe("FirstRunWizard — OpenAI-compatible local/LAN profile (ticket #14)",
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// Ticket #28 — the wizard must survive a detection that never answers. When the
|
||||||
|
// backend `detect_profiles` command panics, the `invoke` promise is neither
|
||||||
|
// resolved nor rejected: nothing after `await detectProfiles(...)` ever runs.
|
||||||
|
// `busy` must therefore never be tied to detection, or both buttons stay greyed
|
||||||
|
// out for good and the first run cannot be completed.
|
||||||
|
describe("FirstRunWizard — detection that never answers (ticket #28)", () => {
|
||||||
|
/** A gateway whose `detectProfiles` promise stays pending forever. */
|
||||||
|
class HangingDetectGateway extends MockProfileGateway {
|
||||||
|
override detectProfiles(): Promise<ProfileAvailability[]> {
|
||||||
|
return new Promise<ProfileAvailability[]>(() => {});
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
it("keeps Save and Detect enabled once firstRunState answered", async () => {
|
||||||
|
renderWizard(new HangingDetectGateway());
|
||||||
|
await waitForLoaded();
|
||||||
|
|
||||||
|
// The rows are rendered, so the blocking load is over: both actions are live.
|
||||||
|
const save = screen.getByRole("button", { name: "Save and continue" });
|
||||||
|
const detect = screen.getByRole("button", { name: "Detect installed CLIs" });
|
||||||
|
expect((save as HTMLButtonElement).disabled).toBe(false);
|
||||||
|
expect((detect as HTMLButtonElement).disabled).toBe(false);
|
||||||
|
|
||||||
|
// Detection is merely reported as in flight, never as a blocking state.
|
||||||
|
expect(screen.getByRole("status").textContent).toMatch(/detecting/i);
|
||||||
|
|
||||||
|
// Re-probing by hand must not freeze them either.
|
||||||
|
fireEvent.click(detect);
|
||||||
|
expect((save as HTMLButtonElement).disabled).toBe(false);
|
||||||
|
expect((detect as HTMLButtonElement).disabled).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("still finishes the first run while detection hangs", async () => {
|
||||||
|
const profile = new HangingDetectGateway();
|
||||||
|
const { onDone } = renderWizard(profile, vi.fn());
|
||||||
|
await waitForLoaded();
|
||||||
|
|
||||||
|
// Nothing got pre-checked (detection never answered): tick a profile by hand.
|
||||||
|
fireEvent.click(screen.getByLabelText("use Claude Code"));
|
||||||
|
fireEvent.click(screen.getByRole("button", { name: "Save and continue" }));
|
||||||
|
|
||||||
|
await waitFor(() => expect(onDone).toHaveBeenCalled());
|
||||||
|
const saved = await profile.listProfiles();
|
||||||
|
expect(saved.map((p) => p.command)).toEqual(["claude"]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("drops the detecting flag on timeout even if the promise never settles", async () => {
|
||||||
|
vi.useFakeTimers({ shouldAdvanceTime: true });
|
||||||
|
try {
|
||||||
|
renderWizard(new HangingDetectGateway());
|
||||||
|
await waitForLoaded();
|
||||||
|
expect(screen.getByRole("status").textContent).toMatch(/detecting/i);
|
||||||
|
|
||||||
|
await act(async () => {
|
||||||
|
vi.advanceTimersByTime(DETECT_TIMEOUT_MS + 1);
|
||||||
|
});
|
||||||
|
expect(screen.queryByRole("status")).toBeNull();
|
||||||
|
} finally {
|
||||||
|
vi.useRealTimers();
|
||||||
|
}
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
describe("FirstRunWizard reopening after the first run (forceOpen)", () => {
|
describe("FirstRunWizard reopening after the first run (forceOpen)", () => {
|
||||||
/** A gateway whose first run is already done (profiles configured). */
|
/** A gateway whose first run is already done (profiles configured). */
|
||||||
async function configuredGateway() {
|
async function configuredGateway() {
|
||||||
|
|||||||
@ -78,6 +78,8 @@ export function FirstRunWizard({
|
|||||||
)}
|
)}
|
||||||
|
|
||||||
<Toolbar>
|
<Toolbar>
|
||||||
|
{/* `detecting` deliberately does NOT disable this button (ticket #28):
|
||||||
|
detection is best-effort and may never answer. */}
|
||||||
<Button
|
<Button
|
||||||
onClick={() => void vm.detect()}
|
onClick={() => void vm.detect()}
|
||||||
disabled={vm.busy}
|
disabled={vm.busy}
|
||||||
@ -85,6 +87,11 @@ export function FirstRunWizard({
|
|||||||
>
|
>
|
||||||
Detect installed CLIs
|
Detect installed CLIs
|
||||||
</Button>
|
</Button>
|
||||||
|
{vm.detecting && (
|
||||||
|
<span role="status" className="text-xs text-muted">
|
||||||
|
Detecting…
|
||||||
|
</span>
|
||||||
|
)}
|
||||||
</Toolbar>
|
</Toolbar>
|
||||||
|
|
||||||
<ul className="flex list-none flex-col gap-3 p-0">
|
<ul className="flex list-none flex-col gap-3 p-0">
|
||||||
|
|||||||
@ -3,9 +3,17 @@
|
|||||||
* (L5). Owns the wizard state (selectable reference candidates + selection +
|
* (L5). Owns the wizard state (selectable reference candidates + selection +
|
||||||
* availability) and the actions. Consumes the {@link ProfileGateway}
|
* availability) and the actions. Consumes the {@link ProfileGateway}
|
||||||
* exclusively — never `invoke()` — so the feature is testable with a mock.
|
* exclusively — never `invoke()` — so the feature is testable with a mock.
|
||||||
|
*
|
||||||
|
* **Invariant (ticket #28)**: `busy` gates only the requests the wizard cannot
|
||||||
|
* render without (`firstRunState`) or that mutate state (`configureProfiles`).
|
||||||
|
* It NEVER depends on `detectProfiles`: CLI detection is a detached, best-effort
|
||||||
|
* step tracked by the separate {@link FirstRunViewModel.detecting} flag, bounded
|
||||||
|
* by {@link DETECT_TIMEOUT_MS}. A detection that panics backend-side (leaving the
|
||||||
|
* `invoke` promise forever pending) must not freeze "Save and continue" /
|
||||||
|
* "Detect installed CLIs".
|
||||||
*/
|
*/
|
||||||
|
|
||||||
import { useCallback, useEffect, useState } from "react";
|
import { useCallback, useEffect, useRef, useState } from "react";
|
||||||
|
|
||||||
import type {
|
import type {
|
||||||
AgentProfile,
|
AgentProfile,
|
||||||
@ -15,6 +23,9 @@ import type {
|
|||||||
} from "@/domain";
|
} from "@/domain";
|
||||||
import { useGateways } from "@/app/di";
|
import { useGateways } from "@/app/di";
|
||||||
|
|
||||||
|
/** How long the UI waits for a detection round before giving up on it. */
|
||||||
|
export const DETECT_TIMEOUT_MS = 3_000;
|
||||||
|
|
||||||
/** One row in the wizard: a candidate profile, selected state, availability. */
|
/** One row in the wizard: a candidate profile, selected state, availability. */
|
||||||
export interface WizardEntry {
|
export interface WizardEntry {
|
||||||
profile: AgentProfile;
|
profile: AgentProfile;
|
||||||
@ -31,8 +42,10 @@ export interface FirstRunViewModel {
|
|||||||
entries: WizardEntry[];
|
entries: WizardEntry[];
|
||||||
/** Last error message, or null. */
|
/** Last error message, or null. */
|
||||||
error: string | null;
|
error: string | null;
|
||||||
/** Whether a request is in flight. */
|
/** Whether a blocking request is in flight (load / save). Never detection. */
|
||||||
busy: boolean;
|
busy: boolean;
|
||||||
|
/** Whether a best-effort detection round is in flight. Never disables actions. */
|
||||||
|
detecting: boolean;
|
||||||
/** Toggles selection of a candidate by id. */
|
/** Toggles selection of a candidate by id. */
|
||||||
toggle: (id: string) => void;
|
toggle: (id: string) => void;
|
||||||
/** Replaces a candidate's profile (edited command/args/injection). */
|
/** Replaces a candidate's profile (edited command/args/injection). */
|
||||||
@ -60,41 +73,101 @@ export function useFirstRun(): FirstRunViewModel {
|
|||||||
const [entries, setEntries] = useState<WizardEntry[]>([]);
|
const [entries, setEntries] = useState<WizardEntry[]>([]);
|
||||||
const [error, setError] = useState<string | null>(null);
|
const [error, setError] = useState<string | null>(null);
|
||||||
const [busy, setBusy] = useState(false);
|
const [busy, setBusy] = useState(false);
|
||||||
|
const [detecting, setDetecting] = useState(false);
|
||||||
|
|
||||||
|
/** Monotonic id of the latest detection round; older rounds are ignored. */
|
||||||
|
const detectRun = useRef(0);
|
||||||
|
const detectTimer = useRef<ReturnType<typeof setTimeout> | null>(null);
|
||||||
|
|
||||||
|
useEffect(
|
||||||
|
() => () => {
|
||||||
|
// Unmount: invalidate any in-flight round (its promise may never settle).
|
||||||
|
detectRun.current += 1;
|
||||||
|
if (detectTimer.current !== null) clearTimeout(detectTimer.current);
|
||||||
|
},
|
||||||
|
[],
|
||||||
|
);
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Best-effort detection round, detached from `busy`. A round is abandoned
|
||||||
|
* after {@link DETECT_TIMEOUT_MS} — the underlying promise may stay pending
|
||||||
|
* forever (backend panic ⇒ no IPC response), so `detecting` is released by a
|
||||||
|
* timer, never by the promise alone. `preselect` ticks the installed CLIs (the
|
||||||
|
* auto-detection on open); the manual button only refreshes availability.
|
||||||
|
*/
|
||||||
|
const runDetection = useCallback(
|
||||||
|
async (
|
||||||
|
candidates: AgentProfile[],
|
||||||
|
opts: { preselect: boolean; reportError: boolean },
|
||||||
|
) => {
|
||||||
|
const run = detectRun.current + 1;
|
||||||
|
detectRun.current = run;
|
||||||
|
const stale = () => detectRun.current !== run;
|
||||||
|
|
||||||
|
setDetecting(true);
|
||||||
|
if (detectTimer.current !== null) clearTimeout(detectTimer.current);
|
||||||
|
detectTimer.current = setTimeout(() => {
|
||||||
|
if (!stale()) setDetecting(false);
|
||||||
|
}, DETECT_TIMEOUT_MS);
|
||||||
|
|
||||||
|
try {
|
||||||
|
const results: ProfileAvailability[] =
|
||||||
|
await profile.detectProfiles(candidates);
|
||||||
|
if (stale()) return;
|
||||||
|
const byId = new Map(results.map((r) => [r.profile.id, r.available]));
|
||||||
|
setEntries((prev) =>
|
||||||
|
prev.map((e) => {
|
||||||
|
const available = byId.get(e.profile.id) ?? e.available;
|
||||||
|
return {
|
||||||
|
...e,
|
||||||
|
available,
|
||||||
|
selected: opts.preselect ? available === true : e.selected,
|
||||||
|
};
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
} catch (e) {
|
||||||
|
// Detection unavailable: rows stay as they are (the user can tick them
|
||||||
|
// by hand and re-run). Only the explicit button surfaces the failure.
|
||||||
|
if (stale()) return;
|
||||||
|
if (opts.reportError) setError(describe(e));
|
||||||
|
} finally {
|
||||||
|
if (!stale()) {
|
||||||
|
if (detectTimer.current !== null) clearTimeout(detectTimer.current);
|
||||||
|
detectTimer.current = null;
|
||||||
|
setDetecting(false);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
},
|
||||||
|
[profile],
|
||||||
|
);
|
||||||
|
|
||||||
const reload = useCallback(async () => {
|
const reload = useCallback(async () => {
|
||||||
setBusy(true);
|
setBusy(true);
|
||||||
setError(null);
|
setError(null);
|
||||||
|
let refs: AgentProfile[] | null = null;
|
||||||
try {
|
try {
|
||||||
const state: FirstRunState = await profile.firstRunState();
|
const state: FirstRunState = await profile.firstRunState();
|
||||||
setIsFirstRun(state.isFirstRun);
|
setIsFirstRun(state.isFirstRun);
|
||||||
const refs = state.referenceProfiles;
|
refs = state.referenceProfiles;
|
||||||
// Show the rows immediately (nothing pre-checked yet).
|
// Show the rows immediately (nothing pre-checked yet).
|
||||||
setEntries(
|
setEntries(
|
||||||
refs.map((p) => ({ profile: p, selected: false, available: null })),
|
refs.map((p) => ({ profile: p, selected: false, available: null })),
|
||||||
);
|
);
|
||||||
// Auto-detect once on open: only the CLIs actually installed end up
|
|
||||||
// pre-checked. The manual "Detect installed CLIs" button stays available to
|
|
||||||
// re-probe. If detection is unavailable, rows stay unchecked (the user can
|
|
||||||
// still tick them by hand and re-run detection).
|
|
||||||
try {
|
|
||||||
const results = await profile.detectProfiles(refs);
|
|
||||||
const byId = new Map(results.map((r) => [r.profile.id, r.available]));
|
|
||||||
setEntries(
|
|
||||||
refs.map((p) => {
|
|
||||||
const available = byId.get(p.id) ?? null;
|
|
||||||
return { profile: p, selected: available === true, available };
|
|
||||||
}),
|
|
||||||
);
|
|
||||||
} catch {
|
|
||||||
/* detection unavailable: leave rows unchecked */
|
|
||||||
}
|
|
||||||
} catch (e) {
|
} catch (e) {
|
||||||
setError(describe(e));
|
setError(describe(e));
|
||||||
setIsFirstRun(false);
|
setIsFirstRun(false);
|
||||||
} finally {
|
} finally {
|
||||||
|
// Released as soon as the rows are rendered — detection is not awaited.
|
||||||
setBusy(false);
|
setBusy(false);
|
||||||
}
|
}
|
||||||
}, [profile]);
|
|
||||||
|
// Auto-detect once on open, detached: only the CLIs actually installed end
|
||||||
|
// up pre-checked. The manual "Detect installed CLIs" button stays available
|
||||||
|
// to re-probe, whatever happens to this round.
|
||||||
|
if (refs !== null) {
|
||||||
|
void runDetection(refs, { preselect: true, reportError: false });
|
||||||
|
}
|
||||||
|
}, [profile, runDetection]);
|
||||||
|
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
void reload();
|
void reload();
|
||||||
@ -119,25 +192,12 @@ export function useFirstRun(): FirstRunViewModel {
|
|||||||
}, []);
|
}, []);
|
||||||
|
|
||||||
const detect = useCallback(async () => {
|
const detect = useCallback(async () => {
|
||||||
setBusy(true);
|
|
||||||
setError(null);
|
setError(null);
|
||||||
try {
|
await runDetection(
|
||||||
const candidates = entries.map((e) => e.profile);
|
entries.map((e) => e.profile),
|
||||||
const results: ProfileAvailability[] =
|
{ preselect: false, reportError: true },
|
||||||
await profile.detectProfiles(candidates);
|
|
||||||
const byId = new Map(results.map((r) => [r.profile.id, r.available]));
|
|
||||||
setEntries((prev) =>
|
|
||||||
prev.map((e) => ({
|
|
||||||
...e,
|
|
||||||
available: byId.get(e.profile.id) ?? e.available,
|
|
||||||
})),
|
|
||||||
);
|
);
|
||||||
} catch (e) {
|
}, [runDetection, entries]);
|
||||||
setError(describe(e));
|
|
||||||
} finally {
|
|
||||||
setBusy(false);
|
|
||||||
}
|
|
||||||
}, [profile, entries]);
|
|
||||||
|
|
||||||
const finish = useCallback(async () => {
|
const finish = useCallback(async () => {
|
||||||
setBusy(true);
|
setBusy(true);
|
||||||
@ -158,6 +218,7 @@ export function useFirstRun(): FirstRunViewModel {
|
|||||||
entries,
|
entries,
|
||||||
error,
|
error,
|
||||||
busy,
|
busy,
|
||||||
|
detecting,
|
||||||
toggle,
|
toggle,
|
||||||
updateProfile,
|
updateProfile,
|
||||||
remove,
|
remove,
|
||||||
|
|||||||
Reference in New Issue
Block a user