diff --git a/packages/shared/src/connection-setup-state.test.ts b/packages/shared/src/connection-setup-state.test.ts new file mode 100644 index 00000000000..e4cc83af584 --- /dev/null +++ b/packages/shared/src/connection-setup-state.test.ts @@ -0,0 +1,111 @@ +import { describe, expect, it } from "vitest"; +import { + CONNECTABLE_APP_DEFINITIONS, + getAvailableConnectionMethods, + getConnectableAppDefinition, +} from "./app-definitions.js"; +import { + connectionSetupStateForApp, + connectionSetupVerbForApp, + type ConnectionSetupState, +} from "./connection-setup-state.js"; + +/** + * These assert against the shipped catalog rather than fixtures, because the + * claim being protected is about real connectors: the gallery card's verb has + * to match what the connect screen then asks for. + */ +describe("connectionSetupStateForApp", () => { + it("puts one-click OAuth connectors in the authorize state", () => { + for (const slug of ["notion", "linear", "sentry", "stripe", "jira", "asana"]) { + expect(connectionSetupStateForApp(getConnectableAppDefinition(slug)), slug).toBe("authorize"); + } + }); + + it("follows the instance's ownership availability rather than the catalog order", () => { + // Google and GitHub publish a Paperclip-managed method, but it is + // `platform_shared` and therefore unavailable until an operator configures + // the cloud connector. The state has to reflect what this instance can + // actually do, or the card promises one click and the screen shows a form. + const gmail = getConnectableAppDefinition("gmail")!; + expect(connectionSetupStateForApp(gmail)).toBe("register"); + expect( + connectionSetupStateForApp({ + ...gmail, + ownershipAvailability: { ...gmail.ownershipAvailability, platform_shared: true }, + }), + ).toBe("authorize"); + }); + + it("reserves register for providers that genuinely cannot issue a client", () => { + for (const slug of ["box", "xero"]) { + expect(connectionSetupStateForApp(getConnectableAppDefinition(slug)), slug).toBe("register"); + } + }); + + it("treats an API key as a paste, and says so on the card", () => { + for (const slug of ["honcho", "mem0", "openrouter"]) { + expect(connectionSetupStateForApp(getConnectableAppDefinition(slug)), slug).toBe("paste"); + expect(connectionSetupVerbForApp(getConnectableAppDefinition(slug)), slug).toBe("Add key"); + } + }); + + it("reaches instant only when the catalog already knows the endpoint", () => { + // Composio and Context7 ship a fixed server URL and ask for nothing. + expect(connectionSetupStateForApp(getConnectableAppDefinition("composio"))).toBe("instant"); + expect(connectionSetupStateForApp(getConnectableAppDefinition("context7"))).toBe("instant"); + }); + + it("does not promise instant for a provider-generated URL", () => { + // Zapier, Arcade and Executor declare no server URL: the operator brings + // one. Calling these instant is the mistake this helper exists to prevent. + for (const slug of ["zapier", "arcade", "executor"]) { + expect(connectionSetupStateForApp(getConnectableAppDefinition(slug)), slug).toBe("paste"); + } + }); + + it("does not promise instant for a templated endpoint", () => { + // Shopify's URL has a {storeDomain} placeholder the operator fills in. + expect(connectionSetupStateForApp(getConnectableAppDefinition("shopify"))).toBe("paste"); + }); + + it("defaults the AI providers to their subscription sign-in", () => { + for (const slug of ["anthropic", "openai", "xai"]) { + expect(connectionSetupStateForApp(getConnectableAppDefinition(slug)), slug).toBe("authorize"); + expect(connectionSetupVerbForApp(getConnectableAppDefinition(slug)), slug).toBe("Connect"); + } + }); + + it("never says Add key for something that does not want a secret", () => { + for (const app of CONNECTABLE_APP_DEFINITIONS) { + if (connectionSetupVerbForApp(app) !== "Add key") continue; + const wantsSecret = getAvailableConnectionMethods(app) + .filter((method) => (method.purpose ?? "tool") !== "channel") + .some((method) => (method.credentialFields?.length ?? 0) > 0); + expect(wantsSecret, app.slug).toBe(true); + } + }); + + it("resolves a state for every connectable tool connector", () => { + const states = new Map(); + for (const app of CONNECTABLE_APP_DEFINITIONS) { + const hasToolMethod = app.methods.some((method) => (method.purpose ?? "tool") !== "channel"); + if (!hasToolMethod) continue; + states.set(app.slug, connectionSetupStateForApp(app)); + } + expect([...states].filter(([, state]) => state === null)).toEqual([]); + }); + + it("returns null rather than guessing for an unknown app", () => { + expect(connectionSetupStateForApp(null)).toBeNull(); + }); + + it("survives a gallery row that carries no method list", () => { + // Rows are assembled from the gallery, live connections and custom + // endpoints, so a row can reach a card without methods. Throwing here would + // take the whole connectors page down with it. + const partial = { slug: "mystery", name: "Mystery" } as never; + expect(connectionSetupStateForApp(partial)).toBeNull(); + expect(connectionSetupVerbForApp(partial)).toBe("Connect"); + }); +}); diff --git a/packages/shared/src/connection-setup-state.ts b/packages/shared/src/connection-setup-state.ts new file mode 100644 index 00000000000..becd04545f0 --- /dev/null +++ b/packages/shared/src/connection-setup-state.ts @@ -0,0 +1,112 @@ +import { + connectionMethodAcceptsCustomerOAuthClient, + connectionMethodRequiresConfiguration, + connectionMethodSupportsAutomaticOAuth, + getAvailableConnectionMethods, + getRecommendedConnectionMethod, +} from "./app-definitions.js"; +import type { AppDefinition, ConnectionMethodDef } from "./types/app-definition.js"; + +/** + * The four states every connect screen lands in (PAP-659, "one shell, four + * states"). + * + * The point of naming them here rather than in the wizard is that two surfaces + * have to agree: the gallery card decides its verb from the state, and the + * connect screen decides its body from the same state. When each re-derived it + * from raw method flags they drifted — a card offering "Connect" for something + * that then demanded a pasted URL is exactly the inconsistency the ticket is + * about. + * + * - `instant` nothing to supply: connect resolves entirely from the catalog. + * - `authorize` one click, then the provider's own consent screen. + * - `paste` one thing the operator has to bring: a key, a URL, a tenant. + * - `register` the provider cannot issue a client, so one must be registered + * in its console first. A recovery state, never a normal step. + */ +export type ConnectionSetupState = "instant" | "authorize" | "paste" | "register"; + +/** + * True when the catalog already knows the endpoint. A `serverUrlTemplate` does + * not count: its placeholders are the operator's to fill, which is a paste. + */ +function hasResolvedEndpoint(method: ConnectionMethodDef): boolean { + if (method.transport !== "mcp_remote") return true; + if (method.defaults?.serverUrlTemplate) return false; + return Boolean(method.defaults?.serverUrl); +} + +export function connectionSetupStateForMethod( + method: ConnectionMethodDef | null | undefined, +): ConnectionSetupState | null { + if (!method) return null; + if (method.auth === "oauth") { + // An AI subscription signs in through the adapter's own device/OAuth flow. + // No client is ever registered, so `register` would be a false detour. + if (method.transport === "runtime_auth") return "authorize"; + if (connectionMethodSupportsAutomaticOAuth(method)) return "authorize"; + // A customer-owned client is only a *register* state when the provider + // offers nothing better. Live discovery can still overrule the catalog at + // connect time, which is why the flow re-resolves rather than trusting this. + if (connectionMethodAcceptsCustomerOAuthClient(method)) return "register"; + return "authorize"; + } + if (method.auth === "none") { + return connectionMethodRequiresConfiguration(method) || !hasResolvedEndpoint(method) + ? "paste" + : "instant"; + } + return "paste"; +} + +/** + * The default tool method for an app, or null. + * + * Gallery rows are not guaranteed to carry a method list: the display entry is + * assembled from several sources, and a row can stand for a connection whose + * catalog entry has since been withdrawn. Resolving that to null — rather than + * throwing inside a card render — is the difference between a missing verb and + * a blank connectors page. + */ +function defaultToolMethod( + app: AppDefinition | null | undefined, + methodKey?: string | null, +): ConnectionMethodDef | null { + if (!app || !Array.isArray(app.methods)) return null; + const methods = getAvailableConnectionMethods(app).filter( + (method) => (method.purpose ?? "tool") !== "channel", + ); + return methodKey + ? methods.find((candidate) => candidate.key === methodKey) ?? null + : getRecommendedConnectionMethod(methods); +} + +export function connectionSetupStateForApp( + app: AppDefinition | null | undefined, + methodKey?: string | null, +): ConnectionSetupState | null { + return connectionSetupStateForMethod(defaultToolMethod(app, methodKey)); +} + +/** + * The gallery card's verb (PAP-659 C4). + * + * Only a method that genuinely wants a secret says so. "Add key" on a connector + * that actually wants a pasted URL would be worse than the generic verb, so a + * paste state without credential fields keeps "Connect" — the operator still + * lands on one screen with one field, which the verb does not need to restate. + */ +export function connectionSetupVerbForMethod( + method: ConnectionMethodDef | null | undefined, +): "Connect" | "Add key" { + if (!method) return "Connect"; + const wantsSecret = (method.credentialFields?.length ?? 0) > 0; + return connectionSetupStateForMethod(method) === "paste" && wantsSecret ? "Add key" : "Connect"; +} + +export function connectionSetupVerbForApp( + app: AppDefinition | null | undefined, + methodKey?: string | null, +): "Connect" | "Add key" { + return connectionSetupVerbForMethod(defaultToolMethod(app, methodKey)); +} diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index 44b61a6e9b7..7b1131c1aac 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -333,6 +333,13 @@ export { recommendedDefaultsForApp, resolveConnectionMethodServerUrl, } from "./app-definitions.js"; +export { + connectionSetupStateForApp, + connectionSetupStateForMethod, + connectionSetupVerbForApp, + connectionSetupVerbForMethod, + type ConnectionSetupState, +} from "./connection-setup-state.js"; export { APP_DEFINITIONS } from "./app-definitions.generated.js"; export * from "./google-workspace-connectors.js"; export * from "./github-connectors.js"; diff --git a/ui/src/pages/apps/Browse.test.tsx b/ui/src/pages/apps/Browse.test.tsx index b81ae392b23..8b6a64542f8 100644 --- a/ui/src/pages/apps/Browse.test.tsx +++ b/ui/src/pages/apps/Browse.test.tsx @@ -243,10 +243,28 @@ describe("Connectors landing page", () => { } expect(container.textContent).not.toContain("Private bot"); expect(container.textContent).not.toContain("Chat with agents"); - await act(() => void container.querySelector('button[aria-label="Connect GitHub"]')!.click()); + await act(() => void container.querySelector('button[aria-label="Add key GitHub"]')!.click()); expect(navigateMock).toHaveBeenLastCalledWith("/apps/connect?source=github"); }); + it("names what the card will actually ask for", async () => { + // PAP-659 C4. The verb is derived from the connector's resolved default + // method, so it changes with what this instance can do rather than being a + // fixed string: Notion signs in, GitHub-without-the-cloud-connector wants a + // token, and GitHub with it signs in too. + const github = getAppStoreDefinition("github")!; + listGalleryMock.mockResolvedValue({ + apps: [ + getAppStoreDefinition("notion"), + { ...github, ownershipAvailability: { ...github.ownershipAvailability, platform_shared: true } }, + ], + }); + await renderBrowse(); + expect(container.querySelector('button[aria-label="Connect Notion"]')).not.toBeNull(); + expect(container.querySelector('button[aria-label="Connect GitHub"]')).not.toBeNull(); + expect(container.querySelector('button[aria-label="Add key GitHub"]')).toBeNull(); + }); + it("restores the GitHub intent chooser when enabled and hides cached chat rows immediately when disabled", async () => { listGalleryMock.mockResolvedValue({ apps: [getAppStoreDefinition("github")] }); chatListMock.mockResolvedValue([{ id: "endpoint-1", provider: "github", status: "active", assignedAgentName: "Chat agent", botLabel: "Chat bot", assignedAgentId: "agent-1" }]); @@ -260,7 +278,7 @@ describe("Connectors landing page", () => { expect(container.querySelector('[data-app-slug="telegram"]')).toBeNull(); expect(container.textContent).not.toContain("Chat agent"); expect(container.querySelector('a[href*="/apps/chat/"]')).toBeNull(); - await act(() => void container.querySelector('button[aria-label="Connect GitHub"]')!.click()); + await act(() => void container.querySelector('button[aria-label="Add key GitHub"]')!.click()); expect(navigateMock).toHaveBeenLastCalledWith("/apps/connect?source=github"); }); diff --git a/ui/src/pages/apps/Browse.tsx b/ui/src/pages/apps/Browse.tsx index e6aa890438b..b4ee6951336 100644 --- a/ui/src/pages/apps/Browse.tsx +++ b/ui/src/pages/apps/Browse.tsx @@ -1,4 +1,8 @@ -import { isRetiredComposioConnection, RETIRED_COMPOSIO_MESSAGE } from "@paperclipai/shared"; +import { + connectionSetupVerbForApp, + isRetiredComposioConnection, + RETIRED_COMPOSIO_MESSAGE, +} from "@paperclipai/shared"; import { ManagedAiConnectionRow } from "@/components/ai-connections/ManagedAiConnectionDetails"; import { useEffect, useMemo, useState, type ReactNode } from "react"; import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; @@ -250,7 +254,14 @@ function connectorAction( }; } if (chatHref) return { label: "Connect", href: chatHref }; - if (row.entry) return { label: "Connect", href: connectHrefFor(row.entry) }; + // PAP-659 C4: the card's verb comes from the same four-state resolver the + // connect screen uses, so "Connect" never turns out to mean "paste a key". + if (row.entry) { + return { + label: connectionSetupVerbForApp(row.entry), + href: connectHrefFor(row.entry), + }; + } return { label: "Connect", href: applicationId ? `/apps/app/${applicationId}/permissions` : null,