mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-23 00:53:38 +02:00
fix(connections): make 'Set all' change every action in the group
The group control called the single-action setter once per row. Each call rebuilt the whole enabled/ask-first set from the same render and saved it, so the saves overwrote one another and only one action changed. The setter now takes a list of ids and the group sends one change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
43a65aabc5
commit
ff111ca620
7 files changed
+148
-35
No files matched your search
@@ -135,7 +135,7 @@ export function RemoteMcpConnectionSetup({ provider, state: s, actions: a, agent
|
||||
enabledIds={new Set(s.tools.filter((entry) => s.permissions[entry.id] !== "off").map((entry) => entry.id))}
|
||||
askFirstIds={new Set(s.tools.filter((entry) => s.permissions[entry.id] === "ask_first").map((entry) => entry.id))}
|
||||
disabled={!s.connected} refreshPending={s.refreshing} canConfigure
|
||||
onSetPermission={(id, next) => change({ permissions: { ...s.permissions, [id]: next === "ask" ? "ask_first" : next } })}
|
||||
onSetPermission={(ids, next) => change({ permissions: { ...s.permissions, ...Object.fromEntries(ids.map((id) => [id, next === "ask" ? "ask_first" : next])) } })}
|
||||
onReviewQuarantined={() => {}} onRefreshActions={a.refresh} />
|
||||
</>}
|
||||
<div className="mx-auto max-w-2xl space-y-6">
|
||||
|
||||
@@ -52,6 +52,7 @@ import { appTabHref, appTabLabel, isAppTabKey, type AppTabKey } from "./app-tabs
|
||||
import { ConnectionProvenanceChip } from "./ConnectionProvenanceChip";
|
||||
import { IdentitiesSection } from "./app-detail/IdentitiesSection";
|
||||
import { PermissionsPanel } from "./app-detail/PermissionsPanel";
|
||||
import { actionPermissionMutation } from "./app-detail/action-permissions";
|
||||
import { RailwayAccessPanel } from "./app-detail/RailwayAccessPanel";
|
||||
import { ReviewPanel } from "./app-detail/ReviewPanel";
|
||||
import {
|
||||
@@ -691,7 +692,7 @@ export function AppDetail({ renderActions, onReconnect }: {
|
||||
}
|
||||
onSaveAccess={(next) => apply({ access: connection.connectionPurpose === "ai" || managesRemoteMcpAccess ? next : accessIncludingInstalls(next, install) })}
|
||||
onRefreshActions={() => refreshTools.mutate()}
|
||||
onSetActionPermission={(id, next) => apply(actionPermissionMutation(id, next, enabledIds, askFirstIds))}
|
||||
onSetActionPermission={(ids, next) => apply(actionPermissionMutation(ids, next, enabledIds, askFirstIds))}
|
||||
onReviewQuarantined={reviewQuarantined}
|
||||
/>
|
||||
{managesRemoteMcpAccess && isRemoteMcpConnectorId(connection.config?.sourceTemplateKey) && <RemoteMcpManagement
|
||||
@@ -957,21 +958,3 @@ function galleryEntryFor(
|
||||
null;
|
||||
}
|
||||
|
||||
function actionPermissionMutation(
|
||||
id: string,
|
||||
next: "off" | "allowed" | "ask",
|
||||
enabledIds: Set<string>,
|
||||
askFirstIds: Set<string>,
|
||||
) {
|
||||
const enabled = new Set(enabledIds);
|
||||
const askFirst = new Set(askFirstIds);
|
||||
if (next === "off") {
|
||||
enabled.delete(id);
|
||||
askFirst.delete(id);
|
||||
} else {
|
||||
enabled.add(id);
|
||||
if (next === "ask") askFirst.add(id);
|
||||
else askFirst.delete(id);
|
||||
}
|
||||
return { enabled, askFirst };
|
||||
}
|
||||
@@ -0,0 +1,72 @@
|
||||
// @vitest-environment jsdom
|
||||
|
||||
import { flushSync } from "react-dom";
|
||||
import { createRoot } from "react-dom/client";
|
||||
import { MemoryRouter } from "react-router-dom";
|
||||
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
import type { ToolCatalogEntry } from "@paperclipai/shared";
|
||||
import { ActionsSection } from "./PermissionsPanel";
|
||||
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
(globalThis as any).IS_REACT_ACT_ENVIRONMENT = true;
|
||||
|
||||
vi.mock("@/context/CompanyContext", () => ({
|
||||
useCompany: () => ({ selectedCompanyId: "company-1", selectedCompany: { id: "company-1", issuePrefix: "PAP" } }),
|
||||
}));
|
||||
|
||||
let container: HTMLDivElement | null = null;
|
||||
|
||||
afterEach(() => {
|
||||
container?.remove();
|
||||
container = null;
|
||||
});
|
||||
|
||||
const entry = (id: string, riskLevel: string) =>
|
||||
({ id, toolName: id, title: id, riskLevel, status: "active" }) as unknown as ToolCatalogEntry;
|
||||
|
||||
describe("Permissions group control", () => {
|
||||
it("sets every action in the group in one change", () => {
|
||||
const onSetPermission = vi.fn();
|
||||
const writes = [entry("send", "write"), entry("delete", "destructive"), entry("label", "write")];
|
||||
container = document.createElement("div");
|
||||
document.body.appendChild(container);
|
||||
const root = createRoot(container);
|
||||
flushSync(() =>
|
||||
root.render(
|
||||
<QueryClientProvider client={new QueryClient()}>
|
||||
<MemoryRouter>
|
||||
<ActionsSection
|
||||
connectionId="conn-1"
|
||||
appName="Gmail"
|
||||
readOnly={[entry("search", "read")]}
|
||||
canChange={writes}
|
||||
quarantined={[]}
|
||||
enabledIds={new Set(["search", "send", "delete", "label"])}
|
||||
askFirstIds={new Set(["delete"])}
|
||||
disabled={false}
|
||||
refreshPending={false}
|
||||
canConfigure
|
||||
onSetPermission={onSetPermission}
|
||||
onReviewQuarantined={vi.fn()}
|
||||
onRefreshActions={vi.fn()}
|
||||
/>
|
||||
</MemoryRouter>
|
||||
</QueryClientProvider>,
|
||||
),
|
||||
);
|
||||
|
||||
const select = container.querySelector<HTMLSelectElement>('select[aria-label="Set every action in Write (3)"]');
|
||||
expect(select).toBeTruthy();
|
||||
expect(select!.value).toBe("");
|
||||
flushSync(() => {
|
||||
select!.value = "ask";
|
||||
select!.dispatchEvent(new Event("change", { bubbles: true }));
|
||||
});
|
||||
|
||||
expect(onSetPermission).toHaveBeenCalledTimes(1);
|
||||
const [ids, next] = onSetPermission.mock.calls[0]!;
|
||||
expect([...ids].sort()).toEqual(["delete", "label", "send"]);
|
||||
expect(next).toBe("ask");
|
||||
});
|
||||
});
|
||||
@@ -53,7 +53,7 @@ export function PermissionsPanel({
|
||||
connectionId: string;
|
||||
install: InstallState;
|
||||
onSaveAccess: (next: AccessDraft) => void;
|
||||
onSetActionPermission: (id: string, next: ActionPermission) => void;
|
||||
onSetActionPermission: (ids: string[], next: ActionPermission) => void;
|
||||
onReviewQuarantined: (enabledIds: string[]) => void;
|
||||
onRefreshActions: () => void;
|
||||
refreshPending: boolean;
|
||||
@@ -222,7 +222,7 @@ export function ActionsSection({
|
||||
focusId?: string | null;
|
||||
canConfigure: boolean;
|
||||
permissionChangeWarning?: string;
|
||||
onSetPermission: (id: string, next: ActionPermission) => void;
|
||||
onSetPermission: (ids: string[], next: ActionPermission) => void;
|
||||
onReviewQuarantined: (enabledIds: string[]) => void;
|
||||
onRefreshActions: () => void;
|
||||
}) {
|
||||
@@ -316,9 +316,9 @@ export function ActionsSection({
|
||||
disabled={disabled}
|
||||
focusId={focusId}
|
||||
canConfigure={canConfigure}
|
||||
onSetPermission={(id, next) => {
|
||||
onSetPermission={(ids, next) => {
|
||||
setShowPermissionChangeWarning(true);
|
||||
onSetPermission(id, next);
|
||||
onSetPermission(ids, next);
|
||||
}}
|
||||
/>
|
||||
<ActionGroup
|
||||
@@ -331,9 +331,9 @@ export function ActionsSection({
|
||||
disabled={disabled}
|
||||
focusId={focusId}
|
||||
canConfigure={canConfigure}
|
||||
onSetPermission={(id, next) => {
|
||||
onSetPermission={(ids, next) => {
|
||||
setShowPermissionChangeWarning(true);
|
||||
onSetPermission(id, next);
|
||||
onSetPermission(ids, next);
|
||||
}}
|
||||
/>
|
||||
</div>
|
||||
@@ -381,7 +381,7 @@ function ActionGroup({
|
||||
disabled: boolean;
|
||||
focusId?: string | null;
|
||||
canConfigure: boolean;
|
||||
onSetPermission: (id: string, next: ActionPermission) => void;
|
||||
onSetPermission: (ids: string[], next: ActionPermission) => void;
|
||||
}) {
|
||||
if (actions.length === 0) return null;
|
||||
const groupValue = (() => {
|
||||
@@ -395,9 +395,9 @@ function ActionGroup({
|
||||
<div className="mb-1.5 flex flex-wrap items-center justify-between gap-2">
|
||||
<h3 className="text-xs font-semibold uppercase tracking-wide text-muted-foreground">{title}</h3>
|
||||
{canConfigure ? (
|
||||
// PAP-659 C6b: set the whole group at once. Setting seventeen write
|
||||
// actions one row at a time is the reason the armed default would
|
||||
// otherwise be hard to live with; per-row overrides stay underneath.
|
||||
// PAP-659 C6b: set the whole group at once, so narrowing a fresh
|
||||
// connection's writes is one choice rather than one per action;
|
||||
// per-row overrides stay underneath.
|
||||
<label className="flex items-center gap-2 text-xs text-muted-foreground">
|
||||
<span className="sr-only">{`Set every action in ${title}`}</span>
|
||||
<select
|
||||
@@ -405,8 +405,10 @@ function ActionGroup({
|
||||
value={groupValue}
|
||||
disabled={disabled}
|
||||
onChange={(event) => {
|
||||
const next = event.target.value as ActionPermission;
|
||||
for (const action of actions) onSetPermission(action.id, next);
|
||||
// One change for the whole group: each row's setter rebuilds the
|
||||
// full permission set from the same render, so looping it lets
|
||||
// the last call overwrite the others.
|
||||
onSetPermission(actions.map((action) => action.id), event.target.value as ActionPermission);
|
||||
}}
|
||||
className="h-7 rounded-md border border-input bg-background px-2 text-xs text-foreground outline-none focus-visible:border-ring focus-visible:ring-3 focus-visible:ring-ring/30"
|
||||
>
|
||||
@@ -465,7 +467,7 @@ function ActionRow({
|
||||
disabled: boolean;
|
||||
focused: boolean;
|
||||
canConfigure: boolean;
|
||||
onSetPermission: (id: string, next: ActionPermission) => void;
|
||||
onSetPermission: (ids: string[], next: ActionPermission) => void;
|
||||
}) {
|
||||
const rowRef = useRef<HTMLDivElement | null>(null);
|
||||
const [testOpen, setTestOpen] = useState(false);
|
||||
@@ -522,7 +524,7 @@ function ActionRow({
|
||||
aria-checked={selected}
|
||||
aria-label={`${title}: ${option.label}`}
|
||||
disabled={disabled}
|
||||
onClick={() => onSetPermission(action.id, option.value)}
|
||||
onClick={() => onSetPermission([action.id], option.value)}
|
||||
className={cn(
|
||||
"flex h-8 w-8 items-center justify-center rounded-sm text-muted-foreground outline-none transition-colors",
|
||||
"hover:bg-background hover:text-foreground focus-visible:ring-2 focus-visible:ring-ring",
|
||||
|
||||
@@ -0,0 +1,29 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { actionPermissionMutation } from "./action-permissions";
|
||||
|
||||
describe("actionPermissionMutation", () => {
|
||||
const enabled = new Set(["a", "b", "c"]);
|
||||
const askFirst = new Set(["b"]);
|
||||
|
||||
it("sets every action in a group at once", () => {
|
||||
expect(actionPermissionMutation(["a", "b", "c"], "ask", enabled, askFirst)).toEqual({
|
||||
enabled: new Set(["a", "b", "c"]),
|
||||
askFirst: new Set(["a", "b", "c"]),
|
||||
});
|
||||
expect(actionPermissionMutation(["a", "b", "c"], "off", enabled, askFirst)).toEqual({
|
||||
enabled: new Set(),
|
||||
askFirst: new Set(),
|
||||
});
|
||||
expect(actionPermissionMutation(["a", "b", "c"], "allowed", new Set(), askFirst)).toEqual({
|
||||
enabled: new Set(["a", "b", "c"]),
|
||||
askFirst: new Set(),
|
||||
});
|
||||
});
|
||||
|
||||
it("leaves actions outside the group alone", () => {
|
||||
expect(actionPermissionMutation(["a"], "off", enabled, askFirst)).toEqual({
|
||||
enabled: new Set(["b", "c"]),
|
||||
askFirst: new Set(["b"]),
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,27 @@
|
||||
/**
|
||||
* Apply one permission to a set of actions in a single change.
|
||||
*
|
||||
* Every save rewrites the connection's whole enabled/ask-first set, so a
|
||||
* group control must not issue one save per action: each would start from the
|
||||
* same render and the last would overwrite the rest.
|
||||
*/
|
||||
export function actionPermissionMutation(
|
||||
ids: string[],
|
||||
next: "off" | "allowed" | "ask",
|
||||
enabledIds: Set<string>,
|
||||
askFirstIds: Set<string>,
|
||||
) {
|
||||
const enabled = new Set(enabledIds);
|
||||
const askFirst = new Set(askFirstIds);
|
||||
for (const id of ids) {
|
||||
if (next === "off") {
|
||||
enabled.delete(id);
|
||||
askFirst.delete(id);
|
||||
} else {
|
||||
enabled.add(id);
|
||||
if (next === "ask") askFirst.add(id);
|
||||
else askFirst.delete(id);
|
||||
}
|
||||
}
|
||||
return { enabled, askFirst };
|
||||
}
|
||||
@@ -70,7 +70,7 @@ function Permissions() {
|
||||
const [enabled, setEnabled] = useState(new Set(["transcripts", "summary"]));
|
||||
const [ask, setAsk] = useState(new Set(["share"]));
|
||||
const entries = [["transcripts", "List meeting transcripts", false], ["summary", "Get meeting summary and action items", false], ["transcript", "Get full transcript", false], ["share", "Share a meeting", true], ["revoke", "Revoke meeting access", true]].map(([id, title, write]) => ({ id, toolName: id, title, description: title, connectionId: "storybook-fireflies", entryKind: "tool", status: "active", isReadOnly: !write, isWrite: write, isDestructive: id === "revoke", riskLevel: write ? "medium" : "low" } as ToolCatalogEntry));
|
||||
return <Frame><ActionsSection connectionId="storybook-fireflies" appName="Fireflies" readOnly={entries.filter((e) => e.isReadOnly)} canChange={entries.filter((e) => e.isWrite)} quarantined={[]} enabledIds={enabled} askFirstIds={ask} disabled={false} refreshPending={false} canConfigure onSetPermission={(id, next) => { setEnabled((s) => { const n = new Set(s); if (next === "allowed") n.add(id); else n.delete(id); return n; }); setAsk((s) => { const n = new Set(s); if (next === "ask") n.add(id); else n.delete(id); return n; }); }} onReviewQuarantined={noop} onRefreshActions={noop} /></Frame>;
|
||||
return <Frame><ActionsSection connectionId="storybook-fireflies" appName="Fireflies" readOnly={entries.filter((e) => e.isReadOnly)} canChange={entries.filter((e) => e.isWrite)} quarantined={[]} enabledIds={enabled} askFirstIds={ask} disabled={false} refreshPending={false} canConfigure onSetPermission={(ids, next) => { setEnabled((s) => { const n = new Set(s); for (const id of ids) { if (next === "allowed") n.add(id); else n.delete(id); } return n; }); setAsk((s) => { const n = new Set(s); for (const id of ids) { if (next === "ask") n.add(id); else n.delete(id); } return n; }); }} onReviewQuarantined={noop} onRefreshActions={noop} /></Frame>;
|
||||
}
|
||||
export const RetainedPermissions: Story = { name: "08 · Permissions — retained Allowed, Ask first, Off", render: () => <Permissions /> };
|
||||
|
||||
|
||||
Reference in new issue
Block a user