feat(craft): surface external app skills in skills UI (#13374)

This commit is contained in:
Wenxi authored and GitHub committed 2026-07-23 23:35:36 +00:00
1 parent 3a61b8b78e
commit 066bd4d4e8
20 files changed
+997 -72

No files matched your search

@@ -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 ? (
<button type="button" onClick={onSaved}>
Save credentials
</button>
) : 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(
<SetupCard
requestId="request-id"
externalAppId={42}
reason={null}
userApp={userApp}
/>
);
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);
});
});
@@ -86,6 +86,7 @@ export default function SetupCard({
setDecision(result);
if (result === "connected") {
void mutate(SWR_KEYS.buildExternalApps);
void mutate(SWR_KEYS.userSkills);
}
}
@@ -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" }
);
});
});
@@ -190,9 +190,14 @@ export async function upsertUserCredentials(
}
}
/** "Disconnect" by clearing stored user credentials. */
export async function disconnectUserFromApp(
externalAppId: number
): Promise<void> {
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"));
}
}
@@ -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),
+218
View File
@@ -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(<ExternalAppsPage />);
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(<ExternalAppsPage />);
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(<ExternalAppsPage />);
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(<ExternalAppsPage />);
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(<ExternalAppsPage />);
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);
});
});
+61 -4
View File
@@ -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<number, { total: number; selected: number }>();
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({
<div className="flex-1 flex flex-col gap-0.5">
<div className="flex items-center gap-2">
<Text font="main-ui-action">{app.name}</Text>
<SvgCheckCircle className="w-4 h-4 text-status-success-05" />
{needsSkillSetup ? (
<SvgAlertCircle
className="w-4 h-4 text-status-warning-05"
aria-label="Skill setup required"
/>
) : skillSetupKnown ? (
<SvgCheckCircle
className="w-4 h-4 text-status-success-05"
aria-label="App ready"
/>
) : null}
</div>
<Text font="secondary-body" color="text-03">
Connected
{needsSkillSetup
? "Connected · Not all associated skills are enabled. This app may not work correctly."
: "Connected"}
</Text>
</div>
{app.disconnect && (
{needsSkillSetup && app.externalAppId !== null && (
<Button
prominence="secondary"
href={`/craft/v1/skills?externalAppId=${app.externalAppId}`}
>
Review skills
</Button>
)}
{app.disconnect && (
<Button
prominence={needsSkillSetup ? "tertiary" : "secondary"}
disabled={isStarting}
onClick={disconnect}
>
@@ -26,6 +26,7 @@ export function builtinFixture(over: Partial<BuiltinSkill> = {}): BuiltinSkill {
group_shares: [],
public_permission: null,
user_permission: "VIEWER",
external_app: null,
...over,
};
}
@@ -52,6 +53,7 @@ export function customFixture(over: Partial<CustomSkill> = {}): CustomSkill {
group_shares: [],
public_permission: "VIEWER",
user_permission: "VIEWER",
external_app: null,
...over,
};
}
@@ -106,6 +106,38 @@ describe("toPickerSections", () => {
]);
});
it("requires both selection and app readiness for associated customs", () => {
const dependency = {
external_app_id: 42,
name: "Acme CRM",
enabled: true,
ready: false,
};
const data = skillsList({
customs: [
customFixture({
name: "ready-app-skill",
enabled: true,
external_app: { ...dependency, ready: true },
}),
customFixture({
name: "disconnected-app-skill",
enabled: true,
external_app: dependency,
}),
customFixture({
name: "unselected-ready-app-skill",
enabled: false,
external_app: { ...dependency, ready: true },
}),
],
});
expect(
toPickerSections(data, []).skills.map((skill) => skill.slug)
).toEqual(["ready-app-skill"]);
});
it("builds the Apps section from the external-apps payload with auth state", () => {
const apps = [
appFixture({
+9 -3
View File
@@ -28,8 +28,8 @@ export interface PickerSections {
const EMPTY_SECTIONS: PickerSections = { skills: [], apps: [] };
// Skill rows provide per-user enablement; app metadata and credential state
// come from `/api/build/apps`.
// Associated custom skills remain normal per-user selections, with app
// readiness as an additional runtime requirement.
export function toPickerSections(
skillsData: SkillsList | undefined,
externalApps: ExternalAppUserResponse[] | undefined
@@ -49,7 +49,13 @@ export function toPickerSections(
}
for (const c of skillsData?.customs ?? []) {
if (!c.enabled || c.is_valid === false) continue;
if (
!c.enabled ||
c.is_valid === false ||
(c.external_app !== null && !c.external_app.ready)
) {
continue;
}
skills.push({
kind: "skill",
slug: c.name,
+9
View File
@@ -21,6 +21,13 @@ export interface SkillGroupShare {
permission: SkillSharePermission;
}
export interface SkillExternalAppDependency {
external_app_id: number;
name: string;
enabled: boolean;
ready: boolean;
}
export interface Skill {
source: SkillSource;
id: string;
@@ -48,6 +55,7 @@ export interface Skill {
group_shares: SkillGroupShare[];
public_permission: SkillSharePermission | null;
user_permission: SkillAccessLevel | null;
external_app: SkillExternalAppDependency | null;
}
export type BuiltinSkill = Skill & {
@@ -71,6 +79,7 @@ export interface SkillPreview {
description: string;
author_email: string | null;
instructions_markdown: string;
external_app: SkillExternalAppDependency | null;
}
export type SkillEditableDetail = CustomSkill & {
+130
View File
@@ -46,6 +46,7 @@ function custom(overrides: Partial<CustomSkill> = {}): CustomSkillCardItem {
group_shares: [],
public_permission: null,
user_permission: "OWNER",
external_app: null,
...overrides,
};
return {
@@ -121,4 +122,133 @@ describe("SkillCard", () => {
).toBeInTheDocument();
expect(screen.queryByRole("switch")).not.toBeInTheDocument();
});
it("keeps a selected skill on while its app is unavailable", () => {
const { container } = render(
<SkillCard
item={custom({
enabled: true,
external_app: {
external_app_id: 42,
name: "Acme CRM",
enabled: true,
ready: false,
},
})}
/>
);
expect(
screen.getByText("Connect app “Acme CRM” to use")
).toBeInTheDocument();
const preferenceSwitch = screen.getByRole("switch", {
name: "Disable Report Writer for Acme CRM",
});
expect(preferenceSwitch).toBeChecked();
expect(preferenceSwitch).toBeEnabled();
expect(
container.querySelector(".card[data-variant='secondary']")
).toBeInTheDocument();
});
it("does not offer an enable toggle until the app is connected", () => {
const { container } = render(
<SkillCard
item={custom({
enabled: false,
can_toggle: false,
external_app: {
external_app_id: 42,
name: "Acme CRM",
enabled: true,
ready: false,
},
})}
/>
);
expect(
screen.getByText("Connect app “Acme CRM” to enable")
).toBeInTheDocument();
expect(screen.queryByRole("switch")).not.toBeInTheDocument();
expect(
container.querySelector(".card[data-variant='disabled']")
).toBeInTheDocument();
});
it("distinguishes an admin-disabled app dependency", () => {
render(
<SkillCard
item={custom({
external_app: {
external_app_id: 42,
name: "Acme CRM",
enabled: false,
ready: false,
},
})}
/>
);
expect(screen.getByText("App “Acme CRM” is disabled")).toBeInTheDocument();
expect(
screen.getByRole("switch", {
name: "Disable Report Writer for Acme CRM",
})
).toBeChecked();
});
it("shows a selected ready associated skill as active", () => {
const { container } = render(
<SkillCard
item={custom({
enabled: true,
external_app: {
external_app_id: 42,
name: "Acme CRM",
enabled: true,
ready: true,
},
})}
/>
);
expect(screen.getByText("Uses app “Acme CRM”")).toBeInTheDocument();
expect(screen.getByText("App skill")).toBeInTheDocument();
expect(screen.queryByText("Custom")).not.toBeInTheDocument();
expect(
screen.getByRole("switch", {
name: "Disable Report Writer for Acme CRM",
})
).toBeChecked();
expect(
container.querySelector(".card[data-variant='primary']")
).toBeInTheDocument();
});
it("shows when another skill with the same name is enabled", () => {
render(
<SkillCard
item={custom({
enabled: false,
external_app: {
external_app_id: 42,
name: "Acme CRM",
enabled: true,
ready: true,
},
})}
hasEnabledNameConflict
/>
);
expect(
screen.getByText("Another skill with this name is enabled")
).toBeInTheDocument();
const preferenceSwitch = screen.getByRole("switch", {
name: "Enable Report Writer for Acme CRM",
});
expect(preferenceSwitch).not.toBeChecked();
expect(preferenceSwitch).toBeEnabled();
});
});
+52 -13
View File
@@ -3,12 +3,13 @@
import { useCallback, type MouseEvent } from "react";
import { Button, Switch, Tag, Tooltip } from "@opal/components";
import { Content } from "@opal/layouts";
import { SvgBlocks, SvgEdit, SvgUser } from "@opal/icons";
import { SvgBlocks, SvgEdit, SvgPlug, SvgUser } from "@opal/icons";
import { CardItemLayout } from "@/layouts/general-layouts";
import { Interactive } from "@opal/core";
import { Card } from "@/refresh-components/cards";
import { useSettings } from "@/lib/settings/hooks";
import type { CustomSkill } from "@/lib/skills/types";
import { cn } from "@opal/utils";
export type SkillCardSource = "builtin" | "custom";
@@ -38,6 +39,7 @@ export type SkillCardItem = BuiltinSkillCardItem | CustomSkillCardItem;
export interface SkillCardProps {
item: SkillCardItem;
hasEnabledNameConflict?: boolean;
onClick?: (item: SkillCardItem) => void;
onEdit?: (item: CustomSkillCardItem) => void;
onEnabledChange?: (item: SkillCardItem, enabled: boolean) => void;
@@ -46,6 +48,7 @@ export interface SkillCardProps {
export default function SkillCard({
item,
hasEnabledNameConflict = false,
onClick,
onEdit,
onEnabledChange,
@@ -59,14 +62,37 @@ export default function SkillCard({
const authorTitle =
item.source === "builtin" ? appName : item.author_email || appName;
const isDisabled = !item.enabled;
const dependency = item.source === "custom" ? item.skill.external_app : null;
const isDependencyUnavailable = dependency !== null && !dependency.ready;
const isSelectedDependencyUnavailable =
item.enabled && isDependencyUnavailable;
const isInactive = !item.enabled || isDependencyUnavailable;
const isInvalid = item.source === "custom" && item.skill.is_valid === false;
const isBuiltinUnavailable = item.source === "builtin" && !item.is_available;
const tooltip = isInvalid
? "This skill is invalid. Delete it and create a new skill."
: isBuiltinUnavailable
? "Skill is currently unavailable. Click to view details."
: undefined;
let dependencyStatus: string | null = null;
if (dependency) {
if (!dependency.ready) {
dependencyStatus = dependency.enabled
? `Connect app “${dependency.name}” to ${item.enabled ? "use" : "enable"}`
: `App “${dependency.name}” is disabled`;
} else {
dependencyStatus =
!item.enabled && hasEnabledNameConflict
? "Another skill with this name is enabled"
: `Uses app “${dependency.name}”`;
}
}
let tooltip: string | undefined;
if (isInvalid) {
tooltip = "This skill is invalid. Delete it and create a new skill.";
} else if (isBuiltinUnavailable) {
tooltip = "Skill is currently unavailable. Click to view details.";
} else if (isDependencyUnavailable && dependency) {
tooltip = dependency.enabled
? `Connect app “${dependency.name}” from the Apps page to use this skill.`
: `App “${dependency.name}” is disabled by an administrator.`;
}
const canEdit =
item.source === "custom" &&
(item.skill.user_permission === "OWNER" ||
@@ -88,15 +114,24 @@ export default function SkillCard({
<Interactive.Simple onClick={handleClick} group="group/SkillCard">
<Card
variant={
isDisabled || isInvalid || isBuiltinUnavailable
isInvalid ||
isBuiltinUnavailable ||
(isDependencyUnavailable && !item.enabled)
? "disabled"
: "primary"
: isInactive
? "secondary"
: "primary"
}
padding={0}
gap={0}
height="full"
>
<div className="flex self-stretch h-24">
<div
className={cn(
"flex self-stretch h-24",
isSelectedDependencyUnavailable && "opacity-50"
)}
>
<CardItemLayout
icon={SvgBlocks}
title={item.name}
@@ -124,8 +159,8 @@ export default function SkillCard({
<div className="bg-background-tint-01 p-1 flex flex-row items-center justify-between w-full">
<div className="py-1 px-2 min-w-0 flex-1">
<Content
icon={SvgUser}
title={authorTitle}
icon={dependency ? SvgPlug : SvgUser}
title={dependencyStatus ?? authorTitle}
sizePreset="secondary"
variant="body"
color="muted"
@@ -138,7 +173,9 @@ export default function SkillCard({
checked={item.enabled}
onCheckedChange={handleEnabledChange}
disabled={enablementPending || isInvalid}
aria-label={`${item.enabled ? "Disable" : "Enable"} ${item.name}`}
aria-label={`${item.enabled ? "Disable" : "Enable"} ${item.name}${
dependency ? ` for ${dependency.name}` : ""
}`}
/>
</div>
)}
@@ -153,6 +190,8 @@ export default function SkillCard({
)
) : isInvalid ? (
<Tag title="Invalid" color="amber" />
) : dependency ? (
<Tag title="App skill" color="blue" />
) : item.is_personal ? (
<Tag title="Personal" color="purple" />
) : (
@@ -0,0 +1,69 @@
import { render, screen } from "@tests/setup/test-utils";
import SkillPreviewModal from "@/sections/modals/SkillPreviewModal";
import type { SkillPreview } from "@/lib/skills/types";
const mockUseSWR = jest.fn();
jest.mock("swr", () => ({
__esModule: true,
...jest.requireActual("swr"),
default: (...args: unknown[]) => mockUseSWR(...args),
}));
function preview(overrides: Partial<SkillPreview> = {}): SkillPreview {
return {
source: "custom",
id: "skill-id",
name: "CRM lookup",
description: "Looks up CRM records",
author_email: "owner@example.com",
instructions_markdown: "Look up the requested record.",
external_app: null,
...overrides,
};
}
describe("SkillPreviewModal", () => {
it("explains when the associated external app must be connected", () => {
mockUseSWR.mockReturnValue({
data: preview({
external_app: {
external_app_id: 42,
name: "Acme CRM",
enabled: true,
ready: false,
},
}),
error: undefined,
isLoading: false,
});
render(<SkillPreviewModal open skillId="skill-id" onClose={jest.fn()} />);
expect(screen.getByText("Skill unavailable")).toBeInTheDocument();
expect(
screen.getByText(
"Connect app “Acme CRM” from the Apps page to use this skill."
)
).toBeInTheDocument();
});
it("does not warn when the associated external app is ready", () => {
mockUseSWR.mockReturnValue({
data: preview({
external_app: {
external_app_id: 42,
name: "Acme CRM",
enabled: true,
ready: true,
},
}),
error: undefined,
isLoading: false,
});
render(<SkillPreviewModal open skillId="skill-id" onClose={jest.fn()} />);
expect(screen.queryByText("Skill unavailable")).not.toBeInTheDocument();
});
});
+17 -2
View File
@@ -30,6 +30,12 @@ function metadataRows(
} else if (preview.author_email) {
rows.push({ label: "Created by", value: preview.author_email });
}
if (preview.external_app) {
rows.push({
label: "External app",
value: preview.external_app.name,
});
}
return rows;
}
@@ -50,6 +56,15 @@ export default function SkillPreviewModal({
} = useSWR<SkillPreview>(swrKey, errorHandlingFetcher);
const instructionsMarkdown =
preview?.instructions_markdown || "No instructions found.";
const dependency = preview?.external_app;
const dependencyUnavailableReason =
dependency && !dependency.ready
? dependency.enabled
? `Connect app “${dependency.name}” from the Apps page to use this skill.`
: `App “${dependency.name}” is disabled by an administrator.`
: null;
const displayedUnavailableReason =
unavailableReason ?? dependencyUnavailableReason;
useEffect(() => {
if (open) {
@@ -83,11 +98,11 @@ export default function SkillPreviewModal({
{preview && !isLoading && !error && (
<Section gap={1} alignItems="stretch">
{unavailableReason && (
{displayedUnavailableReason && (
<MessageCard
variant="warning"
title="Skill unavailable"
description={unavailableReason}
description={displayedUnavailableReason}
/>
)}
@@ -125,8 +125,9 @@ export default function ShareSkillModal({
const canEditShares =
skill?.user_permission === "OWNER" || skill?.user_permission === "EDITOR";
const canEditOrgVisibility =
skill?.user_permission === "OWNER" ||
(isAdmin && skill?.user_permission === "EDITOR");
!skill?.external_app &&
(skill?.user_permission === "OWNER" ||
(isAdmin && skill?.user_permission === "EDITOR"));
const canTransfer =
!!skill &&
(skill.user_permission === "OWNER" || (isAdmin && skill.ownership_vacant));
@@ -397,6 +398,12 @@ export default function ShareSkillModal({
}
/>
{skill.external_app && (
<Text color="text-03" font="secondary-body">
{`Organization-wide viewer access is required while this skill is associated with app “${skill.external_app.name}”.`}
</Text>
)}
<Divider paddingParallel="fit" paddingPerpendicular="fit" />
{skill.owner ? (
+64 -24
View File
@@ -33,6 +33,37 @@ async function fillRequiredFields(user: ReturnType<typeof setupUser>) {
);
}
function existingSkill(
overrides: Partial<SkillEditableDetail> = {}
): SkillEditableDetail {
return {
source: "custom",
id: "existing-id",
name: "report-writer",
description: "Writes reports",
instructions_markdown: "Write the requested report.",
files: [],
is_available: true,
unavailable_reason: null,
is_valid: true,
is_personal: true,
enabled: true,
can_toggle: true,
author_user_id: "author-id",
author_email: "author@example.com",
owner: { id: "author-id", email: "author@example.com" },
ownership_vacant: false,
created_at: null,
updated_at: null,
user_shares: [],
group_shares: [],
public_permission: null,
user_permission: "OWNER",
external_app: null,
...overrides,
};
}
jest.mock("next/navigation", () => ({
usePathname: () => "/craft/v1/skills/new",
useRouter: () => ({
@@ -196,30 +227,7 @@ describe("SkillEditorPage", () => {
it("confirms before leaving an existing skill with unsaved changes", async () => {
const user = setupUser();
const skill: SkillEditableDetail = {
source: "custom",
id: "existing-id",
name: "report-writer",
description: "Writes reports",
instructions_markdown: "Write the requested report.",
files: [],
is_available: true,
unavailable_reason: null,
is_valid: true,
is_personal: true,
enabled: true,
can_toggle: true,
author_user_id: "author-id",
author_email: "author@example.com",
owner: { id: "author-id", email: "author@example.com" },
ownership_vacant: false,
created_at: null,
updated_at: null,
user_shares: [],
group_shares: [],
public_permission: null,
user_permission: "OWNER",
};
const skill = existingSkill();
mockUseSWR.mockReturnValue({
data: skill,
error: undefined,
@@ -243,6 +251,38 @@ describe("SkillEditorPage", () => {
expect(mockDiscardSkillCreationDraft).not.toHaveBeenCalled();
});
it("shows the app dependency and required organization visibility", () => {
const skill = existingSkill({
is_personal: false,
public_permission: "VIEWER",
external_app: {
external_app_id: 42,
name: "Acme CRM",
enabled: true,
ready: false,
},
});
mockUseSWR.mockReturnValue({
data: skill,
error: undefined,
isLoading: false,
mutate: mockRefreshSkill,
});
render(<SkillEditorPage skillId={skill.id} />);
expect(
screen.getByText(
"Connect app “Acme CRM” from the Apps page to use this skill."
)
).toBeInTheDocument();
expect(
screen.getByText(
"Organization-wide viewer access is required while this skill is associated with app “Acme CRM”."
)
).toBeInTheDocument();
});
it("requires confirmation before retrying a same-name creation disabled", async () => {
const user = setupUser();
const conflict = Object.assign(new Error("Name conflict"), {
+27
View File
@@ -75,6 +75,13 @@ function getSharingStatus(skill: SkillEditableDetail): {
description: string;
color: "blue" | "gray" | "purple";
} {
if (skill.external_app) {
return {
title: "Organization",
description: `Organization-wide viewer access is required while this skill is associated with app “${skill.external_app.name}”.`,
color: "blue",
};
}
if (skill.public_permission !== null) {
return {
title: "Organization",
@@ -663,6 +670,26 @@ export default function SkillEditorPage({
/>
<Card border="solid" rounding="lg">
<Section>
{skill.external_app && (
<InputHorizontal
title="External app"
description={
skill.external_app.ready
? `This skill uses app “${skill.external_app.name}”.`
: skill.external_app.enabled
? `Connect app “${skill.external_app.name}” from the Apps page to use this skill.`
: `App “${skill.external_app.name}” is disabled by an administrator.`
}
center
>
<Tag
title={skill.external_app.name}
color={
skill.external_app.ready ? "blue" : "amber"
}
/>
</InputHorizontal>
)}
{sharingStatus && (
<InputHorizontal
title="Sharing"
+134 -8
View File
@@ -15,10 +15,12 @@ const mockToastError = jest.fn();
const mockRouterPush = jest.fn();
const mockUseUserSkills = jest.fn();
const mockStageSkillCreationDraft = jest.fn();
const mockSearchParamsGet = jest.fn();
jest.mock("next/navigation", () => ({
useRouter: () => ({ push: mockRouterPush }),
usePathname: () => "/craft/v1/skills",
useSearchParams: () => ({ get: mockSearchParamsGet }),
}));
jest.mock("@/hooks/useUserSkills", () => ({
@@ -109,6 +111,7 @@ function customSkill(id: string, name: string): CustomSkill {
group_shares: [],
public_permission: "VIEWER",
user_permission: "VIEWER",
external_app: null,
};
}
@@ -144,6 +147,8 @@ describe("SkillsPage preference toggles", () => {
return skillsData;
});
mockRouterPush.mockReset();
mockSearchParamsGet.mockReset();
mockSearchParamsGet.mockReturnValue(null);
mockStageSkillCreationDraft.mockReset();
mockStageSkillCreationDraft.mockReturnValue("draft-id");
});
@@ -185,6 +190,35 @@ describe("SkillsPage preference toggles", () => {
await waitFor(() => expect(firstSwitch).toBeEnabled());
});
it("does not flash an unchanged same-named skill while enabling another", async () => {
const user = setupUser();
const mutation = deferred<CustomSkill>();
skillsData = {
builtins: [],
customs: [
customSkill("first-id", "shared-name"),
customSkill("second-id", "shared-name"),
],
};
mockSetSkillEnabled.mockReturnValueOnce(mutation.promise);
render(<SkillsPage />);
const [firstSwitch, secondSwitch] = screen.getAllByRole("switch", {
name: "shared-name",
});
await user.click(firstSwitch!);
expect(firstSwitch).toHaveAttribute("aria-checked", "true");
expect(firstSwitch).toBeDisabled();
expect(secondSwitch).toHaveAttribute("aria-checked", "false");
expect(secondSwitch).toBeEnabled();
await act(async () => {
mutation.resolve(enabledCustomAt(0));
await mutation.promise;
});
});
it("keeps simultaneous skill mutations independently optimistic and pending", async () => {
const user = setupUser();
const firstMutation = deferred<CustomSkill>();
@@ -276,7 +310,15 @@ describe("SkillsPage preference toggles", () => {
...customSkill("first-id", "report-writer"),
enabled: true,
};
const second = customSkill("second-id", "report-writer");
const second = {
...customSkill("second-id", "report-writer"),
external_app: {
external_app_id: 42,
name: "Acme CRM",
enabled: true,
ready: true,
},
};
skillsData = { builtins: [], customs: [first, second] };
mockSetSkillEnabled.mockResolvedValueOnce({ ...second, enabled: true });
render(<SkillsPage />);
@@ -285,12 +327,19 @@ describe("SkillsPage preference toggles", () => {
await user.click(switches[1]!);
expect(mockSetSkillEnabled).not.toHaveBeenCalled();
expect(screen.getByText("Switch active skill?")).toBeInTheDocument();
expect(
screen.getByText("Switch “report-writer” skill?")
).toBeInTheDocument();
expect(
screen.getAllByText(
"Only one skill named “report-writer” can be active at a time."
"Only one skill named “report-writer” can be enabled at a time."
)
).not.toHaveLength(0);
expect(
screen.getByText(
"Continuing will disable the currently enabled skill and enable this one."
)
).toBeInTheDocument();
await user.click(screen.getByRole("button", { name: "Cancel" }));
expect(mockSetSkillEnabled).not.toHaveBeenCalled();
@@ -305,7 +354,9 @@ describe("SkillsPage preference toggles", () => {
expect(switches[0]).toHaveAttribute("aria-checked", "false");
expect(switches[1]).toHaveAttribute("aria-checked", "true");
});
expect(screen.queryByText("Switch active skill?")).not.toBeInTheDocument();
expect(
screen.queryByText("Switch “report-writer” skill?")
).not.toBeInTheDocument();
});
it("keeps the switch confirmation open when replacement fails", async () => {
@@ -326,7 +377,9 @@ describe("SkillsPage preference toggles", () => {
await user.click(screen.getByRole("button", { name: "Switch skill" }));
expect(screen.getByRole("button", { name: "Switching..." })).toBeDisabled();
expect(screen.getByText("Switch active skill?")).toBeInTheDocument();
expect(
screen.getByText("Switch “report-writer” skill?")
).toBeInTheDocument();
await act(async () => {
mutation.reject(new Error("Replacement failed"));
@@ -336,7 +389,9 @@ describe("SkillsPage preference toggles", () => {
await waitFor(() =>
expect(screen.getByRole("button", { name: "Switch skill" })).toBeEnabled()
);
expect(screen.getByText("Switch active skill?")).toBeInTheDocument();
expect(
screen.getByText("Switch “report-writer” skill?")
).toBeInTheDocument();
expect(mockToastError).toHaveBeenCalledWith("Replacement failed");
});
@@ -352,7 +407,9 @@ describe("SkillsPage preference toggles", () => {
await user.click(screen.getByRole("switch", { name: "first-skill" }));
expect(await screen.findByText("Switch active skill?")).toBeInTheDocument();
expect(
await screen.findByText("Switch “first-skill” skill?")
).toBeInTheDocument();
expect(mockToastError).not.toHaveBeenCalled();
await user.click(screen.getByRole("button", { name: "Switch skill" }));
@@ -364,7 +421,76 @@ describe("SkillsPage preference toggles", () => {
)
);
await waitFor(() =>
expect(screen.queryByText("Switch active skill?")).not.toBeInTheDocument()
expect(
screen.queryByText("Switch “first-skill” skill?")
).not.toBeInTheDocument()
);
});
it("focuses the list on skills associated with a reviewed app", () => {
mockSearchParamsGet.mockImplementation((key: string) =>
key === "externalAppId" ? "42" : null
);
skillsData = {
builtins: [],
customs: [
{
...customSkill("associated-id", "crm-workflow"),
external_app: {
external_app_id: 42,
name: "Acme CRM",
enabled: true,
ready: true,
},
},
customSkill("other-id", "other-skill"),
],
};
render(<SkillsPage />);
expect(screen.getByText("Skills for app “Acme CRM”")).toBeInTheDocument();
expect(
screen.getByText(
"Enable every skill associated with app “Acme CRM”. The app may not work correctly without them. If another skill with the same name is enabled, enabling the app-associated skill disables the other skill for you."
)
).toBeInTheDocument();
expect(
screen.getByRole("switch", { name: "crm-workflow" })
).toBeInTheDocument();
expect(
screen.queryByRole("switch", { name: "other-skill" })
).not.toBeInTheDocument();
});
it("shows all skills when an app focus no longer resolves", () => {
mockSearchParamsGet.mockImplementation((key: string) =>
key === "externalAppId" ? "999" : null
);
skillsData = {
builtins: [],
customs: [
{
...customSkill("associated-id", "crm-workflow"),
external_app: {
external_app_id: 42,
name: "Acme CRM",
enabled: true,
ready: true,
},
},
customSkill("other-id", "other-skill"),
],
};
render(<SkillsPage />);
expect(screen.queryByText(/Skills for app/)).not.toBeInTheDocument();
expect(
screen.getByRole("switch", { name: "crm-workflow" })
).toBeInTheDocument();
expect(
screen.getByRole("switch", { name: "other-skill" })
).toBeInTheDocument();
});
});
+68 -13
View File
@@ -1,7 +1,7 @@
"use client";
import { useMemo, useRef, useState } from "react";
import { useRouter } from "next/navigation";
import { useRouter, useSearchParams } from "next/navigation";
import type { Route } from "next";
import {
Button,
@@ -45,6 +45,11 @@ import { isSkillNameConflict, setSkillEnabled } from "@/lib/skills/api";
export default function SkillsPage() {
const router = useRouter();
const externalAppIdParam = useSearchParams().get("externalAppId");
const focusedExternalAppId =
externalAppIdParam !== null && /^\d+$/.test(externalAppIdParam)
? Number(externalAppIdParam)
: null;
const { data, error, isLoading, refresh } = useUserSkills();
const [searchQuery, setSearchQuery] = useState("");
const [createOpen, setCreateOpen] = useState(false);
@@ -89,9 +94,14 @@ export default function SkillsPage() {
return;
}
const affectedItems = enabled
? items.filter((candidate) => candidate.name === item.name)
: [item];
const affectedItems =
enabled && replaceConflict
? items.filter(
(candidate) =>
candidate.id === item.id ||
(candidate.name === item.name && candidate.enabled)
)
: [item];
const affectedIds = new Set(affectedItems.map(({ id }) => id));
setPendingSkillIds((current) => {
const next = new Set(current);
@@ -224,22 +234,50 @@ export default function SkillsPage() {
);
}, [data, optimisticEnabledById]);
const focusedAppName = useMemo(() => {
if (focusedExternalAppId === null) return null;
for (const item of items) {
if (
item.source === "custom" &&
item.skill.external_app?.external_app_id === focusedExternalAppId
) {
return item.skill.external_app.name;
}
}
return null;
}, [focusedExternalAppId, items]);
const enabledItemByName = useMemo(
() =>
new Map(
items
.filter((item) => item.enabled)
.map((item) => [item.name, item] as const)
),
[items]
);
const visibleItems = useMemo(() => {
const q = searchQuery.trim().toLowerCase();
if (!q) return items;
return items.filter(
(item) =>
item.name.toLowerCase().includes(q) ||
item.description.toLowerCase().includes(q)
(focusedAppName === null ||
(item.source === "custom" &&
item.skill.external_app?.external_app_id ===
focusedExternalAppId)) &&
(!q ||
item.name.toLowerCase().includes(q) ||
item.description.toLowerCase().includes(q))
);
}, [items, searchQuery]);
}, [focusedAppName, focusedExternalAppId, items, searchQuery]);
const switchPending =
pendingSwitchTarget !== null && pendingSkillIds.has(pendingSwitchTarget.id);
const previewUnavailableReason =
previewTarget?.source === "builtin" && !previewTarget.is_available
? (previewTarget.unavailable_reason ??
"This skill is currently unavailable.")
: null;
const switchPending =
pendingSwitchTarget !== null && pendingSkillIds.has(pendingSwitchTarget.id);
return (
<SettingsLayouts.Root data-testid="SkillsPage/container">
@@ -291,6 +329,19 @@ export default function SkillsPage() {
</SettingsLayouts.Header>
<SettingsLayouts.Body>
{focusedAppName && (
<MessageCard
variant="info"
title={`Skills for app “${focusedAppName}”`}
description={`Enable every skill associated with app “${focusedAppName}”. The app may not work correctly without them. If another skill with the same name is enabled, enabling the app-associated skill disables the other skill for you.`}
rightChildren={
<Button prominence="secondary" href="/craft/v1/skills">
Show all skills
</Button>
}
/>
)}
{isLoading && <SvgSimpleLoader />}
{error && !isLoading && (
@@ -328,6 +379,9 @@ export default function SkillsPage() {
<SkillCard
key={item.id}
item={item}
hasEnabledNameConflict={
!item.enabled && enabledItemByName.has(item.name)
}
onEdit={handleEdit}
onClick={setPreviewTarget}
onEnabledChange={(skill, enabled) =>
@@ -369,8 +423,8 @@ export default function SkillsPage() {
{pendingSwitchTarget && (
<ConfirmationModalLayout
icon={SvgAlertTriangle}
title="Switch active skill?"
description={`Only one skill named “${pendingSwitchTarget.name}” can be active at a time.`}
title={`Switch “${pendingSwitchTarget.name}” skill?`}
description={`Only one skill named “${pendingSwitchTarget.name}” can be enabled at a time.`}
onClose={
switchPending ? undefined : () => setPendingSwitchTarget(null)
}
@@ -386,7 +440,8 @@ export default function SkillsPage() {
</Button>
}
>
Continuing will disable the active skill and enable the selected one.
Continuing will disable the currently enabled skill and enable this
one.
</ConfirmationModalLayout>
)}
</SettingsLayouts.Root>