feat(frontend): connect-model drawer redesign + connect-gate rework - #5096
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 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:
📝 WalkthroughWalkthroughThis PR redesigns the agent playground's "Model & harness" drawer into three sections (Harness, Model, Provider credentials), introducing a ChangesConnect Model drawer redesign
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant useModelHarness
participant ProviderCredentialsSection
participant ProviderKeyField
participant VaultSecret
User->>useModelHarness: select model / harness
useModelHarness->>ProviderCredentialsSection: pass selectedProviderFamily, modeOptions
ProviderCredentialsSection->>ProviderCredentialsSection: auto-highlight matching provider
User->>ProviderCredentialsSection: toggle Use API key / Use subscription
ProviderCredentialsSection->>ProviderKeyField: render key form for selected provider
User->>ProviderKeyField: submit API key
ProviderKeyField->>VaultSecret: save key
VaultSecret-->>ProviderKeyField: setProviderKeySetupDone(true)
stateDiagram-v2
[*] --> Editing
Editing --> ConfirmClose: scrim/X clicked while dirty
ConfirmClose --> Editing: Keep editing
ConfirmClose --> Discarded: Discard
ConfirmClose --> Saved: Save changes
Editing --> Saved: footer Save
Editing --> Discarded: footer Cancel
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
D1: section order (default Harness → Model → Credentials)
ok
D2: SectionRail restyle global vs variant (recommend global — all 5 consumers share the broken token)
global
D3: drawer width 880px+history (recommended) vs prototype 640px
go with recommended
D4: keep named-connection Select in the API-key pane (recommended) vs drop
go with recommended
D5: CustomProviderForm home in @agenta/entity-ui with listed package moves
whatever you think. is better and consistent with rest
D6: cloud-gating via DrillInUIContext.deployment (recommended)
if that works ok
lgtm
…-self-managed, first-time users Claude-Session: https://claude.ai/code/session_01FNiAiGuzfi1kkWvXPPcrVw
…pply on agent creation Claude-Session: https://claude.ai/code/session_01FNiAiGuzfi1kkWvXPPcrVw
…ovider credentials pane Harness/Model/Provider-credentials sections; Use API key / Use subscription toggle (subscription clickable on cloud with Not-on-cloud badge); provider rail + immediate key saves; four Add-provider rows opening the Configure-provider drawer preselected; connection Select removed; mode UI removed from Advanced; unsaved-changes guard; SectionRail selected-state fix (colorPrimaryBg was never emitted); CustomProviderForm extracted to @agenta/entity-ui; connection-slug threading replaces model-id guessing; shared provider-family normalization; colorWarningBg token. Claude-Session: https://claude.ai/code/session_01FNiAiGuzfi1kkWvXPPcrVw
Follow-up to the connect-model drawer redesign: self_managed covers any harness auth that skips the Agenta vault, not just the openai-codex subscription OAuth path. Claude-Session: https://claude.ai/code/session_01FNiAiGuzfi1kkWvXPPcrVw
…nfiguration sync Claude-Session: https://claude.ai/code/session_01FNiAiGuzfi1kkWvXPPcrVw
mmabrouk
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/packages/agenta-entities/src/secret/state/useVaultSecret.ts (1)
91-97: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winArm the setup flag for custom/named secret saves too.
providerKeySetupDoneAtomis only set onhandleModifyVaultSecret, so users whose first key is added throughhandleModifyCustomVaultSecretorhandleModifyNamedSecretcan still see the connect-model gate again after that key is deleted. Set the flag in those success paths as well.
🧹 Nitpick comments (17)
web/oss/src/components/AgentChatSlice/hooks/useAgentModelKeyStatus.ts (1)
76-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding unit tests for
gateActive.This hook now encodes the entire connect-a-model gating contract (vault-empty, self-managed, key-setup-done, provider-entry match) behind a single consolidated flag consumed by both the composer block and the banner. Given its centrality and the multiple boolean inputs, a focused unit test (mocking
vaultSecretsQueryAtom,standardSecretsAtom,providerKeySetupDoneAtom, andconfig) covering the loading/empty/self-managed/keySetupDone matrix would guard against regressions — similar to the siblingconnectionUtils.test.ts/agent-creation-prefs.test.tsadded elsewhere in this stack.docs/design/connect-model-drawer/plan.md (1)
90-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMarkdown list-indent inconsistency (MD005).
Static analysis flags lines 97, 99, 103, 104 for inconsistent list indentation (expected 2, actual 0). The nested layering-rule sub-bullets (lines 91-96, indented 2 spaces) sit inside the same parent list as the top-level bullets that follow at 0 indentation, tripping markdownlint's list-indent rule.
📝 Suggested indentation fix
-- Rewire `web/oss/src/components/ModelRegistry/Drawers/ConfigureProviderDrawer/index.tsx` - to a thin shell (`EnhancedDrawer` + footer buttons) around the extracted form. -- Do NOT touch `web/oss/src/components/ModelRegistry/Modals/ConfigureProviderModal/` - (a sibling directory under `ModelRegistry/Modals/`, NOT nested inside - `Drawers/ConfigureProviderDrawer/` — research.md §6 mis-locates it one level down). - It is a separate standard-key modal and stays untouched. -- Export the form from the `@agenta/entity-ui` index. -- Respect the import hierarchy (`shared ← ui ← entities ← entity-ui`); no `web/oss` - imports inside packages. + - Rewire `web/oss/src/components/ModelRegistry/Drawers/ConfigureProviderDrawer/index.tsx` + to a thin shell (`EnhancedDrawer` + footer buttons) around the extracted form. + - Do NOT touch `web/oss/src/components/ModelRegistry/Modals/ConfigureProviderModal/` + (a sibling directory under `ModelRegistry/Modals/`, NOT nested inside + `Drawers/ConfigureProviderDrawer/` — research.md §6 mis-locates it one level down). + It is a separate standard-key modal and stays untouched. + - Export the form from the `@agenta/entity-ui` index. + - Respect the import hierarchy (`shared ← ui ← entities ← entity-ui`); no `web/oss` + imports inside packages.Source: Linters/SAST tools
web/packages/agenta-shared/src/utils/validators.ts (1)
47-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate slug regex across packages.
URL_SAFE_SLUGhere is byte-identical to theURL_SAFEregex still defined inweb/oss/src/lib/helpers/utils.ts(used byisVariantNameInputValid). Consider havingisVariantNameInputValidreuse this new sharedisSlugInputValid/regex instead of keeping a second copy of the same pattern, to avoid future divergence.♻️ Proposed consolidation
-const URL_SAFE = /^[a-zA-Z0-9_-]+$/ - export const isVariantNameInputValid = (input: string) => { - return URL_SAFE.test(input) + return isSlugInputValid(input) }web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/SectionDrawer.tsx (1)
43-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding tests for the new confirm-guard flow.
The dirty-guard/confirm-modal interaction (scrim close → confirm → Discard/Save/Keep-editing) is new, non-trivial branching logic with real data-loss stakes, but no test file for
SectionDrawer.tsxis present in this change.Want me to draft RTL/vitest coverage for
handleRequestClose(dirty vs non-dirty) and the modal's three actions?Also applies to: 88-119
web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentTemplateControl.tsx (1)
182-191: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
sectionDirtyrecomputesdeepEqualon every render, not just when the draft changes.
isCurrentSectionDirtyis correctly memoized viauseCallback, butsectionDirty = isCurrentSectionDirty()(Line 255) invokes it unconditionally on every render of this large component (which owns many unrelated pieces of local state —referenceSelectorOpen,integrationDrawerOpen, tool/mcp/skill lists, etc.). Any unrelated state change re-runs thedeepEqualcomparison over the full draft config even though the callback reference (and its inputs) haven't changed.♻️ Proposed fix to memoize the derived dirty flag
- // Enable Save only when the draft actually differs from what we opened with (config or build-kit). - const sectionDirty = isCurrentSectionDirty() + // Enable Save only when the draft actually differs from what we opened with (config or build-kit). + const sectionDirty = useMemo(() => isCurrentSectionDirty(), [isCurrentSectionDirty])As per coding guidelines, "Optimize React performance by minimizing re-renders, memoizing expensive computations, stabilizing props/functions in lists, virtualizing large lists, and debouncing or throttling heavy handlers."
Also applies to: 254-255
Source: Coding guidelines
web/packages/agenta-entities/src/workflow/state/agentCreationPrefs.ts (1)
46-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: connection overlay reads from stale
llminstead ofnextLlm.At Line 55-56, the
connectionfallback readsllm.connectionrather thannextLlm.connection. It's correct today since nothing mutates.connectiononnextLlmbefore this point, but it's a latent trap if a future edit reorders the assignments.🛡️ Defensive tweak
if (prefs.connectionMode) { const connection = - llm.connection && - typeof llm.connection === "object" && - !Array.isArray(llm.connection) - ? (llm.connection as Record<string, unknown>) + nextLlm.connection && + typeof nextLlm.connection === "object" && + !Array.isArray(nextLlm.connection) + ? (nextLlm.connection as Record<string, unknown>) : {} nextLlm.connection = {...connection, mode: prefs.connectionMode} }web/oss/src/components/DrillInView/OSSdrillInUIProvider.tsx (2)
649-651: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment exceeds one-line guideline.
Two-line comment block here; as per coding guidelines,
**/*.{js,jsx,ts,tsx}in-code comments should be "one short line maximum unless a genuinely surprising constraint requires a brief exception." This explains a design rationale which may qualify as an exception, but consider trimming to a single line and moving the design-doc pointer elsewhere.Source: Coding guidelines
649-651: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
deploymentobject is recreated every render.
{isCloud: isDemo()}is a fresh object each render; sinceisDemo()never changes at runtime, wrap it inuseMemo(() => ({isCloud: isDemo()}), [])to give consumers a stable reference. Minor, since the surroundingcomponents={{...}}object is already unmemoized in the existing pattern.Also applies to: 662-662, 677-677, 750-750
web/oss/src/hooks/useLLMProviderConfig.tsx (1)
40-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment exceeds one-line guideline.
As per coding guidelines,
**/*.{js,jsx,ts,tsx}: "Keep in-code comments to one short line maximum unless a genuinely surprising constraint requires a brief exception." This 3-line comment could be trimmed.Source: Coding guidelines
web/oss/src/components/ModelRegistry/Drawers/ConfigureProviderDrawer/assets/ConfigureProviderDrawerContent.tsx (1)
5-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment exceeds one-line guideline.
As per coding guidelines,
**/*.{js,jsx,ts,tsx}: "Keep in-code comments to one short line maximum unless a genuinely surprising constraint requires a brief exception."Source: Coding guidelines
web/packages/agenta-entity-ui/src/secretProvider/CustomProviderForm.tsx (3)
148-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing
forminuseEffectdeps.
formis used inside the effect but omitted from the dependency array. SinceForm.useForm()returns a stable instance this is functionally safe, but it will tripeslint-plugin-react-hooksexhaustive-deps.
138-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComments exceed one-line guideline.
Multiple 2–3 line comment blocks here (lines 138-140, 240-242). As per coding guidelines,
**/*.{js,jsx,ts,tsx}: "Keep in-code comments to one short line maximum unless a genuinely surprising constraint requires a brief exception." The either/or auth validation logic is non-obvious and may qualify as a legitimate exception, but consider condensing where possible.Also applies to: 240-249
Source: Coding guidelines
100-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the custom-provider kind list
customProvidersis duplicated as raw strings here and inProviderCredentialsSection. Use the sharedCustomProviderKindvalues, or extract a single provider-kind constant, so both UIs stay aligned when a kind is added or renamed.web/packages/agenta-ui/src/components/presentational/inputs/LabelInput.tsx (2)
31-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLabel isn't programmatically associated with its input.
The visible label is a plain
<span>with nohtmlFor/idlink to the input/textarea/password field, so screen readers can't associate them and clicking the label won't focus the control.♿ Proposed fix using `useId`
-import {memo} from "react" +import {memo, useId} from "react" export const LabelInput = memo(function LabelInput({ label, className, multiLine = false, type, inputType, + id, ...props }: LabelInputProps) { const resolvedInputType = type ?? inputType const isPassword = resolvedInputType === "password" + const generatedId = useId() + const inputId = id ?? generatedId return ( <div className="rounded-lg border border-solid border-[var(--ag-c-BDC7D1)] p-1 pl-2.5"> - <span className="font-medium">{label}</span> + <label htmlFor={inputId} className="font-medium"> + {label} + </label> {multiLine ? ( <Input.TextArea + id={inputId} variant="borderless" ... ) : isPassword ? ( <Input.Password + id={inputId} variant="borderless" ... ) : ( <Input + id={inputId} variant="borderless" ... )}Also applies to: 60-77
46-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Tailwind classes instead of inline
style.The
style={{overflowY, overflowX, maxHeight, resize}}values are static and can be expressed as Tailwind utilities.♻️ Proposed fix
<Input.TextArea variant="borderless" - className={clsx("px-0 rounded-none", className)} + className={clsx( + "px-0 rounded-none overflow-hidden resize-none max-h-[none]", + className, + )} autoSize={{minRows: 1}} spellCheck={false} autoComplete="off" - style={{ - overflowY: "hidden", - overflowX: "hidden", - maxHeight: "none", - resize: "none", - }} {...(props as TextAreaProps)} />As per coding guidelines, "Prefer Tailwind utility classes over CSS-in-JS, inline
style={{...}}, and separate CSS files; use CSS-in-JS only for complex Ant Design overrides, JS-calculated dynamic theme styles, or legacy components."Source: Coding guidelines
web/packages/agenta-entity-ui/src/secretProvider/ModelNameInput.tsx (1)
13-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIcon-only delete button lacks an accessible name; input text can overlap the absolutely-positioned button.
The
Trashicon button has noaria-label, so screen readers announce nothing meaningful. Also,<Input>has no reserved right padding, so a long model name can render underneath the absolutely-positioned delete button.♻️ Proposed fix
<Input placeholder="Enter model name" className="w-full" {...props} /> <Button icon={<Trash size={14} />} type="link" - className="absolute top-[1px] right-1" + className="absolute top-[1px] right-1" + aria-label="Remove model" onClick={onDelete} disabled={disabled} />Also consider
pr-8on theInputclassName so text doesn't slide under the button.web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/useModelHarness.tsx (1)
188-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate provider-family normalization logic.
The
_api_key$stripping +normalizeProviderFamilypattern here duplicatesstandardSecretFamilyinProviderCredentialsSection.tsx(normalizeProviderFamily((secret.name ?? "").replace(/_api_key$/i, ""))). Given the PR's stated goal of sharing provider-family normalization, consider exporting a single helper (e.g. fromconnectionUtils.ts) and using it in both places.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e0d816f-1d5b-408f-ab7a-52c7c53e262f
📒 Files selected for processing (50)
docs/design/agent-workflows/documentation/agent-configuration.mddocs/design/connect-model-drawer/README.mddocs/design/connect-model-drawer/context.mddocs/design/connect-model-drawer/design.mddocs/design/connect-model-drawer/plan.mddocs/design/connect-model-drawer/research.mddocs/design/connect-model-drawer/status.mdweb/oss/src/components/AgentChatSlice/AgentChatPanel.tsxweb/oss/src/components/AgentChatSlice/components/ConnectModelBanner.tsxweb/oss/src/components/AgentChatSlice/hooks/useAgentModelKeyStatus.tsweb/oss/src/components/DrillInView/OSSdrillInUIProvider.tsxweb/oss/src/components/ModelRegistry/Drawers/ConfigureProviderDrawer/assets/ConfigureProviderDrawerContent.tsxweb/oss/src/components/ModelRegistry/Drawers/ConfigureProviderDrawer/assets/types.d.tsweb/oss/src/components/ModelRegistry/Drawers/ConfigureProviderDrawer/index.tsxweb/oss/src/hooks/useLLMProviderConfig.tsxweb/oss/src/lib/helpers/utils.tsweb/packages/agenta-entities/src/secret/core/index.tsweb/packages/agenta-entities/src/secret/core/providerFields.tsweb/packages/agenta-entities/src/secret/index.tsweb/packages/agenta-entities/src/secret/state/atoms.tsweb/packages/agenta-entities/src/secret/state/index.tsweb/packages/agenta-entities/src/secret/state/useVaultSecret.tsweb/packages/agenta-entities/src/workflow/commitDiff/classify.tsweb/packages/agenta-entities/src/workflow/index.tsweb/packages/agenta-entities/src/workflow/state/agentCreationPrefs.tsweb/packages/agenta-entities/src/workflow/state/appUtils.tsweb/packages/agenta-entities/src/workflow/state/index.tsweb/packages/agenta-entities/tests/unit/agent-commit-diff.test.tsweb/packages/agenta-entities/tests/unit/agent-creation-prefs.test.tsweb/packages/agenta-entity-ui/package.jsonweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentTemplateControl.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/SectionDrawer.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ProviderCredentialsSection.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ProviderKeyField.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/useModelHarness.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/connectionUtils.tsweb/packages/agenta-entity-ui/src/drawers/shared/SectionRail.tsxweb/packages/agenta-entity-ui/src/secretProvider/CustomProviderForm.tsxweb/packages/agenta-entity-ui/src/secretProvider/ModelNameInput.tsxweb/packages/agenta-entity-ui/src/secretProvider/index.tsweb/packages/agenta-entity-ui/tests/unit/connectionUtils.test.tsweb/packages/agenta-shared/src/utils/index.tsweb/packages/agenta-shared/src/utils/llmProviders.tsweb/packages/agenta-shared/src/utils/validators.tsweb/packages/agenta-shared/tests/unit/provider-family.test.tsweb/packages/agenta-ui/src/SelectLLMProvider/SelectLLMProviderBase.tsxweb/packages/agenta-ui/src/components/presentational/index.tsweb/packages/agenta-ui/src/components/presentational/inputs/LabelInput.tsxweb/packages/agenta-ui/src/components/presentational/inputs/index.tsweb/packages/agenta-ui/src/drill-in/context/DrillInUIContext.tsx
💤 Files with no reviewable changes (1)
- web/packages/agenta-entity-ui/tests/unit/connectionUtils.test.ts
| ``` | ||
| before: !bg-[var(--ag-colorPrimaryBg)] !font-medium !text-[var(--ag-colorPrimary)] | ||
| after: !bg-[var(--ag-colorFillSecondary)] !font-semibold !text-[var(--ag-colorText)] | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Label the fenced example block.
Markdownlint will keep flagging this fence until it has a language tag; text is enough for this before/after snippet.
Suggested fix
-```
+```text
before: !bg-[var(--ag-colorPrimaryBg)] !font-medium !text-[var(--ag-colorPrimary)]
after: !bg-[var(--ag-colorFillSecondary)] !font-semibold !text-[var(--ag-colorText)]
-```
+```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| before: !bg-[var(--ag-colorPrimaryBg)] !font-medium !text-[var(--ag-colorPrimary)] | |
| after: !bg-[var(--ag-colorFillSecondary)] !font-semibold !text-[var(--ag-colorText)] | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 42-42: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
Source: Linters/SAST tools
| {selectedStandardSecret ? ( | ||
| <ProviderKeyField | ||
| provider={selectedStandardSecret} | ||
| disabled={disabled} | ||
| /> | ||
| ) : selectedCustomProvider ? ( |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Provider switch can leak a typed key into the wrong provider's save.
ProviderKeyField is rendered here without a key prop tied to the selected provider's identity, so switching the rail selection (selectedStandardSecret changes) reuses the same component instance. Its internal typed-value state (useState("") in ProviderKeyField.tsx) isn't reset by a prop change, so a half-typed key for provider A can still be sitting in the field when the user switches to provider B and clicks Save — saving A's typed value under B's slot.
🔒 Proposed fix
{selectedStandardSecret ? (
<ProviderKeyField
+ key={selectedStandardSecret.name}
provider={selectedStandardSecret}
disabled={disabled}
/>Full analysis in the companion review comment on ProviderKeyField.tsx.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {selectedStandardSecret ? ( | |
| <ProviderKeyField | |
| provider={selectedStandardSecret} | |
| disabled={disabled} | |
| /> | |
| ) : selectedCustomProvider ? ( | |
| {selectedStandardSecret ? ( | |
| <ProviderKeyField | |
| key={selectedStandardSecret.name} | |
| provider={selectedStandardSecret} | |
| disabled={disabled} | |
| /> | |
| ) : selectedCustomProvider ? ( |
| {props.llm ? ( | ||
| <ProviderCredentialsSection | ||
| mode={connection.mode} | ||
| onModeChange={(m) => writeModel({mode: m})} | ||
| selectedProviderFamily={selectedProviderFamily} | ||
| modeOptions={modeOptions} | ||
| isCloud={isCloud} | ||
| selfHostingGuideUrl={deployment?.selfHostingGuideUrl} | ||
| providerNeedsKey={providerNeedsKey} | ||
| openConfigureProvider={llmProviderConfig?.openConfigureProvider} | ||
| disabled={disabled} | ||
| /> | ||
| ) : null} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
ProviderCredentialsSection block duplicated across both layout branches.
The exact same JSX (props included) is repeated in the capabilities layout and the fallback layout. Extracting it into a single local variable/element and reusing it in both branches would prevent the two copies from drifting when props change.
♻️ Suggested refactor
+ const providerCredentialsSection = props.llm ? (
+ <ProviderCredentialsSection
+ mode={connection.mode}
+ onModeChange={(m) => writeModel({mode: m})}
+ selectedProviderFamily={selectedProviderFamily}
+ modeOptions={modeOptions}
+ isCloud={isCloud}
+ selfHostingGuideUrl={deployment?.selfHostingGuideUrl}
+ providerNeedsKey={providerNeedsKey}
+ openConfigureProvider={llmProviderConfig?.openConfigureProvider}
+ disabled={disabled}
+ />
+ ) : nullThen reference {providerCredentialsSection} in both modelHarnessControls branches instead of the two inline copies.
Also applies to: 604-615
| const onSubmit = async (values: LlmProvider) => { | ||
| try { | ||
| if (form.getFieldValue("models").length === 0 || !form.getFieldValue("models")[0]) { | ||
| setErrorMessage("Please add a model name before submitting") | ||
| return | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Potential crash if models field value is null.
form.getFieldValue("models").length will throw if models is null/undefined (e.g. if selectedProvider.models is explicitly null, setFieldsValue will set the field to null, overriding the [""] initial value). Guard against this.
🛡️ Proposed fix
- if (form.getFieldValue("models").length === 0 || !form.getFieldValue("models")[0]) {
+ const models = form.getFieldValue("models") ?? []
+ if (models.length === 0 || !models[0]) {
setErrorMessage("Please add a model name before submitting")
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onSubmit = async (values: LlmProvider) => { | |
| try { | |
| if (form.getFieldValue("models").length === 0 || !form.getFieldValue("models")[0]) { | |
| setErrorMessage("Please add a model name before submitting") | |
| return | |
| } | |
| const onSubmit = async (values: LlmProvider) => { | |
| try { | |
| const models = form.getFieldValue("models") ?? [] | |
| if (models.length === 0 || !models[0]) { | |
| setErrorMessage("Please add a model name before submitting") | |
| return | |
| } |
| const ModelNameInput = ({onDelete, disabled, ...props}: ModelNameInputProps) => { | ||
| return ( | ||
| <div className="w-full relative"> | ||
| <Input placeholder="Enter model name" className="w-full" {...props} /> | ||
| <Button | ||
| icon={<Trash size={14} />} | ||
| type="link" | ||
| className="absolute top-[1px] right-1" | ||
| onClick={onDelete} | ||
| disabled={disabled} | ||
| /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
disabled prop is never applied to the <Input>, only to the delete button.
disabled is destructured out of props (Line 11) so it's excluded from {...props} spread onto <Input> (Line 14). Only the <Button> receives disabled. When a consumer disables this row, the model-name text remains editable while only the delete action is blocked.
🐛 Proposed fix
- <Input placeholder="Enter model name" className="w-full" {...props} />
+ <Input placeholder="Enter model name" className="w-full" disabled={disabled} {...props} />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const ModelNameInput = ({onDelete, disabled, ...props}: ModelNameInputProps) => { | |
| return ( | |
| <div className="w-full relative"> | |
| <Input placeholder="Enter model name" className="w-full" {...props} /> | |
| <Button | |
| icon={<Trash size={14} />} | |
| type="link" | |
| className="absolute top-[1px] right-1" | |
| onClick={onDelete} | |
| disabled={disabled} | |
| /> | |
| const ModelNameInput = ({onDelete, disabled, ...props}: ModelNameInputProps) => { | |
| return ( | |
| <div className="w-full relative"> | |
| <Input placeholder="Enter model name" className="w-full" disabled={disabled} {...props} /> | |
| <Button | |
| icon={<Trash size={14} />} | |
| type="link" | |
| className="absolute top-[1px] right-1" | |
| onClick={onDelete} | |
| disabled={disabled} | |
| /> |
| /** | ||
| * Deployment (host app) facts the package can't determine itself. Today: whether this | ||
| * deployment is Agenta cloud, which gates the Provider credentials section's "Use | ||
| * subscription" connection mode (design.md D6, docs/design/connect-model-drawer). | ||
| * Absent (older hosts) reads as not-cloud, i.e. ungated. | ||
| */ | ||
| deployment?: { | ||
| /** Policy: gates the self_managed connection mode in the UI. Never changes at runtime. */ | ||
| isCloud: boolean | ||
| /** Link target for the self-managed info card's "Read the self-hosting guide" pill. */ | ||
| selfHostingGuideUrl?: string | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Misleading doc: isCloud doesn't actually gate the mode.
The comments here say isCloud "gates the ... connection mode" / "gates the self_managed connection mode in the UI," but the actual consumer (ProviderCredentialsSection.tsx) explicitly documents the opposite: the "Use subscription" toggle stays clickable regardless of isCloud — it only gates the "Not on cloud" badge/explanation on the card. A future reader relying on this comment could assume isCloud enforces a real restriction and skip adding one where it's actually needed.
📝 Suggested comment fix
/**
- * Deployment (host app) facts the package can't determine itself. Today: whether this
- * deployment is Agenta cloud, which gates the Provider credentials section's "Use
- * subscription" connection mode (design.md D6, docs/design/connect-model-drawer).
+ * Deployment (host app) facts the package can't determine itself. Today: whether this
+ * deployment is Agenta cloud, which gates only the self-managed card's "Not on cloud"
+ * badge/explanation in the Provider credentials section (design.md D6). The "Use
+ * subscription" toggle itself is always clickable when capability-allowed.
* Absent (older hosts) reads as not-cloud, i.e. ungated.
*/
deployment?: {
- /** Policy: gates the self_managed connection mode in the UI. Never changes at runtime. */
+ /** Policy: gates only the self-managed card's "Not on cloud" badge. Never changes at runtime. */
isCloud: boolean📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Deployment (host app) facts the package can't determine itself. Today: whether this | |
| * deployment is Agenta cloud, which gates the Provider credentials section's "Use | |
| * subscription" connection mode (design.md D6, docs/design/connect-model-drawer). | |
| * Absent (older hosts) reads as not-cloud, i.e. ungated. | |
| */ | |
| deployment?: { | |
| /** Policy: gates the self_managed connection mode in the UI. Never changes at runtime. */ | |
| isCloud: boolean | |
| /** Link target for the self-managed info card's "Read the self-hosting guide" pill. */ | |
| selfHostingGuideUrl?: string | |
| } | |
| /** | |
| * Deployment (host app) facts the package can't determine itself. Today: whether this | |
| * deployment is Agenta cloud, which gates only the self-managed card's "Not on cloud" | |
| * badge/explanation in the Provider credentials section (design.md D6). The "Use | |
| * subscription" toggle itself is always clickable when capability-allowed. | |
| * Absent (older hosts) reads as not-cloud, i.e. ungated. | |
| */ | |
| deployment?: { | |
| /** Policy: gates only the self-managed card's "Not on cloud" badge. Never changes at runtime. */ | |
| isCloud: boolean | |
| /** Link target for the self-managed info card's "Read the self-hosting guide" pill. */ | |
| selfHostingGuideUrl?: string | |
| } |
…accordion section, adopt-new-provider flow Rail shows only providers that can serve the selected model (CUSTOM_PROVIDER_KIND_FAMILIES capability map); SectionRail-recipe selection states legible in dark; single RailRow anatomy for provider and Add rows; pane is a ConfigAccordionSection with Connect-key status; a provider added via the Add rows adopts its first model + slug. Claude-Session: https://claude.ai/code/session_01FNiAiGuzfi1kkWvXPPcrVw
|
@coderabbitai review |
✅ Action performedReview finished.
|
…vider icon/label fixes Custom-provider kinds that are plain provider families now gate against allowedProviders instead of allowedDeployments (they never matched deployments, dropping their models for every harness). Provider icon lookup normalizes keys (fixes Together AI, Anyscale, SageMaker, Perplexity); openai-codex reuses the OpenAI mark but displays as OpenAI Codex. 'Add provider' renamed to 'Add custom provider' in the dropdown footer and rail. Claude-Session: https://claude.ai/code/session_01FNiAiGuzfi1kkWvXPPcrVw
…d copy, red cloud badge Segmented switcher active state now unmistakable in both themes (antd selected pill was white-on-white in light); self-managed card copy restructured to three skimmable lines with the self-hosted requirement bold; badge reworded to 'Unavailable in the cloud' on error tokens. Claude-Session: https://claude.ai/code/session_01FNiAiGuzfi1kkWvXPPcrVw
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/useModelHarness.tsx (1)
258-261: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThread the vault option’s provider family into
writeModel.
vaultModelGroupsalready carriesmetadata.provider, butwriteModelignores it wheneverslugis present and falls back tofamilyFromModelId(...). That dropsllm.providerfor slug-backed custom connections whose model ids are not in the catalog, including plain-family connections likeopenaicustom providers. Pass the option’s provider family through on selection/adoption, and keep deployment kinds out ofllm.provider.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e3477830-e2e7-4eab-a397-f7a38cccbb13
📒 Files selected for processing (6)
web/oss/src/hooks/useLLMProviderConfig.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ProviderCredentialsSection.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/useModelHarness.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/connectionUtils.tsweb/packages/agenta-entity-ui/tests/unit/connectionUtils.test.tsweb/packages/agenta-ui/src/SelectLLMProvider/utils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- web/oss/src/hooks/useLLMProviderConfig.tsx
- web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ProviderCredentialsSection.tsx
| export const PROVIDER_ICON_MAP: Record<string, string> = { | ||
| anthropic: "Anthropic", | ||
| openai: "OpenAI", | ||
| // OpenAI's ChatGPT/Codex subscription provider — reuses the OpenAI mark (no distinct icon | ||
| // asset exists). Its display name still needs to read "OpenAI Codex" — see | ||
| // PROVIDER_DISPLAY_NAME_OVERRIDES below, since this map is also read by getProviderDisplayName. | ||
| openai_codex: "OpenAI", | ||
| groq: "Groq", | ||
| mistral: "Mistral AI", | ||
| gemini: "Google Gemini", | ||
| google: "Google Gemini", | ||
| cohere: "Cohere", | ||
| anyscale: "Anyscale", | ||
| deepinfra: "DeepInfra", | ||
| openrouter: "OpenRouter", | ||
| perplexity: "Perplexity AI", | ||
| perplexityai: "Perplexity AI", | ||
| together_ai: "Together AI", | ||
| vertex_ai: "Google Vertex AI", | ||
| bedrock: "AWS Bedrock", | ||
| sagemaker: "AWS SageMaker", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add Azure OpenAI to the normalized provider map.
LLMIconMap exposes "Azure OpenAI", but PROVIDER_ICON_MAP has no azure / azure_openai entry. Azure provider rows will fall back to no icon for azure and a weaker display label for normalized Azure keys.
Suggested fix
// PROVIDER_DISPLAY_NAME_OVERRIDES below, since this map is also read by getProviderDisplayName.
openai_codex: "OpenAI",
+ azure: "Azure OpenAI",
+ azure_openai: "Azure OpenAI",
groq: "Groq",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const PROVIDER_ICON_MAP: Record<string, string> = { | |
| anthropic: "Anthropic", | |
| openai: "OpenAI", | |
| // OpenAI's ChatGPT/Codex subscription provider — reuses the OpenAI mark (no distinct icon | |
| // asset exists). Its display name still needs to read "OpenAI Codex" — see | |
| // PROVIDER_DISPLAY_NAME_OVERRIDES below, since this map is also read by getProviderDisplayName. | |
| openai_codex: "OpenAI", | |
| groq: "Groq", | |
| mistral: "Mistral AI", | |
| gemini: "Google Gemini", | |
| google: "Google Gemini", | |
| cohere: "Cohere", | |
| anyscale: "Anyscale", | |
| deepinfra: "DeepInfra", | |
| openrouter: "OpenRouter", | |
| perplexity: "Perplexity AI", | |
| perplexityai: "Perplexity AI", | |
| together_ai: "Together AI", | |
| vertex_ai: "Google Vertex AI", | |
| bedrock: "AWS Bedrock", | |
| sagemaker: "AWS SageMaker", | |
| export const PROVIDER_ICON_MAP: Record<string, string> = { | |
| anthropic: "Anthropic", | |
| openai: "OpenAI", | |
| // OpenAI's ChatGPT/Codex subscription provider — reuses the OpenAI mark (no distinct icon | |
| // asset exists). Its display name still needs to read "OpenAI Codex" — see | |
| // PROVIDER_DISPLAY_NAME_OVERRIDES below, since this map is also read by getProviderDisplayName. | |
| openai_codex: "OpenAI", | |
| azure: "Azure OpenAI", | |
| azure_openai: "Azure OpenAI", | |
| groq: "Groq", | |
| mistral: "Mistral AI", | |
| gemini: "Google Gemini", | |
| google: "Google Gemini", | |
| cohere: "Cohere", | |
| anyscale: "Anyscale", | |
| deepinfra: "DeepInfra", | |
| openrouter: "OpenRouter", | |
| perplexity: "Perplexity AI", | |
| perplexityai: "Perplexity AI", | |
| together_ai: "Together AI", | |
| vertex_ai: "Google Vertex AI", | |
| bedrock: "AWS Bedrock", | |
| sagemaker: "AWS SageMaker", |
… picks, key field resets per provider, rail/dropdown kind parity vaultPickedProviderFamily threads option metadata's provider through writeModel so config.llm never loses its provider on family-less vault models (9 new unit tests). ProviderKeyField gets a per-provider key so typed secrets can't leak across rail switches. kindServesFamily now understands both kind flavors, matching the model dropdown. Plus: models null-guard in CustomProviderForm, disabled applied on ModelNameInput, isCloud doc comment corrected, ProviderCredentialsSection JSX deduped. Claude-Session: https://claude.ai/code/session_01FNiAiGuzfi1kkWvXPPcrVw
1 participant
Context
The Model & harness drawer had three problems. Harness selection was unreadable: the
active row only got bold text, because
SectionRail's selected style used--ag-colorPrimaryBg, a token the theme generator never emits, so the backgroundresolved to nothing. Connection mode (API key vs. subscription) was buried inside
Advanced, several clicks away from the model and provider it belonged next to. And the
"connect a model" gate nagged people who already had credentials configured, self-managed
agents, and repeat visitors, because it fired on every load with no memory of the vault
state or the visitor.
Changes
Drawer restructure. The drawer is now three sections in order: Harness, Model,
Provider credentials. Picking a model auto-highlights its provider in the credentials
rail below. The rail selection bug is fixed globally (not just for this drawer): all five
SectionRailconsumers (harness list, workflow-reference selector, trigger run-versionfield, commit modal, agent-home templates gallery) now render a real background on the
selected row.
Provider credentials pane. A "Use API key" / "Use subscription" toggle replaces the
old Advanced auth-mode picker. "Use subscription" is capability-gated per harness (only
offered where the harness supports it) but stays clickable everywhere, including on
cloud, where a "Not on cloud" badge plus a "Self-managed" info card explain why it can't
be used there instead of hiding the option. The provider rail lists standard providers
plus four "Add …" rows (Azure OpenAI, Bedrock, Vertex AI, OpenAI-compatible) that open
the existing Configure-provider drawer preselected to that type. Saving a provider key
writes to the vault immediately, the same as before, and arms the first-time-setup flag
used by the connect gate. The named-connection Select is gone; the drawer always uses
the project's default connection. Mode selection no longer exists in Advanced at all.
Unsaved-changes guard. Closing the drawer via the scrim or the X while a section has
unsaved edits now prompts Save / Discard / Keep editing instead of silently dropping
changes, and drawer state is hardened against being clobbered by a programmatic close.
Connect-a-model gate. The banner and composer block now only fire when all three are
true: the vault has zero stored credentials, the agent is not
self_managed, and abrowser-local "seen it" flag is unset. Before, the gate fired on every load regardless of
existing credentials, so self-managed agents and returning users with a real key already
saved kept seeing it.
Creation prefs. The last-used model, harness, and connection are now persisted to
localStorageand applied when creating a new agent, via a jotaigetOnInithydrationfix so the persisted value is available on the very first render instead of one render
late.
Refactors underneath.
CustomProviderFormwas extracted out ofConfigureProviderDrawerContentinto@agenta/entity-ui/secretProvider, so the drawerand the inline pane share one implementation instead of two. Model selection now threads
the connection slug through explicitly instead of guessing it from the model id, which
fixed a class of stale-slug runtime failures. Provider-family normalization (e.g.
together_ai) moved into a shared@agenta/sharedhelper instead of being duplicatedper call site. The commit-diff classifier now groups
llm.connectionchanges under"Model & harness" instead of leaving them unclassified.
Provider rail rethink (post-review feedback). The rail is model-scoped: it now shows
only the selected model's provider, any configured custom providers that can serve that
model, and the capability-relevant "Add" rows, instead of every provider unconditionally.
The Add-row relevance map is
azure→openai;bedrock→anthropic/meta/amazon/mistral;vertex_ai→google/gemini/anthropic;custom→ anymodel (CONFIRM THIS MAP IN REVIEW). Every row, whether a standard provider, a configured
custom provider, or an Add row, now shares one uniform
RailRowanatomy. Selectionstates were re-checked for dark-mode legibility. The pane itself is now an accordion
section with its own Connect-key status indicator instead of a flat block. Adding a
provider through one of the Add rows adopts that provider's first model and connection
as the active selection.
Before (gate): fires whenever
!hasKey, regardless ofself_managedor prior visits.After: fires only when
vaultCredentialCount === 0 && !self_managed && !seenFirstTimeFlag.Scope / risk
Not touched: chat surfaces beyond the gate's own hook, the
ModelRegistrymodal, andplayground run paths. Key validation is still out of scope, same as before this PR.
Risk areas for review:
SectionRailrestyle is global, so it changes visual state on four surfacesoutside this drawer (workflow-reference selector, trigger run-version field, commit
modal, agent-home templates gallery), not just the Model & harness drawer.
ConfigureProviderDrawerContentlost ~370 lines to theCustomProviderFormextraction; it's now a thin shell over the shared component.
ProviderCredentialsSection.tsxhas an inline fallback(
--ag-colorWarningBg,rgba(250,173,20,0.12)) for a palette token that ships separatelyon another lane (the template-strip work). Once that token lands, the fallback is
redundant but harmless.
model-scoped filtering, self-managed card + badge, Bedrock preselection, guard modal in
an earlier round). Residual items verified in code only after a browser drop:
discard-guard re-click, light-mode parity of the final pane, console sweep — recommend
a quick reviewer click-through of those.
How to QA
Prerequisites: local dev stack (
run.sh --oss --devor--ee --dev, matchingload-env), or the Hetzner dev box. An agent with at least one existing modelconnection helps exercise the "already configured" gate path.
Steps:
through the harness list and confirm the selected row has a visible background, not
just bold text.
auto-highlights that provider.
confirm the "Not on cloud" badge and "Self-managed" card explain why it's disabled
there; off cloud, confirm it's usable.
Configure-provider drawer opens preselected to that provider type.
Save / Discard / Keep editing modal appears.
existing one, and confirm the new agent's defaults match what you last picked.
Automated tests:
Edge cases: a self-managed agent should never show the connect-a-model banner or
composer block, even with an empty vault. A user who dismissed the gate once
(
localStorageflag set) shouldn't see it again after adding a key. Existing namedvault connections should still resolve correctly through the new connection-slug
threading instead of falling back to model-id guessing.
Decisions D1-D6
All six were answered in review on this PR (2026-07-06); every one landed on the plan's
recommended default, no alternative was chosen:
SectionRailrestyle scope: global (all five consumers), not a per-surfacevariant.
skeleton).
CustomProviderFormpackage home:@agenta/entity-ui/secretProvider, per thepackage-placement rule for entity-specific UI.
DrillInUIContextwithdeployment.isCloud.https://claude.ai/code/session_01FNiAiGuzfi1kkWvXPPcrVw