Improve agent-level llm configuration UX#967
Open
menakaj wants to merge 6 commits into
Open
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 24 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 Walkthrough(Note: The hidden artifact above intentionally mirrors the reviewer checkpoints. The placeholder range entry indicates grouping adjustments in the reviewer tool; all other rangeIds from the PR are assigned in layers exactly once.) 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
console/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsx (1)
325-352:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing
pendingProviderByEnvin useMemo dependency array.The
isDirtycomputation referencespendingProviderByEnvat line 329 but the dependency array at line 352 doesn't include it. This causes stale dirty state—when a user selects a new provider,isDirtywon't recalculate and the Save button won't appear.🐛 Proposed fix
- }, [config, envVarNames, guardrailsByEnv]); + }, [config, envVarNames, guardrailsByEnv, pendingProviderByEnv]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@console/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsx` around lines 325 - 352, The useMemo block that computes isDirty references pendingProviderByEnv but the dependency array only lists [config, envVarNames, guardrailsByEnv]; add pendingProviderByEnv to that array so React recalculates isDirty when providers change (update the dependency list for the useMemo that defines isDirty to include pendingProviderByEnv alongside config, envVarNames, and guardrailsByEnv).
🧹 Nitpick comments (1)
console/workspaces/pages/configure-agent/src/AddLLMProvider.Component.tsx (1)
257-259: 💤 Low valueRemove unused state variables.
providerSearchQuery,searchTimerRef, anddebouncedSearchappear to be unused—search state is now managed insideProviderSelectDrawer.Proposed cleanup
const [providerDrawerOpen, setProviderDrawerOpen] = useState(false); - const [providerSearchQuery, setProviderSearchQuery] = useState(""); - const searchTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null); - const [debouncedSearch, setDebouncedSearch] = useState("");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@console/workspaces/pages/configure-agent/src/AddLLMProvider.Component.tsx` around lines 257 - 259, Remove the now-unused local search state from AddLLMProvider by deleting providerSearchQuery, searchTimerRef, and debouncedSearch and any associated imports/usages; the search is handled inside ProviderSelectDrawer so remove these state declarations (providerSearchQuery, setProviderSearchQuery, searchTimerRef, debouncedSearch, setDebouncedSearch) and any references to them in the AddLLMProvider component to avoid dead code and unused refs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@console/workspaces/pages/configure-agent/src/AddLLMProvider.Component.tsx`:
- Around line 717-745: The JSX uses the Search icon component (<Search size={64}
/>) but Search is not imported, causing a runtime error; update the import block
that currently imports icons (the same place that imports Link) to also import
Search from the icon library used in this file, ensuring the Search symbol is
available for the AddLLMProvider.Component render path where
ListingTable.EmptyState uses it.
In `@console/workspaces/pages/configure-agent/src/ProviderSelectDrawer.tsx`:
- Around line 59-71: The debounce timeout held in searchTimerRef can fire after
unmount and update state; add a cleanup useEffect that clears the timeout and
nulls searchTimerRef on unmount, and also ensure any existing timer is cleared
before setting a new one when updating debouncedSearch (the logic that sets the
timeout and calls setDebouncedSearch should clear searchTimerRef.current via
clearTimeout and set it to null). Locate the debounce logic referencing
searchTimerRef and setDebouncedSearch in ProviderSelectDrawer and add a
useEffect(() => () => { if (searchTimerRef.current) {
clearTimeout(searchTimerRef.current); searchTimerRef.current = null; } }) to
avoid state updates after unmount.
In `@console/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsx`:
- Around line 837-857: The TextInput block uses
authInfoByEnv[selectedEnvName].name and .value without ensuring the key exists,
which can throw; update the rendering logic in ViewLLMProvider.Component to
check that authInfoByEnv has a truthy entry for selectedEnvName (e.g.,
authInfoByEnv[selectedEnvName]) before accessing .name/.value or provide safe
fallbacks; specifically, guard the template-building lines that reference
authInfoByEnv[selectedEnvName] (or compute a local const like authEntry =
authInfoByEnv?.[selectedEnvName] and use authEntry?.name || "<header>" and
authEntry?.value || "<api-key>") so the TextInput value never dereferences
undefined.
---
Outside diff comments:
In `@console/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsx`:
- Around line 325-352: The useMemo block that computes isDirty references
pendingProviderByEnv but the dependency array only lists [config, envVarNames,
guardrailsByEnv]; add pendingProviderByEnv to that array so React recalculates
isDirty when providers change (update the dependency list for the useMemo that
defines isDirty to include pendingProviderByEnv alongside config, envVarNames,
and guardrailsByEnv).
---
Nitpick comments:
In `@console/workspaces/pages/configure-agent/src/AddLLMProvider.Component.tsx`:
- Around line 257-259: Remove the now-unused local search state from
AddLLMProvider by deleting providerSearchQuery, searchTimerRef, and
debouncedSearch and any associated imports/usages; the search is handled inside
ProviderSelectDrawer so remove these state declarations (providerSearchQuery,
setProviderSearchQuery, searchTimerRef, debouncedSearch, setDebouncedSearch) and
any references to them in the AddLLMProvider component to avoid dead code and
unused refs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f4f9aac3-1d5e-4910-8c02-13ff005f14bf
📒 Files selected for processing (5)
console/workspaces/pages/configure-agent/src/AddLLMProvider.Component.tsxconsole/workspaces/pages/configure-agent/src/Configure.Component.tsxconsole/workspaces/pages/configure-agent/src/Configure/subComponents/AgentLLMProvidersSection.tsxconsole/workspaces/pages/configure-agent/src/ProviderSelectDrawer.tsxconsole/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsx
Comment on lines
+59
to
+71
| const [searchQuery, setSearchQuery] = useState(""); | ||
| const [debouncedSearch, setDebouncedSearch] = useState(""); | ||
| const searchTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null); | ||
|
|
||
| const filtered = providers.filter((p) => { | ||
| if (!debouncedSearch.trim()) return true; | ||
| const q = debouncedSearch.toLowerCase(); | ||
| return ( | ||
| p.name.toLowerCase().includes(q) || | ||
| (p.template ?? "").toLowerCase().includes(q) || | ||
| (templateMap.get(p.template ?? "")?.displayName ?? "").toLowerCase().includes(q) | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
Clear debounce timer on unmount to prevent state updates on unmounted component.
The searchTimerRef timeout is not cleared when the component unmounts, which can trigger React warnings if the callback fires after unmount.
Proposed fix
+ useEffect(() => {
+ return () => {
+ if (searchTimerRef.current) clearTimeout(searchTimerRef.current);
+ };
+ }, []);
+
const filtered = providers.filter((p) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@console/workspaces/pages/configure-agent/src/ProviderSelectDrawer.tsx` around
lines 59 - 71, The debounce timeout held in searchTimerRef can fire after
unmount and update state; add a cleanup useEffect that clears the timeout and
nulls searchTimerRef on unmount, and also ensure any existing timer is cleared
before setting a new one when updating debouncedSearch (the logic that sets the
timeout and calls setDebouncedSearch should clear searchTimerRef.current via
clearTimeout and set it to null). Locate the debounce logic referencing
searchTimerRef and setDebouncedSearch in ProviderSelectDrawer and add a
useEffect(() => () => { if (searchTimerRef.current) {
clearTimeout(searchTimerRef.current); searchTimerRef.current = null; } }) to
avoid state updates after unmount.
menakaj
force-pushed
the
agent-llm-config-ux-improvement
branch
from
0ed7161 to
decda11
Compare
rasika2012
previously approved these changes
May 26, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
console/workspaces/pages/configure-agent/src/Configure/subComponents/AgentLLMProvidersSection.tsx (1)
78-86:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd optional chaining for
typeproperty.The filter calls
.toLowerCase()onc.typewithout checking if it exists. Iftypeisundefinedornull, this will throw a runtime error.🛡️ Proposed fix to add optional chaining
const filteredConfigs = useMemo(() => { if (!searchValue.trim()) return configs; const lower = searchValue.toLowerCase(); return configs.filter( (c) => c.name.toLowerCase().includes(lower) || - c.type.toLowerCase().includes(lower), + c.type?.toLowerCase().includes(lower), ); }, [configs, searchValue]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@console/workspaces/pages/configure-agent/src/Configure/subComponents/AgentLLMProvidersSection.tsx` around lines 78 - 86, filteredConfigs useMemo filter currently calls c.type.toLowerCase() without guarding against undefined/null; update the filter in the useMemo (the filteredConfigs calculation) to use optional chaining or a safe fallback for c.type (e.g., (c.type ?? '').toLowerCase() or c.type?.toLowerCase()) when checking includes so missing types don't cause a runtime error while preserving existing name/type matching logic.console/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsx (2)
487-492:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock empty environment-variable names before save.
The drawer lets the user clear a variable name, and
handleSavetrims and sends it anyway. A whitespace-only rename becomes"", which makes the injected variable unusable and breaks the generated snippets. Please validate these inputs and keep Save disabled until all names are non-empty.Also applies to: 610-621
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@console/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsx` around lines 487 - 492, The environment variable name handling currently allows whitespace-only names because handleSave trims and accepts empty strings; update the validation so environment variable names are trimmed and must be non-empty before enabling Save and before building environmentVariables. In the ViewLLMProvider.Component functions that produce environmentVariables (the mapping shown) and the code paths around handleSave, reject or filter out entries whose name.trim() === "" and set the Save button disabled state when any env var name is empty; apply the same fix to the second similar block (around lines 610-621) so both creation and update flows validate names and prevent saving until all names are non-empty.
325-352:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
isDirtymemo dependencies to includependingProviderByEnv(console/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsx, lines ~325-352)
isDirtyreadspendingProviderByEnv(Object.keys(pendingProviderByEnv).length) but theuseMemodependency array excludes it, so provider-only edits may not update the “unsaved changes” state.Suggested fix
- }, [config, envVarNames, guardrailsByEnv]); + }, [config, envVarNames, guardrailsByEnv, pendingProviderByEnv]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@console/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsx` around lines 325 - 352, The useMemo computing isDirty reads pendingProviderByEnv (via Object.keys(pendingProviderByEnv).length) but does not include it in the dependency array, causing provider-only edits to not mark the form dirty; update the useMemo dependency list for isDirty to include pendingProviderByEnv (in addition to config, envVarNames, guardrailsByEnv) so changes to pendingProviderByEnv retrigger the memo.
♻️ Duplicate comments (1)
console/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsx (1)
837-857:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the cURL preview by the selected environment entry.
This block still only checks
authInfoByEnv, but Line 842 dereferencesauthInfoByEnv[selectedEnvName]. Switching to an environment without auth info will throw during render.Suggested fix
- {authInfoByEnv && ( + {authInfoByEnv?.[selectedEnvName] && ( <TextInput label="Example cURL" value={[ `curl -X POST ${providerConfig.url || "http://<endpoint-url>"}`, ` --header "${authInfoByEnv[selectedEnvName].name}: ${authInfoByEnv[selectedEnvName].value || "<api-key>"}"`, ` -d '{"your": "data"}'`, ].join(" \\\n")}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@console/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsx` around lines 837 - 857, The TextInput cURL preview dereferences authInfoByEnv[selectedEnvName] without ensuring that entry exists; update the render guard to check that authInfoByEnv[selectedEnvName] is defined (e.g., const authEntry = authInfoByEnv?.[selectedEnvName]) before rendering the TextInput, and use that authEntry when building the curl header (falling back to "<api-key>" if authEntry.value is missing); ensure the conditional uses authEntry (not just authInfoByEnv) so switching to an env without auth info won't throw during render.
🧹 Nitpick comments (2)
console/workspaces/pages/configure-agent/src/Configure/subComponents/AgentLLMProvidersSection.tsx (1)
242-244: 💤 Low valueConsider extracting name formatting logic to a helper function.
The name formatting logic (removing trailing digits and replacing dashes) is inline and could benefit from extraction for better testability and potential reuse across the codebase.
♻️ Optional refactor to extract formatting logic
Create a helper function at the top of the file:
const formatConfigDisplayName = (name: string): string => { return name.replace(/-\d+$/, "").replace(/-/g, " "); };Then update the usage:
- <Typography variant="body2" fontWeight={500} sx={{ textTransform: "capitalize" }}> - {config.name.replace(/-\d+$/, "").replace(/-/g, " ")} - </Typography> + <Typography variant="body2" fontWeight={500} sx={{ textTransform: "capitalize" }}> + {formatConfigDisplayName(config.name)} + </Typography>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@console/workspaces/pages/configure-agent/src/Configure/subComponents/AgentLLMProvidersSection.tsx` around lines 242 - 244, Extract the inline name formatting from the Typography render into a helper function named formatConfigDisplayName, which accepts the raw config.name and returns name.replace(/-\d+$/, "").replace(/-/g, " "); add this helper at the top of the file and replace the inline expression in the JSX (where config.name is currently transformed inside the Typography component) with a call to formatConfigDisplayName(config.name) so the logic is testable and reusable.console/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsx (1)
893-898: ⚡ Quick winUse theme spacing for the edit-button offset.
top: 8andright: 8hardcode spacing insx. Please switch these to theme spacing tokens so this card stays aligned with the rest of the console surfaces.As per coding guidelines, "Use theme tokens via the
sxprop instead of hardcoded colors and spacing values".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@console/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsx` around lines 893 - 898, The IconButton's sx currently uses hardcoded pixel offsets (top: 8, right: 8); change these to theme spacing tokens so the positioning follows the design system. Update the sx on the IconButton (the element that calls setProviderDrawerOpen) to use theme spacing, e.g. top: (theme) => theme.spacing(1) and right: (theme) => theme.spacing(1) or equivalent numeric spacing values (top: 1, right: 1) so the button aligns with other console surfaces and adheres to the sx/theme token guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@console/workspaces/pages/configure-agent/src/AddLLMProvider.Component.tsx`:
- Around line 449-454: The code reads providerName from
existingConfig.envMappings[env].configuration?.providerName but handleSave and
this component store providerName at the env mapping root, so update all reads
to use existingConfig.envMappings?.[envName]?.providerName (not under
configuration); specifically change the edit-mode fallback in
AddLLMProvider.Component (the block checking existingProviderName), and the
checks referenced by hasAnyProvider and allEnvsHaveProvider to read
envMapping.providerName directly so out-of-catalog providers are preserved and
the Save/warning logic behaves correctly.
- Around line 745-775: The provider selection can be saved under an empty key
because selectedEnvName may be "" while useListEnvironments() is still
resolving; update the UI logic so selection is only allowed when a real
environment exists: disable or prevent opening the ProviderSelectDrawer when
environments.length === 0 or selectedEnvName is falsy (e.g., change the Button
disabled condition and/or gate ProviderSelectDrawer render), and guard the
onSelect handler (used to call setProviderByEnv) to no-op unless selectedEnvName
is non-empty so setProviderByEnv((prev) => ({ ...prev, [selectedEnvName]: uuid
})) is never called with an empty key.
---
Outside diff comments:
In
`@console/workspaces/pages/configure-agent/src/Configure/subComponents/AgentLLMProvidersSection.tsx`:
- Around line 78-86: filteredConfigs useMemo filter currently calls
c.type.toLowerCase() without guarding against undefined/null; update the filter
in the useMemo (the filteredConfigs calculation) to use optional chaining or a
safe fallback for c.type (e.g., (c.type ?? '').toLowerCase() or
c.type?.toLowerCase()) when checking includes so missing types don't cause a
runtime error while preserving existing name/type matching logic.
In `@console/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsx`:
- Around line 487-492: The environment variable name handling currently allows
whitespace-only names because handleSave trims and accepts empty strings; update
the validation so environment variable names are trimmed and must be non-empty
before enabling Save and before building environmentVariables. In the
ViewLLMProvider.Component functions that produce environmentVariables (the
mapping shown) and the code paths around handleSave, reject or filter out
entries whose name.trim() === "" and set the Save button disabled state when any
env var name is empty; apply the same fix to the second similar block (around
lines 610-621) so both creation and update flows validate names and prevent
saving until all names are non-empty.
- Around line 325-352: The useMemo computing isDirty reads pendingProviderByEnv
(via Object.keys(pendingProviderByEnv).length) but does not include it in the
dependency array, causing provider-only edits to not mark the form dirty; update
the useMemo dependency list for isDirty to include pendingProviderByEnv (in
addition to config, envVarNames, guardrailsByEnv) so changes to
pendingProviderByEnv retrigger the memo.
---
Duplicate comments:
In `@console/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsx`:
- Around line 837-857: The TextInput cURL preview dereferences
authInfoByEnv[selectedEnvName] without ensuring that entry exists; update the
render guard to check that authInfoByEnv[selectedEnvName] is defined (e.g.,
const authEntry = authInfoByEnv?.[selectedEnvName]) before rendering the
TextInput, and use that authEntry when building the curl header (falling back to
"<api-key>" if authEntry.value is missing); ensure the conditional uses
authEntry (not just authInfoByEnv) so switching to an env without auth info
won't throw during render.
---
Nitpick comments:
In
`@console/workspaces/pages/configure-agent/src/Configure/subComponents/AgentLLMProvidersSection.tsx`:
- Around line 242-244: Extract the inline name formatting from the Typography
render into a helper function named formatConfigDisplayName, which accepts the
raw config.name and returns name.replace(/-\d+$/, "").replace(/-/g, " "); add
this helper at the top of the file and replace the inline expression in the JSX
(where config.name is currently transformed inside the Typography component)
with a call to formatConfigDisplayName(config.name) so the logic is testable and
reusable.
In `@console/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsx`:
- Around line 893-898: The IconButton's sx currently uses hardcoded pixel
offsets (top: 8, right: 8); change these to theme spacing tokens so the
positioning follows the design system. Update the sx on the IconButton (the
element that calls setProviderDrawerOpen) to use theme spacing, e.g. top:
(theme) => theme.spacing(1) and right: (theme) => theme.spacing(1) or equivalent
numeric spacing values (top: 1, right: 1) so the button aligns with other
console surfaces and adheres to the sx/theme token guideline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6ae4239f-93fe-44e6-8024-2df96cc6503a
📒 Files selected for processing (5)
console/workspaces/pages/configure-agent/src/AddLLMProvider.Component.tsxconsole/workspaces/pages/configure-agent/src/Configure.Component.tsxconsole/workspaces/pages/configure-agent/src/Configure/subComponents/AgentLLMProvidersSection.tsxconsole/workspaces/pages/configure-agent/src/ProviderSelectDrawer.tsxconsole/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsx
Comment on lines
449
to
454
| } else if (isEditMode && existingConfig) { | ||
| // Provider not in current catalog page — preserve existing mapping | ||
| // to avoid dropping providers beyond the catalog page limit. | ||
| const existingMapping = existingConfig.envMappings?.[env.name]; | ||
| const existingProviderName = | ||
| existingMapping?.configuration?.providerName; | ||
| const existingProviderName = existingMapping?.configuration?.providerName; | ||
| if (existingProviderName) { |
Contributor
There was a problem hiding this comment.
Read providerName from the env mapping itself, not from configuration.
handleSave() writes providerName alongside configuration, but the edit-mode fallback and the hasAnyProvider / allEnvsHaveProvider checks read existingConfig.envMappings[env].configuration?.providerName. For mappings created by this component, that path is empty, so out-of-catalog providers are treated as missing: the warning indicator stays on, Save stays disabled, and the preservation branch never runs.
Suggested fix
- const existingProviderName = existingMapping?.configuration?.providerName;
+ const existingProviderName =
+ existingMapping?.providerName ??
+ existingMapping?.configuration?.providerName;
@@
- return !!existing?.configuration?.providerName;
+ return !!(existing?.providerName ?? existing?.configuration?.providerName);
@@
- return !!existing?.configuration?.providerName;
+ return !!(existing?.providerName ?? existing?.configuration?.providerName);
@@
- isEditMode && !!existingConfig?.envMappings?.[env.name]?.configuration?.providerName
+ isEditMode &&
+ !!(
+ existingConfig?.envMappings?.[env.name]?.providerName ??
+ existingConfig?.envMappings?.[env.name]?.configuration?.providerName
+ )Also applies to: 571-590, 669-672
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@console/workspaces/pages/configure-agent/src/AddLLMProvider.Component.tsx`
around lines 449 - 454, The code reads providerName from
existingConfig.envMappings[env].configuration?.providerName but handleSave and
this component store providerName at the env mapping root, so update all reads
to use existingConfig.envMappings?.[envName]?.providerName (not under
configuration); specifically change the edit-mode fallback in
AddLLMProvider.Component (the block checking existingProviderName), and the
checks referenced by hasAnyProvider and allEnvsHaveProvider to read
envMapping.providerName directly so out-of-catalog providers are preserved and
the Save/warning logic behaves correctly.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
console/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsx (1)
503-516: 💤 Low valueUnused dependencies in callback array.
navigateandbackHrefare included in the dependency array but are not used in thehandleSavecallback body. This causes unnecessary callback re-creation when these values change.♻️ Remove unused dependencies
}, [ orgId, projectId, agentId, configId, config, guardrailsByEnv, envVarNames, pendingProviderByEnv, providers, updateConfig, - navigate, - backHref, ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@console/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsx` around lines 503 - 516, Unused dependencies navigate and backHref are causing unnecessary re-creations of the handleSave callback; remove navigate and backHref from the useCallback dependency array (the array that currently lists orgId, projectId, agentId, configId, config, guardrailsByEnv, envVarNames, pendingProviderByEnv, providers, updateConfig, navigate, backHref) unless you actually reference them inside handleSave—if you need them, use them in the function body, otherwise delete them from the array to stabilize handleSave.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@console/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsx`:
- Around line 503-516: Unused dependencies navigate and backHref are causing
unnecessary re-creations of the handleSave callback; remove navigate and
backHref from the useCallback dependency array (the array that currently lists
orgId, projectId, agentId, configId, config, guardrailsByEnv, envVarNames,
pendingProviderByEnv, providers, updateConfig, navigate, backHref) unless you
actually reference them inside handleSave—if you need them, use them in the
function body, otherwise delete them from the array to stabilize handleSave.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f0318252-e37f-472b-9bdf-99438d2c4306
📒 Files selected for processing (3)
console/workspaces/libs/shared-component/src/components/CodeBlock.tsxconsole/workspaces/pages/configure-agent/src/AddLLMProvider.Component.tsxconsole/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsx
✅ Files skipped from review due to trivial changes (1)
- console/workspaces/libs/shared-component/src/components/CodeBlock.tsx
rasika2012
previously approved these changes
May 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Fixes #935
Remove the name and description fields and rename Add LLM Provider to Add LLM Configuration
Moved the environment configuration and code snippet to right drawer - not cluttring the ui and Add visual indications for un-saved changes
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
New Features
UI/UX Changes
Bug Fixes / Behavior