FLPATH-4773: Reflect change introduced by environment agent to UI - #4464
FLPATH-4773: Reflect change introduced by environment agent to UI#4464asmasarw wants to merge 2 commits into
Conversation
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Missing ChangesetsThe following package(s) are changed by this PR but do not have a changeset:
See CONTRIBUTING.md for more information about how to add changesets. Changed Packages
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4464 +/- ##
==========================================
+ Coverage 58.62% 58.65% +0.03%
==========================================
Files 2547 2547
Lines 101638 101668 +30
Branches 28588 28583 -5
==========================================
+ Hits 59585 59637 +52
+ Misses 40252 40229 -23
- Partials 1801 1802 +1
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
asmasarw
changed the title
mareklibra
left a comment
mareklibra
left a comment
There was a problem hiding this comment.
A few comments, nothing serious.
| const crud = usePaginatedCrudTab<Agent, AgentForm>({ | ||
| loadFn: ({ pageToken, pageSize: ps }) => | ||
| agentsApi | ||
| .listAgents({ page_token: pageToken, max_page_size: ps }) |
There was a problem hiding this comment.
The PR description and changeset say this tab supports health-status filtering, but listAgents is only called with pagination params and there is no filter UI.
Please either:
- Add a health-status filter (and pass
health_statusintolistAgents), or - Drop that claim from the changeset / PR body so the shipped behavior matches the docs.
The client already supports the query param, so wiring a select/chips control should be small.
| ); | ||
| } | ||
|
|
||
| describe('AgentsTabContent', () => { |
There was a problem hiding this comment.
The PR description and changeset say this tab supports health-status filtering, but listAgents is only called with pagination params and there is no filter UI.
Please either:
- Add a health-status filter (and pass
health_statusintolistAgents), or - Drop that claim from the changeset / PR body so the shipped behavior matches the docs.
The client already supports the query param, so wiring a select/chips control should be small.
| @@ -0,0 +1,14 @@ | |||
| --- | |||
| '@red-hat-developer-hub/backstage-plugin-dcm': minor | |||
There was a problem hiding this comment.
Is there already any production use of the DCM?
This removes @public exports (ProvidersApi, ProvidersClient, providersApiRef, providersRouteRef) from packages already at v1.0.0. That is a breaking change.
Please bump both packages to major unless DCM 1.0.0 is explicitly unpublished / internal-only and that is documented in the changeset.
Also please mention the removed public symbols in the changeset so downstream consumers are not surprised.
| const catalogApi = useApi(catalogApiRef); | ||
| const { t } = useTranslation(); | ||
|
|
||
| const { |
There was a problem hiding this comment.
useInfiniteSelect already exposes loading for the first page, but it is unused. AgentFormFields only spins on loadingMoreServiceTypes, so the first fetch looks like “no service types”.
Please pass loading through and show the same CircularProgress MenuItem while the first page is in flight.
| }, | ||
| instances: { | ||
| emptyTitle: 'No instances provisioned', | ||
| emptyDescription: |
There was a problem hiding this comment.
instances.emptyDescription still says “registered provider infrastructure”. With Providers removed, please reword (e.g. “registered environment agents”) and update the locale files to match.
| 'At least one service type is required', | ||
| ), | ||
| ), | ||
| cost: yup |
There was a problem hiding this comment.
cost is only .required(), so any non-empty string passes isAgentFormValid. Please constrain it to AGENT_COST_OPTIONS:
cost: yup
.string()
.oneOf(AGENT_COST_OPTIONS, m('validation.agent.costRequired', 'Cost is required'))
.required(m('validation.agent.costRequired', 'Cost is required')),| deleteProvider(providerId: string): Promise<void>; | ||
| export interface AgentsApi { | ||
| listAgents( | ||
| params?: PaginationParams & { health_status?: string }, |
There was a problem hiding this comment.
Agent.health_status is typed as AgentHealthStatus, but the list filter is still health_status?: string. Please use the same union so callers cannot pass arbitrary values:
listAgents(
params?: PaginationParams & { health_status?: AgentHealthStatus },
): Promise<AgentList>;There was a problem hiding this comment.
Same change in AgentsClient.listAgents (line 41).
| ), | ||
| }, | ||
| { | ||
| title: t('agents.columns.health'), |
There was a problem hiding this comment.
last_heartbeat is on the Agent model but never shown. A “Last heartbeat” column next to Health (or a tooltip on the badge) would make unavailable vs a stale ready easier to diagnose. Other tabs already render timestamps the same way, e.g.:
{
title: t('agents.columns.lastHeartbeat'),
field: 'last_heartbeat',
render: a =>
a.last_heartbeat ? (
<Typography variant="body2">
{new Date(a.last_heartbeat).toLocaleString()}
</Typography>
) : (
<DcmEmptyCell />
),
},
Optional: in AgentFormFields, default topic_name to dcm.agent.${name} while the user is still on the generated value, so they do not have to retype the prefix.
| onPrimaryAction={crud.handleOpenCreate} | ||
| illustrationSrc={emptyIllustration} | ||
| entityLabel={t('agents.entityLabel')} | ||
| actionError={serviceTypesError} |
There was a problem hiding this comment.
Minor consistency nit vs Providers / Catalog items: they pass onDismissActionError so the service-type load alert can be closed. Here only actionError={serviceTypesError} is set, so MUI renders a non-dismissable banner.
useInfiniteSelect has no clearError, so a local flag is enough:
const [serviceTypesErrorDismissed, setServiceTypesErrorDismissed] = useState(false);
// ...
actionError={serviceTypesErrorDismissed ? null : serviceTypesError}
onDismissActionError={() => setServiceTypesErrorDismissed(true)}
Not blocking - sticky is reasonable for a failed catalog fetch. Nice to match the other tabs.
| onPrimaryAction={crud.handleOpenCreate} | ||
| illustrationSrc={emptyIllustration} | ||
| entityLabel={t('agents.entityLabel')} | ||
| toolbarExtra={healthFilterControl} |
There was a problem hiding this comment.
Health filter on empty results is a dead end. DcmCrudTabLayout only renders toolbarExtra in the table branch; a health filter that returns 0 agents hits items.length === 0 && !hasPrev and shows the global empty state (“No agents registered”) with no way to change the filter short of a full reload.
Please either:
- Render toolbarExtra in the empty-state branch as well, and use different copy when a filter is active, or
- Skip the illustration empty state when healthFilter is set and keep the table + select visible.
resetAndReload() already clears the token stack, so hasPrev will not save this path.
There was a problem hiding this comment.
The empty-state branch now also renders toolbarExtra (with the filter Select) before the action error alert and illustration
There was a problem hiding this comment.
See AgentsTabCOntent.tsxx:200 comment
There was a problem hiding this comment.
| if (items.length === 0 && !cursorPagination?.hasPrev) { | |
| return ( | |
| <> | |
| {toolbarExtra && ( | |
| <Box className={classes.toolbarRow}>{toolbarExtra}</Box> | |
| )} | |
| {/* existing actionError + DcmDataCenterTabEmptyState */} | |
| </> | |
| ); | |
| } |
| loadMoreServiceTypes?: () => void; | ||
| }>; | ||
|
|
||
| export function AgentFormFields({ |
There was a problem hiding this comment.
The Providers form and CopyButton unit tests were deleted and not replaced. AgentsTabContent tests cover list/pagination/filter but never open the register dialog, and usePaginatedCrudTab.test.ts does not cover resetAndReload (the helper the filter depends on).
Please port the old ProviderFormFields / CopyButton cases onto AgentFormFields / CopyButton, and add a resetAndReload test that asserts pageToken is undefined and hasPrev is false after goNext().
| value: AgentHealthStatus; | ||
| label: string; | ||
| }> = [ | ||
| { value: 'ready', label: 'Ready' }, |
| title: t('agents.columns.serviceTypes'), | ||
| field: 'service_types', | ||
| sorting: false, | ||
| render: a => { |
There was a problem hiding this comment.
If service_types is [] the column renders an empty Box. Other tabs use for that. Please add the same fallback before the chip list.
|
|
||
| type CopyState = 'idle' | 'copied' | 'failed'; | ||
|
|
||
| export function CopyButton({ text }: Readonly<{ text: string }>) { |
There was a problem hiding this comment.
This CopyButton no longer swaps the icon on copied/failed (the Providers version used CheckIcon / ErrorOutlineIcon). Tooltip-only feedback is easy to miss. Please restore the icon states or move CopyButton to components/ so every tab shares one implementation.
| helperText={err('name') ?? t('agents.form.nameHelper')} | ||
| error={Boolean(err('name'))} | ||
| value={form.name} | ||
| onChange={e => setForm(prev => ({ ...prev, name: e.target.value }))} |
There was a problem hiding this comment.
while topic_name is still the generated value, default it to dcm.agent.${name} on name change so users do not have to retype the prefix. Stop updating once they edit the topic field.
There was a problem hiding this comment.
Still uses still uses Provider / listProviders / storageKey: 'providers'.
| // While false, typing in the name field auto-fills topic_name to | ||
| // `dcm.agent.<name>`. Once the user edits topic_name directly the | ||
| // auto-fill is disabled for the lifetime of this form instance. | ||
| const topicManuallyEdited = useRef(false); |
There was a problem hiding this comment.
topicManuallyEdited survives dialog close because AgentFormFields stays mounted (DcmFormDialog always renders children) while useCrudTab only resets createForm.
Repro: open Register → type a name (topic auto-fills) → edit topic → Cancel → open Register again → type a name. Topic stays empty.
Please reset the flag when the form is cleared, remount the fields (key on open), or derive “still generated” from topic_name === \dcm.agent.${name}`` instead of a ref.
| // on the wrapping Tooltip span via title attribute propagation. | ||
| // A simpler check: the error icon must NOT be present. | ||
| // After success the error icon must NOT be present | ||
| expect(screen.queryByTestId('ErrorOutlineIcon')).not.toBeInTheDocument(); |
There was a problem hiding this comment.
these cases do not actually assert the icon swap
Success never looks for CheckIcon. Failure is ErrorOutlineIcon || .MuiSvgIcon-root, which is true for the idle copy icon too.
Please assert CheckIcon after resolve and ErrorOutlineIcon after reject (and drop the .MuiSvgIcon-root fallback).
| <TabbedLayout> | ||
| <TabbedLayout.Route path="/" title={t('page.tabs.providers')}> | ||
| <ProvidersTabContent /> | ||
| <TabbedLayout.Route path="/" title={t('page.tabs.agents')}> |
There was a problem hiding this comment.
FLPATH-4773 still says rename SP → Environments and mark resources degraded when the agent is gone. This change uses “Agents” and the Resources column is still “Provider”. If that work is out of scope, please point at the follow-up issue so the story is not silently closed.
| expect(isAgentFormValid({ ...VALID_FORM, service_types: [] })).toBe(false); | ||
| }); | ||
|
|
||
| it('returns false when cost is empty', () => { |
There was a problem hiding this comment.
please add isAgentFormValid({ ...VALID_FORM, cost: 'not-a-cost' as AgentForm['cost'] }) === false so the oneOf constraint cannot regress.
f1edcf1 to
2949339
Compare
2949339 to
b79f169
Compare
|
2 participants
Replace Providers tab with Agents tab
Removes all Providers-related code and introduces a new Agents tab as the default landing tab in the DCM plugin.
dcm-commonProvidersApi,ProvidersClient,ProvidersClient.test.ts, andtypes/providers.tsAgentsApi,AgentsClient,AgentsClient.test.ts, andtypes/agents.tswith types derived from the Agent API OpenAPI spec (v1alpha1):Agent,AgentList,AgentRegistrationRequest,AgentCost,AgentHealthStatus,HeartbeatRequestdcm(frontend plugin)providersApiRef/providersRouteRefwithagentsApiRef/agentsRouteRefpages/providers/directory (all provider UI components)pages/agents/with:AgentsTabContent— paginated table with health-status filtering and agent registration dialogAgentFormFields— registration form backed by service types fetched live from the Catalog APIAgentHealthStatus— status badge componentCopyButton— reusable clipboard copy componentagentFormTypes— form state, Yup validation, and mapping helpersref.ts,de,es,fr,it,ja) to replaceproviders.*keys withagents.*keysminorbump for both packages)