diff --git a/Taskfile.yml b/Taskfile.yml index b9363951a..04e9669df 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -80,8 +80,12 @@ tasks: dir: pkg/agent/tunnel cmd: | # sudo apt install protobuf-compiler - go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@v1.5.1 - go install google.golang.org/protobuf/cmd/protoc-gen-go@v1.36.4 + # Versions must match those recorded in the generated .pb.go headers; + # regenerating with a different one rewrites the whole file. + go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@v1.6.2 + go install google.golang.org/protobuf/cmd/protoc-gen-go@v1.36.11 + # protoc resolves plugins via PATH, and `go install` targets GOPATH/bin. + export PATH="$(go env GOPATH)/bin:$PATH" protoc -I . tunnel.proto --go_out=. --go_opt=paths=source_relative --go-grpc_out=. --go-grpc_opt=paths=source_relative cli:test: diff --git a/cmd/provider/add.go b/cmd/provider/add.go index 54769db41..b45a4b2e2 100644 --- a/cmd/provider/add.go +++ b/cmd/provider/add.go @@ -3,6 +3,7 @@ package provider import ( "context" "fmt" + "os" "strings" "github.com/devsy-org/devsy/cmd/flags" @@ -11,6 +12,7 @@ import ( "github.com/devsy-org/devsy/pkg/flags/names" "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/provider" + "github.com/devsy-org/devsy/pkg/status" "github.com/devsy-org/devsy/pkg/types" "github.com/devsy-org/devsy/pkg/workspace" "github.com/spf13/cobra" @@ -85,18 +87,31 @@ func (cmd *AddCmd) Run(ctx context.Context, devsyConfig *config.Config, args []s return err } + reporter, err := newStatusReporter(cmd.ResultFormat, os.Stdout) + if err != nil { + return err + } + + status.Enter(reporter, status.PhaseInstallingProvider, providerName) providerConfig, options, err := cmd.resolveProviderConfig(ctx, devsyConfig, providerName, args) if err != nil { + status.Fail(reporter, status.PhaseInstallingProvider, err) return err } + status.Leave(reporter, status.PhaseInstallingProvider, providerConfig.Name) log.Infof("installed provider: providerName=%s", providerConfig.Name) if !cmd.Use { log.Infof("To initialize the provider, run: devsy provider init %s", providerConfig.Name) + // No PhaseReady: installed but not initialized. return nil } - return cmd.useProvider(ctx, devsyConfig, providerConfig, options) + if err := cmd.useProvider(ctx, devsyConfig, providerConfig, options, reporter); err != nil { + return err + } + status.Leave(reporter, status.PhaseReady, providerConfig.Name) + return nil } func validateOptionalProviderName(providerName string) error { @@ -158,6 +173,7 @@ func (cmd *AddCmd) useProvider( devsyConfig *config.Config, providerConfig *provider.ProviderConfig, options []string, + reporter status.Reporter, ) error { // First add: there are no prior user values to merge, so // DiscardPriorValues is moot. Set it explicitly so future readers @@ -168,6 +184,7 @@ func (cmd *AddCmd) useProvider( UserOptions: options, DiscardPriorValues: true, SingleMachine: &cmd.SingleMachine, + Reporter: reporter, }) if configureErr != nil { devsyConfig, err := config.LoadConfig(cmd.Context, "") diff --git a/cmd/provider/configure_shared.go b/cmd/provider/configure_shared.go index 14539f7c1..95444122f 100644 --- a/cmd/provider/configure_shared.go +++ b/cmd/provider/configure_shared.go @@ -10,6 +10,7 @@ import ( "github.com/devsy-org/devsy/pkg/log" options2 "github.com/devsy-org/devsy/pkg/options" provider2 "github.com/devsy-org/devsy/pkg/provider" + "github.com/devsy-org/devsy/pkg/status" ) // ProviderOptionsConfig parameterizes ConfigureProvider. @@ -32,6 +33,15 @@ type ProviderOptionsConfig struct { SkipInit bool SkipSubOptions bool SingleMachine *bool + + Reporter status.Reporter +} + +func (cfg ProviderOptionsConfig) reporter() status.Reporter { + if cfg.Reporter == nil { + return status.Nop() + } + return cfg.Reporter } func ConfigureProvider(ctx context.Context, cfg ProviderOptionsConfig) error { @@ -91,13 +101,18 @@ func configureProviderOptions( } // fill defaults + reporter := cfg.reporter() + status.Enter(reporter, status.PhaseResolvingOptions, cfg.Provider.Name) devsyConfig, err = options2.ResolveOptions( ctx, devsyConfig, cfg.Provider, options, cfg.SkipRequired, cfg.SkipSubOptions, cfg.SingleMachine, ) if err != nil { - return nil, fmt.Errorf("resolve options: %w", err) + err = fmt.Errorf("resolve options: %w", err) + status.Fail(reporter, status.PhaseResolvingOptions, err) + return nil, err } + status.Leave(reporter, status.PhaseResolvingOptions, cfg.Provider.Name) // run init command if !cfg.SkipInit { @@ -107,10 +122,13 @@ func configureProviderOptions( stderr := log.Writer(log.LevelError) defer func() { _ = stderr.Close() }() + status.Enter(reporter, status.PhaseRunningInit, cfg.Provider.Name) err = initProvider(ctx, devsyConfig, cfg.Provider, initIO{stdout: stdout, stderr: stderr}) if err != nil { + status.Fail(reporter, status.PhaseRunningInit, err) return nil, err } + status.Leave(reporter, status.PhaseRunningInit, cfg.Provider.Name) } return devsyConfig, nil diff --git a/cmd/provider/init.go b/cmd/provider/init.go index 857cf687e..0e75d281f 100644 --- a/cmd/provider/init.go +++ b/cmd/provider/init.go @@ -1,11 +1,14 @@ package provider import ( + "os" + "github.com/devsy-org/devsy/cmd/completion" "github.com/devsy-org/devsy/cmd/flags" "github.com/devsy-org/devsy/pkg/config" cliflags "github.com/devsy-org/devsy/pkg/flags" "github.com/devsy-org/devsy/pkg/flags/names" + "github.com/devsy-org/devsy/pkg/status" "github.com/devsy-org/devsy/pkg/workspace" "github.com/spf13/cobra" ) @@ -39,14 +42,23 @@ func NewInitCmd(f *flags.GlobalFlags) *cobra.Command { if err != nil { return err } - return ConfigureProvider(cobraCmd.Context(), ProviderOptionsConfig{ + reporter, err := newStatusReporter(cmd.ResultFormat, os.Stdout) + if err != nil { + return err + } + if err := ConfigureProvider(cobraCmd.Context(), ProviderOptionsConfig{ Provider: p.Config, ContextName: devsyConfig.DefaultContext, UserOptions: cmd.Options, DiscardPriorValues: cmd.Reset, SkipInit: cmd.SkipInit, SingleMachine: &cmd.SingleMachine, - }) + Reporter: reporter, + }); err != nil { + return err + } + status.Leave(reporter, status.PhaseReady, name) + return nil }, ValidArgsFunction: func( rootCmd *cobra.Command, diff --git a/cmd/provider/set_source.go b/cmd/provider/set_source.go index 149e4bd6b..732defcc5 100644 --- a/cmd/provider/set_source.go +++ b/cmd/provider/set_source.go @@ -3,12 +3,14 @@ package provider import ( "context" "fmt" + "os" "github.com/devsy-org/devsy/cmd/flags" "github.com/devsy-org/devsy/pkg/config" cliflags "github.com/devsy-org/devsy/pkg/flags" "github.com/devsy-org/devsy/pkg/flags/names" "github.com/devsy-org/devsy/pkg/log" + "github.com/devsy-org/devsy/pkg/status" "github.com/devsy-org/devsy/pkg/workspace" "github.com/spf13/cobra" ) @@ -67,14 +69,23 @@ func (cmd *SetSourceCmd) Run(ctx context.Context, devsyConfig *config.Config, ar providerSource = args[1] } + reporter, err := newStatusReporter(cmd.ResultFormat, os.Stdout) + if err != nil { + return err + } + + status.Enter(reporter, status.PhaseInstallingProvider, args[0]) providerConfig, err := workspace.UpdateProvider(ctx, devsyConfig, args[0], providerSource) if err != nil { + status.Fail(reporter, status.PhaseInstallingProvider, err) return err } + status.Leave(reporter, status.PhaseInstallingProvider, providerConfig.Name) log.Infof("updated provider: providerName=%s", providerConfig.Name) if !cmd.Use { log.Infof("To initialize the provider, run: devsy provider init %s", providerConfig.Name) + // No PhaseReady: not ready until a following `provider init` runs. return nil } @@ -85,11 +96,16 @@ func (cmd *SetSourceCmd) Run(ctx context.Context, devsyConfig *config.Config, ar Provider: providerConfig, ContextName: devsyConfig.DefaultContext, UserOptions: cmd.Options, + Reporter: reporter, }); err != nil { return fmt.Errorf("configure provider: %w", err) } - return writeDefaultProvider(cmd.Context, providerConfig.Name) + if err := writeDefaultProvider(cmd.Context, providerConfig.Name); err != nil { + return err + } + status.Leave(reporter, status.PhaseReady, providerConfig.Name) + return nil } func (cmd *SetSourceCmd) runPinVersion( diff --git a/cmd/provider/status.go b/cmd/provider/status.go new file mode 100644 index 000000000..07936f047 --- /dev/null +++ b/cmd/provider/status.go @@ -0,0 +1,60 @@ +package provider + +import ( + "io" + + config2 "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/log" + "github.com/devsy-org/devsy/pkg/output" + "github.com/devsy-org/devsy/pkg/status" +) + +// newStatusReporter drives provider progress output: one NDJSON status line +// per phase transition in JSON mode, human-readable info lines otherwise. +// Mirrors cmd/workspace/up's reporter so both pipelines look the same to a +// consumer reading the stream. +func newStatusReporter(resultFormat string, out io.Writer) (status.Reporter, error) { + mode, err := output.ResolveMode(resultFormat) + if err != nil { + return nil, err + } + + var r status.Reporter = plainStatusReporter{} + if mode == output.ModeJSON { + r = &jsonStatusReporter{out: out} + } + return status.ForPipeline(r, status.PipelineProvider), nil +} + +type jsonStatusReporter struct { + out io.Writer +} + +func (r *jsonStatusReporter) Report(e status.Event) { + _ = config2.WriteStatusJSON(r.out, e) +} + +type plainStatusReporter struct{} + +func (plainStatusReporter) Report(e status.Event) { + switch { + case e.Phase == status.PhaseFailed: + log.Errorf("provider: phase %q failed: %s", e.Step, e.Err) + case e.Started: + log.Infof("provider: %s", phaseLabel(e.Phase)) + } +} + +var phaseLabels = map[status.Phase]string{ + status.PhaseInstallingProvider: "installing provider", + status.PhaseResolvingOptions: "resolving options", + status.PhaseRunningInit: "running provider init", + status.PhaseReady: "ready", +} + +func phaseLabel(p status.Phase) string { + if label, ok := phaseLabels[p]; ok { + return label + } + return string(p) +} diff --git a/cmd/workspace/up/status.go b/cmd/workspace/up/status.go index c325aa70a..e0c38e9cb 100644 --- a/cmd/workspace/up/status.go +++ b/cmd/workspace/up/status.go @@ -11,10 +11,11 @@ import ( // newStatusReporter drives `up`'s progress output. func newStatusReporter(emitJSON bool, out io.Writer) status.Reporter { + var r status.Reporter = plainStatusReporter{} if emitJSON { - return &jsonStatusReporter{out: out} + r = &jsonStatusReporter{out: out} } - return plainStatusReporter{} + return status.ForPipeline(r, status.PipelineWorkspaceUp) } // jsonStatusReporter serializes write events. diff --git a/desktop/e2e/app.e2e.ts b/desktop/e2e/app.e2e.ts index 43ac75523..d2499b8d4 100644 --- a/desktop/e2e/app.e2e.ts +++ b/desktop/e2e/app.e2e.ts @@ -1,11 +1,14 @@ import type { ElectronApplication, Page } from "@playwright/test" import { expect, test } from "@playwright/test" -import { launchApp } from "./electron-app.js" +import { launchApp, resetMockState } from "./electron-app.js" let app: ElectronApplication let page: Page test.beforeAll(async () => { + // The badge counts below assert the mock's default fixture exactly, so + // start from a clean slate rather than inheriting another spec's CRUD. + resetMockState() ;({ app, page } = await launchApp()) }) diff --git a/desktop/e2e/fixtures/mock-devsy.cjs b/desktop/e2e/fixtures/mock-devsy.cjs index 67d13feda..558851361 100755 --- a/desktop/e2e/fixtures/mock-devsy.cjs +++ b/desktop/e2e/fixtures/mock-devsy.cjs @@ -110,6 +110,19 @@ function out(data) { ) } +// Single-line NDJSON status envelope, matching pkg/status + envelope.go. +function providerStatus(phase, started, step) { + out( + JSON.stringify({ + kind: "status", + pipeline: "provider", + phase, + ...(step ? { step } : {}), + started, + }), + ) +} + // Parse a slice of args (positional + recognized flags) into a result object. function parseArgs(args) { const positional = [] @@ -477,9 +490,16 @@ switch (cmd) { case "add": { const provName = extra if (provName) { - for (const key of Object.keys(state.providers)) { - state.providers[key].default = false + // The desktop passes --use=false, which installs without changing + // the default provider; only `provider use` does that. + const takesDefault = !rawArgs.includes("--use=false") + if (takesDefault) { + for (const key of Object.keys(state.providers)) { + state.providers[key].default = false + } } + // Written uninitialized, exactly as the real CLI does: `provider + // add --use=false` persists before any init runs. state.providers[provName] = { config: { name: provName, @@ -491,11 +511,39 @@ switch (cmd) { optionGroups: [], }, state: { initialized: false }, - default: true, + default: takesDefault, } saveState(state) } - out("") + // No ready phase: install finished, init has not run. + providerStatus("installing_provider", true) + providerStatus("installing_provider", false, provName) + process.exit(0) + break + } + case "init": { + const provName = extra || providerFlag + providerStatus("resolving_options", true, provName) + providerStatus("resolving_options", false, provName) + providerStatus("running_init", true, provName) + // Providers named *probe belong to lifecycle tests, where init must + // outlast the ~1.5s a new card takes to render or there is no + // in-flight state to observe. Everything else stays fast so unrelated + // specs aren't slowed down. + const initMs = /probe$/.test(provName) ? 5000 : 150 + setTimeout(() => { + // Re-read: the snapshot taken at startup is up to 5s stale, and + // writing it back would clobber any add/delete another spec + // performed against this shared fixture meanwhile. + const latest = loadState() + if (provName && latest.providers[provName]) { + latest.providers[provName].state.initialized = true + saveState(latest) + } + providerStatus("running_init", false, provName) + providerStatus("ready", false, provName) + process.exit(0) + }, initMs) break } case "delete": { @@ -541,6 +589,19 @@ switch (cmd) { out("") break } + case "set-source": { + const provName = extra + // Mirrors UpdateProvider: new binaries, so Initialized is cleared and + // the caller is expected to chain `provider init`. + if (provName && state.providers[provName]) { + state.providers[provName].state.initialized = false + saveState(state) + } + providerStatus("installing_provider", true, provName) + providerStatus("installing_provider", false, provName) + process.exit(0) + break + } case "versions": out([]) break diff --git a/desktop/e2e/providers.e2e.ts b/desktop/e2e/providers.e2e.ts index aed7e25b0..9cb3dc005 100644 --- a/desktop/e2e/providers.e2e.ts +++ b/desktop/e2e/providers.e2e.ts @@ -82,3 +82,100 @@ test.describe("Providers Page", () => { await expect(sheet).toContainText("docker") }) }) + +test.describe("Provider lifecycle badges", () => { + // The bug this guards: `provider add` persists the provider with + // initialized:false before init runs, so the card briefly rendered the red + // "not initialized" badge for several seconds. + test("never shows 'not initialized' while add+init is in flight", async () => { + const main = page.locator('[data-slot="sidebar-inset"] main') + + await page.evaluate(async () => { + const api = ( + window as unknown as { + electronAPI: { + invoke: (c: string, a?: Record) => Promise + } + } + ).electronAPI + // Mock CLI state persists across runs, so start from a clean slate; + // a leftover initialized provider would skip the window under test. + await api.invoke("provider_delete", { name: "lifecycleprobe" }) + await api.invoke("provider_add", { name: "lifecycleprobe" }) + // Not awaited: the assertions below run while init is still going. + void api.invoke("provider_init", { name: "lifecycleprobe" }) + }) + + const card = main.locator("button", { hasText: "lifecycleprobe" }).first() + await expect(card).toBeVisible({ timeout: 10000 }) + + // Sample continuously from in-flight through settled. The red badge must + // never appear at any point: not during install/init, and not in the + // window between the job clearing and the provider list catching up. + let sawBusy = false + let settled = false + const deadline = Date.now() + 8000 + while (Date.now() < deadline) { + const text = ((await card.textContent()) ?? "").toLowerCase() + expect(text).not.toContain("not initialized") + if (/installing|initializing/.test(text)) sawBusy = true + // Reached only once the busy label is replaced by the settled badge. + if (sawBusy && /(^|\s)initialized/.test(text)) { + settled = true + break + } + await page.waitForTimeout(100) + } + + expect(sawBusy, "expected a busy badge during install/init").toBe(true) + expect(settled, "expected the card to settle as initialized").toBe(true) + + // Mock CLI state is shared across specs, so don't leave this behind. + await page.evaluate(async () => { + await ( + window as unknown as { + electronAPI: { + invoke: (c: string, a?: Record) => Promise + } + } + ).electronAPI.invoke("provider_delete", { name: "lifecycleprobe" }) + }) + }) + + // An abandoned wizard (skip init, or close mid-flow) leaves the install's + // job open. Without an explicit release the card spins on "installing…" + // forever, since nothing else will ever finish that job. + test("releases the job when a provider is added but never initialized", async () => { + const main = page.locator('[data-slot="sidebar-inset"] main') + + const api = async (channel: string, args: Record) => + page.evaluate( + ([c, a]) => + ( + window as unknown as { + electronAPI: { + invoke: ( + c: string, + a?: Record, + ) => Promise + } + } + ).electronAPI.invoke(c as string, a as Record), + [channel, args] as const, + ) + + await api("provider_delete", { name: "skipprobe" }) + await api("provider_add", { name: "skipprobe" }) + + const card = main.locator("button", { hasText: "skipprobe" }).first() + await expect(card).toContainText(/installing/i, { timeout: 10000 }) + + // What the wizard does on skip/close. + await api("provider_release_job", { name: "skipprobe" }) + + // Settles to the honest uninitialized state rather than staying busy. + await expect(card).toContainText(/not initialized/i, { timeout: 10000 }) + + await api("provider_delete", { name: "skipprobe" }) + }) +}) diff --git a/desktop/src/main/__tests__/cli.test.ts b/desktop/src/main/__tests__/cli.test.ts index 7cdb68908..f0242feb8 100644 --- a/desktop/src/main/__tests__/cli.test.ts +++ b/desktop/src/main/__tests__/cli.test.ts @@ -1,6 +1,7 @@ // @vitest-environment node import { execFile, spawn } from "node:child_process" import { EventEmitter } from "node:events" +import { Readable } from "node:stream" import { beforeEach, describe, expect, it, vi } from "vitest" import { CliRunner } from "../cli.js" @@ -24,6 +25,17 @@ function fakeChild() { return child } +/** A child whose stdout/stderr are real streams, as readline requires. */ +function fakeStreamingChild() { + const child = new EventEmitter() as EventEmitter & { + stdout: Readable + stderr: Readable + } + child.stdout = new Readable({ read() {} }) + child.stderr = new Readable({ read() {} }) + return child +} + vi.mock("node:child_process", async (importOriginal) => { const actual = await importOriginal() return { @@ -213,6 +225,39 @@ describe("CliRunner", () => { }) }) + describe("runStreaming", () => { + it("reports an exit when the child fails to spawn", async () => { + // A spawn failure emits "error" and never "close". Callers wrap this in + // a promise settled from onExit, so without this the promise never + // settles and the provider job stays busy forever. + const child = fakeStreamingChild() + const mockSpawn = vi.mocked(spawn) as unknown as ReturnType + mockSpawn.mockReturnValue(child) + + const onExit = vi.fn() + await cli.runStreaming(["provider", "init", "docker"], () => {}, onExit) + child.emit("error", new Error("spawn ENOENT")) + + await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)) + const [code, cliError] = onExit.mock.calls[0] + expect(code).toBe(-1) + expect(cliError?.message).toContain("spawn ENOENT") + }) + + it("reports the exit once when error and close both fire", async () => { + const child = fakeStreamingChild() + const mockSpawn = vi.mocked(spawn) as unknown as ReturnType + mockSpawn.mockReturnValue(child) + + const onExit = vi.fn() + await cli.runStreaming(["provider", "init", "docker"], () => {}, onExit) + child.emit("error", new Error("boom")) + child.emit("close", 1) + + await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)) + }) + }) + describe("stripAnsi", () => { it("removes ANSI escape sequences", () => { const result = CliRunner.stripAnsi( diff --git a/desktop/src/main/__tests__/ipc-provider-jobs.test.ts b/desktop/src/main/__tests__/ipc-provider-jobs.test.ts new file mode 100644 index 000000000..d57526f48 --- /dev/null +++ b/desktop/src/main/__tests__/ipc-provider-jobs.test.ts @@ -0,0 +1,180 @@ +// @vitest-environment node +import { EventEmitter } from "node:events" +import { beforeEach, describe, expect, it, vi } from "vitest" +import { ProviderJobs } from "../provider-jobs.js" + +const handlers = new Map unknown>() + +vi.mock("electron", () => ({ + app: { getPath: () => "/tmp", getVersion: () => "0.0.0" }, + dialog: {}, + ipcMain: { + handle: (channel: string, fn: (...args: unknown[]) => unknown) => { + handlers.set(channel, fn) + }, + on: () => undefined, + }, +})) + +vi.mock("../analytics.js", () => ({ + hashWorkspaceRef: (v: string) => v, + trackEvent: () => undefined, +})) + +const { registerIpcHandlers } = await import("../ipc.js") + +/** A child that emits the given stdout lines, then exits with `code`. */ +function fakeChild(lines: string[], code: number) { + const child = new EventEmitter() as EventEmitter & { + exitCode: number | null + signalCode: string | null + kill: () => void + } + child.exitCode = null + child.signalCode = null + child.kill = () => undefined + return { child, lines, code } +} + +function setup( + script: (cliArgs: string[]) => { lines: string[]; code: number } = () => ({ + lines: [], + code: 0, + }), +) { + const providerJobs = new ProviderJobs() + const cli = { + run: vi.fn(async () => ({})), + runRaw: vi.fn(async () => ""), + runStreaming: vi.fn( + async ( + cliArgs: string[], + onLine: (line: string, stream: "stdout" | "stderr") => void, + onExit: (code: number, cliError?: unknown) => void, + ) => { + const { lines, code } = script(cliArgs) + const { child } = fakeChild(lines, code) + // Deliver lines then exit asynchronously, as a real child does. + setTimeout(() => { + for (const line of lines) onLine(line, "stdout") + onExit(code, code === 0 ? undefined : { message: "boom" }) + }, 0) + return child + }, + ), + cancelFor: vi.fn(async () => undefined), + } + const deps = { + cli, + state: { workspaceContext: () => "ctx", providerList: () => [] }, + logStore: { + createLogFile: () => "/tmp/log.txt", + appendLog: () => true, + closeLog: async () => undefined, + onDrain: async () => undefined, + }, + pty: { cancelFor: vi.fn(async () => undefined) }, + getMainWindow: () => null, + providerJobs, + } + // biome-ignore lint/suspicious/noExplicitAny: partial test doubles + registerIpcHandlers(deps as any) + return { providerJobs, cli } +} + +function invoke(channel: string, args: Record) { + const handler = handlers.get(channel) + if (!handler) throw new Error(`${channel} not registered`) + return handler({}, args) +} + +function statusLine(phase: string) { + return JSON.stringify({ kind: "status", pipeline: "provider", phase }) +} + +describe("provider job lifecycle over IPC", () => { + beforeEach(() => { + handlers.clear() + vi.clearAllMocks() + }) + + it("clears the job when init succeeds", async () => { + const { providerJobs } = setup(() => ({ + lines: [statusLine("running_init"), statusLine("ready")], + code: 0, + })) + + await invoke("provider_init", { name: "docker" }) + + expect(providerJobs.get("docker")).toBeUndefined() + }) + + it("records the failure when init fails", async () => { + const { providerJobs } = setup(() => ({ lines: [], code: 1 })) + + const result = (await invoke("provider_init", { name: "docker" })) as { + ok: boolean + } + + expect(result.ok).toBe(false) + expect(providerJobs.get("docker")?.error).toBeTruthy() + }) + + it("leaves the job open after add, for the chained init to close", async () => { + // Closing it here would flash the red badge between add and init. + const { providerJobs } = setup(() => ({ + lines: [statusLine("installing_provider")], + code: 0, + })) + + await invoke("provider_add", { name: "docker" }) + + expect(providerJobs.get("docker")?.activity).toBe("installing") + }) + + it("releases an abandoned job so the card stops spinning", async () => { + const { providerJobs } = setup(() => ({ lines: [], code: 0 })) + + await invoke("provider_add", { name: "docker" }) + expect(providerJobs.get("docker")).toBeDefined() + + await invoke("provider_release_job", { name: "docker" }) + + expect(providerJobs.get("docker")).toBeUndefined() + }) + + it("keeps a failed job's error when released", async () => { + const { providerJobs } = setup(() => ({ lines: [], code: 1 })) + + await invoke("provider_init", { name: "docker" }) + await invoke("provider_release_job", { name: "docker" }) + + expect(providerJobs.get("docker")?.error).toBeTruthy() + }) + + it("runs set-source then init on update, clearing the job once", async () => { + const seen: string[][] = [] + const { providerJobs } = setup((cliArgs) => { + seen.push(cliArgs) + return { lines: [], code: 0 } + }) + + await invoke("provider_update", { name: "docker" }) + + expect(seen.map((a) => a[1])).toEqual(["set-source", "init"]) + expect(providerJobs.get("docker")).toBeUndefined() + }) + + it("records the failure when the update's chained init fails", async () => { + const { providerJobs } = setup((cliArgs) => ({ + lines: [], + code: cliArgs[1] === "init" ? 1 : 0, + })) + + await expect( + invoke("provider_update", { name: "docker" }), + ).rejects.toThrow() + + expect(providerJobs.get("docker")?.error).toBeTruthy() + }) +}) diff --git a/desktop/src/main/__tests__/provider-jobs.test.ts b/desktop/src/main/__tests__/provider-jobs.test.ts new file mode 100644 index 000000000..0708c3c88 --- /dev/null +++ b/desktop/src/main/__tests__/provider-jobs.test.ts @@ -0,0 +1,138 @@ +// @vitest-environment node +import { beforeEach, describe, expect, it, vi } from "vitest" +import { ProviderJobs } from "../provider-jobs.js" + +describe("ProviderJobs", () => { + let jobs: ProviderJobs + + beforeEach(() => { + jobs = new ProviderJobs() + }) + + it("tracks activity and phase transitions", async () => { + jobs.start("docker", "installing") + expect(jobs.get("docker")).toEqual({ activity: "installing" }) + + jobs.report("docker", "installing_provider") + expect(jobs.get("docker")).toEqual({ + activity: "installing", + phase: "installing_provider", + }) + + await jobs.finish("docker") + expect(jobs.get("docker")).toBeUndefined() + }) + + it("ignores phase reports for a provider with no active job", () => { + jobs.report("docker", "running_init") + expect(jobs.get("docker")).toBeUndefined() + }) + + it("retains the failure so the UI can explain it", async () => { + jobs.start("docker", "initializing") + await jobs.finish("docker", "init: boom") + + expect(jobs.get("docker")).toEqual({ + activity: "initializing", + phase: "failed", + error: "init: boom", + }) + }) + + it("does not let a later release erase a recorded failure", async () => { + jobs.start("docker", "initializing") + await jobs.finish("docker", "init: boom") + + // The wizard closing after a failed init releases the job; that must not + // turn the failure into a clean success. + await jobs.finish("docker") + + expect(jobs.get("docker")?.error).toBe("init: boom") + }) + + it("refreshes provider state before clearing a finished job", async () => { + // Clearing first would briefly expose initialized:false from the stale + // list — the red badge this class exists to prevent. + const order: string[] = [] + jobs.setRefresh(async () => { + order.push(`refresh(job=${jobs.get("docker") ? "present" : "gone"})`) + }) + + jobs.start("docker", "installing") + await jobs.finish("docker") + + expect(order).toEqual(["refresh(job=present)"]) + expect(jobs.get("docker")).toBeUndefined() + }) + + it("does not clear a newer job started while refresh was in flight", async () => { + // refresh is a real CLI round-trip. If a second command starts during it, + // the first finish() must not delete the entry the new one is using — + // its report() calls would find no job and the card would look idle. + let releaseRefresh: (() => void) | undefined + jobs.setRefresh( + () => + new Promise((resolve) => { + releaseRefresh = resolve + }), + ) + + jobs.start("docker", "installing") + const finishing = jobs.finish("docker") + + // A re-init lands before the first finish resolves. + jobs.start("docker", "initializing") + releaseRefresh?.() + await finishing + + expect(jobs.get("docker")).toEqual({ activity: "initializing" }) + }) + + it("still tracks phases for the superseding job", async () => { + let releaseRefresh: (() => void) | undefined + jobs.setRefresh( + () => + new Promise((resolve) => { + releaseRefresh = resolve + }), + ) + + jobs.start("docker", "installing") + const finishing = jobs.finish("docker") + jobs.start("docker", "initializing") + releaseRefresh?.() + await finishing + + jobs.report("docker", "running_init") + + expect(jobs.get("docker")?.phase).toBe("running_init") + }) + + it("notifies listeners on every mutation", () => { + const listener = vi.fn() + jobs.onChange(listener) + + jobs.start("docker", "installing") + jobs.report("docker", "installing_provider") + jobs.clear("docker") + + expect(listener).toHaveBeenCalledTimes(3) + }) + + it("does not notify when clearing an untracked provider", () => { + const listener = vi.fn() + jobs.onChange(listener) + + jobs.clear("nonexistent") + + expect(listener).not.toHaveBeenCalled() + }) + + it("clears a retained failure so a re-added provider starts clean", () => { + jobs.start("docker", "installing") + jobs.report("docker", "failed", "boom") + jobs.clear("docker") + + expect(jobs.get("docker")).toBeUndefined() + }) +}) diff --git a/desktop/src/main/cli.ts b/desktop/src/main/cli.ts index 367919421..0145c3022 100644 --- a/desktop/src/main/cli.ts +++ b/desktop/src/main/cli.ts @@ -297,7 +297,11 @@ export class CliRunner { }) } - child.on("close", (code) => { + let settled = false + const finish = (code: number, cliError?: CLIError): void => { + // "error" and "close" can both fire; report the outcome once. + if (settled) return + settled = true this.activeChildren.delete(child) if (workspaceId) { const bucket = this.childrenByWorkspace.get(workspaceId) @@ -307,7 +311,18 @@ export class CliRunner { } } this.release() - onExit(code ?? -1, lastCliError) + onExit(code, cliError) + } + + // A spawn failure (missing binary, EACCES) emits "error" and never + // "close". Without this, onExit never fires: callers that wrap this in a + // promise hang forever, and the concurrency slot is never released. + child.on("error", (err) => { + finish(-1, { code: "spawn_failed", message: err.message }) + }) + + child.on("close", (code) => { + finish(code ?? -1, lastCliError) }) return child diff --git a/desktop/src/main/index.ts b/desktop/src/main/index.ts index 7cf9fafa2..e91735174 100644 --- a/desktop/src/main/index.ts +++ b/desktop/src/main/index.ts @@ -6,6 +6,7 @@ import { CliRunner } from "./cli.js" import { DaemonManager } from "./daemon-manager.js" import { registerIpcHandlers } from "./ipc.js" import { LogStore } from "./log-store.js" +import { ProviderJobs } from "./provider-jobs.js" import { PtyManager } from "./pty.js" import { DaemonState } from "./state.js" import { AppTray } from "./tray.js" @@ -160,6 +161,11 @@ app.whenReady().then(() => { stopAutoUpdater() }) + // Shared between the IPC handlers that run provider commands and the + // watcher that broadcasts provider state, so in-flight work is reported + // alongside what's on disk. + const providerJobs = new ProviderJobs() + // Register IPC handlers const { tunnelProcesses, @@ -171,6 +177,7 @@ app.whenReady().then(() => { logStore, pty: ptyManager, getMainWindow: () => mainWindow, + providerJobs, }) // Start state watcher @@ -179,7 +186,13 @@ app.whenReady().then(() => { daemon: daemonManager.daemonClient, state, getMainWindow: () => mainWindow, + providerJobs, }) + // Phase transitions are pushed immediately rather than waiting for the + // next disk poll, which is what made the old badge lag visible. + providerJobs.onChange(() => watcher.broadcastProviders()) + providerJobs.setRefresh(() => watcher.refreshProviders()) + void watcher.start().then(runInitialProviderUpdateCheck) scheduleProviderUpdateCheck() diff --git a/desktop/src/main/ipc.ts b/desktop/src/main/ipc.ts index 191867971..924846476 100644 --- a/desktop/src/main/ipc.ts +++ b/desktop/src/main/ipc.ts @@ -10,6 +10,11 @@ import type { CLIError } from "../shared/cli-error.js" import { parseCliEnvelope } from "../shared/cli-error.js" import { hashWorkspaceRef, trackEvent } from "./analytics.js" import { loadCatalog } from "./image-catalog.js" +import type { + ProviderActivity, + ProviderJobs, + ProviderPhase, +} from "./provider-jobs.js" import type { CliRunner } from "./cli.js" import type { LogStore } from "./log-store.js" import type { PtyManager } from "./pty.js" @@ -90,6 +95,7 @@ interface IpcDependencies { logStore: LogStore pty: PtyManager getMainWindow: () => BrowserWindow | null + providerJobs: ProviderJobs } /** Format a line in zap console format so log-parser.ts can parse it. */ @@ -101,7 +107,11 @@ interface ProgressSink { line(formatted: string): boolean done( finalLine: string, - extra?: { level?: "info" | "warn" | "error"; cliError?: CLIError }, + extra?: { + level?: "info" | "warn" | "error" + cliError?: CLIError + success?: boolean + }, ): Promise } @@ -122,6 +132,7 @@ function createLogSink( message?: string level?: "info" | "warn" | "error" cliError?: CLIError + success?: boolean }, ): void { if (timer) { @@ -161,7 +172,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { scheduleProviderUpdateCheck: () => void runInitialProviderUpdateCheck: () => void } { - const { cli, state, logStore, pty } = deps + const { cli, state, logStore, pty, providerJobs } = deps const tunnelProcesses = new Map< string, import("node:child_process").ChildProcess @@ -243,6 +254,72 @@ export function registerIpcHandlers(deps: IpcDependencies): { }) } + function errorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error) + } + + /** + * Track a provider job for the duration of fn, so the job cannot outlive the + * work it describes. Opening a job in one place and closing it in another + * leaves the card spinning forever on any path that forgets — prefer this to + * calling start/finish by hand. + * + * Errors are recorded on the job and rethrown, leaving the caller's own + * error handling intact. + */ + async function withProviderJob( + name: string, + activity: ProviderActivity, + fn: () => Promise, + ): Promise { + providerJobs.start(name, activity) + try { + const result = await fn() + await providerJobs.finish(name) + return result + } catch (error) { + const cliError = (error as { cliError?: CLIError }).cliError + await providerJobs.finish(name, cliError?.message ?? errorMessage(error)) + throw error + } + } + + /** + * Run a provider CLI command, feeding its NDJSON status lines into the job + * registry so the UI tracks phases as they happen instead of only learning + * the outcome at exit. + */ + function runProviderWithStatus( + name: string, + cliArgs: string[], + ): Promise { + return new Promise((resolve, reject) => { + void cli.runStreaming( + cliArgs, + (line, stream) => { + if (stream !== "stdout") return + const envelope = parseCliEnvelope(line) + if (envelope?.kind === "status") { + providerJobs.report( + name, + envelope.phase as ProviderPhase, + envelope.error, + ) + } + }, + (code, cliError) => { + if (code === 0) { + resolve() + return + } + const message = + cliError?.message ?? `${cliArgs.join(" ")} exited with ${code}` + reject(Object.assign(new Error(message), { cliError })) + }, + ) + }) + } + /** * Compute provider update information by querying the CLI for all installed providers. */ @@ -361,15 +438,37 @@ export function registerIpcHandlers(deps: IpcDependencies): { if (args.singleMachine) { cliArgs.push("--single-machine") } - await cli.runRaw(cliArgs) + + // Not withProviderJob: the job outlives this call so the badge stays busy + // until the wizard's chained provider_init finishes. The opener closes it, + // via provider_release_job on any path that abandons the install. + providerJobs.start(args.name, "installing") + try { + await runProviderWithStatus(args.name, cliArgs) + } catch (error) { + await providerJobs.finish(args.name, errorMessage(error)) + throw error + } }, ) ipcMain.handle("provider_delete", async (_event, args: { name: string }) => { trackEvent("provider_delete") await cli.runRaw(["provider", "delete", args.name]) + // Drop any retained failure so a re-added provider starts clean. + providerJobs.clear(args.name) }) + // Releases a job the caller opened but will not finish — e.g. the wizard + // installs a provider, then the user skips initialization or closes the + // dialog. Without this the card would spin on "installing…" indefinitely. + ipcMain.handle( + "provider_release_job", + async (_event, args: { name: string }) => { + await providerJobs.finish(args.name) + }, + ) + ipcMain.handle("provider_use", async (_event, args: { name: string }) => { await cli.runRaw(["provider", "use", args.name]) }) @@ -380,12 +479,13 @@ export function registerIpcHandlers(deps: IpcDependencies): { // own-properties, so a thrown Error with a .cliError attached would lose it. ipcMain.handle("provider_init", async (_event, args: { name: string }) => { try { - await cli.runRaw(["provider", "init", args.name]) + await withProviderJob(args.name, "initializing", () => + runProviderWithStatus(args.name, ["provider", "init", args.name]), + ) return { ok: true } as const } catch (err) { const cliError = (err as { cliError?: CLIError }).cliError - const message = err instanceof Error ? err.message : String(err) - return { ok: false, message, cliError } as const + return { ok: false, message: errorMessage(err), cliError } as const } }) @@ -395,9 +495,24 @@ export function registerIpcHandlers(deps: IpcDependencies): { const cmdId = crypto.randomUUID() const win = deps.getMainWindow() + // Not withProviderJob: the job outlives the handler, closed from onExit. + providerJobs.start(args.name, "initializing") await cli.runStreaming( ["provider", "init", args.name], - (line, _stream, meta) => { + (line, stream, meta) => { + // Status envelopes drive the job registry; everything else is log + // text for the wizard's output pane. + if (stream === "stdout") { + const envelope = parseCliEnvelope(line) + if (envelope?.kind === "status") { + providerJobs.report( + args.name, + envelope.phase as ProviderPhase, + envelope.error, + ) + return + } + } const formatted = formatLogLine(line) win?.webContents.send("command-progress", { commandId: cmdId, @@ -406,7 +521,15 @@ export function registerIpcHandlers(deps: IpcDependencies): { done: false, }) }, - (code, cliError) => { + async (code, cliError) => { + // Finish first: it refreshes the provider list, so the wizard's + // done signal can't arrive while the card still reads uninitialized. + await providerJobs.finish( + args.name, + code === 0 + ? undefined + : (cliError?.message ?? `provider init exited with ${code}`), + ) const exitMsg = formatLogLine( `Exit code: ${code}`, code === 0 ? "INFO" : "ERROR", @@ -415,6 +538,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { commandId: cmdId, message: exitMsg, level: code === 0 ? "info" : "error", + success: code === 0, cliError, done: true, }) @@ -425,8 +549,19 @@ export function registerIpcHandlers(deps: IpcDependencies): { }, ) + // set-source installs replacement binaries and clears Initialized, since + // the new binary has not run its init. Chain init so the provider ends up + // usable rather than sitting in a needs-re-init state the user must notice. ipcMain.handle("provider_update", async (_event, args: { name: string }) => { - await cli.runRaw(["provider", "set-source", args.name, "--use=false"]) + await withProviderJob(args.name, "updating", async () => { + await runProviderWithStatus(args.name, [ + "provider", + "set-source", + args.name, + "--use=false", + ]) + await runProviderWithStatus(args.name, ["provider", "init", args.name]) + }) }) ipcMain.handle("provider_options", async (_event, args: { name: string }) => { @@ -485,13 +620,18 @@ export function registerIpcHandlers(deps: IpcDependencies): { ipcMain.handle( "provider_set_version", async (_event, args: { name: string; tag: string }) => { - await cli.runRaw([ - "provider", - "set-source", - args.name, - "--version", - args.tag, - ]) + // Pinning a version swaps binaries via the same update path, so it + // clears Initialized and needs the same re-init as a source change. + await withProviderJob(args.name, "updating", async () => { + await runProviderWithStatus(args.name, [ + "provider", + "set-source", + args.name, + "--version", + args.tag, + ]) + await runProviderWithStatus(args.name, ["provider", "init", args.name]) + }) }, ) @@ -813,6 +953,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { const err = error as Error & { cliError?: CLIError } void sink.done(formatLogLine(err.message, "ERROR"), { level: "error", + success: false, cliError: err.cliError ?? { code: "up_failed", message: err.message, @@ -859,7 +1000,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { if (envelope?.kind === "result") { signalledDone = true releaseTask() - void sink.done(formatted) + void sink.done(formatted, { success: true }) return } @@ -868,6 +1009,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { releaseTask() void sink.done(formatted, { level: "error", + success: false, cliError: { code: "up_failed", message: envelope.message }, }) return @@ -887,7 +1029,9 @@ export function registerIpcHandlers(deps: IpcDependencies): { `Exit code: ${code}`, code === 0 ? "INFO" : "ERROR", ), - code === 0 ? undefined : { level: "error", cliError }, + code === 0 + ? { success: true } + : { level: "error", success: false, cliError }, ) }, wsId, @@ -899,6 +1043,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { const err = error as Error & { cliError?: CLIError } void sink.done(formatLogLine(err.message, "ERROR"), { level: "error", + success: false, cliError: err.cliError ?? { code: "up_follow_failed", message: err.message, @@ -943,6 +1088,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { (code) => { void sink.done( formatLogLine(`Exit code: ${code}`, code === 0 ? "INFO" : "ERROR"), + { success: code === 0 }, ) }, args.workspaceId, @@ -988,6 +1134,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { (code) => { void sink.done( formatLogLine(`Exit code: ${code}`, code === 0 ? "INFO" : "ERROR"), + { success: code === 0 }, ) }, args.workspaceId, @@ -1026,7 +1173,9 @@ export function registerIpcHandlers(deps: IpcDependencies): { (code, cliError) => { void sink.done( formatLogLine(`Exit code: ${code}`, code === 0 ? "INFO" : "ERROR"), - code === 0 ? undefined : { level: "error", cliError }, + code === 0 + ? { success: true } + : { level: "error", success: false, cliError }, ) }, args.workspaceId, @@ -1065,7 +1214,9 @@ export function registerIpcHandlers(deps: IpcDependencies): { (code, cliError) => { void sink.done( formatLogLine(`Exit code: ${code}`, code === 0 ? "INFO" : "ERROR"), - code === 0 ? undefined : { level: "error", cliError }, + code === 0 + ? { success: true } + : { level: "error", success: false, cliError }, ) }, args.workspaceId, diff --git a/desktop/src/main/provider-jobs.ts b/desktop/src/main/provider-jobs.ts new file mode 100644 index 000000000..8c9043d6c --- /dev/null +++ b/desktop/src/main/provider-jobs.ts @@ -0,0 +1,132 @@ +/** + * Tracks in-flight provider install/init/update work in the main process. + * + * The persisted `initialized` flag answers "is this provider usable", which + * is false both for a provider the user never initialized and for one being + * installed right now. Those render very differently, so the transient half + * of the lifecycle lives here rather than being inferred from disk state. + * + * Main-process ownership is deliberate: a renderer-local pending set is lost + * when the wizard closes, the user navigates away, or the window reloads, + * which is exactly when a multi-second install is still running. + */ + +/** Phases emitted by the Go provider pipeline (pkg/status). */ +export type ProviderPhase = + | "installing_provider" + | "resolving_options" + | "running_init" + | "ready" + | "failed" + +export type ProviderActivity = "installing" | "initializing" | "updating" + +export interface ProviderJob { + activity: ProviderActivity + phase?: ProviderPhase + /** Set when the job ended in failure; the job is retained so the UI can show why. */ + error?: string +} + +export class ProviderJobs { + private jobs = new Map() + // Lets an in-flight finish() tell whether it still owns the entry. + private generations = new Map() + private lastGeneration = 0 + private listeners = new Set<() => void>() + + /** + * Register a callback fired after every mutation, so a phase transition + * reaches the renderer without waiting for the next 3s disk poll. + */ + onChange(listener: () => void): () => void { + this.listeners.add(listener) + return () => this.listeners.delete(listener) + } + + private emit(): void { + for (const listener of this.listeners) listener() + } + + /** Begin tracking work on a provider, clearing any previous failure. */ + start(name: string, activity: ProviderActivity): void { + this.jobs.set(name, { activity }) + this.generations.set(name, ++this.lastGeneration) + this.emit() + } + + /** + * Record a phase transition. Ignored when no job is active, so a stray + * event can't resurrect a provider the UI already considers settled. + */ + report(name: string, phase: ProviderPhase, error?: string): void { + const job = this.jobs.get(name) + if (!job) return + if (phase === "failed") { + this.jobs.set(name, { ...job, phase, error: error ?? "failed" }) + } else { + this.jobs.set(name, { ...job, phase }) + } + this.emit() + } + + /** Stop tracking a provider, discarding any recorded failure. */ + clear(name: string): void { + this.generations.delete(name) + if (this.jobs.delete(name)) this.emit() + } + + /** + * Finish a job. A failure is retained so the UI can explain it; success + * drops the entry and lets the persisted `initialized` flag speak. + * + * On success the caller must have refreshed the provider list first: + * clearing the job while the list still shows the pre-init `initialized: + * false` would expose the same red badge this class exists to prevent, + * until the next poll caught up. + */ + async finish(name: string, error?: string): Promise { + if (error) { + const job = this.jobs.get(name) + this.jobs.set(name, { + activity: job?.activity ?? "initializing", + phase: "failed", + error, + }) + this.emit() + return + } + const job = this.jobs.get(name) + if (!job) return + // A recorded failure survives a later success-shaped finish. + if (job.error) return + + const generation = this.generations.get(name) + await this.refresh?.() + // refresh is a CLI round-trip; a newer job may own the entry by now. + if (this.generations.get(name) !== generation) return + + this.jobs.delete(name) + this.generations.delete(name) + this.emit() + } + + /** + * Supplies a way to re-read provider state from disk, so a finished job + * isn't cleared before the list reflects what the command just wrote. + */ + setRefresh(refresh: () => Promise): void { + this.refresh = refresh + } + + private refresh?: () => Promise + + get(name: string): ProviderJob | undefined { + return this.jobs.get(name) + } + + /** Snapshot for IPC, keyed by provider name. */ + snapshot(): Record { + return Object.fromEntries(this.jobs) + } +} diff --git a/desktop/src/main/watcher.ts b/desktop/src/main/watcher.ts index b698f9db7..dd8d7b165 100644 --- a/desktop/src/main/watcher.ts +++ b/desktop/src/main/watcher.ts @@ -5,6 +5,7 @@ import { watch } from "chokidar" import type { BrowserWindow } from "electron" import type { CliRunner } from "./cli.js" import type { DaemonClient } from "./daemon-client.js" +import type { ProviderJobs } from "./provider-jobs.js" import type { DaemonState } from "./state.js" interface WatcherDeps { @@ -12,6 +13,7 @@ interface WatcherDeps { daemon?: DaemonClient state: DaemonState getMainWindow: () => BrowserWindow | null + providerJobs: ProviderJobs } interface ContextEntry { @@ -144,6 +146,11 @@ export class Watcher { } } + /** Re-read provider state from disk now, without waiting for the poll. */ + async refreshProviders(): Promise { + await this.pollProviders() + } + private async pollProviders(): Promise { try { const raw = await this.queryWithFallback( @@ -160,15 +167,25 @@ export class Watcher { const providers = parseProviderEntries(raw) const changed = this.deps.state.updateProviders(providers as any[]) if (changed) { - this.send("providers-changed", { - providers: this.deps.state.providerList(), - }) + this.broadcastProviders() } } catch { // Silently ignore poll failures } } + /** + * Push the provider list plus in-flight job state. Sent on one channel so + * the two can't arrive out of order and render a provider as idle-and- + * uninitialized between an install finishing and its job clearing. + */ + broadcastProviders(): void { + this.send("providers-changed", { + providers: this.deps.state.providerList(), + jobs: this.deps.providerJobs.snapshot(), + }) + } + private async pollMachines(): Promise { try { const machines = await this.queryWithFallback( diff --git a/desktop/src/renderer/src/lib/components/provider/ProviderCard.svelte b/desktop/src/renderer/src/lib/components/provider/ProviderCard.svelte index aa7a06572..af5a04c9a 100644 --- a/desktop/src/renderer/src/lib/components/provider/ProviderCard.svelte +++ b/desktop/src/renderer/src/lib/components/provider/ProviderCard.svelte @@ -3,12 +3,13 @@ import { badgeVariants } from "$lib/components/ui/badge/index.js" import { Star, Loader2 } from "@lucide/svelte" import ProviderIcon from "./ProviderIcon.svelte" import { providerVersions } from "$lib/stores/providerVersions.js" -import { initializingProviders } from "$lib/stores/providers.js" +import { providerJobs } from "$lib/stores/providers.js" +import { providerStatus } from "$lib/utils/provider-status.js" import type { Provider } from "$lib/types/index.js" let { provider, onopen }: { provider: Provider; onopen?: () => void } = $props() -let isInitializing = $derived($initializingProviders.has(provider.name)) +let status = $derived(providerStatus(provider, $providerJobs[provider.name])) function sourceDisplay(p: Provider): string { if (p.source?.github) return p.source.github @@ -41,15 +42,19 @@ function sourceDisplay(p: Provider): string { {/if}
- {#if provider.state?.initialized} - initialized - {:else if isInitializing} + {#if status.kind === "ready"} + {status.label} + {:else if status.kind === "busy"} - initializing… + {status.label} + + {:else if status.kind === "failed"} + + {status.label} {:else} - not initialized + {status.label} {/if} {#if provider.version} {provider.version} diff --git a/desktop/src/renderer/src/lib/components/provider/ProviderCard.test.ts b/desktop/src/renderer/src/lib/components/provider/ProviderCard.test.ts index 6e47be242..e56b0fea5 100644 --- a/desktop/src/renderer/src/lib/components/provider/ProviderCard.test.ts +++ b/desktop/src/renderer/src/lib/components/provider/ProviderCard.test.ts @@ -14,11 +14,7 @@ vi.mock("$lib/stores/providerVersions.js", async () => { }) import ProviderCard from "./ProviderCard.svelte" -import { - initializingProviders, - markInitializing, - clearInitializing, -} from "$lib/stores/providers.js" +import { providerJobs } from "$lib/stores/providers.js" function makeProvider(name: string, extras: Partial = {}): Provider { return { @@ -56,8 +52,7 @@ describe("ProviderCard", () => { }) it("shows the initializing badge while an uninitialized provider is in flight", () => { - initializingProviders.set(new Set()) - markInitializing("ssh") + providerJobs.set({ ssh: { activity: "initializing", phase: "running_init" } }) const { container, unmount } = render(ProviderCard, { props: { provider: makeProvider("ssh", { state: { initialized: false } }) }, }) @@ -65,12 +60,44 @@ describe("ProviderCard", () => { const text = container.textContent ?? "" expect(text.toLowerCase()).toContain("initializing") expect(text.toLowerCase()).not.toContain("not initialized") - clearInitializing("ssh") + providerJobs.set({}) unmount() }) - it("shows not initialized when no init is in flight", () => { - initializingProviders.set(new Set()) + // The original bug: `provider add` persists the provider before init runs, + // so the watcher reports initialized:false while the install is in flight. + it("shows installing rather than not initialized during install", () => { + providerJobs.set({ + ssh: { activity: "installing", phase: "installing_provider" }, + }) + const { container, unmount } = render(ProviderCard, { + props: { provider: makeProvider("ssh", { state: { initialized: false } }) }, + }) + + const text = (container.textContent ?? "").toLowerCase() + expect(text).toContain("installing") + expect(text).not.toContain("not initialized") + providerJobs.set({}) + unmount() + }) + + it("shows a failure badge when the job recorded an error", () => { + providerJobs.set({ + ssh: { activity: "initializing", phase: "failed", error: "init: boom" }, + }) + const { container, unmount } = render(ProviderCard, { + props: { provider: makeProvider("ssh", { state: { initialized: false } }) }, + }) + + const text = (container.textContent ?? "").toLowerCase() + expect(text).toContain("failed") + expect(text).not.toContain("not initialized") + providerJobs.set({}) + unmount() + }) + + it("shows not initialized when no job is in flight", () => { + providerJobs.set({}) const { container, unmount } = render(ProviderCard, { props: { provider: makeProvider("ssh", { state: { initialized: false } }) }, }) diff --git a/desktop/src/renderer/src/lib/components/provider/ProviderSheet.svelte b/desktop/src/renderer/src/lib/components/provider/ProviderSheet.svelte index 5ae33b6da..9f8b8afe9 100644 --- a/desktop/src/renderer/src/lib/components/provider/ProviderSheet.svelte +++ b/desktop/src/renderer/src/lib/components/provider/ProviderSheet.svelte @@ -28,11 +28,7 @@ import { providerRename, providerSetVersion, } from "$lib/ipc/commands.js" -import { - providers, - markInitializing, - clearInitializing, -} from "$lib/stores/providers.js" +import { providers, providerJobs } from "$lib/stores/providers.js" import { providerVersions, loadVersionsFor, @@ -40,6 +36,7 @@ import { } from "$lib/stores/providerVersions.js" import { toasts } from "$lib/stores/toasts.js" import { extractErrorMessage } from "$lib/utils/error.js" +import { providerStatus } from "$lib/utils/provider-status.js" import type { Provider, ProviderOption } from "$lib/types/index.js" let { @@ -70,6 +67,7 @@ let updating = $state(false) let confirmSwitchOpen = $state(false) let targetTag = $state("") let switching = $state(false) +let status = $derived(providerStatus(provider, $providerJobs[provider.name])) function openVersionSwitch(tag: string) { targetTag = tag @@ -181,8 +179,11 @@ function handleUpdate() { async function runUpdate() { updating = true try { + // Also re-initializes: the new binaries have not run their init, and + // set-source clears the initialized flag accordingly. await providerUpdate(provider.name) toasts.success(`Updated ${provider.name}`) + providers.set(await providerList()) await loadVersionsFor(provider.name) await refreshUpdates() } catch (err) { @@ -198,6 +199,7 @@ async function runSwitch() { try { await providerSetVersion(provider.name, targetTag) toasts.success(`Switched ${provider.name} to ${targetTag}`) + providers.set(await providerList()) await loadVersionsFor(provider.name) await refreshUpdates() } catch (err) { @@ -227,7 +229,6 @@ function extractCliError(err: unknown): CLIError | null { async function handleInitialize() { initializing = true initError = null - markInitializing(provider.name) try { await providerInit(provider.name) const updated = await providerList() @@ -248,7 +249,6 @@ async function handleInitialize() { } } finally { initializing = false - clearInitializing(provider.name) } } @@ -367,8 +367,19 @@ async function handleSaveOptions() { Default {/if} - {#if provider.state?.initialized} - initialized + {#if status.kind === "ready"} + {status.label} + {:else if status.kind === "busy"} + + + {status.label} + + {:else if status.kind === "failed"} + + {status.label} + + {:else} + {status.label} {/if} {#if provider.description} @@ -377,7 +388,7 @@ async function handleSaveOptions() {
- {#if provider.state?.initialized !== true} + {#if status.kind !== "ready" && status.kind !== "busy"}