From 066bd4d4e854399cada63d3cfd8bdb830c9aadfa Mon Sep 17 00:00:00 2001 From: Wenxi Date: Thu, 23 Jul 2026 16:35:36 -0700 Subject: [PATCH] feat(craft): surface external app skills in skills UI (#13374) --- .../setup-requests/SetupCard.test.tsx | 71 ++++++ .../components/setup-requests/SetupCard.tsx | 1 + .../services/externalAppsService.test.ts | 14 +- .../app/craft/services/externalAppsService.ts | 9 +- web/src/app/craft/v1/apps/connectableApps.ts | 4 + web/src/app/craft/v1/apps/page.test.tsx | 218 ++++++++++++++++++ web/src/app/craft/v1/apps/page.tsx | 65 +++++- web/src/lib/skills/__fixtures__/picker.ts | 2 + web/src/lib/skills/__tests__/picker.test.ts | 32 +++ web/src/lib/skills/picker.ts | 12 +- web/src/lib/skills/types.ts | 9 + web/src/sections/cards/SkillCard.test.tsx | 130 +++++++++++ web/src/sections/cards/SkillCard.tsx | 65 ++++-- .../modals/SkillPreviewModal.test.tsx | 69 ++++++ web/src/sections/modals/SkillPreviewModal.tsx | 19 +- .../modals/skills/ShareSkillModal.tsx | 11 +- web/src/views/SkillEditorPage.test.tsx | 88 +++++-- web/src/views/SkillEditorPage.tsx | 27 +++ web/src/views/SkillsPage.test.tsx | 142 +++++++++++- web/src/views/SkillsPage.tsx | 81 +++++-- 20 files changed, 997 insertions(+), 72 deletions(-) create mode 100644 web/src/app/craft/components/setup-requests/SetupCard.test.tsx create mode 100644 web/src/app/craft/v1/apps/page.test.tsx create mode 100644 web/src/sections/modals/SkillPreviewModal.test.tsx diff --git a/web/src/app/craft/components/setup-requests/SetupCard.test.tsx b/web/src/app/craft/components/setup-requests/SetupCard.test.tsx new file mode 100644 index 0000000000..87da112223 --- /dev/null +++ b/web/src/app/craft/components/setup-requests/SetupCard.test.tsx @@ -0,0 +1,71 @@ +import { render, screen, setupUser, waitFor } from "@tests/setup/test-utils"; +import SetupCard from "@/app/craft/components/setup-requests/SetupCard"; +import type { ExternalAppUserResponse } from "@/app/craft/v1/apps/registry"; +import { SWR_KEYS } from "@/lib/swr-keys"; + +const mockMutate = jest.fn(); +const mockPostConnectAppDecision = jest.fn(); + +jest.mock("swr", () => ({ + ...jest.requireActual("swr"), + useSWRConfig: () => ({ mutate: mockMutate }), +})); + +jest.mock("@/app/craft/services/externalAppsService", () => ({ + postConnectAppDecision: (...args: unknown[]) => + mockPostConnectAppDecision(...args), + startExternalAppOAuth: jest.fn(), + upsertUserCredentials: jest.fn(), +})); + +jest.mock("@/app/craft/v1/apps/UserCredentialsModal", () => ({ + __esModule: true, + default: ({ open, onSaved }: { open: boolean; onSaved: () => void }) => + open ? ( + + ) : null, +})); + +const userApp: ExternalAppUserResponse = { + id: 42, + name: "Acme CRM", + app_type: "CUSTOM", + credential_keys: ["token"], + credential_values: {}, + authenticated: false, + supports_oauth: false, +}; + +describe("SetupCard", () => { + beforeEach(() => { + mockMutate.mockReset(); + mockPostConnectAppDecision.mockReset(); + mockPostConnectAppDecision.mockResolvedValue(undefined); + }); + + it("refreshes app and skill state after credentials are connected", async () => { + const user = setupUser(); + render( + + ); + + await user.click(screen.getByRole("button", { name: "Connect Acme CRM" })); + await user.click(screen.getByRole("button", { name: "Save credentials" })); + + await waitFor(() => + expect(mockPostConnectAppDecision).toHaveBeenCalledWith( + "request-id", + "connected" + ) + ); + expect(mockMutate).toHaveBeenCalledWith(SWR_KEYS.buildExternalApps); + expect(mockMutate).toHaveBeenCalledWith(SWR_KEYS.userSkills); + }); +}); diff --git a/web/src/app/craft/components/setup-requests/SetupCard.tsx b/web/src/app/craft/components/setup-requests/SetupCard.tsx index 4672cbf0bd..34971d9fcf 100644 --- a/web/src/app/craft/components/setup-requests/SetupCard.tsx +++ b/web/src/app/craft/components/setup-requests/SetupCard.tsx @@ -86,6 +86,7 @@ export default function SetupCard({ setDecision(result); if (result === "connected") { void mutate(SWR_KEYS.buildExternalApps); + void mutate(SWR_KEYS.userSkills); } } diff --git a/web/src/app/craft/services/externalAppsService.test.ts b/web/src/app/craft/services/externalAppsService.test.ts index 307dcf1f5d..1fd3d0170c 100644 --- a/web/src/app/craft/services/externalAppsService.test.ts +++ b/web/src/app/craft/services/externalAppsService.test.ts @@ -1,4 +1,7 @@ -import { createCustomExternalApp } from "@/app/craft/services/externalAppsService"; +import { + createCustomExternalApp, + disconnectUserFromApp, +} from "@/app/craft/services/externalAppsService"; describe("createCustomExternalApp", () => { beforeEach(() => { @@ -27,4 +30,13 @@ describe("createCustomExternalApp", () => { expect(request.body).not.toBeInstanceOf(FormData); expect(String(request.body)).not.toContain("bundle"); }); + + it("disconnects through the credential deletion endpoint", async () => { + await disconnectUserFromApp(17); + + expect(global.fetch).toHaveBeenCalledWith( + "/api/build/apps/17/credentials", + { method: "DELETE" } + ); + }); }); diff --git a/web/src/app/craft/services/externalAppsService.ts b/web/src/app/craft/services/externalAppsService.ts index 871db436ad..e8a58eb5ea 100644 --- a/web/src/app/craft/services/externalAppsService.ts +++ b/web/src/app/craft/services/externalAppsService.ts @@ -190,9 +190,14 @@ export async function upsertUserCredentials( } } -/** "Disconnect" by clearing stored user credentials. */ export async function disconnectUserFromApp( externalAppId: number ): Promise { - return upsertUserCredentials(externalAppId, {}); + const res = await fetch( + `${BUILD_API_BASE}/apps/${externalAppId}/credentials`, + { method: "DELETE" } + ); + if (!res.ok) { + throw new Error(await readErrorDetail(res, "Failed to disconnect app")); + } } diff --git a/web/src/app/craft/v1/apps/connectableApps.ts b/web/src/app/craft/v1/apps/connectableApps.ts index 7394ebe2df..108ae9bc3a 100644 --- a/web/src/app/craft/v1/apps/connectableApps.ts +++ b/web/src/app/craft/v1/apps/connectableApps.ts @@ -28,6 +28,8 @@ export interface ConnectableApp { key: string; name: string; description: string; + /** External app identity; null for MCP servers. */ + externalAppId: number | null; /** Deep-link (`?connect=`) target — the external app's id; null for MCP. */ connectId: string | null; authenticated: boolean; @@ -52,6 +54,7 @@ export function externalAppToConnectable( description: app.supports_oauth ? "Connect with OAuth" : "Connect with credentials", + externalAppId: app.id, connectId: String(app.id), authenticated: app.authenticated, logo: getAppTypeLogo(app.app_type), @@ -89,6 +92,7 @@ export function mcpServerToConnectable( key: `mcp-${server.id}`, name: server.name, description: server.description ?? "", + externalAppId: null, connectId: null, authenticated, logo: getActionIcon(server.server_url, server.name), diff --git a/web/src/app/craft/v1/apps/page.test.tsx b/web/src/app/craft/v1/apps/page.test.tsx new file mode 100644 index 0000000000..18919997b5 --- /dev/null +++ b/web/src/app/craft/v1/apps/page.test.tsx @@ -0,0 +1,218 @@ +import { render, screen, setupUser, waitFor } from "@tests/setup/test-utils"; +import ExternalAppsPage from "@/app/craft/v1/apps/page"; +import type { SkillsList } from "@/lib/skills/types"; +import { SWR_KEYS } from "@/lib/swr-keys"; + +const mockUseSWR = jest.fn(); +const mockUseUserSkills = jest.fn(); +const mockMutateApps = jest.fn(); +const mockMutateMcp = jest.fn(); +const mockRefreshSkills = jest.fn(); +const mockDisconnectUserFromApp = jest.fn(); + +jest.mock("swr", () => ({ + ...jest.requireActual("swr"), + __esModule: true, + default: (...args: unknown[]) => mockUseSWR(...args), +})); + +jest.mock("next/navigation", () => ({ + useRouter: () => ({ back: jest.fn(), push: jest.fn() }), + usePathname: () => "/craft/v1/apps", + useSearchParams: () => ({ get: () => null, has: () => false }), +})); + +jest.mock("@/providers/UserProvider", () => ({ + ...jest.requireActual("@/providers/UserProvider"), + useUser: () => ({ isAdmin: false }), +})); + +jest.mock("@/hooks/useUserSkills", () => ({ + __esModule: true, + default: () => mockUseUserSkills(), +})); + +jest.mock("@/app/craft/services/externalAppsService", () => ({ + ...jest.requireActual("@/app/craft/services/externalAppsService"), + disconnectUserFromApp: (...args: unknown[]) => + mockDisconnectUserFromApp(...args), +})); + +function skills( + associatedSkillEnabled: boolean, + includeSelectedSameNameSkill = false, + secondAssociatedSkillEnabled?: boolean +): SkillsList { + const associatedSkill: SkillsList["customs"][number] = { + source: "custom", + id: "associated-skill-id", + name: "crm-workflow", + description: "Use the CRM workflow", + is_available: null, + unavailable_reason: null, + is_valid: true, + is_personal: false, + enabled: associatedSkillEnabled, + can_toggle: true, + author_user_id: "owner-id", + author_email: "owner@example.com", + owner: { id: "owner-id", email: "owner@example.com" }, + ownership_vacant: false, + created_at: null, + updated_at: null, + user_shares: [], + group_shares: [], + public_permission: "VIEWER", + user_permission: "VIEWER", + external_app: { + external_app_id: 42, + name: "Acme CRM", + enabled: true, + ready: true, + }, + }; + const selectedSameNameSkill: SkillsList["customs"][number] = { + ...associatedSkill, + id: "standalone-skill-id", + enabled: true, + external_app: null, + }; + const secondAssociatedSkill: SkillsList["customs"][number] = { + ...associatedSkill, + id: "second-associated-skill-id", + name: "crm-reports", + enabled: secondAssociatedSkillEnabled ?? false, + }; + const customs = [associatedSkill]; + if (includeSelectedSameNameSkill) customs.push(selectedSameNameSkill); + if (secondAssociatedSkillEnabled !== undefined) { + customs.push(secondAssociatedSkill); + } + return { + builtins: [], + customs, + }; +} + +describe("Apps associated-skill setup notice", () => { + beforeEach(() => { + mockMutateApps.mockReset(); + mockMutateMcp.mockReset(); + mockRefreshSkills.mockReset(); + mockDisconnectUserFromApp.mockReset(); + mockDisconnectUserFromApp.mockResolvedValue(undefined); + mockUseSWR.mockImplementation((key: string) => { + if (key === SWR_KEYS.buildExternalApps) { + return { + data: [ + { + id: 42, + name: "Acme CRM", + app_type: "CUSTOM", + credential_keys: [], + credential_values: {}, + authenticated: true, + supports_oauth: false, + }, + ], + mutate: mockMutateApps, + }; + } + return { data: { mcp_servers: [] }, mutate: mockMutateMcp }; + }); + }); + + it("guides a connected user when no associated skill is selected", () => { + mockUseUserSkills.mockReturnValue({ + data: skills(false, true), + refresh: mockRefreshSkills, + }); + + render(); + + expect( + screen.getByText( + "Connected · Not all associated skills are enabled. This app may not work correctly." + ) + ).toBeInTheDocument(); + expect(screen.getByLabelText("Skill setup required")).toBeInTheDocument(); + expect(screen.queryByLabelText("App ready")).not.toBeInTheDocument(); + expect(screen.getByRole("link", { name: "Review skills" })).toHaveAttribute( + "href", + "/craft/v1/skills?externalAppId=42" + ); + }); + + it("does not nag after an associated skill is selected", () => { + mockUseUserSkills.mockReturnValue({ + data: skills(true), + refresh: mockRefreshSkills, + }); + + render(); + + expect( + screen.queryByText( + "Connected · Not all associated skills are enabled. This app may not work correctly." + ) + ).not.toBeInTheDocument(); + expect( + screen.queryByRole("link", { name: "Review skills" }) + ).not.toBeInTheDocument(); + expect(screen.getByLabelText("App ready")).toBeInTheDocument(); + expect( + screen.queryByLabelText("Skill setup required") + ).not.toBeInTheDocument(); + }); + + it("does not claim an external app is ready before skills load", () => { + mockUseUserSkills.mockReturnValue({ + data: undefined, + refresh: mockRefreshSkills, + }); + + render(); + + expect(screen.queryByLabelText("App ready")).not.toBeInTheDocument(); + expect( + screen.queryByLabelText("Skill setup required") + ).not.toBeInTheDocument(); + }); + + it("continues guiding the user until every associated skill is selected", () => { + mockUseUserSkills.mockReturnValue({ + data: skills(true, false, false), + refresh: mockRefreshSkills, + }); + + render(); + + expect( + screen.getByText( + "Connected · Not all associated skills are enabled. This app may not work correctly." + ) + ).toBeInTheDocument(); + expect(screen.getByRole("link", { name: "Review skills" })).toHaveAttribute( + "href", + "/craft/v1/skills?externalAppId=42" + ); + }); + + it("refreshes app and skill state after disconnecting", async () => { + const user = setupUser(); + mockUseUserSkills.mockReturnValue({ + data: skills(true), + refresh: mockRefreshSkills, + }); + + render(); + await user.click(screen.getByRole("button", { name: "Disconnect" })); + + await waitFor(() => + expect(mockDisconnectUserFromApp).toHaveBeenCalledWith(42) + ); + expect(mockMutateApps).toHaveBeenCalledTimes(1); + expect(mockMutateMcp).toHaveBeenCalledTimes(1); + expect(mockRefreshSkills).toHaveBeenCalledTimes(1); + }); +}); diff --git a/web/src/app/craft/v1/apps/page.tsx b/web/src/app/craft/v1/apps/page.tsx index ed5873587e..5a62e46cc7 100644 --- a/web/src/app/craft/v1/apps/page.tsx +++ b/web/src/app/craft/v1/apps/page.tsx @@ -9,7 +9,12 @@ import useOnMount from "@/hooks/useOnMount"; import { cn } from "@opal/utils"; import { Button, Card, InputTypeIn, Text } from "@opal/components"; import { SettingsLayouts, toast } from "@opal/layouts"; -import { SvgCheckCircle, SvgPlug, SvgSettings } from "@opal/icons"; +import { + SvgAlertCircle, + SvgCheckCircle, + SvgPlug, + SvgSettings, +} from "@opal/icons"; import { ExternalAppUserResponse } from "@/app/craft/v1/apps/registry"; import { MCPServersResponse } from "@/lib/tools/interfaces"; import { @@ -19,6 +24,7 @@ import { } from "@/app/craft/v1/apps/connectableApps"; import UserCredentialsModal from "@/app/craft/v1/apps/UserCredentialsModal"; import { useUser } from "@/providers/UserProvider"; +import useUserSkills from "@/hooks/useUserSkills"; // The user's own app connections. Org-wide configuration lives in the admin // panel's Craft section; admins get a shortcut button to it here. @@ -82,11 +88,26 @@ function AppConnections({ query }: AppConnectionsProps) { errorHandlingFetcher, { keepPreviousData: true } ); + const { data: skillsData, refresh: refreshSkills } = useUserSkills(); const connectParam = useSearchParams().get("connect"); + const skillSetupByAppId = useMemo(() => { + const setup = new Map(); + for (const skill of skillsData?.customs ?? []) { + const externalAppId = skill.external_app?.external_app_id; + if (externalAppId === undefined) continue; + const current = setup.get(externalAppId) ?? { total: 0, selected: 0 }; + current.total += 1; + if (skill.enabled) current.selected += 1; + setup.set(externalAppId, current); + } + return setup; + }, [skillsData]); + const refresh = () => { void mutateApps(); void mutateMcp(); + void refreshSkills(); }; const { connected, browse, isLoading, isEmpty } = useMemo(() => { @@ -139,6 +160,14 @@ function AppConnections({ query }: AppConnectionsProps) { key={item.key} variant="row" app={item} + skillSetupKnown={ + item.externalAppId === null || skillsData !== undefined + } + skillSetup={ + item.externalAppId === null + ? undefined + : skillSetupByAppId.get(item.externalAppId) + } onChange={refresh} /> ))} @@ -178,6 +207,8 @@ interface ProviderConnectCardProps { app: ConnectableApp; variant: "row" | "tile"; highlight?: boolean; + skillSetupKnown?: boolean; + skillSetup?: { total: number; selected: number }; onChange: () => void; } @@ -185,6 +216,8 @@ function ProviderConnectCard({ app, variant, highlight, + skillSetupKnown = true, + skillSetup, onChange, }: ProviderConnectCardProps) { const [isStarting, setIsStarting] = useState(false); @@ -231,6 +264,10 @@ function ProviderConnectCard({ } const Logo = app.logo; + const needsSkillSetup = + skillSetup !== undefined && + skillSetup.total > 0 && + skillSetup.selected < skillSetup.total; return ( <> @@ -248,15 +285,35 @@ function ProviderConnectCard({
{app.name} - + {needsSkillSetup ? ( + + ) : skillSetupKnown ? ( + + ) : null}
- Connected + {needsSkillSetup + ? "Connected · Not all associated skills are enabled. This app may not work correctly." + : "Connected"}
- {app.disconnect && ( + {needsSkillSetup && app.externalAppId !== null && ( + )} + {app.disconnect && ( + + } + /> + )} + {isLoading && } {error && !isLoading && ( @@ -328,6 +379,9 @@ export default function SkillsPage() { @@ -369,8 +423,8 @@ export default function SkillsPage() { {pendingSwitchTarget && ( setPendingSwitchTarget(null) } @@ -386,7 +440,8 @@ export default function SkillsPage() { } > - Continuing will disable the active skill and enable the selected one. + Continuing will disable the currently enabled skill and enable this + one. )}