diff --git a/.changeset/calm-tools-report-failures.md b/.changeset/calm-tools-report-failures.md new file mode 100644 index 000000000..c153a99ce --- /dev/null +++ b/.changeset/calm-tools-report-failures.md @@ -0,0 +1,5 @@ +--- +"executor": patch +--- + +Show policy and OAuth app removal failures in the UI, and keep success-only state unchanged when those writes fail. diff --git a/e2e/scenarios/policies-ui.test.ts b/e2e/scenarios/policies-ui.test.ts index dec30dd4e..cc400cd0a 100644 --- a/e2e/scenarios/policies-ui.test.ts +++ b/e2e/scenarios/policies-ui.test.ts @@ -158,6 +158,7 @@ scenario( .getByRole("button") .filter({ hasText: leaf }) .getByLabel(label, { exact: true }); + const internalError = JSON.stringify({ _tag: "InternalError", traceId: "policy-write" }); await step("Open the integration's Tools tab", async () => { await page.goto(`/integrations/${integration}`, { waitUntil: "networkidle" }); @@ -172,6 +173,30 @@ scenario( await policyMenuFor(alpha, `${integration}.records.create`).waitFor(); }); + await step("A rejected policy create reports the failure and writes nothing", async () => { + await page.route("**/api/policies", async (route) => { + if (route.request().method() !== "POST") { + await route.continue(); + return; + } + await route.fulfill({ + status: 500, + contentType: "application/json", + body: internalError, + }); + }); + await policyMenuFor(alpha, `${integration}.records.create`).click(); + await page.getByText(leafPattern, { exact: true }).waitFor(); + await page.getByRole("menuitem", { name: "Block" }).click(); + await page.getByText("Failed to create policy", { exact: true }).waitFor(); + const afterFailure = await Effect.runPromise(client.policies.list()); + expect( + afterFailure.map((policy) => policy.pattern), + "a rejected create does not persist the optimistic policy", + ).not.toContain(leafPattern); + await page.unroute("**/api/policies"); + }); + await step("Block records.create from the per-tool menu", async () => { await policyMenuFor(alpha, `${integration}.records.create`).click(); // The menu is headed by the exact pattern it will store. @@ -180,6 +205,30 @@ scenario( await leafIndicator(alpha, "create", `Blocked (matched ${leafPattern})`).waitFor(); }); + await step("A rejected clear reports the failure and keeps the policy active", async () => { + await page.route("**/api/policies/*", async (route) => { + if (route.request().method() !== "DELETE") { + await route.continue(); + return; + } + await route.fulfill({ + status: 500, + contentType: "application/json", + body: internalError, + }); + }); + await policyMenuFor(alpha, `${integration}.records.create`).click(); + await page.getByRole("menuitem", { name: "Clear" }).click(); + await page.getByText("Failed to clear policy", { exact: true }).waitFor(); + await leafIndicator(alpha, "create", `Blocked (matched ${leafPattern})`).waitFor(); + const afterFailure = await Effect.runPromise(client.policies.list()); + expect( + afterFailure.map((policy) => policy.pattern), + "a rejected clear leaves the stored policy intact", + ).toContain(leafPattern); + await page.unroute("**/api/policies/*"); + }); + await step("Require approval for the whole records category", async () => { await policyMenuFor(alpha, `${integration}.records.*`).click(); await page.getByText(categoryPattern, { exact: true }).waitFor(); diff --git a/e2e/selfhost/oauth-app-modal.test.ts b/e2e/selfhost/oauth-app-modal.test.ts index 4a2f8beea..d699d6c50 100644 --- a/e2e/selfhost/oauth-app-modal.test.ts +++ b/e2e/selfhost/oauth-app-modal.test.ts @@ -129,6 +129,33 @@ scenario( await actions.waitFor(); }); + await step("A rejected removal reports the failure and keeps the app", async () => { + await page.route("**/api/oauth/clients/**", async (route) => { + if (route.request().method() !== "DELETE") { + await route.continue(); + return; + } + await route.fulfill({ + status: 500, + contentType: "application/json", + body: JSON.stringify({ _tag: "InternalError", traceId: "oauth-client-remove" }), + }); + }); + await actions.click(); + await page.getByRole("menuitem", { name: "Remove" }).click(); + await page.getByRole("button", { name: "Remove app" }).click(); + await page.getByText(`Failed to remove ${appName}`, { exact: true }).waitFor(); + await page.getByRole("heading", { name: `Remove ${appName}?` }).waitFor(); + const clients = await Effect.runPromise(client.oauth.listClients()); + expect( + clients.map((candidate) => String(candidate.slug)), + "a rejected removal leaves the registered app in the catalog", + ).toContain(appName); + await page.getByRole("button", { name: "Cancel" }).click(); + await actions.waitFor(); + await page.unroute("**/api/oauth/clients/**"); + }); + await step("Remove the app and confirm", async () => { await actions.click(); await page.getByRole("menuitem", { name: "Remove" }).click(); diff --git a/packages/react/src/components/add-account-modal.tsx b/packages/react/src/components/add-account-modal.tsx index 7259b199c..ef8544f08 100644 --- a/packages/react/src/components/add-account-modal.tsx +++ b/packages/react/src/components/add-account-modal.tsx @@ -1202,7 +1202,7 @@ function AddAccountModalView(props: AddAccountModalProps) { const doRegisterDynamic = useAtomSet(registerDynamicOAuthClient, { mode: "promiseExit", }); - const doRemoveOAuthClient = useAtomSet(removeOAuthClientOptimistic, { mode: "promise" }); + const doRemoveOAuthClient = useAtomSet(removeOAuthClientOptimistic, { mode: "promiseExit" }); const doValidate = useAtomSet(validateConnection, { mode: "promiseExit" }); const doCheckConnectionHealth = useAtomSet(checkConnectionHealth, { mode: "promiseExit" }); const doUpdateConnection = useAtomSet(updateConnection, { mode: "promiseExit" }); @@ -1542,12 +1542,16 @@ function AddAccountModalView(props: AddAccountModalProps) { // reconnect at their next refresh); clear the picked app if it was the one // removed so the connect button doesn't point at a gone slug. const handleRemoveApp = async (client: OAuthClientSummary): Promise => { - setRemovingClient(null); - await doRemoveOAuthClient({ + const exit = await doRemoveOAuthClient({ params: { slug: client.slug }, payload: { owner: client.owner }, reactivityKeys: oauthClientWriteKeys, }); + if (Exit.isFailure(exit)) { + toast.error(messageFromExit(exit, `Failed to remove ${String(client.slug)}`)); + return; + } + setRemovingClient(null); trackEvent("oauth_client_removed", { owner: client.owner }); toast.success(`Removed ${String(client.slug)}`); if (pickedApp === String(client.slug)) setPickedApp(null); diff --git a/packages/react/src/hooks/use-policy-actions.ts b/packages/react/src/hooks/use-policy-actions.ts index c7d594698..af5958a87 100644 --- a/packages/react/src/hooks/use-policy-actions.ts +++ b/packages/react/src/hooks/use-policy-actions.ts @@ -1,6 +1,8 @@ import { useCallback, useMemo, useRef } from "react"; import { useAtomSet, useAtomValue } from "@effect/atom-react"; +import * as Exit from "effect/Exit"; import * as AsyncResult from "effect/unstable/reactivity/AsyncResult"; +import { toast } from "sonner"; import { PolicyId, positionForNewPattern, @@ -16,6 +18,7 @@ import { } from "../api/atoms"; import { policyWriteKeys } from "../api/reactivity-keys"; import { trackEvent } from "../api/analytics"; +import { messageFromExit } from "../api/error-reporting"; export interface PolicyAction { /** Set the action on a pattern. If a user rule with this exact pattern @@ -44,9 +47,9 @@ export interface PolicyAction { */ export const usePolicyActions = (owner: Owner = "org"): PolicyAction => { const policies = useAtomValue(policiesOptimisticAtom); - const doCreate = useAtomSet(createPolicyOptimistic, { mode: "promise" }); - const doUpdate = useAtomSet(updatePolicyOptimistic, { mode: "promise" }); - const doRemove = useAtomSet(removePolicyOptimistic, { mode: "promise" }); + const doCreate = useAtomSet(createPolicyOptimistic, { mode: "promiseExit" }); + const doUpdate = useAtomSet(updatePolicyOptimistic, { mode: "promiseExit" }); + const doRemove = useAtomSet(removePolicyOptimistic, { mode: "promiseExit" }); // Sorted by position ASC (lowest position = highest precedence first), // matching server evaluation order. Optimistic placeholder rows carry @@ -105,23 +108,31 @@ export const usePolicyActions = (owner: Owner = "org"): PolicyAction => { const existing = findExact(pattern); if (existing) { if (existing.action === action) return; - await doUpdate({ + const exit = await doUpdate({ params: { policyId: PolicyId.make(existing.id) }, payload: { owner, action }, reactivityKeys: policyWriteKeys, }); + if (Exit.isFailure(exit)) { + toast.error(messageFromExit(exit, "Failed to update policy")); + return; + } trackEvent("tool_policy_set", { action, pattern_kind: patternKind, owner }); return; } const position = computePosition(pattern); - const created = await doCreate({ + const exit = await doCreate({ payload: position === undefined ? { owner, pattern, action } : { owner, pattern, action, position }, reactivityKeys: policyWriteKeys, }); - createdIdByPattern.current.set(pattern, String(created.id)); + if (Exit.isFailure(exit)) { + toast.error(messageFromExit(exit, "Failed to create policy")); + return; + } + createdIdByPattern.current.set(pattern, String(exit.value.id)); trackEvent("tool_policy_set", { action, pattern_kind: patternKind, owner }); }, [owner, doCreate, doUpdate, findExact, computePosition], @@ -138,12 +149,16 @@ export const usePolicyActions = (owner: Owner = "org"): PolicyAction => { const isReal = (id: string | undefined): id is string => !!id && !id.startsWith("pending-"); const id = [existing?.id, createdIdByPattern.current.get(pattern), policyId].find(isReal); if (!id) return; - createdIdByPattern.current.delete(pattern); - await doRemove({ + const exit = await doRemove({ params: { policyId: PolicyId.make(id) }, payload: { owner }, reactivityKeys: policyWriteKeys, }); + if (Exit.isFailure(exit)) { + toast.error(messageFromExit(exit, "Failed to clear policy")); + return; + } + createdIdByPattern.current.delete(pattern); trackEvent("tool_policy_cleared", { pattern_kind: pattern.endsWith(".*") ? "group" : "exact", owner,