From bc0b5a1642f0bd4529c42606d5dd8e4001f5cb2a Mon Sep 17 00:00:00 2001 From: Tonio Date: Fri, 14 Aug 2026 10:27:59 -0700 Subject: [PATCH] fix(ui): onboarding wizard keeps an invisible disabled adapter selected (#11371) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The wizard defaults `adapterType` to `claude_local`, and a saved draft can name any adapter. The grid only renders adapters the server has enabled, so on an instance where the held adapter is disabled nothing appears selected while the wizard still holds it — and the first agent is hired on an adapter the deployer turned off, which can never acquire a lease. The selection now snaps to the first enabled, non-coming-soon adapter whenever the held one is not visible, and adapter-specific model defaults follow it. The snap waits for the adapter registry to load. External adapter types are registered into the UI registry only once the adapters query resolves, so before that a saved external adapter is indistinguishable from a disabled one — snapping on that transient list would replace the customer's choice with a built-in and the persist effect would write it down. This gate fails closed, unlike the fail-open gates in onboarding, because the directions of harm are opposite: acting early silently rewrites a saved answer, while waiting merely leaves the selection alone, which is the behaviour that existed before the snap did. The test file is named `OnboardingWizard.adapters.test.tsx` rather than `OnboardingWizard.test.tsx`, which is the name #11370 uses for its restore gate. Both merged cleanly onto master alone but collided with each other on add/add, and nothing in either status showed it. Lands the work from #9900's sibling, #9501, by @stubbi, whose commit is included unchanged with their authorship. The rename and the registry gate are mine. Tested: ui typecheck clean; 51 pass across the adapter, hook, dialog, config-form and wizard-step suites, including the other callers of the adapter hook since that module changed. All CI gates green; Greptile 5/5. Co-Authored-By: Claude Opus 5 --- ui/src/adapters/use-disabled-adapters.ts | 31 +++ .../OnboardingWizard.adapters.test.tsx | 191 ++++++++++++++++++ ui/src/components/OnboardingWizard.tsx | 36 +++- 3 files changed, 257 insertions(+), 1 deletion(-) create mode 100644 ui/src/components/OnboardingWizard.adapters.test.tsx diff --git a/ui/src/adapters/use-disabled-adapters.ts b/ui/src/adapters/use-disabled-adapters.ts index d3140fed6d..c1f9467d26 100644 --- a/ui/src/adapters/use-disabled-adapters.ts +++ b/ui/src/adapters/use-disabled-adapters.ts @@ -54,3 +54,34 @@ export function useDisabledAdaptersSync(options: { enabled?: boolean } = {}): Se [adapters], ); } + +/** + * Whether the adapter list has arrived, so callers can tell "this instance + * does not offer that adapter" from "the registry has not loaded yet". + * + * External adapter types are registered into the UI registry by + * {@link useDisabledAdaptersSync} only once the query resolves. Until then + * `listUIAdapters()` returns the built-ins alone, so an external adapter looks + * exactly like one the deployer has disabled. + * + * Deliberately reports arrival rather than settlement, unlike the fail-open + * gates elsewhere in onboarding. The directions of harm are opposite here. A + * caller that acts on an unloaded registry replaces the customer's chosen + * adapter with a built-in and persists that choice; a caller that waits + * forever simply leaves the selection alone, which is the behaviour that + * existed before any of this. Silently changing a saved answer is the error + * worth refusing to make. + * + * Reads the same query key as {@link useDisabledAdaptersSync}, so it shares + * that cache entry rather than adding a request. + */ +export function useAdapterRegistryLoaded(options: { enabled?: boolean } = {}): boolean { + const enabled = options.enabled ?? true; + const { data: adapters } = useQuery({ + queryKey: queryKeys.adapters.all, + queryFn: () => adaptersApi.list(), + enabled, + staleTime: 5 * 60 * 1000, + }); + return adapters !== undefined; +} diff --git a/ui/src/components/OnboardingWizard.adapters.test.tsx b/ui/src/components/OnboardingWizard.adapters.test.tsx new file mode 100644 index 0000000000..5796793f25 --- /dev/null +++ b/ui/src/components/OnboardingWizard.adapters.test.tsx @@ -0,0 +1,191 @@ +// @vitest-environment jsdom + +import { act } from "react"; +import { createRoot } from "react-dom/client"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +// --- Mocks (hoisted so vi.mock factories can close over them) ---------------- + +const ONBOARDING_STORAGE_KEY = "paperclip-onboarding-state"; + +const mockDialog = vi.hoisted(() => ({ + onboardingOpen: true, + onboardingOptions: {} as { initialStep?: number; companyId?: string }, + closeOnboarding: vi.fn(), + onboardingRouteDismissed: false, + setOnboardingRouteDismissed: vi.fn(), +})); + +const mockCompany = vi.hoisted(() => ({ + companies: [] as Array<{ id: string; name: string; issuePrefix: string }>, + setSelectedCompanyId: vi.fn(), + loading: false, +})); + +// The real adapter registry eagerly imports every adapter package. The +// model/harness picker internals are out of scope here, so stub the adapter +// layer entirely and drive the grid through these two knobs. +const mockAdapterRegistry = vi.hoisted(() => ({ + list: [] as Array<{ type: string }>, + disabled: new Set(), + loaded: true, +})); + +vi.mock("@/lib/router", () => ({ + useLocation: () => ({ pathname: "/", search: "", hash: "", state: null }), + useNavigate: () => vi.fn(), + useParams: () => ({}), +})); +vi.mock("../context/DialogContext", () => ({ + useDialog: () => mockDialog, +})); +vi.mock("../context/CompanyContext", () => ({ + useCompany: () => mockCompany, +})); +vi.mock("../adapters", () => ({ + listUIAdapters: () => mockAdapterRegistry.list, + getUIAdapter: () => ({ buildAdapterConfig: () => ({}) }), +})); +vi.mock("../adapters/metadata", () => ({ isVisualAdapterChoice: () => true })); +vi.mock("../adapters/adapter-display-registry", () => ({ + getAdapterDisplay: (type: string) => ({ + type, + recommended: false, + label: type, + description: "", + icon: () => null, + }), +})); +vi.mock("../adapters/use-disabled-adapters", () => ({ + useDisabledAdaptersSync: () => mockAdapterRegistry.disabled, + useAdapterRegistryLoaded: () => mockAdapterRegistry.loaded, +})); +vi.mock("../adapters/use-adapter-capabilities", () => ({ + useAdapterCapabilities: () => () => ({ + supportsInstructionsBundle: false, + supportsSkills: false, + supportsLocalAgentJwt: false, + requiresMaterializedRuntimeSkills: false, + supportsModelProfiles: false, + }), +})); +// Animation / canvas-ish children that add nothing to the logic under test. +vi.mock("./AsciiArtAnimation", () => ({ AsciiArtAnimation: () => null })); +vi.mock("./FrontDoor", () => ({ FrontDoor: () => null })); +vi.mock("./AgentCapsule", () => ({ AgentCapsule: () => null })); + +import { OnboardingWizard } from "./OnboardingWizard"; + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +(globalThis as any).IS_REACT_ACT_ENVIRONMENT = true; + +async function flushReact() { + await act(async () => { + await Promise.resolve(); + await new Promise((resolve) => window.setTimeout(resolve, 0)); + }); +} + +async function mount() { + const container = document.createElement("div"); + document.body.appendChild(container); + const root = createRoot(container); + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + await act(async () => { + root.render( + + + , + ); + }); + await flushReact(); + return { container, root }; +} + +describe("OnboardingWizard adapter selection", () => { + beforeEach(() => { + window.localStorage.clear(); + mockDialog.onboardingOpen = true; + mockDialog.onboardingOptions = {}; + mockCompany.companies = []; + mockAdapterRegistry.list = []; + mockAdapterRegistry.disabled = new Set(); + mockAdapterRegistry.loaded = true; + }); + + afterEach(() => { + document.body.innerHTML = ""; + vi.clearAllMocks(); + }); + + it("snaps a disabled default adapterType to the first enabled adapter", async () => { + // A deployment whose adapter registry omits claude_local disables it, so + // the wizard's claude_local default must not survive as an invisible + // selection (the created agent could never acquire a lease). + mockAdapterRegistry.list = [ + { type: "claude_local" }, + { type: "codex_local" }, + { type: "opencode_local" }, + ]; + mockAdapterRegistry.disabled = new Set(["claude_local"]); + + const { root } = await mount(); + + const saved = JSON.parse( + window.localStorage.getItem(ONBOARDING_STORAGE_KEY) ?? "{}", + ); + expect(saved.adapterType).toBe("codex_local"); + + await act(async () => { + root.unmount(); + }); + }); + + it("keeps an enabled saved adapterType untouched", async () => { + mockAdapterRegistry.list = [ + { type: "claude_local" }, + { type: "codex_local" }, + ]; + window.localStorage.setItem( + ONBOARDING_STORAGE_KEY, + JSON.stringify({ step: 0, adapterType: "claude_local" }), + ); + + const { root } = await mount(); + + const saved = JSON.parse( + window.localStorage.getItem(ONBOARDING_STORAGE_KEY) ?? "{}", + ); + expect(saved.adapterType).toBe("claude_local"); + + await act(async () => { + root.unmount(); + }); + }); + it("does not replace a saved adapter before the registry has loaded", async () => { + // External adapter types are only registered once the adapters query + // resolves. Until then `listUIAdapters()` returns the built-ins alone, so + // a saved external adapter looks exactly like a disabled one — and + // snapping would swap the customer's choice for a built-in and persist it. + window.localStorage.setItem( + ONBOARDING_STORAGE_KEY, + JSON.stringify({ step: 0, adapterType: "acme_external" }), + ); + mockAdapterRegistry.loaded = false; + mockAdapterRegistry.list = [{ type: "codex_local" }]; + + const { root } = await mount(); + + const saved = JSON.parse( + window.localStorage.getItem(ONBOARDING_STORAGE_KEY) ?? "{}", + ); + expect(saved.adapterType).toBe("acme_external"); + + await act(async () => { + root.unmount(); + }); + }); +}); diff --git a/ui/src/components/OnboardingWizard.tsx b/ui/src/components/OnboardingWizard.tsx index bdf0bd3c70..3028262ac5 100644 --- a/ui/src/components/OnboardingWizard.tsx +++ b/ui/src/components/OnboardingWizard.tsx @@ -26,7 +26,7 @@ import { import { getUIAdapter } from "../adapters"; import { listUIAdapters } from "../adapters"; import { isVisualAdapterChoice } from "../adapters/metadata"; -import { useDisabledAdaptersSync } from "../adapters/use-disabled-adapters"; +import { useDisabledAdaptersSync, useAdapterRegistryLoaded } from "../adapters/use-disabled-adapters"; import { useAdapterCapabilities } from "../adapters/use-adapter-capabilities"; import { getAdapterDisplay } from "../adapters/adapter-display-registry"; import { defaultCreateValues } from "./agent-config-defaults"; @@ -181,6 +181,7 @@ export function OnboardingWizard() { // mounted globally, including on /auth, where protected adapter routes are // expected to reject signed-out browsers. const disabledTypes = useDisabledAdaptersSync({ enabled: effectiveOnboardingOpen }); + const adapterRegistryLoaded = useAdapterRegistryLoaded({ enabled: effectiveOnboardingOpen }); const initialStep = effectiveOnboardingOptions.initialStep ?? 0; const existingCompanyId = effectiveOnboardingOptions.companyId; @@ -452,6 +453,39 @@ export function OnboardingWizard() { }; }, [disabledTypes]); + // The default (or a saved) adapterType can name an adapter the server has + // since disabled — e.g. a cloud sandbox registry without claude_local. The + // grid hides it, so without this snap the wizard would silently keep an + // invisible selection and create an agent that can never acquire a lease. + useEffect(() => { + // Not until the registry has loaded. External adapter types are only + // registered once the adapters query resolves, so before that a saved + // external adapter is indistinguishable from a disabled one - and snapping + // would replace the customer's choice with a built-in and persist it. + if (!adapterRegistryLoaded) return; + const visible = [...recommendedAdapters, ...moreAdapters].filter( + (a) => !a.comingSoon, + ); + if (visible.length === 0) return; + if (visible.some((a) => a.type === adapterType)) return; + const next = visible[0].type as AdapterType; + setAdapterType(next); + if (next === "codex_local") return; + if (next === "opencode_local") { + setModel(DEFAULT_OPENCODE_LOCAL_MODEL); + return; + } + if (next === "gemini_local") { + setModel(DEFAULT_GEMINI_LOCAL_MODEL); + return; + } + if (next === "cursor") { + setModel(DEFAULT_CURSOR_LOCAL_MODEL); + return; + } + setModel(""); + }, [adapterRegistryLoaded, recommendedAdapters, moreAdapters, adapterType]); + const COMMAND_PLACEHOLDERS: Record = { claude_local: "claude", codex_local: "codex",