PMM-15216 Migrate the SEP UI into PMM - #5653
Conversation
|
Ignacio Durand seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
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.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
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 <ignacio.durand@l7informatics.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 <ignacio.durand@l7informatics.com>
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.
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 <ignacio.durand@l7informatics.com>
|
@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. |
…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.
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:
Summary by CodeRabbit
New Features
Documentation
Tests