PMM-15216 Migrate the SEP UI into PMM - #5653
Conversation
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## PMM-15288 #5653 +/- ##
=============================================
+ Coverage 45.38% 45.41% +0.02%
=============================================
Files 418 418
Lines 43334 43334
=============================================
+ Hits 19669 19678 +9
+ Misses 21725 21715 -10
- Partials 1940 1941 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| dnf clean all && rm -rf /var/cache/dnf | ||
|
|
||
| # UI build uses pnpm (packageManager in ui/package.json); nodesource nodejs ships npm/yarn only. | ||
| RUN npm install -g pnpm@11.1.3 |
There was a problem hiding this comment.
Is it possible to pass ppm version during docker build (read from some file in repo)? It will simplify the maintenance in future (less files are required to be changed during version bump).
There was a problem hiding this comment.
Good call — done, and the hardcoded version is gone entirely rather than being passed in as a build arg.
All three rpmbuild Dockerfiles now just do:
RUN corepack enablecorepack ships with nodesource's nodejs 22 and resolves pnpm from the packageManager field in ui/package.json at build time, so a pnpm bump only touches that one field. ui/Makefile's setup target does corepack enable pnpm for the same reason, and .devcontainer/setup.sh no longer pins a version either.
This moved to the split-out toolchain PR: #5728.
| dnf clean all && rm -rf /var/cache/dnf | ||
|
|
||
| # UI build uses pnpm (packageManager in ui/package.json); nodesource nodejs ships npm/yarn only. | ||
| RUN npm install -g pnpm@11.1.3 |
There was a problem hiding this comment.
Same fix as on el8 — see the reply above. Applied to el8, el9 and hetzner-el9 in #5728.
|
@copilot resolve the merge conflicts in this pull request |
Resolved by merging |
| dnf clean all && rm -rf /var/cache/dnf | ||
|
|
||
| # UI build uses pnpm (packageManager in ui/package.json); nodesource nodejs ships npm/yarn only. | ||
| RUN npm install -g pnpm@11.1.3 |
There was a problem hiding this comment.
Do we really need both pnpm and yarn?
There was a problem hiding this comment.
Yes — they cover different projects, and both are built from this image:
ui/(the PMM UI workspace) is pnpm.dashboards/pmm-app(the QAN Grafana plugin) is still yarn 1 —packageManager: yarn@1.22.21, anddashboards/Makefilerunsyarn install --frozen-lockfile.
Migrating pmm-app off yarn is out of scope here, so the image needs both until that happens.
I've made the Dockerfile say so, and dropped the hardcoded pnpm version at the same time (per @maxkondr's comment above):
# The UI workspace (ui/) builds with pnpm; the QAN Grafana plugin (dashboards/pmm-app)
# is still on yarn 1, so both are needed. corepack ships with nodesource's nodejs 22
# and resolves pnpm from the `packageManager` field in ui/package.json, so the version
# is pinned in exactly one place and needs no change here on a bump.
RUN corepack enableThis moved to the split-out toolchain PR: #5728.
| is_enabled() { [ "$1" = "1" ] || [ "$1" = "true" ]; } | ||
| declare POSTGRES_DATA_DIR="/srv/postgres14" | ||
| declare POSTGRES_PASSWORD_FILE="/srv/.postgres_password" | ||
| declare POSTGRES_BIN_DIR="/usr/pgsql-14/bin" |
There was a problem hiding this comment.
@fabio-silva I don't really understand what this PR has to do with the entrypoint and postgres-migration.
Please remove that code, it's a separate PR that needs to be tested stand-alone and merged separately, possibly even prior to this one.
There was a problem hiding this comment.
Agreed, and removed. Neither branch touches that code any more — build/docker/server/entrypoint.sh, build/ansible/roles/postgres/* (including the new postgres-sep script), docker-compose.yml, .env.example and managed/utils/envvars are all gone from the diff. It's yours to land stand-alone in #5700.
While removing it we also split the rest of the PR in two, since the toolchain change and the UI migration were reviewing as one 320-file diff:
- PMM-15288 Migrate UI toolchain to pnpm + oxlint/oxfmt #5728 (PMM-15288) — pnpm + oxlint/oxfmt + the library upgrades (React 19, RR7, Vite 8, Vitest 4, TS 6). Targets
main. This is where the Dockerfile/devcontainer/CI changes now live. - PMM-15216 Migrate the SEP UI into PMM #5653 (this PR, PMM-15216) — the SEP UI migration only, now stacked on top of PMM-15288 Migrate UI toolchain to pnpm + oxlint/oxfmt #5728.
That meant rebuilding this branch from main, so its history changed; the previous tip was 9a507d0c.
WalkthroughChangesSEP platform and API
Sequence Diagram(s)sequenceDiagram
participant PMM
participant SEPProxy
participant ATW
participant SEPAPI
PMM->>SEPProxy: Route SEP requests
SEPProxy->>SEPAPI: Forward authenticated API calls
PMM->>ATW: Mount ATW routes
ATW->>SEPAPI: Load incidents, schemas, executions, and send jobs
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
nachodd
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (30)
ui/packages/sep/api/tests/typed-client.test.ts-18-23 (1)
18-23: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winCo-locate this test with
typed-client.ts.Move this file to
ui/packages/sep/api/src/typed-client.test.ts. Updateui/packages/sep/api/vitest.config.tsso Vitest discovers the co-located test files.As per coding guidelines, “Co-locate test files next to the components they test, using
*.test.tsxor*.test.tsnaming.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/api/tests/typed-client.test.ts` around lines 18 - 23, Move the typed-client test from the tests directory to be co-located with typed-client.ts as typed-client.test.ts, preserving its existing coverage and imports. Update vitest.config.ts so Vitest includes co-located *.test.ts and *.test.tsx files under the source tree.Source: Coding guidelines
ui/packages/sep/api/src/client.ts-136-159 (1)
136-159: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winA synchronous throw from
_onRefreshedstill fails the refresh.The comment on Lines 137-140 states that a synchronous throw from the injected
_onRefreshedhandler must not be reported as a failed refresh. The call on Line 157 sits inside the async executor, so a throw rejectsrefreshInFlight. Every awaiting caller then receives a rejection instead of the new token, and the 401 interceptor propagates a non-ApiErrorwithout retrying and without calling_onUnauthorized. Isolate the handler call to match the documented intent.🛡️ Proposed fix to isolate the handler
- _onRefreshed(data.access_token, data.expires_in); + try { + _onRefreshed(data.access_token, data.expires_in); + } catch { + // A failing auth-layer handler must not invalidate a successful + // cookie rotation. + } return data.access_token;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/api/src/client.ts` around lines 136 - 159, Isolate the synchronous _onRefreshed call from the refreshInFlight async executor so its exception cannot reject the shared refresh promise. Update the refresh flow around _onRefreshed to invoke the handler after the promise has resolved, while preserving the successful token return and existing network-error handling.ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginListPage.tsx-166-170 (1)
166-170: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
schema.list_viewbefore dereferencing it.PluginSchema.list_viewis optional. An unknown entity route setsmultito false, thenPluginListPagereadslistView.columnsand can crash. The detail route can also reachOverviewTab, whereschema.list_view!.columnscan crash.
- Render
Not foundwhen the entity is unresolved and no top-levellist_viewexists.- Omit the Task information card when
schema.list_viewis absent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginListPage.tsx` around lines 166 - 170, Guard the unresolved-entity path in PluginListPage so it renders Not found when multi is false and schema.list_view is absent before accessing listView.columns; preserve normal rendering when a list view exists. In PluginDetailPage’s OverviewTab, conditionally omit the Task information card when schema.list_view is unavailable instead of dereferencing schema.list_view!.columns.ui/packages/sep/framework/src/components/ScheduledTasksPanel/ScheduledTasksPanel.tsx-89-106 (1)
89-106: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winA simple enable/disable toggle destroys
kwargson the server.
handleToggleEnabledsendskwargs: '{}'in a full PUT. Any scheduled task created with non-defaultkwargsloses that data when a user flips the switch. The inline comment records the backend gap, but the current code turns a read-only-looking UI action into silent data loss.Consider one of these mitigations until the backend exposes
kwargsinPeriodicTaskResponse:
- Preserve the value when it is present on the response object, and only fall back to
'{}'when it is absent.- Disable the toggle for tasks whose
kwargscannot be round-tripped.I can open a tracking issue for the backend schema gap if that helps.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/components/ScheduledTasksPanel/ScheduledTasksPanel.tsx` around lines 89 - 106, Update handleToggleEnabled to preserve task.kwargs when the response object provides it, using '{}' only as the fallback when kwargs is absent. Keep the existing PeriodicTaskUpdate payload and toggle behavior unchanged otherwise, and retain the fallback until PeriodicTaskResponse exposes kwargs consistently.ui/apps/pmm/src/contexts/navigation/navigation.utils.tsx-300-315 (1)
300-315: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDefine shared SEP route constants.
The navigation builder and router define the same SEP route values independently. Define the route values in
src/lib/constants.tsand consume them from both sites.
ui/apps/pmm/src/contexts/navigation/navigation.utils.tsx#L300-L315: use shared constants forurlandmatches.ui/apps/pmm/src/router.tsx#L88-L107: use the same constants for route paths androuteBase.As per coding guidelines, “Do not hardcode URLs; use constants from
src/lib/constants.ts.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/apps/pmm/src/contexts/navigation/navigation.utils.tsx` around lines 300 - 315, Define shared SEP route constants in src/lib/constants.ts, then update addSepApps in ui/apps/pmm/src/contexts/navigation/navigation.utils.tsx to use them for url and matches, and update the SEP routes and routeBase in ui/apps/pmm/src/router.tsx to use the same constants instead of hardcoded paths.Source: Coding guidelines
ui/apps/pmm/src/sep/SepPage.tsx-14-19 (1)
14-19: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEnforce the PMM-admin gate at the route wrapper.
NavigationProviderhides SEP entries for non-admin users, but direct navigation still matches the SEP routes.SepPagedoes not pass a role restriction toPage, so every authorized user can render these plugins.Apply the same PMM-admin authorization policy at
SepPageor at each SEP route. Do not rely on navigation visibility as an authorization control.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/apps/pmm/src/sep/SepPage.tsx` around lines 14 - 19, Update the SepPage component to pass the existing PMM-admin role restriction to the Page wrapper, ensuring direct SEP route navigation is denied for non-admin users while preserving the current children layout.ui/apps/pmm/vite.config.ts-20-21 (1)
20-21: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse the development environment-variable prefix.
Rename
SEP_BACKEND_URLtoPMM_DEV_SEP_BACKEND_URL. RenameSEP_INTERNAL_TOKENtoPMM_DEV_SEP_INTERNAL_TOKEN. These variables configure only the Vite development proxy.As per coding guidelines, “Use environment-variable prefixes consistently:
PMM_DEV_*for development/testing only.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/apps/pmm/vite.config.ts` around lines 20 - 21, Update the environment-variable references used by the Vite development proxy: rename SEP_BACKEND_URL to PMM_DEV_SEP_BACKEND_URL and SEP_INTERNAL_TOKEN to PMM_DEV_SEP_INTERNAL_TOKEN in the configuration initialization. Preserve the existing proxy behavior and values.Source: Coding guidelines
ui/apps/pmm/src/sep/bootstrap.ts-17-20 (1)
17-20: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAdd production SEP routing and authentication.
The deployed Nginx configuration has no upstream or location for
/api,/sep_app,/stream-logs,/execution-events, or/files. Only the Vite development proxy injectsSEP_INTERNAL_TOKEN. BecauseinitSepAuthreturnsnull, deployed SEP requests have no SEP bearer and cannot work. Add production routing with server-side credentials, or wirepostSessionExchange()before merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/apps/pmm/src/sep/bootstrap.ts` around lines 17 - 20, Update initSepAuth to establish production SEP authentication instead of returning a null token, and ensure deployed Nginx routing forwards /api, /sep_app, /stream-logs, /execution-events, and /files with server-side SEP credentials. Reuse postSessionExchange() if that is the intended authentication flow, and preserve the existing unauthorized-handler registration.ui/packages/plugins/atw/tests/CategoryBrowser.test.tsx-18-22 (1)
18-22: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winComponent tests are not co-located with their components. The ATW package keeps component tests in a separate
tests/directory and imports the component through../src/.... The coding guidelines require co-location.
ui/packages/plugins/atw/tests/CategoryBrowser.test.tsx#L18-L22: move the file toui/packages/plugins/atw/src/CategoryBrowser.test.tsxand change the import to./CategoryBrowser.ui/packages/plugins/atw/tests/ResultsPane.test.tsx#L18-L22: move the file toui/packages/plugins/atw/src/ResultsPane.test.tsxand change the import to./ResultsPane.Keep
ui/packages/plugins/atw/tests/setup.tswhere it is, and confirm thatui/packages/plugins/atw/vitest.config.tsincludeglobs still match the new locations.As per coding guidelines: "Co-locate test files next to the components they test, using
*.test.tsxor*.test.tsnaming."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/plugins/atw/tests/CategoryBrowser.test.tsx` around lines 18 - 22, Move CategoryBrowser.test.tsx to ui/packages/plugins/atw/src/CategoryBrowser.test.tsx and update its CategoryBrowser import to ./CategoryBrowser; likewise move ResultsPane.test.tsx to ui/packages/plugins/atw/src/ResultsPane.test.tsx and update its import to ./ResultsPane. Keep ui/packages/plugins/atw/tests/setup.ts unchanged and verify vitest.config.ts include globs cover the relocated tests.Source: Coding guidelines
ui/packages/plugins/atw/src/CollectPane.tsx-62-81 (1)
62-81: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftFlatten dotted ATW parameter names before building the payload.
ATW fields include
source.path. The form stores this as nested objects, butbuildBatchPayloadpasses those objects throughtoArgsunchanged. Flatten shared and per-snippet values back to their declared keys, and add regression coverage for both paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/plugins/atw/src/CollectPane.tsx` around lines 62 - 81, Update buildBatchPayload and its toArgs flow to flatten nested form objects into the declared dotted ATW parameter keys, including shared values and per-snippet values such as source.path. Preserve existing argument conversion behavior for non-dotted fields, and add regression coverage verifying both shared and per-snippet payload paths produce flattened keys.ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginTaskEditPage.tsx-63-85 (1)
63-85: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
normalizeChoiceDefaultsmisses fields with dotted names.
flattenSectionFieldsalso returnsone_ofbranch fields, whose names are dotted paths such assource.mode(see theone_ofcases inSchemaFormRenderer.test.tsx). This function reads and writes flat keys without[field.name], so a nestedchoicevalue stored at{ source: { mode: 'rsync' } }is never normalized. The case-mismatched value then reaches the form and renders as an empty selection, which is the failure this function exists to prevent.Use the exported
getAtPathandsetAtPathhelpers instead of flat key access. Note thatsetAtPathmutates nested objects, so deep-copy the input first.🐛 Proposed fix using path-aware accessors
-import { - SchemaFormRenderer, - coerceFormValues, - flattenSectionFields, -} from '../SchemaFormRenderer'; +import { + SchemaFormRenderer, + coerceFormValues, + flattenSectionFields, + getAtPath, + setAtPath, +} from '../SchemaFormRenderer'; @@ - const out = { ...form }; + const out = structuredClone(form) as Record<string, unknown>; for (const field of flattenSectionFields(sections)) { if (field.type !== 'choice' && field.type !== 'multi_choice') { continue; } const choiceMap = new Map( field.choices.map((c) => [c.value.toLowerCase(), c.value]) ); - const raw = out[field.name]; + const raw = getAtPath(out, field.name); if (field.type === 'multi_choice' && Array.isArray(raw)) { - out[field.name] = raw.map((v) => { - const canonical = - typeof v === 'string' ? choiceMap.get(v.toLowerCase()) : undefined; - return canonical ?? v; - }); + setAtPath( + out, + field.name, + raw.map((v) => { + const canonical = + typeof v === 'string' ? choiceMap.get(v.toLowerCase()) : undefined; + return canonical ?? v; + }) + ); } else if (field.type === 'choice' && typeof raw === 'string') { const canonical = choiceMap.get(raw.toLowerCase()); if (canonical !== undefined) { - out[field.name] = canonical; + setAtPath(out, field.name, canonical); } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginTaskEditPage.tsx` around lines 63 - 85, Update normalizeChoiceDefaults to deep-copy the input form, then use the exported getAtPath and setAtPath helpers for reading and writing each choice field in flattenSectionFields, including dotted one_of paths such as source.mode. Preserve the existing choice and multi_choice canonicalization behavior while ensuring setAtPath receives the normalized value.ui/packages/sep/framework/src/components/SchemaFormRenderer/hooks/useCascadingField.ts-66-72 (1)
66-72: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
useCascadingFielddoes not follow the form's empty-value contract.buildFormDefaultsinSchemaFormRenderer.tsx(line 85) seedsservice,schema,table, andhostfields to''. These are exactly the cascading selector types named in the doc comment on line 38. This hook instead treatsundefinedas the empty value, which breaks both the clear path and the readiness check. Pick''as the single empty-value sentinel and apply it in both places.
ui/packages/sep/framework/src/components/SchemaFormRenderer/hooks/useCascadingField.ts#L66-L72: changesetValue(fieldName, undefined, …)tosetValue(fieldName, '', …).undefinedalso flips a bound MUI input from controlled to uncontrolled, so React logs a warning and the field can keep showing the stale selection.ui/packages/sep/framework/src/components/SchemaFormRenderer/hooks/useCascadingField.ts#L75-L80: addupstreamValue !== ''to thereadyexpression. An upstream selector with nothing selected holds''today, soreadyreturnstrueand a downstream selector fetches options for an empty parent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/components/SchemaFormRenderer/hooks/useCascadingField.ts` around lines 66 - 72, The useCascadingField clear path and readiness check must use '' as the form’s empty-value sentinel. In ui/packages/sep/framework/src/components/SchemaFormRenderer/hooks/useCascadingField.ts lines 66-72, update setValue in the previousRef change block to clear fieldName with '' instead of undefined; in lines 75-80, update the ready expression to require upstreamValue !== '' so downstream options are not fetched without a selected parent.ui/packages/sep/framework/src/components/SchemaFormRenderer/SchemaFormRenderer.tsx-356-361 (1)
356-361: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winA section-violation block silently disarms the unsaved-changes guard.
handleFormSubmitreturns without throwing whenhasSectionViolationsis true. react-hook-form treats that as a successful submit and setsformState.isSubmitSuccessful = true.useUnsavedChangesGuardcomputesisDirty && !isSubmitSuccessful(useUnsavedChangesGuard.tsline 33), soisGuardedbecomes false. The re-arm effect in that hook only runs whensubmitErroris truthy, andsubmitErrorstays null on this path. The guard therefore stays disarmed: thebeforeunloadprompt andUnsavedChangesBlockerno longer fire, and the user can navigate away and lose the form data.Gate the submit before
handleSubmitruns, or mark the form invalid so RHF does not flag the submit as successful.🐛 Proposed fix — block in the submit event handler instead
const handleSubmitEvent = (event: FormEvent<HTMLFormElement>) => { + if (hasSectionViolations) { + // Section-level rules already render their own inline Alerts. Stop here so + // react-hook-form never marks the submit successful, which would disarm + // useUnsavedChangesGuard. + event.preventDefault(); + return; + } if (appliedServerErrorPaths.current.length > 0) {Then drop the early
returnfromhandleFormSubmit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/components/SchemaFormRenderer/SchemaFormRenderer.tsx` around lines 356 - 361, Move the hasSectionViolations check out of handleFormSubmit and gate the submit before react-hook-form handleSubmit runs, so blocked submissions are not marked successful and the unsaved-changes guard remains active. Remove the early return from handleFormSubmit while preserving its existing onSubmit behavior for valid submissions.ui/packages/sep/framework/src/components/SchemaListView/SchemaListView.tsx-386-398 (1)
386-398: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
SchemaListView.tsxbypasses PMM theme tokens in two places. Both sites inline a design value instead of reading it from the theme, so the schema-driven list does not followpmmThemeOptions. The shared fix is to define the missing tokens inpmmThemeOptionsfrom@percona/percona-uiand read them here.
ui/packages/sep/framework/src/components/SchemaListView/SchemaListView.tsx#L386-L398: replacebgcolor: 'common.white'on bothmuiTablePaperPropsandmuiTableContainerPropswithbgcolor: 'background.paper', and fix the opacity ofbackground.paperinpmmThemeOptionsso every PMM surface benefits. Pinningcommon.whiterenders a white surface in dark mode, as the inline comment already acknowledges.ui/packages/sep/framework/src/components/SchemaListView/SchemaListView.tsx#L146-L154: replacefontFamily: "'Roboto Mono', monospace"in thecodebranch with a monospace token read from the theme, or add a themeTypographyvariant for code cells and use that variant.As per coding guidelines: "Do not use hard-coded colors, font families, or spacing that bypass the theme; prefer
sxwith theme tokens, breakpoints, and MUITypographyvariants" and "UseColorModeContextor existing hooks such asuseColorModefor light/dark mode".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/components/SchemaListView/SchemaListView.tsx` around lines 386 - 398, Update SchemaListView.tsx at lines 386-398 so both muiTablePaperProps and muiTableContainerProps use the theme token background.paper, and update pmmThemeOptions to provide an opaque, mode-appropriate background.paper value. At lines 146-154, replace the hard-coded "'Roboto Mono', monospace" in the code branch with the theme’s monospace token or a dedicated code Typography variant, and use that theme value for code cells.Source: Coding guidelines
ui/packages/sep/framework/src/components/SchemaListView/SchemaListView.tsx-108-116 (1)
108-116: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard
undefinedas well asnull.Line 112 checks only
value === null.dataisRecord<string, unknown>[], and the columns come from a server-supplied schema, so a row can omit a declared column key. In that casevalueisundefinedand line 115 produces the string'undefined'.The formats amplify this:
defaultandchiprender the literal textundefined.daterendersInvalid Date.relativerendersNaNd ago, because everymins/hourscomparison againstNaNis false.Use a loose null check so both cases fall to the em dash.
🐛 Proposed fix
function formatCellValue( value: unknown, format: ListColumn['format'] ): ReactNode { - if (value === null) { + if (value === null || value === undefined) { return '—'; } const str = String(value);Consider also guarding an unparsable date in the
dateandrelativebranches:case 'date': - return new Date(str).toLocaleDateString(); + case 'date': { + const d = new Date(str); + return Number.isNaN(d.getTime()) ? '—' : d.toLocaleDateString(); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/components/SchemaListView/SchemaListView.tsx` around lines 108 - 116, Update formatCellValue to use a loose null check so both null and undefined values return the em dash before String conversion or format handling. Preserve the existing behavior for defined values and formats.ui/packages/sep/framework/src/hooks/useResolvedServiceField.ts-80-96 (1)
80-96: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winExpose the fetch error state.
The hook discards the error state of
useServices. If the services fetch fails,isFetchedbecomestrueandservicesstaysEMPTY_SERVICES.serviceis thenundefinedandisResolvingisfalse.Callers cannot distinguish two different situations:
- The scalar id resolved to no matching service.
- The fetch failed, so resolution never completed.
The doc comment at Lines 44-48 instructs callers to wait rather than treat the parent as missing. On a fetch failure the caller receives the exact signal it was told means "not resolving", so it renders a permanently blank service name with no error surfaced.
Add the error state to
ResolvedServiceField.🔧 Proposed fix
isResolving: boolean; + /** True when the bounded ``useServices`` fetch failed. ``service`` stays undefined. */ + isError: boolean; + /** Error from the bounded ``useServices`` fetch, when it failed. */ + error: Error | null; }- const { data: services = EMPTY_SERVICES, isFetched } = useServices({ + const { + data: services = EMPTY_SERVICES, + isFetched, + isError, + error, + } = useServices({ serviceTypes: types, enabled, });return { parent, service, resetKey: cascadeParentResetKey(parent), isResolving: enabled && !isFetched, + isError: enabled && isError, + error: enabled ? (error ?? null) : null, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/hooks/useResolvedServiceField.ts` around lines 80 - 96, Expose the fetch error from useServices through the ResolvedServiceField return value. Destructure the error alongside data and isFetched, then add it to the returned object so callers can distinguish a failed service lookup from a successful lookup with no matching service; preserve the existing service and isResolving behavior.ui/packages/sep/framework/src/components/HostSelector/StandaloneHostSelector.tsx-73-79 (1)
73-79: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winA failed hosts query disables the selector permanently.
Line 79 disables the
AutocompletewhenisErroris true. Line 73 puts the only retry trigger,refetch(), ononOpen. A disabledAutocompletenever opens, soonOpennever fires. After one hosts-query failure the user cannot recover in this component. The user must remount the page.Add an explicit retry control, or keep the control enabled on error so
onOpencan retry.🔧 Proposed fix — add a retry action to the error state
+import IconButton from '`@mui/material/IconButton`'; +import InputAdornment from '`@mui/material/InputAdornment`'; +import RefreshIcon from '`@mui/icons-material/Refresh`';loading={isLoading} loadingText="Loading hosts…" noOptionsText="No hosts available" - disabled={disabled || isError} + disabled={disabled} renderInput={(params) => ( <TextField {...params} label={label} error={isError} helperText={ isError ? (error?.message ?? 'Failed to load hosts') : undefined } + slotProps={{ + input: { + ...params.InputProps, + endAdornment: ( + <> + {isError && ( + <InputAdornment position="end"> + <IconButton + size="small" + aria-label="Retry loading hosts" + onClick={() => void refetch()} + > + <RefreshIcon fontSize="small" /> + </IconButton> + </InputAdornment> + )} + {params.InputProps.endAdornment} + </> + ), + }, + }} /> )}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/components/HostSelector/StandaloneHostSelector.tsx` around lines 73 - 79, Update the HostSelector Autocomplete error handling so a hosts-query failure does not permanently block recovery: either remove isError from disabled or add an explicit retry control that invokes refetch() while the error state is shown. Preserve disabled behavior for the existing disabled prop and ensure users can trigger the retry without remounting.ui/packages/sep/framework/src/components/FreeSoloSelect/freeSoloValue.ts-112-117 (1)
112-117: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCommitting the trimmed string blocks trailing spaces during typing.
FreeSoloSelectcallsnormalizeChangeon every keystroke throughonInputChange. This function commitstrimmed, not the raw input.toDisplayValuethen returns that trimmed string as the controlled Autocompletevalue, so MUI resets the visible input text to the trimmed form. A user who types a space cannot keep it, because each keystroke removes the trailing space. Custom values that contain spaces become hard to enter.Use
trimmedonly for the blank check and for label matching. Commit the raw string.🐛 Proposed fix
const trimmed = next.trim(); if (trimmed === '') { return null; } const match = options.find((o) => getOptionLabel(o) === trimmed); - return match ? match.id : trimmed; + return match ? match.id : next; }Update the docstring at lines 98-99 to state that a non-empty string is kept verbatim, including surrounding whitespace, and update the test at line 94 to cover a value with an inner space.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/components/FreeSoloSelect/freeSoloValue.ts` around lines 112 - 117, Update normalizeChange around the trimmed blank check so trimmed is used only to detect empty input and match option labels; return the raw next string for unmatched non-empty custom values, preserving surrounding whitespace during typing. Revise the function docstring to state that non-empty strings are retained verbatim and update the related test to cover a value containing an inner space.ui/packages/sep/framework/src/components/HostSelector/HostSelector.tsx-160-160 (1)
160-160: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse a path-aware error lookup for nested field names.
When
nameis a dotted path such asparams.host,errors[name]returnsundefined. Useget(errors, name)?.messageso the autocomplete field displays its validation error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/components/HostSelector/HostSelector.tsx` at line 160, Update the fieldError lookup in the HostSelector component to resolve nested field names through the form error object, using the path-aware get helper with name before reading message. Preserve the existing string-or-undefined typing and validation display behavior.ui/packages/sep/framework/src/utils/extractId.ts-29-41 (1)
29-41: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject whitespace-only and non-integer strings.
Number(value)coerces a whitespace-only string to0, soextractId(' ')returns0instead ofnull. Two callers then treat that as a resolvable id:
useResolvedServiceField.tsline 78 setsenabled = unresolvedServiceId !== null && ..., so a lookup fires for service id0and never matches.SchemaSelector.tsxline 72 computesserviceIdfrom the untrimmed parent, sonoServicebecomesfalseanduseSchemas({ serviceId: 0 })fires against a nonexistent service.
Numberalso accepts'1.5'and'0x10', which produce non-integer or unintended ids whileServiceOption.idis an integer.🐛 Proposed fix
if (typeof value === 'string' && value !== '') { - const n = Number(value); - return Number.isFinite(n) ? n : null; + const trimmed = value.trim(); + if (!/^-?\d+$/.test(trimmed)) { + return null; + } + const n = Number(trimmed); + return Number.isSafeInteger(n) ? n : null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/utils/extractId.ts` around lines 29 - 41, Update extractId to reject whitespace-only strings by trimming before validation, and accept string IDs only when they represent decimal integers. Preserve finite numeric handling and recursive object id extraction, while rejecting values such as "1.5", "0x10", and blank strings with null.ui/packages/sep/framework/src/hooks/useTaskHistoryFiles.ts-41-49 (1)
41-49: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd a production route for
/files/*. PMM’s production nginx configuration defines no/fileslocation or SEP upstream. The Vite proxy applies only in development, so these requests do not reach SEP in production. Add the production proxy or use the deployed SEP route before enabling the download affordance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/hooks/useTaskHistoryFiles.ts` around lines 41 - 49, Update the request path used by the task-history file download flow in the hook’s queryFn so production requests target the deployed SEP route rather than relying on the development-only Vite `/files` proxy. Ensure the resulting URL matches the production routing configuration before exposing the download affordance.ui/packages/sep/framework/test/setup.ts-18-18 (1)
18-18: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winTwo competing Vitest setup files exist in the framework package. The package adds
test/setup.tsandtests/setup.tsin sibling directories. Both register the jest-dom matchers, but only the plural one registersafterEach(cleanup). Whichever pathvitest.config.tslists insetupFilesdecides whether rendered components are torn down between tests, so a future edit to that config silently changes DOM isolation for the whole package.
ui/packages/sep/framework/test/setup.ts#L18-L18: delete this file, because it omitsafterEach(cleanup); without cleanup, queries such asgetByRole('tab')can match nodes left over from an earlier test.ui/packages/sep/framework/tests/setup.ts#L18-L24: keep this file as the single setup entry point, and confirmsetupFilesinui/packages/sep/framework/vitest.config.tsresolves to this path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/test/setup.ts` at line 18, Remove ui/packages/sep/framework/test/setup.ts because it lacks afterEach(cleanup). Retain ui/packages/sep/framework/tests/setup.ts as the sole setup entry point, and update or verify setupFiles in ui/packages/sep/framework/vitest.config.ts resolves to that plural-path file.ui/packages/sep/framework/src/components/SchemaDrivenPlugin/SchemaDrivenPlugin.tsx-245-251 (1)
245-251: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass
submitError,fieldErrors, andcapabilitiesto therenderEditFormslot.
PluginEditPagemaps API failures intosubmitErrorandfieldErrorsat line 199, then passes both to the defaultSchemaFormRendererat lines 257-258. The custom slot invocation omits them. A consumer that suppliesrenderEditFormtherefore cannot render the persistent 422 banner or the inline per-field errors, and themapSubmitErrorresult becomes dead state on that path.The sibling page keeps the contract complete.
PluginTaskEditPagepassescapabilities: schema.capabilities,submitError, andfieldErrorsto the sameRenderFormSlot(seeui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginTaskEditPage.tsx). Align this call so both pages honor one slot contract.🐛 Proposed fix
{renderEditForm?.({ sections, onSubmit: handleSubmit, loading: updateEntity.isPending, defaultValues, + capabilities: schema.capabilities, renderField, + submitError, + fieldErrors, }) ?? (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/components/SchemaDrivenPlugin/SchemaDrivenPlugin.tsx` around lines 245 - 251, Update the renderEditForm invocation in SchemaDrivenPlugin to pass capabilities, submitError, and fieldErrors alongside the existing form props, matching the RenderFormSlot contract and the PluginTaskEditPage implementation. Reuse the values already derived in PluginEditPage, including schema.capabilities and the mapped error state.ui/packages/sep/framework/src/components/SchemaDrivenPlugin/SchemaDrivenPlugin.tsx-300-301 (1)
300-301: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRender related-app tabs and routes for multi-entity schemas. The schema contract allows both
entitiesandrelated_apps. When both are present, the early return bypasses related-app handling, so related-app paths render no route. Add this handling to the multi-entity branch or enforce mutual exclusion in the schema.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/components/SchemaDrivenPlugin/SchemaDrivenPlugin.tsx` around lines 300 - 301, Update the multi-entity handling in SchemaDrivenPlugin so schemas containing both entities and related_apps still render the related-app tabs and routes instead of being bypassed by the early return. Reuse the existing relatedApps and hasRelatedApps flow, or explicitly enforce mutual exclusion in schema validation if that is the established contract.ui/packages/sep/framework/src/components/SchemaFormRenderer/fields/ScriptPreviewField.tsx-87-118 (1)
87-118: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winFetch the preview through TanStack Query instead of a manual effect.
This effect performs server-state fetching with a hand-rolled debounce, abort handling, and status machine. The repository guidelines require TanStack Query for all server state and forbid bypassing it for API calls. A
useQuerykeyed byfield.endpoint_urlplus the serialized dependency values also gives caching and deduplication across remounts of the same field.Keep the debounce by deriving a debounced key with
useState/useEffect, then pass it into the query key andqueryFn.As per coding guidelines: "Use TanStack Query (
useQuery,useMutation) for all server state" and "Do not bypass React Query for API calls; use it for caching, deduplication, and background refetching."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/components/SchemaFormRenderer/fields/ScriptPreviewField.tsx` around lines 87 - 118, Replace the manual fetch effect in ScriptPreviewField with TanStack Query’s useQuery, using field.endpoint_url and the serialized dependency values in the query key and queryFn. Preserve the existing debounce by deriving a debounced dependency key with useState/useEffect, and map query loading, success, and error data to the existing preview state or rendering contract while retaining request cancellation through the query signal.Source: Coding guidelines
ui/packages/sep/framework/src/components/SchemaFormRenderer/utils/validationMapper.ts-59-73 (1)
59-73: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject invalid numeric values before coercion.
parseInt('2.5', 10)submits2.parseFloat('3.14invalid')submits3.14. The current rules do not reject fractional integer values or non-finite numeric values.SchemaFormRenderersends this coerced output to the backend.Add numeric validation with
Number.isFinite(). RequireNumber.isInteger()for integer fields. UseNumber()during coercion so invalid input is not silently truncated.Proposed fix
case 'integer': case 'float': { + rules.validate = (value: unknown) => { + if (value === '' || value === null || value === undefined) { + return true; + } + + const numericValue = Number(value); + if (!Number.isFinite(numericValue)) { + return 'Enter a valid number'; + } + if (field.type === 'integer' && !Number.isInteger(numericValue)) { + return 'Enter a whole number'; + } + return true; + }; if (field.ge !== undefined) { rules.min = {- const num = - field.type === 'integer' - ? parseInt(String(raw), 10) - : parseFloat(String(raw)); - setAtPath(out, field.name, Number.isNaN(num) ? raw : num); + const num = Number(raw); + const valid = + Number.isFinite(num) && + (field.type !== 'integer' || Number.isInteger(num)); + setAtPath(out, field.name, valid ? num : raw);Also applies to: 120-129
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/components/SchemaFormRenderer/utils/validationMapper.ts` around lines 59 - 73, Update the numeric validation and coercion logic in the integer/float handling of validationMapper so values are converted with Number() rather than parseInt/parseFloat, rejecting non-finite results with Number.isFinite(). For integer fields, also require Number.isInteger() to reject fractional input before it reaches the backend; preserve the existing ge/le range rules.ui/packages/sep/framework/src/components/SchemaFormRenderer/fields/YamlField.tsx-48-51 (1)
48-51: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse theme typography through
sx.Lines 48-50 hard-code a font family and font size. These values bypass PMM theme settings. Apply the code-font and font-size tokens through
sxor a theme-aware styled component.As per coding guidelines, do not use hard-coded colors, font families, or spacing that bypass the theme; prefer
sxwith theme tokens, breakpoints, and MUITypographyvariants.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/components/SchemaFormRenderer/fields/YamlField.tsx` around lines 48 - 51, Update the input styling in YamlField’s inputProps to use the theme-aware sx prop with the appropriate typography code-font and font-size tokens instead of hard-coded fontFamily and fontSize values; preserve the existing spellCheck setting.Source: Coding guidelines
ui/packages/sep/framework/src/components/SchemaFormRenderer/fields/FileField.tsx-63-76 (1)
63-76: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd an accessible name to the file picker button.
IconButtoncontains only an icon and has no accessible name. Screen-reader users cannot identify the file selection action.Proposed fix
- <IconButton component="label" htmlFor={inputId} edge="end"> + <IconButton + aria-label={`Select file for ${field.label}`} + component="label" + htmlFor={inputId} + edge="end" + >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/components/SchemaFormRenderer/fields/FileField.tsx` around lines 63 - 76, Add an accessible name to the IconButton in the FileField component, such as an appropriate aria-label describing the file selection action, while preserving its existing label behavior, icon, and file input handling.ui/packages/sep/framework/src/hooks/useTaskLogs.ts-247-260 (1)
247-260: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winLog lines without a step are dropped, which conflicts with the execution-events contract.
Line 250 rejects the payload when
stepis falsy, so a log line withstep: ''is discarded.useExecutionEventstreats''as a valid stepless bucket throughSTEPLESS_KEY, andExecutionEventsPanelandLogStepTabsrender that bucket as "General". The two streams therefore disagree on the meaning of an empty step. If the backend can emit stepless log lines, this silently loses output. Validatetypeof step === 'string'and map''to the same stepless key that the events path uses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/hooks/useTaskLogs.ts` around lines 247 - 260, Update the payload validation and key construction in the task-log handler around offsetsRef to accept empty step strings by validating typeof step === 'string' instead of treating step as falsy. Normalize step === '' to the shared STEPLESS_KEY used by useExecutionEvents so stepless logs join the existing General bucket, while preserving the current offset filtering for other steps.ui/packages/sep/framework/src/hooks/useExecutionEvents.ts-234-246 (1)
234-246: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTransient stream failures retry without a limit and without user feedback.
onerrorreturnsundefinedfor every non-sentinel error.fetchEventSourcethen retries indefinitely. The handler does not setsseErrorand does not clearsseLoading. If the endpoint fails persistently for a non-401 reason, for example a 500 or a DNS failure, the panel keeps showing the loading state and reconnects forever. Count consecutive failures. After a threshold, setsseErrorand stop the retry loop by re-throwing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/hooks/useExecutionEvents.ts` around lines 234 - 246, Update the onerror handler in useExecutionEvents to count consecutive transient stream failures, reset that count after a successful connection, and after the defined threshold set sseError, clear sseLoading, and re-throw the error to stop retries. Preserve the existing StreamRetriableAfterRefresh and StreamFatalError handling, while continuing silent retries below the threshold.
|
@coderabbitai Thanks — went through all 30 major findings. 24 applied in bb8c522d6, 6 declined with reasons below. Applied — correctness / stability
Applied — contract / conventions
Declined
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Commit ==='
git show --no-patch --oneline bb8c522d6
echo '=== Changed files relevant to the reported fixes ==='
git show --format='' --name-only bb8c522d6 | rg \
'client\.ts|SchemaFormRenderer|PluginTaskEditPage|PluginListPage|PluginDetailPage|SchemaListView|HostSelector|extractId|validationMapper|useTaskLogs|useExecutionEvents|StandaloneHostSelector|ScheduledTasksPanel|useCascadingField|SepPage|SchemaDrivenPlugin|constants\.ts|router\.tsx|navigation\.utils\.tsx|vite\.config\.ts|FileField|useResolvedServiceField|freeSoloValue|CollectPane|ScriptPreviewField|useTaskHistoryFiles' || true
echo '=== Symbol map ==='
ast-grep outline ui/packages/sep/api/src/client.ts --items all --match 'refreshAccessToken|_onRefreshed' 2>/dev/null || true
ast-grep outline ui/packages/sep/framework/src/components/SchemaFormRenderer/SchemaFormRenderer.tsx --items all --match 'handleSubmitEvent|handleFormSubmit' 2>/dev/null || true
ast-grep outline ui/packages/sep/framework/src/components/SchemaFormRenderer/utils/fieldPath.ts --items all --match 'getAtPath|setAtPath' 2>/dev/null || true
ast-grep outline ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginTaskEditPage.tsx --items all --match 'normalizeChoiceDefaults' 2>/dev/null || true
ast-grep outline ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginListPage.tsx --items all --match 'PluginListPage' 2>/dev/null || true
ast-grep outline ui/packages/sep/framework/src/components/SchemaListView/SchemaListView.tsx --items all --match 'formatCellValue' 2>/dev/null || true
ast-grep outline ui/packages/sep/framework/src/components/SchemaFormRenderer/fields/HostField.tsx --items all 2>/dev/null || true
ast-grep outline ui/packages/sep/framework/src/utils/extractId.ts --items all --match 'extractId' 2>/dev/null || true
echo '=== Focused implementation excerpts ==='
rg -n -A18 -B8 '_onRefreshed|refreshAccessToken' ui/packages/sep/api/src/client.ts
rg -n -A24 -B8 'handleSubmitEvent|handleFormSubmit' ui/packages/sep/framework/src/components/SchemaFormRenderer/SchemaFormRenderer.tsx
rg -n -A28 -B8 'normalizeChoiceDefaults' ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginTaskEditPage.tsx
rg -n -A18 -B8 'function formatCellValue|const formatCellValue' ui/packages/sep/framework/src/components/SchemaListView/SchemaListView.tsx
rg -n -A24 -B8 'function extractId|const extractId' ui/packages/sep/framework/src/utils/extractId.ts
rg -n -A18 -B8 'SEP_ATW_PATH|SEP_MYSQL_BACKUPS_PATH' ui/apps/pmm/src
rg -n -A16 -B8 'roles=|isPMMAdmin' ui/apps/pmm/src/sep/SepPage.tsxLength of output: 3144 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Local checkout ==='
git rev-parse HEAD
git status --short --branch
git log -1 --oneline
echo '=== Pull request head ==='
gh pr view 5653 --repo percona/pmm --json headRefName,headRefOid,updatedAt,url \
--jq '.headRefName + " " + .headRefOid + " " + .updatedAt + " " + .url'
echo '=== Reported change indicators in the current checkout ==='
rg -n -A12 -B5 '_onRefreshed|refreshAccessToken' ui/packages/sep/api/src/client.ts || true
rg -n -A16 -B5 'handleSubmitEvent|handleFormSubmit' ui/packages/sep/framework/src/components/SchemaFormRenderer/SchemaFormRenderer.tsx || true
rg -n -A22 -B5 'normalizeChoiceDefaults' ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginTaskEditPage.tsx || true
rg -n -A12 -B5 'function extractId|const extractId' ui/packages/sep/framework/src/utils/extractId.ts || true
rg -n -A12 -B5 'SEP_ATW_PATH|SEP_MYSQL_BACKUPS_PATH' ui/apps/pmm/src || true
rg -n -A12 -B5 'roles=|isPMMAdmin' ui/apps/pmm/src/sep/SepPage.tsx || trueLength of output: 11212
The checkout matches PR head The inspected files still contain the prior implementations:
I cannot mark these findings as addressed on the current PR head. Please push the commit containing the fixes, or update the PR head, and request another verification. You are interacting with an AI system. |
|
Sorry — the commit was local at the time. Pushed now as Head is now @coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/packages/sep/framework/src/utils/extractId.ts (1)
30-43: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate direct numeric IDs as safe integers.
The numeric branch accepts
1.5and unsafe integers. The string branch rejects equivalent invalid IDs.useResolvedServiceFieldthen enables a lookup for the returned value.Require
Number.isSafeInteger(value)in the numeric branch. Add tests forextractId(1.5)and an unsafe integer.Proposed fix
export function extractId(value: unknown): number | null { - if (typeof value === 'number' && Number.isFinite(value)) { - return value; + if (typeof value === 'number') { + return Number.isSafeInteger(value) ? value : null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/utils/extractId.ts` around lines 30 - 43, Update extractId so the numeric branch only returns direct values that satisfy Number.isSafeInteger, matching the existing string parsing rules. Keep the current finite-number and trimmed-string handling in extractId intact, but reject 1.5 and unsafe integers before useResolvedServiceField can treat them as resolvable IDs. Add coverage for extractId(1.5) and an unsafe integer to confirm null is returned.ui/packages/sep/framework/src/components/ScheduledTasksPanel/ScheduledTasksPanel.tsx (1)
89-105: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winThe
kwargspreservation logic requires defensive wire-format validation.The code already acknowledges a backend schema gap in its comment. The issue remains valid: if the Tasks API returns
kwargsas a JSON object rather than a string, line 100's checktypeof rawKwargs === 'string'fails silently, and the toggle sends'{}', overwriting the task's arguments without error.The OpenAPI schema declares
kwargsabsent fromPeriodicTaskResponse, yet the code prepares for its presence. The actual runtime shape ofkwargs(if returned) must be verified directly from live API traffic or backend implementation, not schema alone, because the schema gap creates this exact mismatch.The broader issue remains:
handleToggleEnabledperforms a full-replacementPeriodicTaskUpdateusing fields fromPeriodicTaskResponse. Every field the response omits or returns in an unexpected shape—not onlykwargs—gets overwritten. Consider whether the backend can provide a partial-update route for the enabled toggle, or confirm that the response always carries all required fields in the expected types.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/components/ScheduledTasksPanel/ScheduledTasksPanel.tsx` around lines 89 - 105, Update the `handleToggleEnabled` update assembly in `ScheduledTasksPanel` so `kwargs` is validated against the actual runtime shape returned by the Tasks API, not just string-typed assumptions. Preserve `kwargs` when the response includes a usable value in its real wire format, and only fall back to `'{}'` when the field is truly missing or unusable. Keep the `PeriodicTaskUpdate` construction path intact, but verify other fields sourced from `PeriodicTaskResponse` are not being blindly overwritten by unexpected shapes from the response.
🧹 Nitpick comments (4)
ui/packages/plugins/atw/tests/ResultsPane.test.tsx (1)
719-727: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBind the fixture page size to
ATW_PAGE_SIZE.
pagesByOffsethardcodeslimit: 20, and the callers key their pages at offsets0and20.ResultsPanerequestslimit: ATW_PAGE_SIZE. If that constant ever changes, the requested offset stops matching the fixture keys,pages[offset]falls back to[], and the cross-page tests fail for a reason unrelated to the behavior they cover. Import the constant and derive the offsets from it.♻️ Proposed refactor to derive the offsets
-/** Serve `pages` keyed by offset, so a case can select across a page flip. */ -function pagesByOffset(pages: Record<number, unknown[]>, total: number) { - return (offset: number) => ({ - items: pages[offset] ?? [], - total, - offset, - limit: 20, - }); -} +/** Serve `pages` keyed by page index, so a case can select across a page flip. */ +function pagesByOffset(pages: Record<number, unknown[]>, total: number) { + return (offset: number) => ({ + items: pages[offset / ATW_PAGE_SIZE] ?? [], + total, + offset, + limit: ATW_PAGE_SIZE, + }); +}Then key the callers by page index, for example
{ 0: [FINISHED_EXECUTION], 1: [SECOND_PAGE_EXECUTION] }.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/plugins/atw/tests/ResultsPane.test.tsx` around lines 719 - 727, Update pagesByOffset to import and use ATW_PAGE_SIZE for its limit, and change the cross-page fixture callers to key pages by page index rather than hardcoded offsets. Convert the selected page index to an offset using ATW_PAGE_SIZE before looking up the fixture, preserving the existing cross-page test behavior when the page size changes.ui/packages/sep/framework/src/components/SchemaFormRenderer/SchemaFormRenderer.test.tsx (1)
1404-1440: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the spy even when the assertion fails.
removeEventListener.mockRestore()on Line 1439 runs only on the success path. If the assertion on Line 1435 fails, the spy onwindow.removeEventListenersurvives into the remaining tests in this file. Move the restore intoafterEach, or usevi.restoreAllMocks()in a teardown hook, so a single failure does not cascade into unrelated failures.The test itself targets the right invariant: a blocked submit must not set
isSubmitSuccessfuland must not letuseUnsavedChangesGuarddrop itsbeforeunloadlistener while the form is dirty.♻️ Proposed refactor to restore in teardown
- removeEventListener.mockClear(); + removeEventListener.mockClear(); + onTestFinished(() => removeEventListener.mockRestore()); await user.click(screen.getByRole('button', { name: /Run/ })); @@ expect(removeEventListener).not.toHaveBeenCalledWith( 'beforeunload', expect.any(Function) ); - removeEventListener.mockRestore(); });
onTestFinishedis imported fromvitest. AnafterEach(() => vi.restoreAllMocks())at the describe level works equally well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/components/SchemaFormRenderer/SchemaFormRenderer.test.tsx` around lines 1404 - 1440, Ensure the removeEventListener spy in the blocked-submit test is always restored, including when an assertion fails. Move cleanup from the test body into an afterEach teardown using vi.restoreAllMocks(), or the equivalent test-finished hook, while preserving the existing assertion and test behavior.ui/packages/sep/api/src/client.ts (1)
157-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLeave a trace when the refresh handler throws.
The isolation is correct. A throwing
_onRefreshedmust not reject the shared promise and log out every awaiting caller whose cookie rotation already succeeded on the backend.The silent catch has one cost. If
_onRefreshedthrows, the auth layer never records the new token or its expiry, yetrefreshAccessTokenreturns the token as though it were fully applied. The application then runs with inconsistent auth state and no diagnostic signal. Log the handler failure at error level, and never includedata.access_tokenordata.expires_inin that log.♻️ Proposed refactor to record the handler failure
try { _onRefreshed(data.access_token, data.expires_in); - } catch { + } catch (handlerError) { // A throwing auth-layer handler must not invalidate a cookie rotation // that already succeeded on the backend: it would reject the shared // promise and force-logout every awaiting caller. + // Recorded without the token so the inconsistent auth state is + // diagnosable. + console.error( + 'SEP auth: onRefreshed handler threw; token was not recorded', + handlerError + ); }Use this file's existing development logger instead of
console.errorif it is available at this scope.As per coding guidelines: "Do not log, hardcode, or commit secrets, credentials, tokens, S3 keys, TLS material, or debug output".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/api/src/client.ts` around lines 157 - 163, The try/catch around _onRefreshed in the refreshAccessToken flow is swallowing handler failures without any diagnostic signal. Update the catch path to log the thrown error at error level using the existing development logger available in client.ts, and keep the log free of data.access_token and data.expires_in or any other secret values. Preserve the current behavior of not rejecting the shared refresh promise or changing the returned token flow; only add the failure trace in the _onRefreshed handling.Source: Coding guidelines
ui/packages/sep/framework/src/hooks/useExecutionEvents.ts (1)
249-269: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueThe
onerrorreturn contract is correct, but remove the ineffectivectrl.abort()call on line 268.I have confirmed the three return behaviors against the 2.0.1 library documentation:
- Return
undefineduses the default retry interval ✓- Return
0triggers an immediate retry ✓- Throw an error stops the retry loop permanently ✓
The counter logic is correct: failures 1 through 4 retry silently, and the 5th failure sets
sseError, clearssseLoading, setsterminatedCleanlybefore throwing. This order correctly preventsonclosefrom overwriting the error message.However,
ctrl.abort()on line 268 is ineffective. The library creates a new internalAbortControllerfor each retry attempt, and callingabortwithin theonerrorcallback does not reliably stop the retry loop due to race conditions. The library's documented, reliable way to halt retries is thethrow errstatement on line 269. Remove line 268 to eliminate misleading code that does not accomplish its apparent intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/sep/framework/src/hooks/useExecutionEvents.ts` around lines 249 - 269, Remove the ineffective ctrl.abort() call from useExecutionEvents’s onerror retry-failure path and rely on the existing throw err to stop retries. Keep the current transientFailures/MAX_TRANSIENT_STREAM_FAILURES logic, the sseError and sseLoading updates, and the terminatedCleanly assignment in the same final-failure branch. Ensure the change is limited to the onerror handler in useExecutionEvents so the documented return/throw contract remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@ui/packages/sep/framework/src/components/ScheduledTasksPanel/ScheduledTasksPanel.tsx`:
- Around line 89-105: Update the `handleToggleEnabled` update assembly in
`ScheduledTasksPanel` so `kwargs` is validated against the actual runtime shape
returned by the Tasks API, not just string-typed assumptions. Preserve `kwargs`
when the response includes a usable value in its real wire format, and only fall
back to `'{}'` when the field is truly missing or unusable. Keep the
`PeriodicTaskUpdate` construction path intact, but verify other fields sourced
from `PeriodicTaskResponse` are not being blindly overwritten by unexpected
shapes from the response.
In `@ui/packages/sep/framework/src/utils/extractId.ts`:
- Around line 30-43: Update extractId so the numeric branch only returns direct
values that satisfy Number.isSafeInteger, matching the existing string parsing
rules. Keep the current finite-number and trimmed-string handling in extractId
intact, but reject 1.5 and unsafe integers before useResolvedServiceField can
treat them as resolvable IDs. Add coverage for extractId(1.5) and an unsafe
integer to confirm null is returned.
---
Nitpick comments:
In `@ui/packages/plugins/atw/tests/ResultsPane.test.tsx`:
- Around line 719-727: Update pagesByOffset to import and use ATW_PAGE_SIZE for
its limit, and change the cross-page fixture callers to key pages by page index
rather than hardcoded offsets. Convert the selected page index to an offset
using ATW_PAGE_SIZE before looking up the fixture, preserving the existing
cross-page test behavior when the page size changes.
In `@ui/packages/sep/api/src/client.ts`:
- Around line 157-163: The try/catch around _onRefreshed in the
refreshAccessToken flow is swallowing handler failures without any diagnostic
signal. Update the catch path to log the thrown error at error level using the
existing development logger available in client.ts, and keep the log free of
data.access_token and data.expires_in or any other secret values. Preserve the
current behavior of not rejecting the shared refresh promise or changing the
returned token flow; only add the failure trace in the _onRefreshed handling.
In
`@ui/packages/sep/framework/src/components/SchemaFormRenderer/SchemaFormRenderer.test.tsx`:
- Around line 1404-1440: Ensure the removeEventListener spy in the
blocked-submit test is always restored, including when an assertion fails. Move
cleanup from the test body into an afterEach teardown using
vi.restoreAllMocks(), or the equivalent test-finished hook, while preserving the
existing assertion and test behavior.
In `@ui/packages/sep/framework/src/hooks/useExecutionEvents.ts`:
- Around line 249-269: Remove the ineffective ctrl.abort() call from
useExecutionEvents’s onerror retry-failure path and rely on the existing throw
err to stop retries. Keep the current
transientFailures/MAX_TRANSIENT_STREAM_FAILURES logic, the sseError and
sseLoading updates, and the terminatedCleanly assignment in the same
final-failure branch. Ensure the change is limited to the onerror handler in
useExecutionEvents so the documented return/throw contract remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b4c0fd6-e168-499e-8aca-d24b2e337e13
📒 Files selected for processing (34)
ui/apps/pmm/src/contexts/navigation/navigation.utils.tsxui/apps/pmm/src/lib/constants.tsui/apps/pmm/src/router.tsxui/apps/pmm/src/sep/SepPage.tsxui/apps/pmm/src/sep/bootstrap.tsui/apps/pmm/vite.config.tsui/packages/plugins/atw/src/ResultsPane.tsxui/packages/plugins/atw/tests/ResultsPane.test.tsxui/packages/sep/api/src/auth.tsui/packages/sep/api/src/client.tsui/packages/sep/framework/src/components/HostSelector/HostSelector.tsxui/packages/sep/framework/src/components/HostSelector/StandaloneHostSelector.test.tsxui/packages/sep/framework/src/components/HostSelector/StandaloneHostSelector.tsxui/packages/sep/framework/src/components/RemoteChoiceSelector/RemoteChoiceSelector.test.tsxui/packages/sep/framework/src/components/RemoteChoiceSelector/RemoteChoiceSelector.tsxui/packages/sep/framework/src/components/ScheduledTasksPanel/ScheduledTasksPanel.tsxui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginDetailPage.tsxui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginListPage.tsxui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginTaskEditPage.test.tsxui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginTaskEditPage.tsxui/packages/sep/framework/src/components/SchemaDrivenPlugin/SchemaDrivenPlugin.tsxui/packages/sep/framework/src/components/SchemaFormRenderer/SchemaFormRenderer.test.tsxui/packages/sep/framework/src/components/SchemaFormRenderer/SchemaFormRenderer.tsxui/packages/sep/framework/src/components/SchemaFormRenderer/fields/FileField.tsxui/packages/sep/framework/src/components/SchemaFormRenderer/hooks/useCascadingField.tsui/packages/sep/framework/src/components/SchemaFormRenderer/utils/validationMapper.tsui/packages/sep/framework/src/components/SchemaListView/SchemaListView.test.tsxui/packages/sep/framework/src/components/SchemaListView/SchemaListView.tsxui/packages/sep/framework/src/hooks/useExecutionEvents.tsui/packages/sep/framework/src/hooks/useResolvedServiceField.tsui/packages/sep/framework/src/hooks/useTaskLogs.tsui/packages/sep/framework/src/utils/extractId.test.tsui/packages/sep/framework/src/utils/extractId.tsui/turbo.json
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
percona/pmm-qa(manual)percona/pmm(manual)
🚧 Files skipped from review as they are similar to previous changes (19)
- ui/apps/pmm/vite.config.ts
- ui/apps/pmm/src/contexts/navigation/navigation.utils.tsx
- ui/apps/pmm/src/router.tsx
- ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginTaskEditPage.test.tsx
- ui/packages/sep/framework/src/components/SchemaFormRenderer/hooks/useCascadingField.ts
- ui/packages/sep/api/src/auth.ts
- ui/packages/sep/framework/src/components/SchemaDrivenPlugin/SchemaDrivenPlugin.tsx
- ui/packages/sep/framework/src/components/HostSelector/StandaloneHostSelector.test.tsx
- ui/packages/sep/framework/src/components/SchemaFormRenderer/SchemaFormRenderer.tsx
- ui/packages/sep/framework/src/components/RemoteChoiceSelector/RemoteChoiceSelector.tsx
- ui/packages/sep/framework/src/components/SchemaFormRenderer/fields/FileField.tsx
- ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginListPage.tsx
- ui/packages/sep/framework/src/components/SchemaListView/SchemaListView.tsx
- ui/packages/sep/framework/src/components/SchemaFormRenderer/utils/validationMapper.ts
- ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginTaskEditPage.tsx
- ui/packages/sep/framework/src/components/HostSelector/HostSelector.tsx
- ui/packages/sep/framework/src/hooks/useTaskLogs.ts
- ui/packages/plugins/atw/src/ResultsPane.tsx
- ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginDetailPage.tsx
CodeRabbit reviewed the PMM side again (percona/pmm#5653) after the first round landed there; these are the follow-ups, ported here for the same reason as the original set. - `extractId`: hold the numeric branch to the same bar as the string one. `Number.isSafeInteger` rejects 1.5 and unsafe integers, which previously passed straight through and enabled a service lookup nothing can satisfy — the exact asymmetry the string branch was tightened to close. - `ScheduledTasksPanel`: the preserved `kwargs` now accepts either wire shape. `PeriodicTaskResponse` does not declare the field, so a decoded object is as likely as a JSON string, and only the string case was kept — the object case fell back to '{}' and wiped the arguments this guard exists to protect. A partial-update route for the toggle would remove the whole class of problem; that is a backend change. - `client.ts`: log when the injected `_onRefreshed` handler throws. The isolation is right, but swallowing it silently left the auth layer without the rotated token while `refreshAccessToken` returned it as applied, with no diagnostic. The trace carries neither the token nor its expiry. - `SchemaFormRenderer` test: restore the `removeEventListener` spy through `onTestFinished`, so a failing assertion cannot leak the spy into the rest of the file. - `HostSelector` had the same wedge as `StandaloneHostSelector`, in all three of its branches: the control was disabled on a hosts-query failure while `onOpen` held the only `refetch()` trigger, so one failure blocked recovery until the page remounted. Not flagged by the review — the diff hunk did not reach those lines — but the same defect, so it is fixed here rather than left behind. Declined, with the reasoning recorded on the PMM PR: removing `ctrl.abort()` from `useExecutionEvents`' terminal path. The claim was that the library creates a fresh `AbortController` per retry, so aborting ours is 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. Every other terminal path in this file (`finish`, `sep-error`) stops the stream the same way, so the call stays. Gates: type-check clean across every package, oxlint 0 errors, 1178 tests pass (743 in @sep/framework), oxfmt clean.
|
Second pass addressed in Applied
Declined
Two extra fixes in the same commit, same defect class as the reviewed ones, found while porting this to SEP:
Gates: check-types clean, oxlint 0 errors, 1105 tests pass, oxfmt clean. |
Brings SEP's frontend packages into ui/ and mounts the migrated plugins as native PMM routes, so SEP surfaces render inside the PMM shell instead of an iframe. Builds on PMM-15288, which moved the workspace to pnpm and the library versions SEP's code targets. Packages, ported from SEP's frontend workspace: - packages/sep/api — typed API client, generated OpenAPI surfaces, hooks - packages/sep/framework — schema-driven form/list/task components - packages/sep/shared — shared primitives - packages/plugins/atw — Collect Diagnostic Data (ATW) SEP's "app" vocabulary is renamed to "plugin" throughout the port, since "app" already means a workspace app in ui/: SchemaDrivenApp -> SchemaDrivenPlugin, useAppSchema -> usePluginSchema, useAppTasks -> usePluginTasks, app-schema.ts -> plugin-schema.ts. Ported files also carry PMM's AGPL header. Wiring in apps/pmm: - router.tsx mounts the plugins under their own routes; SepPage gives them the standard PMM Page chrome (padding, width, auth gate, footer). - navigation gates the SEP entries behind admin + the inventory settings flag. - main.tsx calls initSepAuth, which points SEP's axios client at PMM's session. Auth is still the interim Option D: the dev proxy injects SEP_INTERNAL_TOKEN server-side, so no token reaches the browser. - vite.config.ts proxies SEP's paths (/api, /sep_app, /stream-logs, /execution-events, /files) to SEP_BACKEND_URL, and lets PMM_SERVER_URL override the PMM target. - A SyntaxHighlighter component backs the schema renderer's script/JSON fields. Page gains maxWidth so SEP pages can opt into the full-width container from @percona/percona-ui; Settings.tsx moves to it in place of the removed fullWidth flag. *.tsbuildinfo is gitignored; one had been committed by accident. Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
Catches the migrated packages up with SEP's frontend, which moved on after the initial port. Ported commit by commit rather than by copying files, so PMM's app -> plugin rename and license headers survive. - SEP-1629 / SEP-1684 / SEP-1689 / SEP-1696 / SEP-1668: refresh the generated OpenAPI surface (specs/*.json + src/generated/*.ts) from SEP head. Two schema components are now namespaced — ConnectivityWarning and TaskExecuteWrite became framework__ConnectivityWarning and framework__TaskExecuteWrite — so their consumers move with them. - SEP-1663: honor HostRef/HostField `allow_custom`. HostField passes it to HostSelector, which renders FreeSoloSelect instead of the closed AutoCompleteInput and commits a scalar id/string (including from cascade auto-select). FreeSoloSelect resolves a stored string against option ids, not just labels, so string host ids like "nomad-1" display as their option. SEP's multi-host half (FreeSoloMultiSelect, MultiHostField) is not ported — PMM's snapshot has no multi-host selector to extend. - SEP-1653: hide the task-history Download files button unless the files API returns a non-empty listing. `has_logs` was the wrong signal: logs exist even when the output dir holds only the hidden .sep-run-result.json marker, which left a dead download action. Probes are cached for 30s so the history table's poll loop does not re-hit the files API every tick. - SEP-1692: add postSession / postSessionExchange to @sep/api. The exchange endpoint trades PMM's session cookie for a short-lived SEP bearer, which is what replaces the interim SEP_INTERNAL_TOKEN wiring — that token's service principal hardcodes is_admin = False and so 403s every admin-gated surface. Only the client surface lands here; flipping bootstrap.ts over to it needs a SEP backend carrying the endpoint and is left to its own change. Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
The interim SEP dev proxy could never see SEP_INTERNAL_TOKEN, so every call to the SEP backend from the migrated pages returned 401. Two independent faults: turbo.json declared no passThroughEnv, and Turbo 2.x defaults to envMode strict, which strips undeclared variables before spawning a task. vite was therefore started without the variable no matter how it was exported. Declare the three variables vite.config.ts reads. Vite exposes .env files to client code as import.meta.env but never to the config file's own process.env, so the only working setup was an export in the exact shell launching the dev server, and anything else fell back to the defaults silently. Load the files explicitly with loadEnv, keeping real environment variables ahead of file values so CI and the devcontainer are unaffected. PMM_SERVER_URL was broken the same way and is fixed by the same change. Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
…ch-up SEP moved on again after the previous sync. Of the eleven commits touching the mirrored packages, nine were already carried; the two that were not both ship frontend code. The rest are spec/generated-only or belong to apps PMM has not migrated. - SEP-1666: page-scoped select-all on the ATW Results pane. The header toggle reuses `isSelectable`, so it can never disagree with the row checkboxes about which rows are eligible, and deselecting removes only the current page's ids — the selection deliberately outlives a page flip. - SEP-1631: make a cascading RemoteChoices field usable with `allow_custom`. Free-text entry no longer needs the parent (or a successful fetch) before it is typable, a typed value survives the parent being set afterwards, and `required` now rejects whitespace-only input. SchemaFormRenderer defaults a `remote_choice` field to `null` rather than `''`, which the backend's NonEmptyStr rejects. One deviation from SEP: SEP-1666's test asserts `aria-checked="mixed"` on the select-all toggle, which MUI only emits after 7.3.7 — the version this repo's lockfile resolves, against SEP's 7.3.11. The helper here also accepts the `data-indeterminate` attribute MUI documents for 7.3.7, so the case covers the same state on both. PMM-15296 tracks bumping MUI and dropping the shim. Gates (node 22): @sep/plugins-atw 55 and @sep/framework 610 tests pass, check-types clean across framework/atw/ui, oxfmt clean tree-wide.
Correctness / stability: - client.ts: isolate a throwing _onRefreshed so a successful cookie rotation is not reported as a failed refresh. - SchemaFormRenderer: gate a section-violation submit before react-hook-form runs, so the unsaved-changes guard stays armed. - PluginTaskEditPage: normalize choice defaults through getAtPath / setAtPath so dotted one-of field names are covered. - PluginListPage / PluginDetailPage: guard the optional list_view before dereferencing it on an unresolved entity route. - SchemaListView: treat undefined cell values like null (em dash). - HostSelector: path-aware error lookup for dotted field names. - extractId: accept only decimal integers. - validationMapper: reject non-finite and fractional numeric input instead of silently truncating it. - useTaskLogs: keep stepless log lines (step: '') instead of dropping them. - useExecutionEvents: bound transient stream retries and surface the failure instead of reconnecting forever in the loading state. - StandaloneHostSelector: stay enabled on a failed hosts query so onOpen can retry. - ScheduledTasksPanel: preserve kwargs on an enable/disable toggle. - useCascadingField: use '' as the form's empty-value sentinel. Contract / conventions: - SepPage: enforce the PMM-admin gate at the route wrapper, not only in the navigation. - SchemaDrivenPlugin: pass capabilities / submitError / fieldErrors to the renderEditForm slot, matching PluginTaskEditPage. - Share SEP route constants between the router and the nav builder. - Rename the dev-only proxy variables to PMM_DEV_SEP_BACKEND_URL and PMM_DEV_SEP_INTERNAL_TOKEN. - SchemaListView: mode-aware opaque table surface instead of common.white. - FileField: give the file picker button an accessible name. - useResolvedServiceField: expose the services fetch error. - Delete the orphaned framework test/setup.ts (no afterEach(cleanup)). Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
- extractId: hold the numeric branch to the same bar as the string one.
`Number.isSafeInteger` rejects 1.5 and unsafe integers, which previously
passed straight through and enabled a service lookup nothing can satisfy.
- ScheduledTasksPanel: the preserved `kwargs` now accepts either wire
shape. `PeriodicTaskResponse` does not declare the field, so a decoded
object was as likely as a JSON string, and only the string case was
kept — the object case fell back to '{}' and wiped the arguments it was
added to protect.
- client.ts: log when the injected `_onRefreshed` handler throws. The
isolation is right, but swallowing it silently left the auth layer
without the rotated token while this function returned it as applied,
with no diagnostic. The trace carries neither token nor expiry.
- SchemaFormRenderer test: restore the `removeEventListener` spy through
`onTestFinished` so a failing assertion cannot leak it into the rest of
the file.
Two fixes of the same class as the reviewed ones, found while porting
this to SEP:
- HostSelector had the same wedge as StandaloneHostSelector: both its
free-solo and standard branches disabled the control on a hosts-query
failure while `onOpen` held the only `refetch()` trigger, so one failure
blocked recovery until the page remounted.
- SchemaListView.formatCellValue now renders an absent `date` / `relative`
value as an empty cell instead of an em dash, adopting SEP's variant of
this guard: an em dash for a task that has never run reads as a
placeholder for a time that exists. Keeps the mirrored file aligned with
its source.
Declined, with reasons on the PR: removing `ctrl.abort()` from
useExecutionEvents' terminal path (the library disposes on the signal we
pass — its own controller is separate — and every other terminal path here
aborts the same way), and binding the ATW ResultsPane fixture to
ATW_PAGE_SIZE (trivial, and that file mirrors SEP's).
Gates: check-types clean, oxlint 0 errors, 1105 tests pass, oxfmt clean.
Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
`Number(' ')` is 0, so a numeric field containing only whitespace passed
the validate rule and `coerceFormValues` submitted 0 for a value the user
never typed. RHF's built-in `required` rule does not fire on it either,
since the string is non-empty.
Both the validate rule and the coercion now trim string input and treat a
trimmed-empty string as empty: required fields report the required error,
optional fields serialise as absent.
Ported from SEP-1760 (percona/SEP#1283, 4bf7adcc4), where a Copilot review
caught it on the same code.
Signed-off-by: Ignacio Durand <nachodurand@gmail.com>
) The CodeRabbit review of the SEP-UI-into-PMM migration PR ([percona/pmm#5653](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](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.
Ticket number: PMM-15216
Feature build: SUBMODULES-0
What
Brings SEP's frontend into
ui/and mounts the migrated plugins as native PMM routes, so SEP surfaces render inside the PMM shell instead of an iframe.Packages ported from SEP's frontend workspace
ui/packages/sep/apiui/packages/sep/frameworkui/packages/sep/sharedui/packages/plugins/atwSEP's "app" vocabulary is renamed to "plugin" throughout the port, since "app" already means a workspace app in
ui/:SchemaDrivenApp→SchemaDrivenPlugin,useAppSchema→usePluginSchema,useAppTasks→usePluginTasks,app-schema.ts→plugin-schema.ts. Ported files also carry PMM's AGPL header.Wiring in
apps/pmmrouter.tsxmounts the plugins under their own routes;SepPagegives them the standard PMMPagechrome (padding, width, auth gate, footer).main.tsxcallsinitSepAuth, pointing SEP's axios client at PMM's session. Auth is still the interim Option D: the dev proxy injectsSEP_INTERNAL_TOKENserver-side, so no token reaches the browser.vite.config.tsproxies SEP's paths (/api,/sep_app,/stream-logs,/execution-events,/files) toSEP_BACKEND_URL, and letsPMM_SERVER_URLoverride the PMM target.SyntaxHighlightercomponent backs the schema renderer's script/JSON fields.PagegainsmaxWidthso SEP pages can opt into the full-width container from@percona/percona-ui;Settings.tsxmoves to it in place of the removedfullWidthflag.SEP catch-up (second commit)
SEP moved on after the initial port. Ported commit by commit rather than by copying files, so the app→plugin rename and license headers survive:
ConnectivityWarning→framework__ConnectivityWarning,TaskExecuteWrite→framework__TaskExecuteWrite; their consumers move with them.HostRef/HostField.allow_custom.HostFieldpasses it toHostSelector, which rendersFreeSoloSelectinstead of the closedAutoCompleteInputand commits a scalar id/string (including from cascade auto-select).FreeSoloSelectnow resolves a stored string against option ids, not just labels, so string host ids like"nomad-1"display as their option.has_logswas the wrong signal: logs exist even when the output dir holds only the hidden.sep-run-result.jsonmarker, leaving a dead download action. Probes are cached for 30s so the history table's poll loop doesn't re-hit the files API every tick.postSession/postSessionExchangeto@sep/api. The exchange endpoint trades PMM's session cookie for a short-lived SEP bearer, which is what replaces the interimSEP_INTERNAL_TOKENwiring (that token's service principal hardcodesis_admin = Falseand so 403s every admin-gated surface).Deliberately not ported
FreeSoloMultiSelect,MultiHostField) — PMM's snapshot has no multi-host selector to extend. Porting it means first porting SEP's multi-host feature, which is its own change.bootstrap.tsto the token exchange (Option B) — only the@sep/apiclient surface lands here. Switching over needs a SEP backend carryingPOST /api/oauth/session/exchangeand live verification, so it gets its own ticket.Review comments addressed
postgres-migration. Please remove that code, it's a separate PR." Removed.build/ansible/roles/postgres/*,build/docker/server/entrypoint.sh,docker-compose.yml,.env.exampleandmanaged/utils/envvarsare no longer touched by either branch; those changes belong to PMM-15238: Expose the built-in PostgreSQL to SEP #5700.Also gitignores
*.tsbuildinfo; one had been committed by accident.Verification
From
ui/, on this branch (i.e. including the base PR):pnpm installmake format-check(oxfmt)make lint(oxlint)make test(vitest)make buildturbo run check-typesNot yet verified against a live PMM Server with a SEP backend attached — that needs a feature build, which is tracked below.
Related
If this PR adds, removes or alters one or more API endpoints, please review and update the relevant API documentation as well: