feat(agent-platform): move the installation picker to the top of the new-agent form - #2009
Conversation
…new-agent form The Installation select sat in the middle of the "Configuration" card, after the system prompt — but both the avatar preview (in the "Identity" card above it) and the model picker depend on the selected installation. Promote it to its own card at the very top of the form so it's chosen first. When only one installation is configured for access, the picker is hidden entirely and that installation is auto-selected: customer instances wired to a single management cluster no longer see a single-option dropdown with nothing to choose. The gate reads the full `useInstallations()` list rather than the progressively-loading `availableInstallations`, so it doesn't flicker while the fleet query settles. Also extracts the form's private `SectionHeader` helper into `ui-react` so the new card can reuse it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| }; | ||
|
|
||
| /** Title + description pair used to introduce a card's contents. */ | ||
| export function SectionHeader({ title, description }: SectionHeaderProps) { |
There was a problem hiding this comment.
CI gate: this exported ui-react component has no story. plugins/ui-react enforces Storybook coverage on every PR touching plugins/ui-react/** (.github/workflows/storybook.yaml → yarn storybook:coverage). Running it on this branch fails:
ui-react story coverage: 22/23 exported components documented (0 allowlisted).
✗ These exported ui-react components have no *.stories.tsx:
- SectionHeader
Add SectionHeader.stories.tsx next to the component (using componentDocs() from src/storybook/docs.ts; migration status would be mixed — bui Text + MUI v4 makeStyles), or allowlist it with a reason in .storybook/story-coverage-allowlist.json.
There was a problem hiding this comment.
Good catch — reproduced the failure locally (22/23, SectionHeader listed). Added SectionHeader.stories.tsx in c31dcb0 rather than allowlisting it, with migration: 'mixed' as you suggested (bui Text + MUI v4 makeStyles). Three stories: Default, In a card (the intended usage, fields inside a Card), and Long description (showing the 70ch wrap).
yarn storybook:coverage now reports 23/23 ✓, and yarn typecheck:storybook + yarn storybook:build both pass locally (the story lands in the built site as SectionHeader.stories-*.js).
| } | ||
| }, [singleInstallation, state.installation, setInstallation]); | ||
|
|
||
| if (singleInstallation) { |
There was a problem hiding this comment.
The gate keys on "how many installations are configured", not on whether the one installation is usable — so the only surface that explains an unusable installation disappears.
Scenario (the exact deployment shape this PR targets: a customer instance wired to one MC): the single installation is degraded / session-expired / muted, so useReachableInstallations drops it and useResources never queries it. Consequences:
availableInstallationsis[]andunreachableInstallationsis[](no query ⇒ no errors), so nothing here would have claimed "couldn't read" anyway…- …but we now
return nullbefore thehasInstallationsbranch, so the "No installations with models" card andUnreachableInstallationsAlertnever render at all. - We still auto-select the installation, so
validationErrorsno longer contains "Select an installation".
Net effect: the user sees only ModelConfigPicker's !hasError fallback — "No ModelConfigs are provisioned on X. A platform admin needs to add one" — which is a misdiagnosis (the cluster wasn't reachable / their session expired), and the form gives no hint that the installation is the problem. Consider gating the hide on the installation actually being usable, e.g. only skip the field when installations.length === 1 && availableInstallations.length === 1, and keep rendering the card (with the unreachable/no-models guidance) otherwise.
There was a problem hiding this comment.
Agreed, that's a real hole — the one deployment shape this PR targets is exactly where it bites. Fixed in c31dcb0.
I split the two concerns rather than gating both on the same condition:
- auto-select stays keyed on
installations.length === 1— with one configured installation there is only one possible choice, and selecting it doesn't assert it's healthy. - hiding now additionally requires it to be usable:
so when the sole installation is unreachable / model-less we fall through to the
if (singleInstallation && (isLoading || availableInstallations.length > 0)) { return null; }
hasInstallationsbranch and the card still rendersUnreachableInstallationsAlertor "No installations with models".
The isLoading term in there is what keeps this from re-introducing the flicker you flagged in the other thread: while the fleet query is settling we can't yet tell whether it's usable, so we render nothing instead of a loading card that would vanish.
Two new tests cover it: "still explains itself when the sole installation has no models" (also asserts the auto-select still fired) and "…when the sole installation is unreachable" (asserts we don't claim "no models" when the read never succeeded).
|
|
||
| export function InstallationSelect() { | ||
| const { state, setInstallation } = useNewAgentForm(); | ||
| const { installations } = useInstallations(); |
There was a problem hiding this comment.
isLoading from useInstallations() is dropped, so the single-installation case still flickers. installations is [] until the post-sign-in GET /api/gs/installations fetch resolves (installationsConfig.ts — a module-level async source, isLoading === (snapshot === undefined)). On a cold, direct load of /agent-platform/agents/new (hard reload or a pasted link) the first renders therefore have singleInstallation === undefined and availableInstallations === [] + isLoading === true, so the Installation card renders with the disabled "Finding installations with models…" select — and then vanishes once the config arrives and turns out to hold one installation. That's a visible card appear/disappear jump, i.e. the flicker the PR description says this approach avoids.
| const { installations } = useInstallations(); | |
| const { installations, isLoading: isLoadingInstallations } = useInstallations(); |
…and then treat "still loading the installations config" as "don't render the field yet" (return null, or the loading card, until it resolves).
There was a problem hiding this comment.
You're right, and the PR description overclaimed. Gating on installations.length avoided the availableInstallations flicker but introduced its own via the installations === [] loading state. Took the suggestion in c31dcb0:
const { installations, isLoading: isLoadingInstallations } = useInstallations();
const singleInstallation =
!isLoadingInstallations && installations.length === 1
? installations[0].name
: undefined;
// Until the installations config resolves we don't know whether this field
// belongs on the page at all.
if (isLoadingInstallations) {
return null;
}I went with return null rather than the loading card: on a single-installation instance the field never appears at all, so rendering anything first is the appear-then-vanish jump. On a multi-installation instance the config fetch resolves before the per-installation ModelConfig queries do, so the "Finding installations with models…" card still shows for that (longer) wait — it just no longer leads it.
Also folded isLoadingInstallations into singleInstallation itself so the auto-select effect can't fire on the empty pre-fetch snapshot. Covered by "renders nothing until the installations config resolves" (asserts empty DOM and no setInstallation) and "renders nothing while the fleet query is still settling".
| <SectionHeader title="Installation" description={description} /> | ||
| <Flex direction="column" gap="2"> | ||
| <Select | ||
| label="Installation" |
There was a problem hiding this comment.
"Installation" now renders twice in the same card: once as the SectionHeader <h3> title and again as the Select's visible field label (same duplication in the loading branch at L57-58). Every other card in the form avoids this — Identity → "Name"/"Slug", Configuration → "System prompt"/"Model" — so this one reads as a stub. The new test even encodes it (getAllByText('Installation').length).toBeGreaterThan(0)).
Keep an accessible name but drop the visible duplicate, e.g. aria-label="Installation" instead of label, or drop the title from the SectionHeader and keep the field label.
There was a problem hiding this comment.
Fixed in c31dcb0 — went with your first option (aria-label), keeping the SectionHeader <h3> as the card heading so the card still matches Identity / Configuration visually.
One wrinkle worth recording: bui's FieldLabel bails with if (!label) return null, so dropping label also drops secondaryLabel and the (Required) marker. isRequired still sets aria-required, but the secondaryLabel={isLoading ? 'still checking…' : undefined} affordance would have silently disappeared — that one tells the user the list is still growing, so I moved it to its own line under the select instead:
{isLoading && (
<Text variant="body-small" color="secondary">
Still checking the remaining installations…
</Text>
)}Also pulled the three card branches behind a small local InstallationCard shell so the heading/description can't drift between them.
The test now pins the fix instead of encoding the bug:
expect(getAllByText('Installation')).toHaveLength(1); // heading only
expect(getByLabelText('Installation')).toBeInTheDocument(); // name preserved| description="The agent's system message. Pre-filled from the chart's default — edit it to fit the role, or leave it empty to keep the default." | ||
| /> | ||
| <InstallationSelect /> | ||
| <ModelConfigPicker /> |
There was a problem hiding this comment.
Stale copy after the move: the Configuration card's description (L199) still promises "…what powers the agent and shapes how it behaves: where it runs, which model it uses, its system prompt, and its skills" — but "where it runs" (the installation) has just moved out of this card into its own card above. Drop that clause so the description matches the card's contents.
There was a problem hiding this comment.
Fixed in c31dcb0 — dropped the "where it runs" clause, so the description is now "What powers the agent and shapes how it behaves: which model it uses, its system prompt, and its skills." The installation's own card carries that part of the explanation.
- Only hide the field when the sole installation is actually usable. Keying the hide on "one installation configured" meant a degraded / session-expired single MC hid the one surface that explains itself, leaving the model picker's "no ModelConfigs provisioned" fallback to misdiagnose it. The auto-select still happens as soon as the choice is knowable; the card now falls through to the unreachable / no-models guidance when it isn't usable. - Respect `isLoading` from `useInstallations()`. `installations` is `[]` until the post-sign-in fetch resolves, so a cold load of the page rendered the "Finding installations with models…" card and then dropped it once the config turned out to hold one installation — the very flicker this was meant to avoid. Render nothing until it resolves. - Stop rendering "Installation" twice (section heading + field label). The heading names the field; the controls use `aria-label` instead. The "still checking" hint moves to its own line, since bui's FieldLabel drops `secondaryLabel` when there is no visible `label`. - Drop "where it runs" from the Configuration card's description — that moved out to its own card. - Add SectionHeader.stories.tsx: plugins/ui-react enforces Storybook coverage on every exported component (scripts/check-story-coverage.mts). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
What does this PR do?
Reorders the "Create an agent" form (
/agent-platform/agents/new) so theInstallation picker comes first, in its own card ahead of Identity and
Configuration. It previously sat in the middle of the Configuration card,
after the system prompt — even though both the avatar preview (rendered in the
Identity card above it) and the model picker depend on which installation
is selected.
Additionally, the picker is hidden and the sole installation auto-selected
when the instance has only one installation configured and it's usable — so
customer instances wired to a single management cluster don't see a one-option
dropdown with nothing to choose.
Those two conditions are deliberately separate (see review discussion):
useInstallations()) — with oneconfigured installation there is only one possible choice, and selecting it
doesn't assert it's healthy.
(
availableInstallations). If the sole installation is unreachable,session-expired, or has no kagent
ModelConfig, the card keeps rendering soUnreachableInstallationsAlert/ "No installations with models" can explainwhy the form is stuck — otherwise the model picker's "no ModelConfigs
provisioned on X" fallback would be the only feedback, and it misdiagnoses
the cause.
Both the installations-config fetch and the fleet-wide ModelConfig query are
treated as "don't render the field yet", so on a single-installation instance
the card never appears-then-vanishes.
Also extracts the form's private
SectionHeaderhelper (title + descriptionpair introducing a card) into
ui-react— with a Storybook story, per theplugins/ui-reactstory-coverage gate — so the new Installation card can reuseit rather than duplicating it inside
agent-platform.What is the effect of this change to users?
depend on it, instead of part-way down.
single-option "Installation" dropdown with nothing to choose — it disappears
and the sole installation is selected automatically.
says so rather than silently blaming missing ModelConfigs.
How does it look like?
The form now reads: Installation → Identity → Configuration →
actions. Verified locally against a two-installation dev config:
setInstallationbehaviour) — the Model section switches from "Select aninstallation first" to the installation's ModelConfigs.
https://avatars.$BASE_DOMAIN/v1/preview/128/go-service-reviewer.png.The card heading names the field, so the select uses
aria-labelrather than avisible label that would print "Installation" twice.
Any background context you can provide?
Follow-up polish on the Agent Platform agent-creation flow. The
single-installation case matters for customer Backstage instances that are only
connected to one management cluster.
Do the docs need to be updated?
No.
Should this change be mentioned in the release notes?