From e0f338e8aeda7982afbeebb4f437784efa81ffd3 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Fri, 24 Apr 2026 22:47:46 -0700 Subject: [PATCH] fix(canvas): plug timer leak + optimistic-install semantics in SkillsTab MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review-driven fixes plus regression coverage for the bugs landed in 176b703d / deedb5ef: 1. clearTimeout the prior reload handle before scheduling a new one in both installFromSource and handleUninstall. Two installs within the PLUGIN_RELOAD_DELAY_MS window (15s) used to queue two loadInstalled() calls; the unmount cleanup only cleared the latest handle, and the second reconciliation could overwrite a still- correct optimistic state with a stale snapshot mid-restart. 2. Drop `setInstalledLoaded(true)` from the optimistic block. That flag's contract is "the initial GET has succeeded at least once" — it gates the auto-expand-registry effect. A user installing a custom-source plugin BEFORE the initial fetch returned would flip the gate prematurely, the auto-expand would never fire, and a followup loadInstalled racing with the optimistic write could overwrite our entry with [] mid-restart. 3. Don't force `supported_on_runtime: true` on the optimistic record. The "inert on this runtime" badge in the row renders on the value `=== false`. Forcing true would hide the badge for 15s if the user installed a plugin that doesn't actually support the workspace's runtime; the real value lands at refetch. Leaving the field undefined keeps the badge neutral until reconciliation arrives. Plus a behavioral test (SkillsTab.install.test.tsx) that asserts: - the install POST URL contains the workspaceId (not "undefined") - the row's "Install" button is replaced by the green "Installed" tag synchronously after POST resolves, without advancing any timer — locks in the optimistic-update contract so a future refactor can't silently regress it. 995 canvas tests pass (2 new); tsc clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../__tests__/SkillsTab.install.test.tsx | 143 ++++++++++++++++++ canvas/src/components/tabs/SkillsTab.tsx | 25 ++- 2 files changed, 166 insertions(+), 2 deletions(-) create mode 100644 canvas/src/components/__tests__/SkillsTab.install.test.tsx diff --git a/canvas/src/components/__tests__/SkillsTab.install.test.tsx b/canvas/src/components/__tests__/SkillsTab.install.test.tsx new file mode 100644 index 00000000..24f48b7d --- /dev/null +++ b/canvas/src/components/__tests__/SkillsTab.install.test.tsx @@ -0,0 +1,143 @@ +// @vitest-environment jsdom +// +// Behavioral coverage for the install flow. Two regressions to pin +// down: +// +// 1. The install POST URL has to include the workspace id. A pre-fix +// bug routed it to /workspaces/undefined/plugins because the +// component read `data.id`, but `WorkspaceNodeData` has no `id` +// field — its `extends Record` index signature +// hid the bad access from TS. The component now takes +// `workspaceId` as an explicit prop; this test asserts the URL. +// +// 2. The optimistic install update has to flip the registry row to +// "Installed" without waiting for the 15s reload timer (the +// PLUGIN_RELOAD_DELAY_MS gap). This test asserts the row's "Install" +// button is replaced by the green "Installed" tag synchronously +// after the POST resolves. +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { render, screen, fireEvent, waitFor, cleanup } from "@testing-library/react"; + +const mockApiGet = vi.fn(); +const mockApiPost = vi.fn(); +vi.mock("@/lib/api", () => ({ + api: { + get: (...args: unknown[]) => mockApiGet(...args), + post: (...args: unknown[]) => mockApiPost(...args), + put: vi.fn().mockResolvedValue({}), + del: vi.fn().mockResolvedValue({}), + patch: vi.fn().mockResolvedValue({}), + }, +})); + +vi.mock("@/store/canvas", () => ({ + useCanvasStore: Object.assign( + vi.fn((selector: (s: Record) => unknown) => + selector({ setPanelTab: vi.fn() } as Record), + ), + { getState: () => ({ setPanelTab: vi.fn() }) }, + ), + summarizeWorkspaceCapabilities: vi.fn(() => ({ skills: [], tools: [] })), +})); + +vi.mock("../Toaster", () => ({ showToast: vi.fn() })); + +import { SkillsTab } from "../tabs/SkillsTab"; + +function makeData() { + return { + name: "Test WS", + status: "online", + tier: 1, + agentCard: null, + activeTasks: 0, + collapsed: false, + role: "agent", + lastErrorRate: 0, + lastSampleError: "", + url: "http://localhost:9000", + parentId: null, + currentTask: "", + runtime: "langgraph", + needsRestart: false, + budgetLimit: null, + }; +} + +const REGISTRY = [ + { + name: "browser-automation", + version: "1.1.0", + description: "Browser automation + testing", + author: "molecule", + tags: ["browser", "playwright"], + skills: [], + runtimes: ["claude-code"], + }, +]; + +beforeEach(() => { + // Order matches the component's loadInstalled / loadRegistry + // /loadSourceSchemes calls. Schemes endpoint resolves with an + // empty list so the Install-from-source input doesn't blow up. + mockApiGet.mockReset(); + mockApiPost.mockReset(); + mockApiGet.mockImplementation((path: string) => { + if (path.endsWith("/plugins") && path.startsWith("/workspaces/")) { + return Promise.resolve([]); // installed + } + if (path === "/plugins") { + return Promise.resolve(REGISTRY); // registry + } + if (path === "/plugins/sources") { + return Promise.resolve({ schemes: ["github://", "local://"] }); + } + return Promise.resolve(null); + }); + mockApiPost.mockResolvedValue({ status: "installed", plugin: "browser-automation" }); +}); + +afterEach(() => { + cleanup(); + vi.clearAllMocks(); +}); + +// Returns the registry row's Install button. The custom-source input +// also renders an "Install" button, so `findByRole({name: /install/})` +// throws on multiple matches; scope by the row's plugin-name text. +async function findRowInstallButton() { + const nameNode = await screen.findByText("browser-automation"); + const row = nameNode.closest("div.flex.items-center.justify-between") as HTMLElement; + if (!row) throw new Error("could not locate row container for browser-automation"); + const buttons = row.querySelectorAll("button"); + const install = Array.from(buttons).find((b) => b.textContent?.trim() === "Install"); + if (!install) throw new Error("row has no Install button (already installed?)"); + return install; +} + +describe("SkillsTab install flow", () => { + it("POSTs to /workspaces//plugins (no `undefined` in URL)", async () => { + render(); + + fireEvent.click(await findRowInstallButton()); + + await waitFor(() => expect(mockApiPost).toHaveBeenCalled()); + expect(mockApiPost).toHaveBeenCalledWith( + "/workspaces/ws-abc-123/plugins", + { source: "local://browser-automation" }, + ); + }); + + it("flips the registry row to 'Installed' synchronously after POST resolves (no 15s wait)", async () => { + render(); + + fireEvent.click(await findRowInstallButton()); + + // The "Installed" green tag must appear without advancing the + // reload timer — the optimistic update is the entire point of + // this fix. If this test ever regresses to needing fake timers + // + advanceTimersByTime, the optimistic path is broken. + const installedTag = await screen.findByText(/^Installed$/i); + expect(installedTag).toBeDefined(); + }); +}); diff --git a/canvas/src/components/tabs/SkillsTab.tsx b/canvas/src/components/tabs/SkillsTab.tsx index e6310352..83207e10 100644 --- a/canvas/src/components/tabs/SkillsTab.tsx +++ b/canvas/src/components/tabs/SkillsTab.tsx @@ -235,13 +235,31 @@ export function SkillsTab({ workspaceId, data }: Props) { await api.post(`/workspaces/${workspaceId}/plugins`, { source }); showToast(`Installed ${label} — restarting workspace`, "success"); if (optimistic && mountedRef.current) { + // Push with `supported_on_runtime` left undefined — the + // server's ListInstalled annotates the real value (true / + // false) at refetch time. Forcing `true` here would hide the + // "inert on this runtime" badge for 15s if the user + // installed a plugin that doesn't actually support the + // workspace's runtime; the badge only renders on `=== false`, + // so undefined keeps it neutral until reconciliation arrives. setInstalled((prev) => prev.some((p) => p.name === optimistic.name) ? prev - : [...prev, { ...optimistic, supported_on_runtime: true }], + : [...prev, { ...optimistic, supported_on_runtime: undefined }], ); - setInstalledLoaded(true); + // Note: we intentionally do NOT set `installedLoaded` here. + // That flag means "the initial GET has succeeded at least + // once" and gates the auto-expand-registry effect. A fast + // optimistic install BEFORE the initial fetch returns must + // not flip the gate, or the auto-expand never fires and a + // followup loadInstalled racing with the optimistic write + // could overwrite our entry with [] mid-restart. } + // Drop any prior reload timer before scheduling a new one — + // back-to-back installs within PLUGIN_RELOAD_DELAY_MS would + // otherwise queue multiple loadInstalled() calls and the + // unmount cleanup only clears the latest handle. + clearTimeout(reloadTimerRef.current); reloadTimerRef.current = setTimeout(() => loadInstalled(), PLUGIN_RELOAD_DELAY_MS); } catch (e) { showToast(e instanceof Error ? e.message : "Install failed", "error"); @@ -268,6 +286,9 @@ export function SkillsTab({ workspaceId, data }: Props) { await api.del(`/workspaces/${workspaceId}/plugins/${pluginName}`); showToast(`Removed ${pluginName} — restarting workspace`, "success"); setInstalled((prev) => prev.filter((p) => p.name !== pluginName)); + // Drop any prior reload timer (see installFromSource for the + // back-to-back-action leak rationale). + clearTimeout(reloadTimerRef.current); reloadTimerRef.current = setTimeout(() => loadInstalled(), PLUGIN_RELOAD_DELAY_MS); } catch (e) { showToast(e instanceof Error ? e.message : "Uninstall failed", "error");