diff --git a/app/src/hooks/useProjects.test.ts b/app/src/hooks/useProjects.test.ts new file mode 100644 index 0000000..1130479 --- /dev/null +++ b/app/src/hooks/useProjects.test.ts @@ -0,0 +1,142 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { act, renderHook } from "@testing-library/react"; +import { useProjects } from "./useProjects"; +import { useAppState } from "../store/appState"; +import type { Project, ProjectStatus } from "../lib/types"; + +const startProjectContainer = vi.fn(); +const stopProjectContainer = vi.fn(); +const rebuildProjectContainer = vi.fn(); +const listProjects = vi.fn(); + +vi.mock("../lib/tauri-commands", () => ({ + startProjectContainer: (id: string) => startProjectContainer(id), + stopProjectContainer: (id: string) => stopProjectContainer(id), + rebuildProjectContainer: (id: string) => rebuildProjectContainer(id), + listProjects: () => listProjects(), + addProject: vi.fn(), + removeProject: vi.fn(), + updateProject: vi.fn(), +})); + +const project = (status: ProjectStatus): Project => + ({ id: "p1", name: "whp", status }) as unknown as Project; + +/** The status the sidebar row and Project Home both read. */ +const statusOf = () => useAppState.getState().projects.find((p) => p.id === "p1")?.status; + +beforeEach(() => { + vi.clearAllMocks(); + useAppState.setState({ projects: [project("running")] }); +}); + +/** + * The optimistic status is what makes a click move the row immediately. It is + * also what strands the row when the command it was betting on never ran. + * + * Since every lifecycle command started taking the per-project lock and failing + * fast, all three can be refused *before* the backend changes anything — and + * `stop` could not fail this way at all before, because it took no exclusion. + * `isTransitioning` disables both Start and Stop, and the only thing that + * clears it is `reconcileProjectStatuses`, which runs once when Docker first + * appears. So a stale optimistic status is not a cosmetic problem: it is a row + * the user cannot operate again until the app is restarted. + */ +describe("useProjects puts the status back when a refused command never ran", () => { + const refusal = + "This project's snapshot is being compacted. Wait for it to finish before starting or recreating its container."; + + it("does not leave a refused Stop showing 'stopping' forever", async () => { + stopProjectContainer.mockRejectedValue(refusal); + // The lock is taken before `update_status`, so the backend still holds the + // truth — which is why re-reading it is the correction, not a guess. + listProjects.mockResolvedValue([project("running")]); + + const { result } = renderHook(() => useProjects()); + await act(async () => { + await expect(result.current.stop("p1")).rejects.toBe(refusal); + }); + + expect(statusOf()).toBe("running"); + }); + + it("does not leave a refused Start showing 'starting' forever", async () => { + useAppState.setState({ projects: [project("stopped")] }); + startProjectContainer.mockRejectedValue(refusal); + listProjects.mockResolvedValue([project("stopped")]); + + const { result } = renderHook(() => useProjects()); + await act(async () => { + await expect(result.current.start("p1")).rejects.toBe(refusal); + }); + + expect(statusOf()).toBe("stopped"); + }); + + it("does not leave a refused Reset showing 'starting' forever", async () => { + rebuildProjectContainer.mockRejectedValue(refusal); + listProjects.mockResolvedValue([project("running")]); + + const { result } = renderHook(() => useProjects()); + await act(async () => { + await expect(result.current.rebuild("p1")).rejects.toBe(refusal); + }); + + expect(statusOf()).toBe("running"); + }); + + it("falls back to what was on screen when even the re-read fails", async () => { + // Two failures in a row must not land on the one state there is no way out + // of. The backend's answer is preferred, but "unknown" is never a reason to + // keep showing a transition that is not happening. + stopProjectContainer.mockRejectedValue(refusal); + listProjects.mockRejectedValue("Docker is not running"); + + const { result } = renderHook(() => useProjects()); + await act(async () => { + await expect(result.current.stop("p1")).rejects.toBe(refusal); + }); + + expect(statusOf()).toBe("running"); + }); + + it("prefers the backend's answer over the status it captured", async () => { + // A start that dies half-way really has changed the world, so the captured + // status would be a lie. `listProjects` is the thing that knows. + useAppState.setState({ projects: [project("stopped")] }); + startProjectContainer.mockRejectedValue("container exited during startup"); + listProjects.mockResolvedValue([project("error")]); + + const { result } = renderHook(() => useProjects()); + await act(async () => { + await expect(result.current.start("p1")).rejects.toBe( + "container exited during startup", + ); + }); + + expect(statusOf()).toBe("error"); + }); + + it("still paints the optimistic status on the way in", async () => { + let release: (() => void) | undefined; + stopProjectContainer.mockReturnValue( + new Promise((resolve) => { + release = resolve; + }), + ); + listProjects.mockResolvedValue([project("stopped")]); + + const { result } = renderHook(() => useProjects()); + let pending: Promise | undefined; + act(() => { + pending = result.current.stop("p1"); + }); + expect(statusOf()).toBe("stopping"); + + await act(async () => { + release?.(); + await pending; + }); + expect(statusOf()).toBe("stopped"); + }); +}); diff --git a/app/src/hooks/useProjects.ts b/app/src/hooks/useProjects.ts index ba5a442..66868a0 100644 --- a/app/src/hooks/useProjects.ts +++ b/app/src/hooks/useProjects.ts @@ -2,7 +2,7 @@ import { useCallback } from "react"; import { useShallow } from "zustand/react/shallow"; import { useAppState } from "../store/appState"; import * as commands from "../lib/tauri-commands"; -import type { ProjectPath } from "../lib/types"; +import type { ProjectPath, ProjectStatus } from "../lib/types"; export function useProjects() { const { @@ -61,34 +61,85 @@ export function useProjects() { [updateProjectInList], ); - const start = useCallback( - async (id: string) => { - setOptimisticStatus(id, "starting"); - const updated = await commands.startProjectContainer(id); - updateProjectInList(updated); - return updated; + /** + * Paint the optimistic status, run the command, and **put the status back if + * the command never happened.** + * + * The optimistic write exists so a click moves the row immediately. It used + * to be safe to leave in place on failure, because the only way these three + * commands could fail was after the backend had already started changing + * things — so a stale "starting" was at worst premature. + * + * That stopped being true when every lifecycle command started taking the + * per-project lock and failing fast: a compaction, a reset or another start + * holding the project now refuses `start`, `stop` **and** `rebuild` before + * one byte of state changes. `stop` in particular could not fail this way at + * all before — it took no exclusion. The optimistic paint then has nothing to + * become, and `isTransitioning` disables both Start and Stop, so the row is + * stuck: the only thing that clears it is `reconcileProjectStatuses`, which + * runs once, from `App.tsx`, when Docker first appears. A restart. + * + * Re-reading the list is preferred over restoring what was on screen, because + * a refusal is not the only way these throw — a start that dies half-way + * really has changed the world, and `listProjects` is the thing that knows. + * The captured status is only the fallback for when that call fails too: + * leaving the row transitioning is the one outcome the user cannot get out + * of, so it must not be what a second failure lands on. + */ + const withOptimisticStatus = useCallback( + async ( + id: string, + status: "starting" | "stopping", + run: () => Promise, + ): Promise => { + const previous: ProjectStatus | null = + useAppState.getState().projects.find((p) => p.id === id)?.status ?? null; + setOptimisticStatus(id, status); + try { + return await run(); + } catch (e) { + try { + setProjects(await commands.listProjects()); + } catch { + const project = useAppState.getState().projects.find((p) => p.id === id); + if (project && previous) updateProjectInList({ ...project, status: previous }); + } + // Rethrown unchanged: `useProjectActions` is what turns this into a + // toast, and it must still see the original failure. + throw e; + } }, - [updateProjectInList, setOptimisticStatus], + [setOptimisticStatus, setProjects, updateProjectInList], + ); + + const start = useCallback( + (id: string) => + withOptimisticStatus(id, "starting", async () => { + const updated = await commands.startProjectContainer(id); + updateProjectInList(updated); + return updated; + }), + [updateProjectInList, withOptimisticStatus], ); const stop = useCallback( - async (id: string) => { - setOptimisticStatus(id, "stopping"); - await commands.stopProjectContainer(id); - const list = await commands.listProjects(); - setProjects(list); - }, - [setProjects, setOptimisticStatus], + (id: string) => + withOptimisticStatus(id, "stopping", async () => { + await commands.stopProjectContainer(id); + const list = await commands.listProjects(); + setProjects(list); + }), + [setProjects, withOptimisticStatus], ); const rebuild = useCallback( - async (id: string) => { - setOptimisticStatus(id, "starting"); - const updated = await commands.rebuildProjectContainer(id); - updateProjectInList(updated); - return updated; - }, - [updateProjectInList, setOptimisticStatus], + (id: string) => + withOptimisticStatus(id, "starting", async () => { + const updated = await commands.rebuildProjectContainer(id); + updateProjectInList(updated); + return updated; + }), + [updateProjectInList, withOptimisticStatus], ); const update = useCallback( diff --git a/app/src/lib/types.ts b/app/src/lib/types.ts index cda7312..691dadf 100644 --- a/app/src/lib/types.ts +++ b/app/src/lib/types.ts @@ -52,7 +52,13 @@ export interface Project { * certificate file or a directory of them). null falls back to * `AppSettings.ca_cert_path`. Changing it recreates the container. */ ca_cert_path: string | null; - git_token: string | null; + /** **Write-only.** Rust marks this `#[serde(skip_serializing)]`, so it is + * absent from every project the backend hands back — reading it gives + * `undefined`, never `null`. Optional here so that is the type, and so a + * `=== null` test against it is a compile error rather than a branch that + * silently never runs. Still sent on the way *in*: this is how the secret + * is set. */ + git_token?: string | null; git_user_name: string | null; git_user_email: string | null; custom_env_vars: EnvVar[]; @@ -96,11 +102,23 @@ export type BedrockAuthMethod = "static_credentials" | "profile" | "bearer_token export interface BedrockConfig { auth_method: BedrockAuthMethod; aws_region: string; - aws_access_key_id: string | null; - aws_secret_access_key: string | null; - aws_session_token: string | null; + /** **Write-only.** Rust marks this `#[serde(skip_serializing)]`, so it is + * absent from every project the backend hands back — reading it gives + * `undefined`, never `null`. Optional here so that is the type, and so a + * `=== null` test against it is a compile error rather than a branch that + * silently never runs. Still sent on the way *in*: this is how the secret + * is set. */ + aws_access_key_id?: string | null; + aws_secret_access_key?: string | null; + aws_session_token?: string | null; aws_profile: string | null; - aws_bearer_token: string | null; + /** **Write-only.** Rust marks this `#[serde(skip_serializing)]`, so it is + * absent from every project the backend hands back — reading it gives + * `undefined`, never `null`. Optional here so that is the type, and so a + * `=== null` test against it is a compile error rather than a branch that + * silently never runs. Still sent on the way *in*: this is how the secret + * is set. */ + aws_bearer_token?: string | null; model_id: string | null; disable_prompt_caching: boolean; service_tier: string | null; @@ -127,7 +145,13 @@ export interface LlamaCppConfig { * implement the **Anthropic** Messages API — e.g. LiteLLM. */ export interface OpenAiCompatibleConfig { base_url: string; - api_key: string | null; + /** **Write-only.** Rust marks this `#[serde(skip_serializing)]`, so it is + * absent from every project the backend hands back — reading it gives + * `undefined`, never `null`. Optional here so that is the type, and so a + * `=== null` test against it is a compile error rather than a branch that + * silently never runs. Still sent on the way *in*: this is how the secret + * is set. */ + api_key?: string | null; model_id: string | null; /** See `OllamaConfig.haiku_model_id`. */ haiku_model_id: string | null;