molecule-ai/molecule-core
35
0
Fork 2
Code Issues 16 Pull Requests 2 Actions 49 Packages Projects Releases Wiki Activity

fix(canvas): plug timer leak + optimistic-install semantics in SkillsTab

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
Hongming Wang 2026-04-24 22:47:46 -07:00
parent deedb5eff6
commit e0f338e8ae
2 changed files with 166 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

143
canvas/src/components/__tests__/SkillsTab.install.test.tsx Normal file
View File

@ -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<string, unknown>` 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<string, unknown>) => unknown) =>
selector({ setPanelTab: vi.fn() } as Record<string, unknown>),
),
{ 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/<workspaceId>/plugins (no `undefined` in URL)", async () => {
render(<SkillsTab workspaceId="ws-abc-123" data={makeData() as never} />);
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(<SkillsTab workspaceId="ws-abc-123" data={makeData() as never} />);
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();
});
});

25
canvas/src/components/tabs/SkillsTab.tsx
View File

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