From bf75409b6f24d0aed0468fdec39ddb7ecbba839a Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 28 Aug 2026 05:33:57 -0700 Subject: [PATCH 1/4] Add tests for plugins page tenant-switch race Proves a fast bench switch doesn't let a slower tenant's late plugins/skills fetch overwrite the newly selected tenant's state. --- apps/web/test/plugins-page.test.tsx | 143 +++++++++++++++++++++++++++- 1 file changed, 141 insertions(+), 2 deletions(-) diff --git a/apps/web/test/plugins-page.test.tsx b/apps/web/test/plugins-page.test.tsx index 1f92dc530..ab6e4104a 100644 --- a/apps/web/test/plugins-page.test.tsx +++ b/apps/web/test/plugins-page.test.tsx @@ -6,11 +6,13 @@ // data into that component correctly. import { afterEach, describe, expect, test } from "bun:test"; -import { act } from "react"; +import { act, useState } from "react"; +import type { ReactNode } from "react"; import { createRoot } from "react-dom/client"; import type { Root } from "react-dom/client"; -import { BenchProvider } from "../src/bench-context"; +import type { BenchState } from "../src/bench-context"; +import { BenchContext, BenchProvider } from "../src/bench-context"; import { NavigationProvider } from "../src/navigation"; import { PluginsRoute } from "../src/pages/plugins-page"; import { @@ -412,4 +414,141 @@ describe("PluginsRoute", () => { expect(navigated).toContain("/skills/summarize"); }); + + // CL-7138: a fast bench switch used to let the previous tenant's + // in-flight fetch resolve after the new tenant's, overwriting its data. + test("a fast bench switch never lets the previous tenant's late fetch overwrite the new one's data", async () => { + function deferredResponse() { + let resolve: (response: Response) => void = () => undefined; + const promise = new Promise((res) => { + resolve = res; + }); + return { promise, resolve }; + } + + const skillsDeferred: Record< + string, + ReturnType + > = { + tnt_a: deferredResponse(), + tnt_b: deferredResponse(), + }; + + globalThis.fetch = ((input: RequestInfo | URL) => { + const path = typeof input === "string" ? input : String(input); + if (path.includes("/credentials/resolve/")) + return Promise.resolve(json(null, 404)); + if (path.includes("/connections/provider-health")) + return Promise.resolve( + json({ providers: {}, connectedProviderCount: 0 }), + ); + const skillsMatch = /\/api\/tenants\/(tnt_[ab])\/skills/.exec(path); + if (skillsMatch) { + const tenantId = skillsMatch[1] as string; + return ( + skillsDeferred[tenantId]?.promise ?? + Promise.resolve(json({ skills: [] })) + ); + } + return Promise.resolve(json({ data: [], nextCursor: null })); + }) as typeof fetch; + + let selectTenant: (tenantId: string) => void = () => undefined; + + function BenchHarness({ children }: { readonly children: ReactNode }) { + const [tenantId, setTenantId] = useState("tnt_a"); + selectTenant = setTenantId; + const value: BenchState = { + memberships: { kind: "loading" }, + selectedTenantId: tenantId, + selectedPrincipalId: "prn_1", + selectTenant: setTenantId, + onBenchCreated: () => undefined, + }; + return ( + {children} + ); + } + + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); + await act(async () => { + root?.render( + + undefined}> + + + undefined} /> + + + + , + ); + }); + for (let i = 0; i < 5; i++) { + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + } + + act(() => { + selectTenant("tnt_b"); + }); + for (let i = 0; i < 5; i++) { + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + } + + skillsDeferred.tnt_b?.resolve( + json({ + skills: [ + { + assetId: "skill_b", + name: "beta-only", + description: "Tenant B's skill.", + scope: "tenant", + creatorPrincipalId: "prn_1", + updatedAtIso: "2026-01-01T00:00:00.000Z", + }, + ], + }), + ); + for (let i = 0; i < 10; i++) { + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + } + + skillsDeferred.tnt_a?.resolve( + json({ + skills: [ + { + assetId: "skill_a", + name: "alpha-only", + description: "Tenant A's skill.", + scope: "tenant", + creatorPrincipalId: "prn_1", + updatedAtIso: "2026-01-01T00:00:00.000Z", + }, + ], + }), + ); + for (let i = 0; i < 10; i++) { + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + } + + const skillsTab = [...container.querySelectorAll("button")].find( + (button) => button.textContent?.includes("Skills") === true, + ); + act(() => { + skillsTab?.dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); + + expect(container.textContent).toContain("beta-only"); + expect(container.textContent).not.toContain("alpha-only"); + }); }); From 48336205d6c6ddb1179dc4bcf165eadb8efec7e4 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 28 Aug 2026 05:33:58 -0700 Subject: [PATCH 2/4] Plugins page: guard reload effects against a stale tenant's late fetch reloadPlugins/reloadSkills had no cancellation, so switching benches while a fetch was still in flight could let the previous tenant's response land after the new tenant's and overwrite its state. Both effects now track a cancelled flag in their cleanup, the same pattern people-section.tsx already uses, and the effect itself owns re-fetching (via a reload key) so the existing imperative reloadPlugins/reloadSkills call sites still refresh the current tenant. Fixes CL-7138. --- apps/web/src/pages/plugins-page.tsx | 56 +++++++++++++++++++---------- 1 file changed, 38 insertions(+), 18 deletions(-) diff --git a/apps/web/src/pages/plugins-page.tsx b/apps/web/src/pages/plugins-page.tsx index bc7bad089..8bb88f9c2 100644 --- a/apps/web/src/pages/plugins-page.tsx +++ b/apps/web/src/pages/plugins-page.tsx @@ -92,33 +92,53 @@ export function PluginsRoute({ setConnectDeepLinkNotFound(false); }, []); + const [pluginsReloadKey, setPluginsReloadKey] = useState(0); + const [skillsReloadKey, setSkillsReloadKey] = useState(0); + const reloadPlugins = useCallback(() => { - if (selectedTenantId === null) return; - listPluginsForTenant(selectedTenantId) - .then((plugins) => setPluginsState({ status: "ready", plugins })) - .catch((cause: unknown) => - setPluginsState({ status: "error", message: messageOf(cause) }), - ); - }, [selectedTenantId]); + setPluginsReloadKey((key) => key + 1); + }, []); const reloadSkills = useCallback(() => { - if (selectedTenantId === null) return; - listSkills(selectedTenantId) - .then((skills) => setSkillsState({ status: "ready", skills })) - .catch((cause: unknown) => - setSkillsState({ status: "error", message: messageOf(cause) }), - ); - }, [selectedTenantId]); + setSkillsReloadKey((key) => key + 1); + }, []); + // Guarded by `cancelled` (same pattern as settings-ui's `people-section`) + // so a tenant switch mid-flight can't have the previous tenant's late + // response overwrite the newly selected tenant's state. useEffect(() => { + if (selectedTenantId === null) return; + let cancelled = false; setPluginsState({ status: "loading" }); - reloadPlugins(); - }, [reloadPlugins]); + listPluginsForTenant(selectedTenantId) + .then((plugins) => { + if (!cancelled) setPluginsState({ status: "ready", plugins }); + }) + .catch((cause: unknown) => { + if (!cancelled) + setPluginsState({ status: "error", message: messageOf(cause) }); + }); + return () => { + cancelled = true; + }; + }, [selectedTenantId, pluginsReloadKey]); useEffect(() => { + if (selectedTenantId === null) return; + let cancelled = false; setSkillsState({ status: "loading" }); - reloadSkills(); - }, [reloadSkills]); + listSkills(selectedTenantId) + .then((skills) => { + if (!cancelled) setSkillsState({ status: "ready", skills }); + }) + .catch((cause: unknown) => { + if (!cancelled) + setSkillsState({ status: "error", message: messageOf(cause) }); + }); + return () => { + cancelled = true; + }; + }, [selectedTenantId, skillsReloadKey]); // The shell banner's "Fix it" deep link (CL-6092): once the gallery has // loaded, pick up any pending provider id and open its connect panel — From 671564f099a27cf852e5ca051f904eb5a2561b3e Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 28 Aug 2026 05:49:31 -0700 Subject: [PATCH 3/4] Add tests for plugins page reload not tearing down to the loading skeleton Proves a connect/disconnect-triggered reload keeps the gallery on screen and swaps in fresh data when it lands, instead of tearing down to the full loading skeleton on every imperative reload. --- apps/web/test/plugins-page.test.tsx | 109 ++++++++++++++++++++++++++++ 1 file changed, 109 insertions(+) diff --git a/apps/web/test/plugins-page.test.tsx b/apps/web/test/plugins-page.test.tsx index ab6e4104a..e47281b92 100644 --- a/apps/web/test/plugins-page.test.tsx +++ b/apps/web/test/plugins-page.test.tsx @@ -551,4 +551,113 @@ describe("PluginsRoute", () => { expect(container.textContent).toContain("beta-only"); expect(container.textContent).not.toContain("alpha-only"); }); + + // CL-7138: cancellation must not turn every imperative reload (a + // connect/disconnect panel's `onChanged`, the error screen's Retry) into + // a full teardown to the loading skeleton — only a genuine tenant change + // should do that. This drives the real disconnect flow end to end. + test("disconnecting a plugin reloads without tearing the gallery down to the loading skeleton", async () => { + let deferCredentialResolves = false; + const deferredResolvers: (() => void)[] = []; + const githubCredential = { + id: "cred_1", + tenantId: "tnt_1", + providerId: "prov_1", + principalId: null, + oauthClientId: null, + name: "GitHub", + type: "api_key", + description: null, + scopes: [], + expiresAt: null, + status: "active", + metadata: {}, + createdAt: "2026-01-01T00:00:00.000Z", + updatedAt: "2026-01-01T00:00:00.000Z", + }; + + globalThis.fetch = ((input: RequestInfo | URL, init?: RequestInit) => { + const path = typeof input === "string" ? input : String(input); + const method = (init?.method ?? "GET").toUpperCase(); + if (method === "DELETE" && path.includes("/credentials/cred_1")) + return Promise.resolve(new Response(null, { status: 204 })); + if (path.includes("/api/me/principals")) + return Promise.resolve(json(membership)); + if (path.includes("/api/workbench-tenancies/kinds")) + return Promise.resolve(json({ workbenchTenantIds: [] })); + if (path.includes("/credentials/resolve/GitHub")) { + if (deferCredentialResolves) { + return new Promise((resolve) => { + deferredResolvers.push(() => resolve(json(null, 404))); + }); + } + return Promise.resolve(json(githubCredential)); + } + if (path.includes("/connections/provider-health")) + return Promise.resolve( + json({ providers: {}, connectedProviderCount: 1 }), + ); + if (path.includes("/credentials/resolve/")) + return Promise.resolve(json(null, 404)); + if (path.includes("/api/tenants/tnt_1/skills")) + return Promise.resolve(json({ skills: [] })); + return Promise.resolve(json({ data: [], nextCursor: null })); + }) as typeof fetch; + + const el = await mount(); + expect(el.textContent).toContain("Connected here"); + + const manageButton = el.querySelector( + 'button[aria-label="Manage GitHub"]', + ); + await act(async () => { + manageButton?.click(); + }); + for (let i = 0; i < 5; i++) { + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + } + + const disconnectButton = () => + [...document.body.querySelectorAll("button")].find( + (button) => button.textContent?.includes("Disconnect") === true, + ); + expect(disconnectButton()).not.toBeUndefined(); + + // From here on, the reload the disconnect triggers must not resolve + // until we say so — gives us a window to inspect the mid-reload DOM. + deferCredentialResolves = true; + + // First click arms the confirm button, second click fires the delete. + await act(async () => { + disconnectButton()?.dispatchEvent( + new MouseEvent("click", { bubbles: true }), + ); + }); + await act(async () => { + disconnectButton()?.dispatchEvent( + new MouseEvent("click", { bubbles: true }), + ); + }); + for (let i = 0; i < 5; i++) { + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + } + + // The delete resolved and `onChanged` fired `reloadPlugins`; its fetch + // is now the deferred one above, still pending. + expect(el.textContent).not.toContain("Loading plugins…"); + expect(el.textContent).toContain("GitHub"); + + deferredResolvers.forEach((resolve) => resolve()); + for (let i = 0; i < 10; i++) { + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + } + + expect(el.textContent).not.toContain("Connected here"); + }); }); From 8091fa5087fa95eccdccd268a8e1baa5e8ffaeab Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 28 Aug 2026 05:49:31 -0700 Subject: [PATCH 4/4] Plugins page: only show the loading skeleton on an actual tenant change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reload-key restructure from the previous commit made every imperative reload (a connect/disconnect panel's onChanged, the error screen's Retry) re-run the fetch effect, which unconditionally set status: "loading" and tore the page down to the full skeleton — a regression from main, where an imperative reload just fetched and swapped in the result. Each effect now tracks the tenant it last fetched for in a ref and only shows the loading state when selectedTenantId actually changed; a same-tenant reload keeps whatever is on screen until the fresh data (or a new error) lands. Fixes CL-7138. --- apps/web/src/pages/plugins-page.tsx | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/apps/web/src/pages/plugins-page.tsx b/apps/web/src/pages/plugins-page.tsx index 8bb88f9c2..31979f871 100644 --- a/apps/web/src/pages/plugins-page.tsx +++ b/apps/web/src/pages/plugins-page.tsx @@ -24,7 +24,7 @@ import { import type { ResolvedPlugin } from "@workbench/connections/plugins"; import { listPluginsForTenant } from "@workbench/connections/plugins"; import { Plus, SquaresFour, Warning } from "@corbits/icons"; -import { useCallback, useEffect, useState } from "react"; +import { useCallback, useEffect, useRef, useState } from "react"; import { useBench } from "../bench-context"; import { SKILLS_PATH_PREFIX } from "../path-ids"; @@ -105,11 +105,21 @@ export function PluginsRoute({ // Guarded by `cancelled` (same pattern as settings-ui's `people-section`) // so a tenant switch mid-flight can't have the previous tenant's late - // response overwrite the newly selected tenant's state. + // response overwrite the newly selected tenant's state. The loading + // skeleton only shows for a tenant this page hasn't fetched yet — an + // imperative reload (a connect/disconnect's `onChanged`, the error + // screen's Retry) keeps whatever is already on screen and swaps in the + // fresh data once it lands, the same way it worked before cancellation + // was added. + const pluginsLoadedTenantRef = useRef(null); + const skillsLoadedTenantRef = useRef(null); + useEffect(() => { if (selectedTenantId === null) return; let cancelled = false; - setPluginsState({ status: "loading" }); + const isTenantChange = pluginsLoadedTenantRef.current !== selectedTenantId; + pluginsLoadedTenantRef.current = selectedTenantId; + if (isTenantChange) setPluginsState({ status: "loading" }); listPluginsForTenant(selectedTenantId) .then((plugins) => { if (!cancelled) setPluginsState({ status: "ready", plugins }); @@ -126,7 +136,9 @@ export function PluginsRoute({ useEffect(() => { if (selectedTenantId === null) return; let cancelled = false; - setSkillsState({ status: "loading" }); + const isTenantChange = skillsLoadedTenantRef.current !== selectedTenantId; + skillsLoadedTenantRef.current = selectedTenantId; + if (isTenantChange) setSkillsState({ status: "loading" }); listSkills(selectedTenantId) .then((skills) => { if (!cancelled) setSkillsState({ status: "ready", skills });