Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -119,12 +119,15 @@ const ModelHarnessSectionBody = ({
section,
...params
}: {
section: "model-harness" | "advanced"
section: "model-harness" | "advanced" | "connect-key"
} & Parameters<typeof useModelHarness>[0]) => {
const mh = useModelHarness(params)
if (section === "advanced") {
return <>{mh.advancedDrawerBody}</>
}
if (section === "connect-key") {
return <>{mh.providerCredentialsSection}</>
}
return <>{mh.modelHarnessDrawerBody}</>
}

Expand Down Expand Up @@ -495,6 +498,15 @@ export const AgentTemplateControl = memo(function AgentTemplateControl({
}),
[sectionChanges.draft, onChange, config, committed],
)
// Whether this edit touched the provider/connection specifically, not just any model-harness field.
const credentialsPathChanged = useMemo(
() =>
(sectionChanges.draft.sectionsByKey.get("model-harness")?.scalarChanges ?? []).some(
(c) => c.key === "llm.provider" || c.key.startsWith("llm.connection"),
Comment thread
bekossy marked this conversation as resolved.
),
[sectionChanges.draft],
)

// The inline body for a drawer-backed section: its own controls, narrowed to what changed — the
// same affordance as the Connect-key field, with a different filter (see SectionChangeBody).
// The body is a COMPONENT rendered inside the providers, never `mh`'s pre-built output: the hook
Expand Down Expand Up @@ -814,7 +826,8 @@ export const AgentTemplateControl = memo(function AgentTemplateControl({
// What the section surfaces inline, in precedence order. Dropping `onOpen` is what makes
// a section expand inline instead of routing to the drawer.
// 1. Required info missing (no provider key) — BLOCKING, so it wins: the same key field
// the drawer uses, right here.
// the drawer uses, right here, swapped for the changed-aware variant when this edit
// touched the provider/connection (`credentialsPathChanged`) so that diff isn't lost.
// 2. Uncommitted changes — informational: what changed (see `changeBodyFor`).
// 3. Neither — the plain drawer row it has always been.
...(showKeyPane
Expand All @@ -829,7 +842,22 @@ export const AgentTemplateControl = memo(function AgentTemplateControl({
onOpenDetails={() => openSectionDrawer("model-harness")}
disabled={disabled}
>
{mh.providerCredentialsInline}
{credentialsPathChanged ? (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This drops the inline model picker from the connect-key pane.

providerCredentialsSection is the non-bare variant: it never receives modelControl, and it renders railDetail rather than topRail (ProviderCredentialsSectionView.tsx:612-653). So this branch loses the model select and swaps in the 190px side rail that the comment on topRail describes as leaving the key form cramped in a narrow column.

That hits the common path, not an edge case: the usual way into the needs-key state is a catalog model pick, which moves llm.provider, so credentialsPathChanged is true. Threading bare + modelControl into the changed-aware render would keep the picker and the compact rail alongside the new badge/indicator/revert.

<ChangedPathsProvider changes={panelChangedPaths}>
<ModelHarnessSectionBody
section="connect-key"
Comment thread
bekossy marked this conversation as resolved.
schema={schema}
config={config}
onChange={onChange}
disabled={disabled}
withTooltip={withTooltip}
revisionId={revisionId}
savedHarnessValue={savedHarnessValue}
/>
</ChangedPathsProvider>
) : (
mh.providerCredentialsInline
)}
</SectionQuickAction>
</div>
</HeightCollapse>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -422,10 +422,24 @@ export function useModelHarness({
// `harness.kind` (NOT all of `harness`) so a permissions edit doesn't light up the Harness header.
const harnessKindChanged = useHasChangedUnder("harness.kind")
const modelChanged = useHasChangedUnder("llm.model")
const credentialsChanged = useHasChangedUnder("llm.connection")
// Provider credentials cover both the connection (mode/slug) and the bare `llm.provider` field a
// catalog model switch can move on its own — track both, or a provider-only change gets a "Connect
// key" badge with no changed indicator or Restore to go with it.
const connectionChanged = useHasChangedUnder("llm.connection")
const providerFieldChanged = useHasChangedUnder("llm.provider")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

credentialsInFocus (line 468) wasn't widened along with credentialsChanged — it still checks llm.connection only.

In the non-key "what changed" branch, a provider-only diff therefore leaves harnessKindInFocus, modelInFocus and credentialsInFocus all false, and the expanded section renders nothing but the "Detailed configuration" link. That's the #5544 symptom again, in the branch this PR doesn't touch — and it's the same provider-moves-on-its-own case the comment right here describes.

const credentialsChanged = connectionChanged || providerFieldChanged
const revertHarnessKind = useRevertUnder("harness.kind")
const revertModel = useRevertUnder("llm.model")
const revertCredentials = useRevertUnder("llm.connection")
const revertConnection = useRevertUnder("llm.connection")
const revertProviderField = useRevertUnder("llm.provider")
// `writeModel` can move `llm.model` atomically with `llm.provider`/`llm.connection.slug` (a
// catalog pick clears the old custom slug) — revert all three together or the group's Restore
// reattaches a stale slug to the new model, a combination the backend rejects.
const revertCredentials = useCallback(() => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This restores only llm.model.

Each of revertConnection / revertProviderField / revertModel is onChange(revertPathsTo(config, committed, paths)) (AgentTemplateControl.tsx:497), closed over the same render-time config. Calling them in sequence is three full-config writes in one handler — last write wins, so the reverted provider and connection slug are overwritten by the model-only revert. That leaves the new provider/slug with the old model: the exact combination the comment above says the backend rejects.

Needs to be one revert() call with the union of paths, e.g. a multi-prefix useRevertUnder, or revert([...pathsUnder("llm.connection"), "llm.provider", "llm.model"]).

revertPermissions a few lines up has the same shape and the same latent bug.

revertConnection?.()
revertProviderField?.()
revertModel?.()
}, [revertConnection, revertProviderField, revertModel])
// Confirmed — see `RevertGroupButton`, which owns the confirm step.
const revertAction = (onRevert: (() => void) | null) =>
onRevert ? <RevertGroupButton onConfirm={onRevert} disabled={disabled} /> : undefined
Expand Down Expand Up @@ -668,7 +682,7 @@ export function useModelHarness({
disabled,
revisionId: revisionId ?? null,
indicator: changedIndicator(credentialsChanged),
revertControl: revertAction(revertCredentials),
revertControl: credentialsChanged ? revertAction(revertCredentials) : undefined,
}
: null
// The full pane (own header + rail) for the drawer body; the `bare` variant (toggle + key form /
Expand Down Expand Up @@ -1014,6 +1028,10 @@ export function useModelHarness({
// self-managed card; no nested header/badge, no rail) for the inline "Connect key"
// quick-action under the section header — aligned with the drawer without duplicating it.
providerCredentialsInline,
// The full accordion variant (header, "Connect key" badge, changed-path indicator, group
// revert) — used instead of the bare pane when the connect-key state coincides with an
// uncommitted change, so the change stays visible rather than being silently dropped.
providerCredentialsSection,
Comment thread
coderabbitai[bot] marked this conversation as resolved.
// A model is selected but the chosen harness can't run it — a *model* problem (the harness
// itself stays valid), so the config panel flags the Model & harness section as invalid.
modelUnsupported: !!modelId && !selectedKeepsModel,
Expand Down
Loading