SEP-1845: Report failed UI actions in SEP's own component tree so a refused action is never silent (#1388)

Stacked on #1387 (SEP-1844), which this branch is based on. Retarget to
`main` once that merges.

## Why

SEP-1838 made every state-changing API route refuse a non-admin with a
403 and a JSON reason, turning a rare failure into a routine one. An
audit of the 47 mutation call sites in the frontend found:

* 9 that surface nothing at all on failure
* 2 that keep the server's message only for HTTP 400 and substitute a
generic string otherwise
* 6 that report only through a notistack toast

The toast family depends on a `SnackbarProvider` the host application
mounts. In standalone SEP that is `@sep/shell`; in the PMM-embedded
deployment SEP ships no frontend of its own, so that provider is not
SEP's to guarantee. A toast-only report can therefore be no report at
all, which is the reported symptom: a refused create leaves the form
open with nothing shown.

## What changed

**A shared primitive in `@sep/framework`** (`ActionErrorAlert` /
`useActionError` / `actionErrorMessage`) that derives the server's own
reason and renders it inline, with no notistack dependency.
`useActionError` is for actions fired from a dialog that closes before
the request settles; where the mutation object is in scope at the render
site, its `error` goes straight to the alert.

**The message is the server's own reason.** A string `detail` arrives as
`ApiError.message`. A 422 carries `detail` as a per-field array that the
lift skips, so the field entries are read through `parseFieldErrors` and
joined; a form uses `mapSubmitError` instead, which places them on their
fields. The fallback string is reached only when neither path yields
text.

**One signal per failure.** At every repaired site the error toast is
removed rather than kept alongside the new alert. Success toasts are
unaffected.

**`mapSubmitError` no longer returns an empty state for non-422
errors**, so AppCreatePage, AppTaskEditPage and the SchemaDrivenApp edit
form all show a persistent banner carrying the server's reason. The 422
per-field behaviour is unchanged, and a 422 whose `detail` is a string
keeps that string.

**`TaskHistoryTable` gained `actionError` / `onDismissActionError`.**
The connected variant reports its own stop mutation; a caller owning the
mutation threads its error in. The stop confirmation closes on confirm,
so the alert renders above the rows the user is left looking at. This
covers the stop-task sites on AppDetailPage, SnippetExecutionAccordion,
DipperApp and the tasks detail page.

**`AppDetailPage`'s execute confirmation** now closes in a `finally`
like the adjacent delete, and reports on the page behind it. Reopening
the same execute action keeps the composed chain, so a refused chained
execute can be retried without rebuilding it; opening a different action
still starts empty.

**The two status-narrowed sites** (`SyncControl`, batch approve in
`SnippetsListPage`) report on any status. Batch approve reads the reason
off the hook's `raw` wrapper, since the wrapper's own message says
nothing.

**A guard test** walks every package for `.mutate(` / `.mutateAsync(`
call sites and fails on a file with no in-tree failure path. Sites that
already reported in-tree before the primitive existed are allowlisted
with the mechanism each uses. Granularity is the file, not the
individual call: a file that already reports one action passes even if a
second unwired mutation is added to it. Tightening that would trade a
mechanical check for a heuristic one, and the omissions this guard
exists to catch were whole files with no failure path at all.

## Sites repaired

Previously silent: AppDetailPage (stop task, delete entity from detail
page), TaskHistoryTable (stop), SnippetExecutionAccordion (stop),
DipperApp (stop), tasks TaskDetailPage (stop), SnippetsListPage
(download, remove approval, approve).

Previously toast-only: AppListPage (delete), AppDetailPage (execute,
delete task), ConnectivityControl, InventoryAppNavigation (nested
delete), SyncControl. The three create/edit forms are resolved by the
`mapSubmitError` change.

Status-narrowed: SyncControl, batch approve.

## UX trade-off worth checking in design QA

Removing the error toasts is deliberate: an inline alert is less
attention-grabbing than a toast, so each alert is placed where the user
is actually looking after the dialog closes (above the rows for stop, on
the page behind the confirm for execute/delete, above the table for the
snippet row actions). The snippets page shares one alert across every
row action, so each attempt clears it first.

ConnectivityControl now renders a failed probe inline as well; its
passing case stays a success toast.

## Out of scope

Backend authorization (SEP-1838), hiding write controls from non-admins
(SEP-1844), migrating the ~30 sites that already render an in-tree
signal, and a central runtime mutation-error handler (rejected: it runs
outside the failing component's tree, so a host-provided toast is the
only thing it could render).

## Test plan

* `pnpm -r test` (14 packages, all green), `pnpm typecheck`, `pnpm exec
oxlint` (0 errors), `pnpm format:check`.
* New tests per repaired site cover both halves: the failure renders the
server's message, and the success path is unchanged.
* `ActionErrorAlert.test.tsx` covers the 403 reason, the 422 per-field
join, an array `detail` on a non-422 status keeping its own message,
transport failures, and the fallback.
* The guard was verified against a planted probe file: a new `.mutate(`
call with no failure path fails the test.

---------

Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
Co-authored-by: yyyyyyy <yan.orestes@percona.com>
Co-authored-by: yyyyyyy <contact@yyyyyyyan.tech>

Author: nachodd

changelog.d/SEP-1845.fixed.md

@@ -0,0 +1 @@
+Failed UI actions now report the reason inline, from SEP's own component tree, instead of relying on a toast the host application has to provide: a refused action (HTTP 403 for a non-admin), a 409, a 5xx or a network failure all render the server's own message where the action was fired. Create and edit forms show a persistent banner, confirmation dialogs close on confirm and surface the failure on the page behind them, and stop-task, delete, execute, sync, connectivity-check and snippet approve/download failures are no longer silent.

frontend/packages/api/src/errors.ts

@@ -116,6 +116,52 @@ export function parseFieldErrors(error: unknown): FieldValidationError[] {
   return result;
 }
 
+/**
+ * Recover the server's reason from a request made with ``responseType: 'blob'``.
+ *
+ * Axios hands back the error body in the response type it was asked for, so a
+ * download's 403 arrives as a ``Blob`` rather than parsed JSON and
+ * ``messageFromPayload`` cannot see its ``detail`` — leaving the synthesized
+ * ``HTTP 403``. Read the blob, parse it, and return an ``ApiError`` carrying
+ * the reason (and the parsed payload, so a 422's ``detail`` array is still
+ * reachable through {@link parseFieldErrors}).
+ *
+ * Async by necessity — ``Blob.text()`` is a promise — so callers await it in
+ * their own catch rather than getting it from an interceptor.
+ *
+ * Falls back to the unmodified error whenever the body is not a readable JSON
+ * blob: an HTML error page, an opaque binary body, or a network failure with no
+ * response at all.
+ */
+export async function normalizeBlobError(error: unknown): Promise<ApiError> {
+  const apiError = normalizeAxiosError(error);
+  const body = apiError.data;
+  if (typeof Blob === 'undefined' || !(body instanceof Blob)) {
+    return apiError;
+  }
+
+  let payload: unknown;
+  try {
+    payload = JSON.parse(await body.text());
+  } catch {
+    return apiError;
+  }
+
+  const message = messageFromPayload(payload, apiError.message);
+  return new ApiError(
+    {
+      kind: apiError.kind,
+      status: apiError.status,
+      code: apiError.code,
+      message,
+      url: apiError.url,
+      method: apiError.method,
+      data: payload,
+    },
+    apiError.original,
+  );
+}
+
 export function normalizeAxiosError(error: unknown): ApiError {
   if (error instanceof ApiError) {
     return error;

frontend/packages/api/src/index.ts

@@ -34,7 +34,7 @@ export type { MintedToken } from './client';
 export { createQueryClient, defaultQueryClientConfig } from './queryClient';
 
 // Errors
-export { ApiError, normalizeAxiosError, parseFieldErrors } from './errors';
+export { ApiError, normalizeAxiosError, normalizeBlobError, parseFieldErrors } from './errors';
 export type { ApiErrorDetails, ApiErrorKind, FieldValidationError } from './errors';
 
 // Auth context (provider lives in @sep/shell; the context lives here so the

frontend/packages/api/tests/errors.test.ts

@@ -16,7 +16,7 @@
  */
 
 import { describe, expect, it } from 'vitest';
-import { ApiError, parseFieldErrors } from '../src/errors';
+import { ApiError, normalizeBlobError, parseFieldErrors } from '../src/errors';
 
 function http422(detail: unknown): ApiError {
   return new ApiError({ kind: 'http', status: 422, message: 'HTTP 422', data: { detail } });
@@ -75,3 +75,54 @@ describe('parseFieldErrors', () => {
     expect(parseFieldErrors('boom')).toEqual([]);
   });
 });
+
+describe('normalizeBlobError', () => {
+  function blobFailure(status: number, body: string, type = 'application/json'): ApiError {
+    return new ApiError({
+      kind: 'http',
+      status,
+      message: `HTTP ${status}`,
+      data: new Blob([body], { type }),
+      url: '/apps/snippets/snippet/download',
+      method: 'GET',
+    });
+  }
+
+  it("recovers a refusal's reason from a blob response body", async () => {
+    const recovered = await normalizeBlobError(
+      blobFailure(
+        403,
+        JSON.stringify({ detail: "You don't have permission to perform this action" }),
+      ),
+    );
+
+    expect(recovered.message).toBe("You don't have permission to perform this action");
+    expect(recovered.status).toBe(403);
+    expect(recovered.method).toBe('GET');
+  });
+
+  it("keeps a 422's detail array reachable through parseFieldErrors", async () => {
+    const recovered = await normalizeBlobError(
+      blobFailure(
+        422,
+        JSON.stringify({ detail: [{ loc: ['body', 'name'], msg: 'field required' }] }),
+      ),
+    );
+
+    expect(parseFieldErrors(recovered)).toEqual([{ path: 'name', message: 'field required' }]);
+  });
+
+  it('leaves the error untouched when the body is not JSON', async () => {
+    const original = blobFailure(502, '<html>Bad gateway</html>', 'text/html');
+
+    const recovered = await normalizeBlobError(original);
+
+    expect(recovered.message).toBe('HTTP 502');
+  });
+
+  it('passes through an error with no blob body', async () => {
+    const original = new ApiError({ kind: 'network', message: 'Network error' });
+
+    expect(await normalizeBlobError(original)).toBe(original);
+  });
+});

frontend/packages/apps/dipper/src/DipperApp.test.tsx

@@ -25,8 +25,9 @@ import {
   useDipperAppSchema,
 } from './hooks';
 
-const { stopMutate, authMock } = vi.hoisted(() => ({
+const { stopMutate, stopState, authMock } = vi.hoisted(() => ({
   stopMutate: vi.fn(),
+  stopState: { error: null as unknown },
   /** Flipped per test to cover the read-only (non-admin) rendering. */
   authMock: { canMutate: true },
 }));
@@ -82,10 +83,12 @@ vi.mock('@sep/framework', async () => {
       data,
       onViewLogs,
       onStopTask,
+      actionError,
     }: {
       data: Array<{ id: number; status: string; task?: { name: string } }>;
       onViewLogs: (entry: unknown) => void;
       onStopTask?: (entry: { id: number }) => void;
+      actionError?: unknown;
     }) => (
       <div>
         <span>history rows: {data.length}</span>
@@ -97,12 +100,24 @@ vi.mock('@sep/framework', async () => {
             Stop {String(data[0].id)}
           </button>
         ) : null}
+        {actionError ? (
+          <div data-testid="task-history-action-error">
+            {actionError instanceof Error ? actionError.message : String(actionError)}
+          </div>
+        ) : null}
       </div>
     ),
     TaskLogViewer: ({ taskHistoryId }: { taskHistoryId: number }) => (
       <div>logs for {taskHistoryId}</div>
     ),
-    useStopTaskHistory: () => ({ mutate: stopMutate, isPending: false }),
+    useStopTaskHistory: () => ({
+      mutate: stopMutate,
+      isPending: false,
+      error: stopState.error,
+      reset: () => {
+        stopState.error = null;
+      },
+    }),
     ReadOnlyNotice: ({ action, testId }: { action?: string; testId?: string }) => (
       <div data-testid={testId}>no permission to {action}</div>
     ),
@@ -119,6 +134,7 @@ describe('DipperApp', () => {
 
   beforeEach(() => {
     vi.clearAllMocks();
+    stopState.error = null;
     authMock.canMutate = true;
     mockAppSchema.mockReturnValue({
       data: {
@@ -206,6 +222,67 @@ describe('DipperApp', () => {
       42,
       expect.objectContaining({ onSuccess: expect.any(Function) }),
     );
+    expect(screen.queryByTestId('task-history-action-error')).not.toBeInTheDocument();
+  });
+
+  it("reports a failed stop above the history with the server's own reason", () => {
+    stopState.error = new Error("You don't have permission to perform this action");
+
+    render(<DipperApp />);
+
+    expect(screen.getByTestId('task-history-action-error')).toHaveTextContent(
+      "You don't have permission to perform this action",
+    );
+  });
+});
+
+describe('DipperApp — write access', () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+    stopState.error = null;
+    authMock.canMutate = true;
+    mockAppSchema.mockReturnValue({
+      data: { display_name: 'Dipper', description: 'Collect' },
+      isLoading: false,
+      error: null,
+    } as unknown as ReturnType<typeof useDipperAppSchema>);
+    mockFormSchema.mockReturnValue({
+      data: { forms: [] },
+      isLoading: false,
+      error: null,
+    } as unknown as ReturnType<typeof useDipperFormSchema>);
+    mockHistory.mockReturnValue({
+      data: { items: [] },
+      isLoading: false,
+      error: null,
+      refetch: vi.fn(),
+    } as unknown as ReturnType<typeof useDipperHistory>);
+    mockExecution.mockReturnValue({
+      mutate: vi.fn(),
+      isPending: false,
+      isError: false,
+      error: null,
+    } as unknown as ReturnType<typeof useDipperExecution>);
+  });
+
+  it('renders the execute form for a session that may mutate', () => {
+    render(<DipperApp />);
+
+    fireEvent.click(screen.getByRole('button', { name: 'Select service' }));
+
+    expect(screen.getByRole('button', { name: 'Execute' })).toBeInTheDocument();
+    expect(screen.queryByTestId('dipper-execute-read-only')).not.toBeInTheDocument();
+  });
+
+  it('renders no execute form for a non-admin, keeping the history readable', () => {
+    authMock.canMutate = false;
+    render(<DipperApp />);
+
+    fireEvent.click(screen.getByRole('button', { name: 'Select service' }));
+
+    expect(screen.getByTestId('dipper-execute-read-only')).toBeInTheDocument();
+    expect(screen.queryByRole('button', { name: 'Execute' })).not.toBeInTheDocument();
+    expect(screen.getByText('Execution history')).toBeInTheDocument();
   });
 });
 

frontend/packages/apps/dipper/src/DipperApp.tsx

@@ -235,6 +235,8 @@ export function DipperApp() {
             }
           }}
           isStopping={stop.isPending}
+          actionError={stop.error}
+          onDismissActionError={stop.reset}
         />
       )}
 

frontend/packages/apps/inventory/src/ConnectivityControl.test.tsx

@@ -144,12 +144,34 @@ describe('ConnectivityControl', () => {
     await screen.findByText(/missing node or port information/i);
   });
 
-  it('shows a generic snackbar when the request never reaches the server', async () => {
+  it('reports in-tree when the request never reaches the server', async () => {
     const user = userEvent.setup();
     vi.spyOn(apiClient, 'post').mockRejectedValue(new Error('network down'));
     renderControl('mysql');
     await user.click(await screen.findByRole('button', { name: CHECK_BUTTON }));
-    await screen.findByText(/could not be started/i);
+    expect(await screen.findByTestId('connectivity-action-error')).toHaveTextContent(
+      /network down/i,
+    );
+  });
+
+  it("reports a refusal in-tree with the server's own reason", async () => {
+    const user = userEvent.setup();
+    await stubPostError(403, "You don't have permission to perform this action");
+    renderControl('mysql');
+    await user.click(await screen.findByRole('button', { name: CHECK_BUTTON }));
+    expect(await screen.findByTestId('connectivity-action-error')).toHaveTextContent(
+      "You don't have permission to perform this action",
+    );
+  });
+
+  it('reports nothing when the probe connects', async () => {
+    const user = userEvent.setup();
+    stubPostResult({ success: true });
+    renderControl('mysql');
+    await user.click(await screen.findByRole('button', { name: CHECK_BUTTON }));
+    await screen.findByText(/connectivity check passed/i);
+    expect(screen.queryByTestId('connectivity-action-error')).not.toBeInTheDocument();
+    expect(screen.queryByTestId('connectivity-probe-failure')).not.toBeInTheDocument();
   });
 
   it('re-enables the button after a failed check so it can be retried', async () => {

frontend/packages/apps/inventory/src/ConnectivityControl.tsx

@@ -15,9 +15,11 @@
  * along with this program. If not, see <https://www.gnu.org/licenses/>.
  */
 
-import { Box, Button, CircularProgress, Tooltip } from '@mui/material';
+import { useState } from 'react';
+import { Alert, Box, Button, CircularProgress, Tooltip } from '@mui/material';
 import NetworkCheckIcon from '@mui/icons-material/NetworkCheck';
-import { ApiError, useAuth } from '@sep/api';
+import { useAuth } from '@sep/api';
+import { ActionErrorAlert, useActionError } from '@sep/framework';
 import { useSnackbar } from 'notistack';
 import { useCheckServiceConnectivity } from './hooks';
 
@@ -45,29 +47,29 @@ export function ConnectivityControl({
   const { canMutate } = useAuth();
   const checkConnectivity = useCheckServiceConnectivity(serviceId);
   const { enqueueSnackbar } = useSnackbar();
+  const checkError = useActionError();
+  const [probeFailure, setProbeFailure] = useState<string | null>(null);
 
   const isConnectable =
     typeof serviceType === 'string' && CONNECTABLE_SERVICE_TYPES.has(serviceType);
   const isPending = checkConnectivity.isPending;
 
   function handleCheck() {
+    setProbeFailure(null);
+    checkError.clearError();
     checkConnectivity.mutate(undefined, {
       onSuccess: (result) => {
         if (result.success) {
           enqueueSnackbar('Connectivity check passed', { variant: 'success' });
         } else {
-          enqueueSnackbar(`Connectivity check failed: ${result.error ?? 'Unknown error'}`, {
-            variant: 'error',
-          });
+          // A reachability failure is a successful request reporting a bad
+          // result, so it renders here rather than through the error alert.
+          setProbeFailure(result.error ?? 'Unknown error');
         }
       },
-      onError: (err) => {
-        const message =
-          err instanceof ApiError
-            ? err.message
-            : 'Connectivity check could not be started. Please try again.';
-        enqueueSnackbar(message, { variant: 'error' });
-      },
+      // The request itself failing (refused, 5xx, network) reports in-tree and
+      // nowhere else: one signal, and one that does not need a host snackbar.
+      onError: (err) => checkError.reportError(err),
     });
   }
 
@@ -96,6 +98,25 @@ export function ConnectivityControl({
           </Button>
         </Box>
       </Tooltip>
+
+      <ActionErrorAlert
+        error={checkError.error}
+        onClose={checkError.clearError}
+        fallback="Connectivity check could not be started. Please try again."
+        sx={{ mt: 2 }}
+        testId="connectivity-action-error"
+      />
+
+      {probeFailure !== null && (
+        <Alert
+          severity="error"
+          onClose={() => setProbeFailure(null)}
+          sx={{ mt: 2 }}
+          data-testid="connectivity-probe-failure"
+        >
+          Connectivity check failed: {probeFailure}
+        </Alert>
+      )}
     </Box>
   );
 }