-
Notifications
You must be signed in to change notification settings - Fork 616
[AGE-4014] fix(frontend): Show changed-path affordance for provider/connection edits in config drawer #5627
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
f601b57
14754a8
18aef94
77cba2e
23f80db
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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}</> | ||
| } | ||
|
|
||
|
|
@@ -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"), | ||
| ), | ||
| [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 | ||
|
|
@@ -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 | ||
|
|
@@ -829,7 +842,22 @@ export const AgentTemplateControl = memo(function AgentTemplateControl({ | |
| onOpenDetails={() => openSectionDrawer("model-harness")} | ||
| disabled={disabled} | ||
| > | ||
| {mh.providerCredentialsInline} | ||
| {credentialsPathChanged ? ( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This drops the inline model picker from the connect-key pane.
That hits the common path, not an edge case: the usual way into the needs-key state is a catalog model pick, which moves |
||
| <ChangedPathsProvider changes={panelChangedPaths}> | ||
| <ModelHarnessSectionBody | ||
| section="connect-key" | ||
|
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> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In the non-key "what changed" branch, a provider-only diff therefore leaves |
||
| 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(() => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This restores only Each of Needs to be one
|
||
| 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 | ||
|
|
@@ -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 / | ||
|
|
@@ -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, | ||
|
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, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.