SEP-1760: Backport review fixes found during the PMM UI migration

[nachodd]

Summary

The CodeRabbit review of the SEP-UI-into-PMM migration PR (percona/pmm#5653, PMM-15216) raised these findings against the ported copy of @sep/api / @sep/framework. Every one of them is present in the original, so they are fixed here too — otherwise the next sync in either direction re-introduces them. PMM-only wiring (route constants, the route-level admin gate, the dev-proxy env variables) is out of scope.

Token-minter seam (upstreamed, not a review fix)

The last commit is a different kind of change from the rest of this PR, so it is called out separately: it upstreams a capability PMM needed (percona/pmm#5739, PMM-15293) rather than fixing a review finding. It lands here for the same reason as everything else — otherwise the next sync clobbers it.

refreshAccessToken() hardcoded POST /oauth/refresh as the only way to obtain a token. That is correct for this SPA. It is not workable for PMM, which embeds SEP with no refresh cookie at all: it trades its own session cookie for a bearer through POST /oauth/session/exchange (SEP-1692), so the default 401s on every recovery attempt and the entire retry path is dead weight there.

• setTokenMinter() replaces just that one call, defaulting to the existing behaviour — nothing changes for a standalone SEP deployment. Everything downstream was already minter-agnostic: the single-flight coalescer, the axios 401 retry, and the setOnRefreshed notification do not care which endpoint produced the token.
• isTokenMintRequest composes the existing isRefreshRequest and isSessionRequest guards — which the axios retry condition already listed separately — and is exported so the typed client shares one definition rather than growing a second copy. Keeping them together is load-bearing: minting is single-flighted, so letting a mint's own 401 into the retry path hands the interceptor the very promise it is running inside, an await on itself that never settles. There is a regression test that times out rather than fails if that guard is lost.
• The openapi-fetch transport gained the 401 retry the axios one already had. This one is a fix for both deployments: it previously only reported unauthorized, so every typed hook — useCurrentUser and all the generated-path ones — surfaced an expired token as a failure instead of recovering. fetch consumes a Request's body, so the middleware stashes a clone taken before dispatch and replays that; only replay-eligible requests are cloned, and the replay goes through raw fetch so it cannot re-enter the middleware and loop.

@sep/api grows 10 tests for this: minting through a registered minter, coalescing a burst of 401s into one exchange, the self-await regression guard, a null-resolving minter, restoring the default, and on the typed side mint-and-replay, replaying a request body, replaying at most once, one mint across concurrent 401s, and no recovery attempt on a minting endpoint's own 401.

Correctness

• refreshAccessToken called the injected _onRefreshed handler inside the async executor, so a synchronous throw from it rejected the shared refreshInFlight promise — every awaiting caller saw a failed refresh, and a force-logout, for a cookie rotation that had already succeeded on the backend. The function's own comment says this must not happen.
• SchemaFormRenderer returned from handleFormSubmit on a section violation, which react-hook-form reads as a successful submit (isSubmitSuccessful = true). useUnsavedChangesGuard is isDirty && !isSubmitSuccessful and only re-arms when submitError is truthy — never on this path — so the guard stayed disarmed: no beforeunload prompt, no UnsavedChangesBlocker, and the user could navigate away from a dirty form and lose it. The gate now runs in the submit event handler, ahead of handleSubmit.
• normalizeChoiceDefaults read and wrote flat keys, but flattenSectionFields also returns one_of branch fields, whose names are dotted paths stored nested. A case-mismatched nested choice value was never canonicalised and rendered as an empty selection — the exact failure that function exists to prevent.
• AppListPage / AppDetailPage dereferenced the optional list_view (schema.list_view!.columns), reachable through an unresolved entity route. The list page now renders Not found; the Overview tab falls back to an empty column set and still lists the task's own fields.
• HostSelector looked up errors[name], which never resolves for a dotted branch-field name, so an affected field showed no validation error.
• extractId used Number, which turns a whitespace-only string into 0 and accepts '1.5' / '0x10'. Each result reads as a resolvable inventory id downstream: useResolvedServiceField enables a lookup for service 0, and SchemaSelector fires useSchemas({ serviceId: 0 }) for a service that cannot exist.
• validationMapper submitted parseInt('2.5', 10) as 2 and parseFloat('3.14invalid') as 3.14. Numeric fields now validate with Number.isFinite (plus Number.isInteger for integer) and coerce with Number.
• useTaskLogs guarded on !step, dropping a log line with step: ''. useExecutionEvents treats '' as the stepless bucket and the viewer labels it "General", so the two streams disagreed and stepless output was silently lost.

Stability

• useExecutionEvents returned undefined from onerror for every non-sentinel error, so fetchEventSource retried forever while sseError stayed unset and sseLoading stayed true — a persistently failing endpoint (500, DNS failure) left the panel spinning and reconnecting indefinitely. Consecutive failures are now counted, reset on a successful open, and the loop stops with the error surfaced.
• StandaloneHostSelector disabled its Autocomplete when the hosts query failed, but onOpen holds the only refetch() trigger and a disabled Autocomplete never opens — one failure wedged the control until the page remounted.
• A scheduled-task enable/disable toggle sent kwargs: '{}' in a full PUT, wiping the arguments of any task created with non-default kwargs. kwargs is preserved when the response carries it; '{}' stays the fallback until PeriodicTaskResponse declares the field.

Contract and consistency

• useCascadingField cleared with undefined while buildFormDefaults seeds these selector types to '', and counted '' as ready — a downstream selector fetched options for an empty parent, and a bound MUI input flipped from controlled to uncontrolled.
• SchemaDrivenApp's renderEditForm invocation omitted capabilities, submitError and fieldErrors, so a consumer supplying the slot could render neither the 422 banner nor the inline field errors. AppTaskEditPage already passes all three to the same slot type.
• SchemaListView pinned bgcolor: 'common.white', which renders a white table in dark mode; it now reads the mode's own opaque surface.
• FileField's file-picker IconButton had no accessible name.
• useResolvedServiceField discarded useServices' error, so callers could not tell a failed lookup from an id that matched no service.
• packages/framework/test/setup.ts was an unreferenced sibling of tests/setup.ts registering the jest-dom matchers but no afterEach(cleanup). Deleted, so a future setupFiles edit cannot silently drop DOM isolation for the package.

Deliberately not ported, matching the decisions recorded on the PMM PR: production SEP routing / token exchange, flattening dotted app argument names (they denote nested backend models, which coerceFormValues already produces), converting ScriptPreviewField to TanStack Query, committing untrimmed free-solo input, relocating the api / atw test suites, related_apps alongside entities, and the hard-coded Roboto Mono stacks. formatCellValue's undefined guard is already here — in a better form than the port has, which should go back to PMM.

Tested

Automated, no manual run: this is a set of narrow fixes in already-covered code, and each behaviour change is pinned by a test.

• pnpm test — 1187 tests pass (746 in @sep/framework, 103 in @sep/api), including three new/updated cases plus the 10 minter-seam cases above:
  • extractId.test.ts (new): whitespace-only, fractional, hex and partly-numeric strings all resolve to null.
  • SchemaFormRenderer.test.tsx: a violation-blocked submit must not drop the beforeunload listener. Verified this test fails against the pre-fix ordering, so it pins the actual regression.
  • AppTaskEditPage.test.tsx: a one_of branch choice stored at source.transport is canonicalised, and the input object is not mutated.
• Two existing cases encoded the old behaviour and were updated, not removed: StandaloneHostSelector (disabled-on-error → enabled and retryable on open) and the SchemaFormRenderer field-rendering case (the file-picker button's new accessible name).
• pnpm type-check clean across every package; pnpm lint 0 errors; oxfmt clean; pre-commit hooks pass.
• The minter seam is covered by unit tests only. Its embedded consumer cannot be exercised end to end yet: that path needs a Grafana-backed SEP with the service account from PMM-15280 provisioned, which is still in progress. The default minter — the one this deployment uses — is unchanged and covered by the pre-existing suite.
• Not exercised in a browser: the dark-mode table surface is worth an eyeball in both modes, and the bounded stream retry is easiest to confirm against a deliberately failing /execution-events endpoint.

Checklist

• New/modified functions have type hints and rST docstrings
• New tests added for new features or bug fixes
• All tests pass locally (make test)
• Pre-commit hooks pass (make run-pre-commit)
• Database migrations generated if models changed (make makemigrations) — N/A, no model changes
• User-facing changes documented (README, inline help, UI text) — N/A beyond the changelog fragment; no UI copy changes except the new aria-label and the numeric validation messages
• Configuration changes documented with examples — N/A
• Changelog fragment added under changelog.d/ if the change is user-facing (make changelog-add), or confirmed N/A

[Copilot]

Pull request overview

Backports a set of correctness/stability/accessibility fixes to the in-repo copies of @sep/api and @sep/framework so they don’t reappear during ongoing SEP↔PMM UI migration syncing.

Changes:

• Fixes several edge-case correctness issues (token refresh handler isolation, schema-driven form submission gating, dotted-path choice normalization, safer id extraction, numeric validation/coercion, stepless log handling).
• Improves stability/UX around streaming and selectors (bounded SSE reconnects, retryable host selector after failures, preserve scheduled-task kwargs on toggle).
• Addresses UI/accessibility consistency (dark-mode table surface, file-picker accessible name) and adds/updates tests + changelog fragment.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
frontend/packages/framework/test/setup.ts	Removes an unreferenced test setup file.
frontend/packages/framework/src/utils/extractId.ts	Tightens string-id parsing to decimal integers only and safe integers.
frontend/packages/framework/src/utils/extractId.test.ts	Adds coverage for id extraction edge cases.
frontend/packages/framework/src/hooks/useTaskLogs.ts	Accepts step: '' as a valid “General/stepless” log line.
frontend/packages/framework/src/hooks/useResolvedServiceField.ts	Surfaces useServices error state to distinguish “no match” vs “failed lookup”.
frontend/packages/framework/src/hooks/useExecutionEvents.ts	Bounds transient SSE retry loop and surfaces persistent failures.
frontend/packages/framework/src/components/SchemaListView/SchemaListView.tsx	Uses a theme-aware opaque surface instead of pinning white background.
frontend/packages/framework/src/components/SchemaFormRenderer/utils/validationMapper.ts	Updates numeric validation/coercion to avoid parseInt/parseFloat truncation.
frontend/packages/framework/src/components/SchemaFormRenderer/SchemaFormRenderer.tsx	Prevents section-violation submits from being treated as successful by RHF.
frontend/packages/framework/src/components/SchemaFormRenderer/SchemaFormRenderer.test.tsx	Updates rendering assertions and adds regression coverage for unsaved-changes guard.
frontend/packages/framework/src/components/SchemaFormRenderer/hooks/useCascadingField.ts	Aligns cascading clears/readiness with '' sentinel to avoid uncontrolled inputs and premature fetches.
frontend/packages/framework/src/components/SchemaFormRenderer/fields/FileField.tsx	Adds an accessible name to the file-picker icon button.
frontend/packages/framework/src/components/SchemaDrivenApp/SchemaDrivenApp.tsx	Passes capabilities + error props through the edit-form render slot.
frontend/packages/framework/src/components/SchemaDrivenApp/AppTaskEditPage.tsx	Normalizes choice defaults for dotted-path one-of branch fields using path-aware getters/setters.
frontend/packages/framework/src/components/SchemaDrivenApp/AppTaskEditPage.test.tsx	Adds coverage for dotted-path one-of normalization and non-mutation.
frontend/packages/framework/src/components/SchemaDrivenApp/AppListPage.tsx	Guards optional list_view and renders “Not found” when absent.
frontend/packages/framework/src/components/SchemaDrivenApp/AppDetailPage.tsx	Avoids crashing when list_view is absent; falls back to task fields.
frontend/packages/framework/src/components/ScheduledTasksPanel/ScheduledTasksPanel.tsx	Preserves kwargs on enable/disable toggle when present in the response payload.
frontend/packages/framework/src/components/HostSelector/StandaloneHostSelector.tsx	Keeps selector enabled after query failure so onOpen can retry.
frontend/packages/framework/src/components/HostSelector/StandaloneHostSelector.test.tsx	Updates behavior expectations to match retryable-on-open design.
frontend/packages/framework/src/components/HostSelector/HostSelector.tsx	Uses path-aware error lookup for dotted field names (get(errors, name)).
frontend/packages/api/src/client.ts	Prevents _onRefreshed sync throws from rejecting the shared refresh promise.
changelog.d/SEP-1760.fixed.md	Documents the user-visible fixes in a changelog fragment.

Suppressed comments (1)

frontend/packages/framework/src/components/SchemaFormRenderer/utils/validationMapper.ts:133

• Number(raw) will coerce whitespace-only strings to 0 (e.g. ' ' → 0). Because this block only treats '' as empty, a whitespace-only numeric field would be coerced and submitted as 0. Consider trimming string input first and treating a trimmed-empty string as empty (coerce to undefined).

    if (field.type === 'integer' || field.type === 'float') {
      if (raw === '' || raw === undefined || raw === null) {
        setAtPath(out, field.name, undefined);
        continue;
      }

[nachodd]

Second commit (3322f4bab): CodeRabbit reviewed the PMM side again after the first round landed there, so the follow-ups are ported here too.

• extractId — the numeric branch still accepted 1.5 and unsafe integers while the string branch rejected their equivalents. Now Number.isSafeInteger, with coverage for 1.5, MAX_SAFE_INTEGER + 1, and the recursive { id: 1.5 } path.
• ScheduledTasksPanel — the preserved kwargs accepted only a string. Since PeriodicTaskResponse doesn't declare the field at all, a decoded object is as likely as a JSON string, and that case fell back to '{}' — wiping the arguments the guard exists to protect. Both shapes are now handled. A partial-update route for the enabled toggle would remove the whole class of problem; that's a backend change, not something the client can fix.
• client.ts — the isolation of a throwing _onRefreshed was right, but swallowing it silently left the auth layer without the rotated token while refreshAccessToken returned it as applied, with no signal at all. Now logged at error level, without the token or its expiry.
• SchemaFormRenderer test — the removeEventListener spy is restored via onTestFinished, so a failing assertion can't leak it into the rest of the file.
• HostSelector — the same wedge as StandaloneHostSelector, in all three branches: disabled on a hosts-query failure while onOpen holds the only refetch(), so one failure blocked recovery until remount. Not flagged by the review (its diff hunk didn't reach those lines), but the same defect, so it's fixed rather than left behind. The existing "disables the input on error" case was inverted to cover retry-on-open.

Declined: removing ctrl.abort() from useExecutionEvents' terminal path. The claim was that the library allocates a fresh AbortController per retry, making our abort ineffective. fetch-event-source@2.0.1 registers an abort listener on the signal we pass and calls its own dispose() from it, which clears the retry timer and aborts the in-flight request — curRequestController is a separate internal controller. It is redundant next to throw err, but finish and sep-error in this same hook stop the stream exactly that way, so the third terminal path stays consistent with them.

The changelog fragment was updated in place (the executor-host bullet now covers both selectors rather than just the standalone one).

Gates: type-check clean across every package, oxlint 0 errors, 1178 tests pass (743 in @sep/framework), oxfmt clean.

[nachodd]

Ported one more finding from the PMM side (f930c69a7). CodeRabbit caught it in PMM's copy of this config (percona/pmm#5728), and it applies here identically since oxlintrc.json is the same file.

Oxlint never ran our React or import rules. Only eslint, typescript, unicorn and oxc are on by default — --react-plugin and --import-plugin are opt-in, and the config-file equivalent is the plugins array, which we never set. Every react/*, react-hooks/* and import/* entry in the file was parsed, matched no plugin, and silently did nothing. Measured on the pinned 1.64: 108 rules over 474 files before, 135 rules over 491 files after.

plugins replaces the default set rather than extending it, so typescript, unicorn and oxc are listed explicitly (verified that core eslint rules still fire with it set).

What the rules found once live — 1 error, 8 warnings:

• react/no-children-prop in packages/shell/src/appRouteGuard.ts (the error). The rule wants JSX, but that module is plain .ts. Moving element to createElement's third argument doesn't type-check either: AppDisabledGuardProps.children is required and the overload doesn't satisfy it (TS2769). Suppressed on the line with that reasoning — weakening the component's prop contract to satisfy a linter seemed like the wrong trade.
• 4 × react-hooks/exhaustive-deps — AppListPage, ScriptPreviewField, ExecutionEventsPanel, useExecutionEvents (the query.data ?? [] fresh-array-per-render kind). 4 × import/no-cycle — the App ↔ routes pairs in alters and backup_mongo. All pre-existing and all warnings, so CI stays green; I've left them for their own triage rather than folding unrelated hook/structure changes into a backport PR. Worth a follow-up ticket.

Also from that review: $schema now points at the package-local ./node_modules/oxlint/configuration_schema.json instead of the Oxc main branch, so editor validation tracks the pinned version (matching what .oxfmtrc.json already does); and the *.config.ts / *.config.js ignore patterns are gone, since the Vite and Vitest configs are as much part of the build as anything under src — that accounts for the 17 extra linted files.

No changelog fragment for this one: tooling-only, no user-visible behaviour change. The existing SEP-1760.fixed.md still covers the functional fixes.

Gates: oxlint 0 errors, type-check clean across every package, 1178 tests pass, oxfmt clean.