Conversation
There was a problem hiding this comment.
Pull request overview
Centralizes inline mutation-error reporting so failures remain visible without a snackbar provider, with broad regression coverage.
Changes:
- Adds shared action-error utilities and persistent form error mapping.
- Wires inline errors into task, snippet, inventory, and Dipper actions.
- Adds site-specific tests and a mutation-reporting guard.
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
framework/tests/mutationFailureReporting.guard.test.ts |
Guards mutation failure reporting. |
framework/src/index.ts |
Exports error utilities. |
framework/.../TaskHistoryTable.types.ts |
Adds action-error props. |
framework/.../TaskHistoryTable.tsx |
Renders stop failures. |
framework/.../TaskHistoryTable.test.tsx |
Tests stop failures. |
framework/.../SnippetExecutionAccordion.tsx |
Threads stop errors. |
framework/.../SnippetExecutionAccordion.test.tsx |
Tests error wiring. |
framework/.../submitErrorMapping.ts |
Maps all submit failures. |
framework/.../submitErrorMapping.test.ts |
Tests error mapping. |
framework/.../SchemaDrivenApp.tsx |
Uses persistent edit errors. |
framework/.../SchemaDrivenApp.test.tsx |
Tests edit failures. |
framework/.../AppTaskEditPage.tsx |
Uses persistent task errors. |
framework/.../AppTaskEditPage.test.tsx |
Tests task-edit failures. |
framework/.../AppListPage.tsx |
Reports delete failures inline. |
framework/.../AppListPage.test.tsx |
Tests list deletion errors. |
framework/.../AppDetailPage.tsx |
Reports task/entity action failures. |
framework/.../AppDetailPage.test.tsx |
Tests detail action failures. |
framework/.../AppCreatePage.tsx |
Uses persistent create errors. |
framework/.../AppCreatePage.test.tsx |
Tests create failures. |
framework/.../useActionError.ts |
Adds failure-state hook. |
framework/.../ActionErrorAlert/index.ts |
Exports primitive internals. |
framework/.../actionErrorMessage.ts |
Derives failure messages. |
framework/.../ActionErrorAlert.tsx |
Adds inline error alert. |
framework/.../ActionErrorAlert.test.tsx |
Tests shared primitive. |
apps/tasks/src/TaskDetailPage.tsx |
Displays stop failures. |
apps/tasks/src/TaskDetailPage.test.tsx |
Tests task stop errors. |
apps/snippets/src/SnippetsListPage.tsx |
Reports row and batch failures. |
apps/snippets/src/SnippetsListPage.test.tsx |
Tests snippet action errors. |
apps/inventory/src/SyncControl.tsx |
Reports sync errors inline. |
apps/inventory/src/SyncControl.test.tsx |
Tests sync failures. |
apps/inventory/src/InventoryNestedDelete.test.tsx |
Tests nested deletion errors. |
apps/inventory/src/InventoryAppNavigation.tsx |
Reports nested delete failures. |
apps/inventory/src/ConnectivityControl.tsx |
Reports probe failures inline. |
apps/inventory/src/ConnectivityControl.test.tsx |
Tests connectivity failures. |
apps/dipper/src/DipperApp.tsx |
Threads stop errors. |
apps/dipper/src/DipperApp.test.tsx |
Tests Dipper stop failures. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
No open review threads here, so this push is the port back from PMM, where this change landed as PMM-15359 (percona/pmm#5820) and picked up review that has no counterpart on this PR.
Same commit swaps the guard's
Worth a look: this caught two production sites here that PMM has no equivalent of. Also merged |
The React shell gated its own Settings and Admin Apps pages on `useAuth().isAdmin`, but no per-app control did, so a non-admin was offered create / execute / stop / retry / delete buttons that answer 403 now that the server enforces them. The gap was structural rather than an oversight per control: `useAuth` lived in `@sep/shell`, and the workspace dependency direction is shell -> apps -> framework -> api, so neither the app packages nor the framework could reach it. Exactly one app was role-aware, and only via a shell-side wrapper threading a boolean down as a prop. Move the context down the graph instead of threading the boolean across it. `@sep/api` has no `@sep` dependencies, already carries React as a peer and hosts the query hooks, and every consuming package already depends on it, so it hosts the context and `useAuth` while `@sep/shell` keeps owning the provider and all session/token state. A missing provider now resolves to a signed-out, non-admin session instead of throwing, so the many test and Storybook renders that mount these components bare keep working; a once-per-bundle dev warning keeps that from failing silently. Gated controls read a derived `canMutate`, never `isAdmin`. The server already resolves a minimum role per route rather than one administrator flag, so widening the UI to match is an edit to `deriveCanMutate` and to no call site. `isAdmin` survives only where the question really is "is this an administrator": the shell's two page guards and their `enabled: isAdmin` query suppression. Gating the framework's shared components covers every schema-driven app at once; the bespoke apps' local mutations are swept at their rendering call sites, so a hidden control issues no request and the hooks stay usable unchanged on the admin path. Selection affordances are hidden alongside the action they feed, so no bulk toolbar is left stranded. The create and edit pages keep their back chrome behind the guard state, since nothing links a read-only session there and anyone who arrives did so by URL. `SchemaListView` now drops an `actions` column with no delete handler rather than rendering a header over empty cells. Reads are untouched: the server gate keys on HTTP method, so every GET still succeeds for a non-admin, and no query suppression was added. This is what the UI advertises, never a security control -- the backend gate remains the only boundary. Also collapses the snippets prop-threading onto the hook and deletes the registry wrapper that fed it. The e2e harness mocked `isAdmin: false` while driving write controls, which the flag did not previously affect; those stubs now say admin, matching what the specs exercise. Addressed review feedback: dropped the dead `actions` column in the shared list view, kept back navigation on the page guards, moved the read-only wording into one component so it cannot drift, and added direct tests for the capability derivation and the missing-provider fallback.
…ery flag Address review feedback. The snippet accordion withheld its execute form from a read-only session by disabling the schema query, but a disabled query still serves a cached entry. The schema is held with `staleTime: Infinity` under a key carrying no identity, and the shell never clears the query cache, so an admin's fetch would render the form for a non-admin opening the same snippet later in the same tab. The render now gates on `canMutate` as well; the query flag stays as the request optimization. Covered by a test that populates the cache as an admin and re-renders read-only, which fails without the render gate. Also restores two e2e session fixtures to non-admin. The snippet-download spec exercises a GET that stays permitted, and the sidebar-navigation specs only navigate and assert readable sentinels, so leaving both non-admin keeps end-to-end coverage of the read-only experience this change creates rather than broadening privileges the specs never use.
Co-authored-by: yyyyyyy <contact@yyyyyyyan.tech>
- ATW's collect pane left a read-only session at a dead end: the picker,
the spinner and the conditional-fields warning all rendered, then the
form silently did not. It now says why, where the form would be, and
skips the merged-schema fetch whose only consumer was that form —
which also stops the warning firing for a session that cannot execute.
- Drop "Create one to get started" from ATW's incident empty state for a
session offered no create control.
- Hoist the duplicated admin session fixture to ADMIN_SESSION in
auth-context, beside the UNAUTHENTICATED_SESSION it mirrors, replacing
four copies. @sep/test-utils looked like the better home but drags
@sep/api into every package's vitest setup file, where the eagerly
loaded real module defeats vi.mock('@sep/api') elsewhere.
- SchemaListView: state why the actions column is dropped in terms of the
missing handler, not one caller's motive. TasksListPage and
TargetHostsPage never wire row deletion either, so the old wording was
false for an admin there.
- Render Dipper's read-only notice bare, as every other site does; the
Alert wrapper fought the notice's own secondary text colour.
- useRefreshSnippets' doc pointed the next caller at isAdmin; its only
caller reads canMutate.
Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
Applying the four inline suggestions from code review left three files
broken, because each suggestion replaced the comment line above the code
it quoted rather than the code itself:
- ResultsPane.tsx had `{canMutate && (` twice, which is a parse error.
Both this file and IncidentWorkspacePage.tsx failed to transform, so
34 ATW tests never ran.
- SchemaDrivenApp.tsx lost the comma that joined its two comment lines,
leaving "...(see AppCreatePage). // and the back chrome stays...".
AppTaskEditPage.tsx:174 has the intended wording.
- _template.spec.ts carried its first comment line twice.
Also rewraps the auth-context paragraph the ticket-key removal left with
an orphaned line.
Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
- Drop the ticket key from the 19 remaining spec fixture comments. The reviewer's suggestion covered only _template.spec.ts, and their point holds for the rest: the sentence carries itself, and repeating the key on identical boilerplate adds nothing. - Collapse the edit-guard comment to its cross-reference in both SchemaDrivenApp and AppTaskEditPage. AppCreatePage already spells out why the page is the control and why the back chrome stays, so the second line was restating it. My earlier repair of the mangled suggestion read the break as a lost comma and put the restatement back; the reviewer wanted it gone. Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
Every state-changing SEP API route now refuses a non-admin with a 403 and a JSON reason, which turned a rare failure into a routine one. An audit of the 47 mutation call sites found 9 that surfaced nothing at all, 2 that kept the server's message only for HTTP 400, and 6 that reported through a notistack toast the host application has to provide. In the PMM-embedded build that provider is not SEP's to guarantee, so a toast-only report can be no report. Add a shared primitive in @sep/framework (actionErrorMessage, useActionError, ActionErrorAlert) that derives the server's own reason and renders it inline with no notistack dependency, then wire every repaired site onto it. Where a site previously raised an error toast, the toast is removed rather than kept alongside the alert, so one failure produces one signal. Success toasts are unchanged. Details: * mapSubmitError no longer returns an empty state for non-422 failures, so the create and edit forms show a persistent banner carrying ApiError.message. The 422 per-field path is unchanged, and a 422 whose detail is a string keeps that string instead of falling back to a generic sentence. * TaskHistoryTable gained actionError and onDismissActionError props and the connected variant reports its own stop mutation, which covers the stop-task sites on AppDetailPage, SnippetExecutionAccordion, DipperApp and the tasks detail page. The stop confirmation closes on confirm, so the alert renders above the rows the user is returned to. * AppDetailPage's execute confirmation now closes in a finally like the adjacent delete, and its failure reports on the page behind it. Reopening the same execute action keeps the composed chain so a refused execute can be retried without rebuilding it. * SyncControl and the batch-approve path in SnippetsListPage no longer narrow the server's message to HTTP 400. * A guard test walks every package for mutation call sites and fails on one with no in-tree failure path, so the tenth omission is caught mechanically rather than in review. Sites that already reported in-tree before the primitive existed are allowlisted with the mechanism each one uses. Tests cover both halves per repaired site: the failure renders the server's message, and the success path is unchanged. Addressed review feedback: gate the per-field branch on the 422 status rather than the payload shape so a batch endpoint's array detail keeps its own message, read the batch-approve reason off the wrapper's raw error, clear the shared snippets row alert before each attempt so a refusal cannot outlive it, and match the guard's marker on the JSX prop form so a same-named local cannot satisfy it.
* Parse a blob response's JSON error body so a refused snippet download reports the server's reason instead of the bare status. A request made with responseType 'blob' gets its error body in that type too, leaving messageFromPayload nothing to read; normalizeBlobError in @sep/api recovers the reason and keeps a 422's detail array reachable through parseFieldErrors. * actionErrorMessage falls back for a 422 that parses into no field entries and carries no string detail, since the synthesized HTTP 422 is not a server reason. mapSubmitError now delegates that branch instead of repeating it. * ConnectivityControl passes its fallback to the alert, which derives the message, rather than to the hook, whose derived message it does not render. * Correct the actionError prop doc: any caller owning the stop mutation must pass its error, in either mode; only the connected variant without an onStopTask reports for itself. * Add the changelog fragment for the user-visible reporting change.
- Pass the failure state to a custom create-form slot. The slot bypasses SchemaFormRenderer, and this ticket removed the error toast beside it, so a caller supplying `renderCreateForm` was left with no failure signal at all. AppTaskEditPage and SchemaDrivenApp's edit page already threaded it. Documented the slot's obligation to render it, and corrected the type's now-stale "success / error snackbars" wording. - Replace the guard's file-count sanity check with one sentinel per scanned area. A count drifts with the repo and can be satisfied by the wrong tree. Both found on the PMM port of this change (percona/pmm#5820). Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
The mutation guard does not enforce this on its own: it is file-level,
so a file containing any accepted marker passes even if a
`<TaskHistoryTable onStopTask=...>` inside it drops `actionError`.
`TaskHistoryTableProps` now carries a discriminated stop contract:
supplying `onStopTask` requires `actionError`, and omitting it forbids
both, since the connected variant reports from its own mutation and
would ignore them.
The internal split omits from the base interface rather than the props
union, because `Omit` is not distributive and would collapse the two
branches.
TaskDetailPage's two tables now say `actionError={null}` explicitly:
they share one stop mutation that the page already reports once above
them, so forwarding the error would render the same refusal three times.
Six test call sites say it too, and a `@ts-expect-error` case pins the
contract so it cannot silently relax.
Ported from the PMM counterpart (percona/pmm#5820), where CodeRabbit
pushed back on the claim that the guard already covered this.
Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
Conflicts resolved in two inventory frontend files: - InventoryAppNavigation.tsx: main removed the nested-list delete feature that this branch had gated behind canMutate. Took main's version; the gating is moot because the deleted-row control no longer exists here. Row deletion now lives in the framework's AppListPage. - InventorySchedulePage.tsx: main renamed label to identityLabel and added multi-task support (Task column, ROW_COLUMNS 8). Kept all of that and re-applied this branch's canMutate gating on top: the Enabled switch falls back to read-only text, and the Actions column, its header, and the Attach schedule button stay hidden for non-mutating sessions.
Brings main into the stack via the updated SEP-1844. Conflict resolved in InventoryAppNavigation.tsx: main removed the nested-list delete feature, and this branch's only change to the file was routing that delete's failure through ActionErrorAlert instead of a toast. Took SEP-1844's version, so the reporting change goes away with the code it reported on. Deletion now lives in the framework's AppListPage, which already carries this branch's useActionError reporting, so the contract still holds. Removed InventoryNestedDelete.test.tsx for the same reason: both of its cases covered the deleted nested-list flow, and AppListPage.test.tsx already asserts the same refused-delete and successful-delete behaviour on the surviving path. The mutation failure reporting guard still passes.
SEP-1844 landed on main as a squash (#1387), so main and this branch now carry the same read-only gating through different commits. Every conflict below is that overlap, and in each case this branch already contained main's gating with SEP-1845's failure reporting layered on top. Kept this branch's version, which is a superset of main's: framework/src/index.ts, TaskHistoryTable.tsx, ConnectivityControl.tsx, SyncControl.tsx, SnippetsListPage.tsx, AppCreatePage.test.tsx, AppTaskEditPage.test.tsx, SchemaDrivenApp.test.tsx ConnectivityControl.tsx and SyncControl.tsx dropped the ApiError import that main still lists. That is intentional: SEP-1845 replaced the ApiError-based message with reportError, so re-adding it would leave an unused import. Merged both sides in the remaining tests, since main added coverage next to the conflicting hunks that a wholesale take would have dropped: TaskHistoryTable.test.tsx keeps main's unlaunchable status case and the required actionError prop; AppListPage.test.tsx keeps main's non-admin case and this branch's delete failure reporting block; DipperApp.test.tsx keeps main's authMock and this branch's stopState. Verified: typecheck clean, 1848 frontend tests pass, oxlint 0 errors, oxfmt clean, and the mutation failure reporting guard still passes.
Stacked on #1387 (SEP-1844), which this branch is based on. Retarget to
mainonce 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:
The toast family depends on a
SnackbarProviderthe 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.useActionErroris for actions fired from a dialog that closes before the request settles; where the mutation object is in scope at the render site, itserrorgoes straight to the alert.The message is the server's own reason. A string
detailarrives asApiError.message. A 422 carriesdetailas a per-field array that the lift skips, so the field entries are read throughparseFieldErrorsand joined; a form usesmapSubmitErrorinstead, 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.
mapSubmitErrorno 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 whosedetailis a string keeps that string.TaskHistoryTablegainedactionError/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 afinallylike 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 inSnippetsListPage) report on any status. Batch approve reads the reason off the hook'srawwrapper, 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
mapSubmitErrorchange.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.ActionErrorAlert.test.tsxcovers the 403 reason, the 422 per-field join, an arraydetailon a non-422 status keeping its own message, transport failures, and the fallback..mutate(call with no failure path fails the test.