Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded EnvVar.IsSet and used it for persistence decisions; expanded provider key schemas with optional _auth_type and cross-field refinements; introduced per-provider auth-method UI state/tabs and submit sanitization; adjusted key-status rendering for env-var issues; added unit and DB round-trip tests. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
impoiler
force-pushed
the
04-06-fix_validation_in_all_provider_configuration_forms
branch
from
f67113e to
5665c43
Compare
impoiler
changed the title
impoiler
force-pushed
the
graphite-base/2529
branch
from
bd776f8 to
48868d4
Compare
impoiler
force-pushed
the
04-06-fix_validation_in_all_provider_configuration_forms
branch
from
5665c43 to
3c2678d
Compare
impoiler
force-pushed
the
04-06-fix_validation_in_all_provider_configuration_forms
branch
4 times, most recently
from
9eeae8e to
982888d
Compare
Contributor
Confidence Score: 4/5Safe to merge after fixing the Azure auth-tab detection bug; all other changes are correct and well-tested. One P1 defect: the Azure useEffect auth-type detection fires on new-key creation with empty form values and forces the tab to 'Default Credential', which can cause unintended provider configuration. The Go-side changes (IsSet(), BeforeSave rewrites, encryption tests) are solid. Score is 4 rather than 5 solely because of that one current behavioral defect in the UI. ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx — Azure auth-type detection useEffect (lines 102–120)
|
| Filename | Overview |
|---|---|
| core/schemas/envvar.go | Adds IsSet() method (correct logic, but semantically overlaps with existing IsDefined()); also adds MarshalJSON referenced by new tests. |
| core/schemas/envvar_test.go | Comprehensive tests for IsSet(), MarshalJSON auto-redaction, non-mutation, and round-trip; test cases cover nil, empty, env-backed, and plain-value states correctly. |
| framework/configstore/tables/key.go | Replaces GetValue() != "" with IsSet() in BeforeSave for Azure, Vertex, Bedrock, VLLM, Ollama, and SGL fields — correctly preserves unresolved env-var references during persistence. |
| framework/configstore/tables/encryption_test.go | New encryption round-trip tests for all provider configs using in-memory SQLite; covers both encrypt/decrypt and raw-DB verification. |
| ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx | Auth-type detection useEffect for Azure runs on new-key creation with empty form values and incorrectly switches the selected tab from "API Key" to "Default Credential"; Vertex and Bedrock defaults are unaffected. |
| ui/app/workspace/providers/views/providerKeyForm.tsx | Correctly strips internal _auth_type fields from Azure, Vertex, and Bedrock configs before submitting to the API; form validation and dirty-state guards look correct. |
| ui/app/workspace/providers/views/modelProviderKeysTableView.tsx | Adds env-var heuristic to show an orange warning icon instead of red error for keys whose list_models failures may be due to unresolved env vars. |
| ui/lib/types/schemas.ts | Adds global Zod error map, isEnvVarSet helper, and auth-method-aware refinements for Azure, Vertex, and Bedrock schemas; validation logic correctly gates required fields on the selected _auth_type. |
Reviews (8): Last reviewed commit: "Fix: Validation in all provider configur..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
impoiler
force-pushed
the
04-06-fix_validation_in_all_provider_configuration_forms
branch
from
982888d to
ca3b8b7
Compare
![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: 4
🧹 Nitpick comments (1)
core/schemas/envvar.go (1)
265-274: MakeIsDefined()delegate toIsSet().This adds a second public “is this configured?” check with slightly different semantics.
IsSet()treatsenv_varas configured even whenFromEnvis stale/missing, whileIsDefined()below still depends onFromEnv, so future call sites can easily reintroduce the same unresolved-env bug this PR is fixing.♻️ Proposed fix
func (e *EnvVar) IsDefined() bool { - if e == nil { - return false - } - if e.IsFromEnv() { - return e.EnvVar != "" - } - return e.Val != "" + return e.IsSet() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/envvar.go` around lines 265 - 274, The IsDefined() method should delegate to IsSet() to ensure both public checks share the same semantics; update the EnvVar.IsDefined() implementation to return e.IsSet() (handling nil receiver) instead of checking FromEnv or other fields so that env var references are considered configured even when unresolved, and remove any lingering FromEnv-dependent logic in IsDefined().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/configstore/tables/key.go`:
- Line 133: The OllamaKeyConfig.URL and SGLKeyConfig.URL branches still check
GetValue() != "" and thus clear stored env.* references when the env var is
currently unset; change those checks to use EnvVar.IsSet() (the same pattern
used for AzureKeyConfig.Endpoint.IsSet()) so unresolved env references are
preserved on round-trip—update the checks around the OllamaKeyConfig.URL and
SGLKeyConfig.URL handling (also the similar spots noted at the other ranges) to
rely on IsSet() instead of GetValue() != "".
In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx`:
- Around line 102-120: The effect that computes Azure auth type (the useEffect
block that reads form.getValues and calls setAzureAuthType and
form.setValue("key.azure_key_config._auth_type")) only depends on isAzure and
the stable form reference, so it never reruns after form.reset() populates
values; update the effect to also depend on the actual form fields (use
form.watch or add the specific watched keys like
"key.azure_key_config.client_id", "key.azure_key_config.client_secret",
"key.azure_key_config.tenant_id", and "key.value") so the effect re-evaluates
when reset/populated values arrive, and make the same change for the equivalent
Vertex and Bedrock effects (the blocks that set detected auth types and call
setXAuthType / form.setValue for their _auth_type fields).
In `@ui/app/workspace/providers/views/modelProviderKeysTableView.tsx`:
- Around line 176-185: The env-var detection in hasEnvVarConfig is too narrow
and misses many auth fields; extend the boolean to also check Azure Entra
credential fields (e.g. key.azure_key_config?.client_id?.from_env,
key.azure_key_config?.client_secret?.from_env,
key.azure_key_config?.tenant_id?.from_env and similar Entra/credential
subfields), Vertex additional fields (e.g.
key.vertex_key_config?.auth_credentials?.*?.from_env,
key.vertex_key_config?.project_number?.from_env), Bedrock explicit and
assume-role fields (e.g. key.bedrock_key_config?.credentials?.from_env,
key.bedrock_key_config?.assume_role?.*?.from_env), and other model provider
URL/auth fields like Ollama/SGL (e.g. key.ollama_key_config?.url?.from_env,
key.sgl_key_config?.url?.from_env), so that hasEnvVarConfig (and thus
isEnvResolutionError) flags all env-backed credential and URL fields referenced
by the key object.
In `@ui/app/workspace/providers/views/providerKeyForm.tsx`:
- Around line 80-103: The client-side code in providerKeyForm.tsx is removing
provider-specific _auth_type fields (azure_key_config, vertex_key_config,
bedrock_key_config) before calling createProviderKey/updateProviderKey, which
hides the auth selection from server-side validation; stop stripping those
fields so the server handlers
(transports/bifrost-http/handlers/provider_keys.go) receive the full payload and
can enforce proper auth-combination validation. Concretely, remove the
destructuring that drops _auth_type (the three blocks that rewrite
key.azure_key_config, key.vertex_key_config, key.bedrock_key_config) and send
the original key object to createProviderKey/updateProviderKey so the server can
validate the auth type, or alternatively ensure the server handler implements
equivalent checks before reintroducing client-side stripping.
---
Nitpick comments:
In `@core/schemas/envvar.go`:
- Around line 265-274: The IsDefined() method should delegate to IsSet() to
ensure both public checks share the same semantics; update the
EnvVar.IsDefined() implementation to return e.IsSet() (handling nil receiver)
instead of checking FromEnv or other fields so that env var references are
considered configured even when unresolved, and remove any lingering
FromEnv-dependent logic in IsDefined().
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: faded5da-fcd8-4f92-bc32-11a9f7a038fe
📒 Files selected for processing (6)
core/schemas/envvar.goframework/configstore/tables/key.goui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/app/workspace/providers/views/providerKeyForm.tsxui/lib/types/schemas.ts
11 tasks
impoiler
force-pushed
the
04-06-fix_validation_in_all_provider_configuration_forms
branch
from
ca3b8b7 to
b02ecee
Compare
impoiler
force-pushed
the
04-06-fix_validation_in_all_provider_configuration_forms
branch
2 times, most recently
from
a1fc8b1 to
47c6f82
Compare
Contributor
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)
ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx (1)
429-444:⚠️ Potential issue | 🟡 MinorDead conditional in label text.
The label at line 436 checks
isVertexandisVLLM, but this code is inside the{isAzure && (...)}block where both will always befalse. The conditional is unreachable and the label will always render as just "API Key".🔧 Proposed fix
{azureAuthType === "api_key" && ( <FormField control={control} name={`key.value`} render={({ field }) => ( <FormItem> - <FormLabel> - API Key {isVertex ? "(Supported only for gemini and fine-tuned models)" : isVLLM ? "(Optional)" : ""} - </FormLabel> + <FormLabel>API Key</FormLabel> <FormControl> <EnvVarInput placeholder="API Key or env.MY_KEY" type="text" {...field} /> </FormControl> <FormMessage /> </FormItem> )} /> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx` around lines 429 - 444, The API Key label contains an unreachable conditional (isVertex/isVLLM) because this JSX is already inside the isAzure block; update the label in the FormField render for name `key.value` to remove the dead checks (isVertex/isVLLM) and render a static "API Key" or move the conditional logic to the parent that determines when those suffixes should appear (use the existing isAzure / isVertex / isVLLM flags to decide at a higher level). Target the FormField rendering block where `azureAuthType === "api_key"` and adjust the FormLabel text accordingly.
🧹 Nitpick comments (1)
ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx (1)
14-14: Remove unusedTextareaimport.The
Textareacomponent is imported but never used in this file. The textarea functionality is achieved viaEnvVarInputwithvariant="textarea"at line 650.🧹 Proposed fix
-import { Textarea } from "@/components/ui/textarea";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx` at line 14, Remove the unused import of Textarea from this module: locate the import statement "import { Textarea } from "@/components/ui/textarea"; and delete it because the file uses EnvVarInput with variant="textarea" (see EnvVarInput) for textarea functionality and Textarea is not referenced elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/configstore/tables/encryption_test.go`:
- Around line 1819-1848: The AfterFind method currently only reconstructs
AzureKeyConfig when AzureEndpoint is non-nil; update the guard in AfterFind so
it checks for presence of any Azure-related stored fields (e.g., AzureEndpoint,
AzureAPIVersion, AzureClientID, AzureClientSecret, AzureTenantID,
AzureScopesJSON) and reconstructs schemas.AzureKeyConfig whenever any of these
is non-nil; locate the reconstruction logic in the AfterFind method and replace
the single-field check (AzureEndpoint) with a combined OR condition across those
Azure* fields so APIVersion-only rows correctly rebuild AzureKeyConfig.
---
Outside diff comments:
In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx`:
- Around line 429-444: The API Key label contains an unreachable conditional
(isVertex/isVLLM) because this JSX is already inside the isAzure block; update
the label in the FormField render for name `key.value` to remove the dead checks
(isVertex/isVLLM) and render a static "API Key" or move the conditional logic to
the parent that determines when those suffixes should appear (use the existing
isAzure / isVertex / isVLLM flags to decide at a higher level). Target the
FormField rendering block where `azureAuthType === "api_key"` and adjust the
FormLabel text accordingly.
---
Nitpick comments:
In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx`:
- Line 14: Remove the unused import of Textarea from this module: locate the
import statement "import { Textarea } from "@/components/ui/textarea"; and
delete it because the file uses EnvVarInput with variant="textarea" (see
EnvVarInput) for textarea functionality and Textarea is not referenced
elsewhere.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b16493ca-30c1-462c-804a-dcd712dce9a8
📒 Files selected for processing (8)
core/schemas/envvar.gocore/schemas/envvar_test.goframework/configstore/tables/encryption_test.goframework/configstore/tables/key.goui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/app/workspace/providers/views/providerKeyForm.tsxui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (2)
- ui/app/workspace/providers/views/modelProviderKeysTableView.tsx
- framework/configstore/tables/key.go
🚧 Files skipped from review as they are similar to previous changes (2)
- core/schemas/envvar.go
- ui/app/workspace/providers/views/providerKeyForm.tsx
impoiler
force-pushed
the
04-06-fix_validation_in_all_provider_configuration_forms
branch
from
47c6f82 to
8882712
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx (2)
14-14: Unused import:Textareais imported but not directly used in this file.The
EnvVarInputcomponent withvariant="textarea"handles its own rendering internally. This import appears to be leftover from development or a refactor.🧹 Remove unused import
-import { Textarea } from "@/components/ui/textarea";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx` at line 14, Remove the unused Textarea import: the file imports Textarea but rendering is handled inside the EnvVarInput component (when using variant="textarea"), so delete the import statement for Textarea to eliminate the unused symbol and keep the module clean; search for the Textarea import line and remove it while leaving EnvVarInput and any usages of variant="textarea" intact.
568-578: Validation timing is acceptable but could be optimized.The
shouldValidate: trueon_auth_typetriggers validation before the subsequent field clears. This works correctly because:
- When switching away from
service_account_json, the refinement no longer requiresauth_credentials- When switching to
service_account_json, validation will show the required field error (expected UX)However, for smoother UX, you could defer validation until all field updates complete:
💡 Optional: Batch updates to avoid intermediate validation states
onValueChange={(v) => { setVertexAuthType(v as "service_account" | "service_account_json" | "api_key"); - form.setValue("key.vertex_key_config._auth_type", v, { shouldDirty: true, shouldValidate: true }); + form.setValue("key.vertex_key_config._auth_type", v, { shouldDirty: true }); if (v === "service_account" || v === "api_key") { - form.setValue("key.vertex_key_config.auth_credentials", undefined, { shouldDirty: true }); + form.setValue("key.vertex_key_config.auth_credentials", undefined, { shouldDirty: true }); } if (v === "service_account" || v === "service_account_json") { - form.setValue("key.value", undefined, { shouldDirty: true }); + form.setValue("key.value", undefined, { shouldDirty: true, shouldValidate: true }); } + // Trigger validation after all updates + if (v === "service_account" || v === "api_key") { + form.trigger("key.vertex_key_config"); + } }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx` around lines 568 - 578, The validation on setting _auth_type fires too early and can run before the dependent fields are cleared; update the onValueChange in setVertexAuthType to batch updates: call form.setValue("key.vertex_key_config._auth_type", v, { shouldDirty: true, shouldValidate: false }), then perform the conditional clears via form.setValue(...) for "key.vertex_key_config.auth_credentials" and "key.value" as needed, and finally call form.trigger(...) or form.validate() once (e.g., trigger the "key" namespace or the specific affected fields) so validation runs after all updates complete.ui/app/workspace/providers/views/modelProviderKeysTableView.tsx (1)
176-215: Env-var detection heuristic covers common cases but omits some fields.The
hasEnvVarConfigcheck now includes Vertexproject_idandregion, which addresses the previous review feedback. However, these fields are still not checked:
- Azure Entra credentials (
client_id,client_secret,tenant_id)- Vertex
auth_credentials- Bedrock
access_key/secret_key- Ollama/SGL
urlSince this is a UX heuristic (not a security check), the current coverage may be sufficient for the most common cases. If you want complete coverage, consider extracting a helper that recursively checks all
from_envfields in the key's config objects.💡 Optional: More comprehensive env-var detection
const hasEnvVarConfig = key.azure_key_config?.endpoint?.from_env || + key.azure_key_config?.client_id?.from_env || + key.azure_key_config?.client_secret?.from_env || + key.azure_key_config?.tenant_id?.from_env || key.vertex_key_config?.project_id?.from_env || key.vertex_key_config?.region?.from_env || + key.vertex_key_config?.auth_credentials?.from_env || key.bedrock_key_config?.region?.from_env || + key.bedrock_key_config?.access_key?.from_env || + key.bedrock_key_config?.secret_key?.from_env || key.vllm_key_config?.url?.from_env || + key.ollama_key_config?.url?.from_env || + key.sgl_key_config?.url?.from_env || key.value?.from_env;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/views/modelProviderKeysTableView.tsx` around lines 176 - 215, The env-var detection heuristic in modelProviderKeysTableView.tsx (the hasEnvVarConfig/isEnvResolutionError logic) misses several config fields (Azure client_id/client_secret/tenant_id, Vertex auth_credentials, Bedrock access_key/secret_key, Ollama/SGL url); replace the ad-hoc boolean expression with a small helper (e.g., hasEnvVarInConfig(config): boolean) that recursively traverses the key object (use the existing key.* config objects referenced in the diff) and returns true if any property has a from_env flag or is a value object with from_env; then use that helper in place of hasEnvVarConfig to compute isEnvResolutionError so all present env-backed fields are detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx`:
- Line 14: Remove the unused Textarea import: the file imports Textarea but
rendering is handled inside the EnvVarInput component (when using
variant="textarea"), so delete the import statement for Textarea to eliminate
the unused symbol and keep the module clean; search for the Textarea import line
and remove it while leaving EnvVarInput and any usages of variant="textarea"
intact.
- Around line 568-578: The validation on setting _auth_type fires too early and
can run before the dependent fields are cleared; update the onValueChange in
setVertexAuthType to batch updates: call
form.setValue("key.vertex_key_config._auth_type", v, { shouldDirty: true,
shouldValidate: false }), then perform the conditional clears via
form.setValue(...) for "key.vertex_key_config.auth_credentials" and "key.value"
as needed, and finally call form.trigger(...) or form.validate() once (e.g.,
trigger the "key" namespace or the specific affected fields) so validation runs
after all updates complete.
In `@ui/app/workspace/providers/views/modelProviderKeysTableView.tsx`:
- Around line 176-215: The env-var detection heuristic in
modelProviderKeysTableView.tsx (the hasEnvVarConfig/isEnvResolutionError logic)
misses several config fields (Azure client_id/client_secret/tenant_id, Vertex
auth_credentials, Bedrock access_key/secret_key, Ollama/SGL url); replace the
ad-hoc boolean expression with a small helper (e.g., hasEnvVarInConfig(config):
boolean) that recursively traverses the key object (use the existing key.*
config objects referenced in the diff) and returns true if any property has a
from_env flag or is a value object with from_env; then use that helper in place
of hasEnvVarConfig to compute isEnvResolutionError so all present env-backed
fields are detected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 16ad0a62-ab4e-4284-a5bb-5e172872a03b
📒 Files selected for processing (8)
core/schemas/envvar.gocore/schemas/envvar_test.goframework/configstore/tables/encryption_test.goframework/configstore/tables/key.goui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/app/workspace/providers/views/providerKeyForm.tsxui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (2)
- framework/configstore/tables/key.go
- framework/configstore/tables/encryption_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/app/workspace/providers/views/providerKeyForm.tsx
- core/schemas/envvar_test.go
impoiler
force-pushed
the
04-06-fix_validation_in_all_provider_configuration_forms
branch
from
8882712 to
2c660e6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
ui/app/workspace/providers/views/modelProviderKeysTableView.tsx (1)
176-185:⚠️ Potential issue | 🟡 Minor
hasEnvVarConfigmissing Ollama and SGL URL checks.The Go tests added in this PR (
TestTableKey_OllamaUnresolvedEnvVar_RoundTripandTestTableKey_SGLUnresolvedEnvVar_RoundTrip) explicitly verify that Ollama/SGL URL env-var references persist through round-trips. However, the UI detection logic doesn't check these providers:const hasEnvVarConfig = key.azure_key_config?.endpoint?.from_env || key.vertex_key_config?.project_id?.from_env || key.vertex_key_config?.region?.from_env || key.bedrock_key_config?.region?.from_env || key.vllm_key_config?.url?.from_env || + key.ollama_key_config?.url?.from_env || + key.sgl_key_config?.url?.from_env || key.value?.from_env;Without this, Ollama/SGL keys with unresolved env vars will show the generic error icon instead of the env-var warning, even when the backend description matches the resolution-error pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/views/modelProviderKeysTableView.tsx` around lines 176 - 185, The env-var resolution detection is missing checks for Ollama and SGL provider URLs, so update the hasEnvVarConfig computation (used alongside isEnvResolutionError) to include key.ollama_key_config?.url?.from_env and key.sgl_key_config?.url?.from_env in the OR chain (alongside the existing checks like key.azure_key_config?.endpoint?.from_env, key.vertex_key_config?.project_id?.from_env, key.vertex_key_config?.region?.from_env, key.bedrock_key_config?.region?.from_env, key.vllm_key_config?.url?.from_env, and key.value?.from_env) so unresolved Ollama/SGL env-var references correctly trigger the env-var warning icon.ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx (1)
102-120:⚠️ Potential issue | 🟠 MajorRecompute the detected auth mode from watched field values.
ProviderKeyFormrepopulates the form aftercurrentKeyarrives, but these three effects still only depend on the provider flags and the stableformobject. In edit flows, that can leave Azure/Vertex/Bedrock on the default tab and write the wrong hidden_auth_typeafterform.reset(...).You can verify the reset/effect mismatch with:
#!/bin/bash set -e printf '%s\n' 'Parent reset flow:' sed -n '54,66p' ui/app/workspace/providers/views/providerKeyForm.tsx printf '\n%s\n' 'Auth detection effects:' sed -n '101,157p' ui/app/workspace/providers/fragments/apiKeysFormFragment.tsxExpected: these effects should also depend on watched auth-related values (for example via
useWatch) so they rerun afterform.reset(...).Also applies to: 122-138, 140-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx` around lines 102 - 120, The effect that detects Azure auth mode (the useEffect that reads form.getValues and calls setAzureAuthType and form.setValue("key.azure_key_config._auth_type", ...)) only depends on isAzure and the stable form object, so it doesn't recompute after ProviderKeyForm calls form.reset(...); fix this by watching the real auth-related form fields (e.g., via react-hook-form's useWatch for "key.azure_key_config.client_id", "key.azure_key_config.client_secret", "key.azure_key_config.tenant_id", and "key.value") and include those watched values in the effect dependencies so the detection reruns after resets; apply the same change pattern to the corresponding Vertex/Bedrock detection effects (the other useEffect blocks that call setVertexAuthType/setBedrockAuthType and set the _auth_type hidden fields).
🧹 Nitpick comments (1)
ui/app/workspace/providers/views/providerKeyForm.tsx (1)
80-93: Move_auth_typesanitization to a shared request boundary.This screen strips the UI-only auth markers, but
ui/app/workspace/providers/views/modelProviderKeysTableView.tsx:232-235still callsupdateProviderKeywith a rawkey. A sharedsanitizeProviderKeyForApi()helper, or request types that omit_auth_type, will keep every mutation path consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/views/providerKeyForm.tsx` around lines 80 - 93, Create a shared sanitization helper (e.g., sanitizeProviderKeyForApi(key)) that removes any UI-only _auth_type fields from nested configs (azure_key_config, vertex_key_config, bedrock_key_config) and replace the inline stripping logic in providerKeyForm.tsx with a call to that helper; then update any other mutation call sites (notably updateProviderKey invocation in modelProviderKeysTableView.tsx) to pass the sanitized object instead of the raw key so all API-bound requests use the same sanitized shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx`:
- Around line 649-655: The new Vertex auth inputs (the EnvVarInput instance used
for auth_credentials and the Vertex API-key input referenced nearby) lack stable
selectors for E2E tests; add data-testid attributes following the pattern
data-testid="<entity>-<element>-<qualifier>" to both interactive inputs—e.g.,
add data-testid values like "vertex-auth_credentials-input" to the EnvVarInput
and "vertex-apiKey-input" (or similar consistent names) to the API-key input so
tests can reliably target them; update both occurrences (the EnvVarInput at the
shown fragment and the inputs around lines 677–678) to include these attributes.
In `@ui/lib/types/schemas.ts`:
- Around line 65-69: The helper isEnvVarSet currently trims values and env_var
before checking, causing a mismatch with backend EnvVar.IsSet(); change it to
consider any non-empty raw string as set (no trimming) so that whitespace-only
strings count as set like core/schemas/envvar.go does—update the function that
accepts { value?: string; env_var?: string } | undefined (isEnvVarSet) to return
true when v.value or v.env_var is not strictly empty/undefined without calling
.trim(), preserving the undefined check but removing whitespace trimming logic.
- Around line 235-245: The transform on the weight field currently uses
parseFloat which accepts numeric prefixes (e.g., "0.5abc"); update the
validation in the transform((val, ctx) => { ... }) block to reject
partially-numeric strings by converting using Number(val) and checking
Number.isFinite(num) (after handling empty-string -> 1.0 and numeric inputs),
and if invalid call ctx.addIssue with the same message and return z.NEVER; keep
references to val, ctx, transform, Number.isFinite and z.NEVER so you replace
parseFloat-based logic with this stricter check.
---
Duplicate comments:
In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx`:
- Around line 102-120: The effect that detects Azure auth mode (the useEffect
that reads form.getValues and calls setAzureAuthType and
form.setValue("key.azure_key_config._auth_type", ...)) only depends on isAzure
and the stable form object, so it doesn't recompute after ProviderKeyForm calls
form.reset(...); fix this by watching the real auth-related form fields (e.g.,
via react-hook-form's useWatch for "key.azure_key_config.client_id",
"key.azure_key_config.client_secret", "key.azure_key_config.tenant_id", and
"key.value") and include those watched values in the effect dependencies so the
detection reruns after resets; apply the same change pattern to the
corresponding Vertex/Bedrock detection effects (the other useEffect blocks that
call setVertexAuthType/setBedrockAuthType and set the _auth_type hidden fields).
In `@ui/app/workspace/providers/views/modelProviderKeysTableView.tsx`:
- Around line 176-185: The env-var resolution detection is missing checks for
Ollama and SGL provider URLs, so update the hasEnvVarConfig computation (used
alongside isEnvResolutionError) to include key.ollama_key_config?.url?.from_env
and key.sgl_key_config?.url?.from_env in the OR chain (alongside the existing
checks like key.azure_key_config?.endpoint?.from_env,
key.vertex_key_config?.project_id?.from_env,
key.vertex_key_config?.region?.from_env,
key.bedrock_key_config?.region?.from_env, key.vllm_key_config?.url?.from_env,
and key.value?.from_env) so unresolved Ollama/SGL env-var references correctly
trigger the env-var warning icon.
---
Nitpick comments:
In `@ui/app/workspace/providers/views/providerKeyForm.tsx`:
- Around line 80-93: Create a shared sanitization helper (e.g.,
sanitizeProviderKeyForApi(key)) that removes any UI-only _auth_type fields from
nested configs (azure_key_config, vertex_key_config, bedrock_key_config) and
replace the inline stripping logic in providerKeyForm.tsx with a call to that
helper; then update any other mutation call sites (notably updateProviderKey
invocation in modelProviderKeysTableView.tsx) to pass the sanitized object
instead of the raw key so all API-bound requests use the same sanitized shape.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a834793-5563-441a-b287-8aa595e5fbc7
📒 Files selected for processing (8)
core/schemas/envvar.gocore/schemas/envvar_test.goframework/configstore/tables/encryption_test.goframework/configstore/tables/key.goui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/app/workspace/providers/views/providerKeyForm.tsxui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- core/schemas/envvar.go
- framework/configstore/tables/key.go
- core/schemas/envvar_test.go
impoiler
force-pushed
the
04-06-fix_validation_in_all_provider_configuration_forms
branch
from
2c660e6 to
bf47ff7
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
ui/lib/types/schemas.ts (2)
235-245:⚠️ Potential issue | 🟡 MinorReject partially numeric weight strings.
Line 238 uses
parseFloat(), so inputs like"0.5abc"are normalized to0.5and still pass the later0..1range check. UseNumber()plusNumber.isFinite()here so only fully numeric strings are accepted.Suggested fix
.transform((val, ctx) => { if (typeof val === "number") return val; if (val.trim() === "") return 1.0; - const num = parseFloat(val); - if (isNaN(num)) { + const num = Number(val); + if (!Number.isFinite(num)) { ctx.addIssue({ code: "custom", message: "Weight must be a valid number between 0 and 1", }); return z.NEVER; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/lib/types/schemas.ts` around lines 235 - 245, The transform in the schema currently uses parseFloat (inside the .transform callback) which allows partially-numeric strings like "0.5abc"; change the parsing to use Number(val) and validate with Number.isFinite(parsed) so only fully numeric strings are accepted, keep the existing trim-empty-string => 1.0 behavior, and if Number.isFinite returns false call ctx.addIssue (same message) and return z.NEVER; update the transform logic around the typeof val === "number" check and the ctx.addIssue/z.NEVER paths accordingly.
66-69:⚠️ Potential issue | 🟡 MinorKeep
isEnvVarSet()aligned with the new backendEnvVar.IsSet()semantics.Line 68 trims before checking, so whitespace-only raw
value/env_varentries are treated as unset here even though the Go-sideIsSet()now treats any non-empty raw string as configured. That reintroduces a UI/backend mismatch in every refine that now depends on this helper.Suggested fix
function isEnvVarSet(v: { value?: string; env_var?: string } | undefined): boolean { if (!v) return false; - return !!v.value?.trim() || !!v.env_var?.trim(); + return (v.value ?? "") !== "" || (v.env_var ?? "") !== ""; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/lib/types/schemas.ts` around lines 66 - 69, isEnvVarSet currently trims before checking which treats whitespace-only strings as unset, but the backend EnvVar.IsSet considers any non-empty raw string as configured; update isEnvVarSet to consider a value/env_var as set if it is non-null/undefined and has length > 0 (do not trim or ignore whitespace). Locate the isEnvVarSet function and replace the trim-based checks (v.value?.trim(), v.env_var?.trim()) with non-empty raw-string checks (e.g., v.value !== undefined && v.value.length > 0 and same for v.env_var) so the UI matches backend EnvVar.IsSet semantics.ui/app/workspace/providers/views/modelProviderKeysTableView.tsx (1)
178-185:⚠️ Potential issue | 🟡 MinorCover all env-backed auth fields in
hasEnvVarConfig.Line 178 only checks endpoint/project_id/region/VLLM URL/top-level value. That misses Azure
api_version+ Entra fields, Vertexauth_credentials/project_number, Bedrock explicit + role fields, and Ollama/SGL URLs, so those failures still fall through to the generic red error state instead of the env-resolution warning this PR is adding.Suggested fix
- const hasEnvVarConfig = - key.azure_key_config?.endpoint?.from_env || - key.vertex_key_config?.project_id?.from_env || - key.vertex_key_config?.region?.from_env || - key.bedrock_key_config?.region?.from_env || - key.vllm_key_config?.url?.from_env || - key.value?.from_env; + const hasEnvRef = (value: unknown): boolean => { + if (!value || typeof value !== "object") return false; + if ("from_env" in value && Boolean((value as { from_env?: boolean }).from_env)) { + return true; + } + return Object.values(value as Record<string, unknown>).some(hasEnvRef); + }; + + const hasEnvVarConfig = [ + key.value, + key.azure_key_config, + key.vertex_key_config, + key.bedrock_key_config, + key.vllm_key_config, + key.ollama_key_config, + key.sgl_key_config, + ].some(hasEnvRef);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/views/modelProviderKeysTableView.tsx` around lines 178 - 185, Update the hasEnvVarConfig boolean in modelProviderKeysTableView.tsx to include all env-backed auth fields so env-resolution errors are detected: extend the OR chain that currently references key.azure_key_config?.endpoint, key.azure_key_config?.api_version, and all Entra-related fields (e.g., client_id/client_secret/tenant_id if present) to check their .from_env; add Vertex checks for key.vertex_key_config?.auth_credentials?.from_env and ?.project_number?.from_env in addition to project_id/region; include Bedrock explicit endpoint and role fields (e.g., key.bedrock_key_config?.endpoint?.from_env and any role-related .from_env flags); and add Ollama/SGL URL fields (e.g., key.ollama_key_config?.url?.from_env and key.sgl_key_config?.url?.from_env); keep the isEnvResolutionError logic the same but rely on the expanded hasEnvVarConfig.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ui/app/workspace/providers/views/modelProviderKeysTableView.tsx`:
- Around line 178-185: Update the hasEnvVarConfig boolean in
modelProviderKeysTableView.tsx to include all env-backed auth fields so
env-resolution errors are detected: extend the OR chain that currently
references key.azure_key_config?.endpoint, key.azure_key_config?.api_version,
and all Entra-related fields (e.g., client_id/client_secret/tenant_id if
present) to check their .from_env; add Vertex checks for
key.vertex_key_config?.auth_credentials?.from_env and ?.project_number?.from_env
in addition to project_id/region; include Bedrock explicit endpoint and role
fields (e.g., key.bedrock_key_config?.endpoint?.from_env and any role-related
.from_env flags); and add Ollama/SGL URL fields (e.g.,
key.ollama_key_config?.url?.from_env and key.sgl_key_config?.url?.from_env);
keep the isEnvResolutionError logic the same but rely on the expanded
hasEnvVarConfig.
In `@ui/lib/types/schemas.ts`:
- Around line 235-245: The transform in the schema currently uses parseFloat
(inside the .transform callback) which allows partially-numeric strings like
"0.5abc"; change the parsing to use Number(val) and validate with
Number.isFinite(parsed) so only fully numeric strings are accepted, keep the
existing trim-empty-string => 1.0 behavior, and if Number.isFinite returns false
call ctx.addIssue (same message) and return z.NEVER; update the transform logic
around the typeof val === "number" check and the ctx.addIssue/z.NEVER paths
accordingly.
- Around line 66-69: isEnvVarSet currently trims before checking which treats
whitespace-only strings as unset, but the backend EnvVar.IsSet considers any
non-empty raw string as configured; update isEnvVarSet to consider a
value/env_var as set if it is non-null/undefined and has length > 0 (do not trim
or ignore whitespace). Locate the isEnvVarSet function and replace the
trim-based checks (v.value?.trim(), v.env_var?.trim()) with non-empty raw-string
checks (e.g., v.value !== undefined && v.value.length > 0 and same for
v.env_var) so the UI matches backend EnvVar.IsSet semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04f83897-f13b-4ee1-bec8-f04f820f208b
📒 Files selected for processing (8)
core/schemas/envvar.gocore/schemas/envvar_test.goframework/configstore/tables/encryption_test.goframework/configstore/tables/key.goui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/app/workspace/providers/views/providerKeyForm.tsxui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (1)
- ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- core/schemas/envvar.go
- framework/configstore/tables/key.go
- core/schemas/envvar_test.go
- ui/app/workspace/providers/views/providerKeyForm.tsx
impoiler
force-pushed
the
04-06-fix_validation_in_all_provider_configuration_forms
branch
from
bf47ff7 to
20058ea
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
ui/lib/types/schemas.ts (2)
65-68:⚠️ Potential issue | 🟠 MajorKeep
isEnvVarSet()byte-for-byte aligned withEnvVar.IsSet().The backend now treats any non-empty raw
valueorenv_varas configured. Trimming here makes whitespace-only inputs count as unset on the client, so validation can still reject or clear fields thatcore/schemas/envvar.go/framework/configstore/tables/key.gowill preserve.🛠️ Proposed fix
function isEnvVarSet(v: { value?: string; env_var?: string } | undefined): boolean { if (!v) return false; - return !!v.value?.trim() || !!v.env_var?.trim(); + return (v.value ?? "") !== "" || (v.env_var ?? "") !== ""; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/lib/types/schemas.ts` around lines 65 - 68, The client helper isEnvVarSet currently trims value/env_var which diverges from the backend's EnvVar.IsSet behavior; update isEnvVarSet (the function named isEnvVarSet) to consider any non-empty raw string as set by removing the .trim() calls and using simple existence checks (e.g., check v.value and v.env_var for non-empty strings) so the client is byte-for-byte aligned with EnvVar.IsSet.
233-250:⚠️ Potential issue | 🟡 MinorThis stricter parser is still bypassed by the form blur handler.
At Lines 206-212 in
ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx, the weight input still callsparseFloat()on blur and writes the numeric prefix back into form state. That means"0.5abc"becomes0.5before this schema runs, so the newNumber()check only helps submit-without-blur cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/lib/types/schemas.ts` around lines 233 - 250, The blur handler that currently calls parseFloat on the weight input is bypassing the stricter zod parser—locate the weight input's onBlur (the handler using parseFloat) and stop coercing the field to a numeric prefix; instead, change the blur logic to use Number(val) and only write the numeric value back into form state if Number.isFinite(num), otherwise leave the raw string in the input so the zod transform (which uses Number(...) and rejects non-numeric strings) runs on validation; update any calls to parseFloat(...) to this Number + Number.isFinite check (or remove the coercion entirely and rely on your form library's validation trigger) so "0.5abc" is preserved until zod validates it.ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx (1)
101-157:⚠️ Potential issue | 🟠 MajorRecompute auth type after
form.reset()populates an existing key.These effects only depend on the provider flags and the stable
formobject. If edit values arrive after mount, none of them rerun, so Azure/Vertex/Bedrock stay on their defaults and_auth_typecan disagree with the loaded credentials. That hides the wrong fields and can force the wrong validation branch; for example, an existing Azure Entra ID key can still be treated asapi_key.Watching the relevant credential fields (or deriving the tab directly from watched values) will keep the UI and resolver state in sync.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx` around lines 101 - 157, The effects that detect auth type (the useEffect blocks that call setAzureAuthType/setVertexAuthType/setBedrockAuthType and form.setValue) only depend on isAzure/isVertex/isBedrock and the stable form object, so they don't rerun when form.reset() populates values; update each effect to depend on the actual credential fields (or use form.watch) instead of only the provider flag so they recompute when values change—specifically watch the Azure fields "key.azure_key_config.client_id", "key.azure_key_config.client_secret", "key.azure_key_config.tenant_id", and "key.value" in the Azure effect, the Vertex fields "key.vertex_key_config.auth_credentials" and "key.value" in the Vertex effect, and the Bedrock fields "key.bedrock_key_config.access_key", "key.bedrock_key_config.secret_key", and "key.value" in the Bedrock effect, then run the same detection logic and call setAzureAuthType/setVertexAuthType/setBedrockAuthType and form.setValue("key.*._auth_type", detected) so UI and resolver state stay in sync after form.reset().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/lib/types/schemas.ts`:
- Around line 20-37: The custom error mapping checks (for issue.code ===
"too_small" and "too_big") use issue.type which in Zod v4 became issue.origin,
so updates are needed: change the conditional inspections from issue.type to
issue.origin for the relevant branches (the checks that compare to "string",
"number", and "array") so the string/number/array branches in the error handling
logic correctly match Zod v4.2.1 error shapes (keep the same comparisons to
"string"/"number"/"array" and preserve the existing minimum/maximum checks and
return messages).
---
Duplicate comments:
In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx`:
- Around line 101-157: The effects that detect auth type (the useEffect blocks
that call setAzureAuthType/setVertexAuthType/setBedrockAuthType and
form.setValue) only depend on isAzure/isVertex/isBedrock and the stable form
object, so they don't rerun when form.reset() populates values; update each
effect to depend on the actual credential fields (or use form.watch) instead of
only the provider flag so they recompute when values change—specifically watch
the Azure fields "key.azure_key_config.client_id",
"key.azure_key_config.client_secret", "key.azure_key_config.tenant_id", and
"key.value" in the Azure effect, the Vertex fields
"key.vertex_key_config.auth_credentials" and "key.value" in the Vertex effect,
and the Bedrock fields "key.bedrock_key_config.access_key",
"key.bedrock_key_config.secret_key", and "key.value" in the Bedrock effect, then
run the same detection logic and call
setAzureAuthType/setVertexAuthType/setBedrockAuthType and
form.setValue("key.*._auth_type", detected) so UI and resolver state stay in
sync after form.reset().
In `@ui/lib/types/schemas.ts`:
- Around line 65-68: The client helper isEnvVarSet currently trims value/env_var
which diverges from the backend's EnvVar.IsSet behavior; update isEnvVarSet (the
function named isEnvVarSet) to consider any non-empty raw string as set by
removing the .trim() calls and using simple existence checks (e.g., check
v.value and v.env_var for non-empty strings) so the client is byte-for-byte
aligned with EnvVar.IsSet.
- Around line 233-250: The blur handler that currently calls parseFloat on the
weight input is bypassing the stricter zod parser—locate the weight input's
onBlur (the handler using parseFloat) and stop coercing the field to a numeric
prefix; instead, change the blur logic to use Number(val) and only write the
numeric value back into form state if Number.isFinite(num), otherwise leave the
raw string in the input so the zod transform (which uses Number(...) and rejects
non-numeric strings) runs on validation; update any calls to parseFloat(...) to
this Number + Number.isFinite check (or remove the coercion entirely and rely on
your form library's validation trigger) so "0.5abc" is preserved until zod
validates it.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d434cd7a-d6ab-4c1c-aab0-94b18bef4b18
📒 Files selected for processing (8)
core/schemas/envvar.gocore/schemas/envvar_test.goframework/configstore/tables/encryption_test.goframework/configstore/tables/key.goui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/app/workspace/providers/views/providerKeyForm.tsxui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- ui/app/workspace/providers/views/providerKeyForm.tsx
- framework/configstore/tables/key.go
- ui/app/workspace/providers/views/modelProviderKeysTableView.tsx
- core/schemas/envvar_test.go
impoiler
force-pushed
the
04-06-fix_validation_in_all_provider_configuration_forms
branch
from
20058ea to
bf2c06c
Compare
Pratham-Mishra04
approved these changes
Apr 8, 2026
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/app/workspace/providers/views/providerKeyForm.tsx (1)
131-139:⚠️ Potential issue | 🟡 MinorRemoving
isValidfrom the disabled condition allows submitting invalid forms.The Save button is now only disabled when
!form.formState.isDirty, meaning users can click Save on a dirty but invalid form. While Zod validation will still run on submit and show errors, this degrades UX by allowing an avoidable failed submission attempt.Consider restoring the validity check:
<Button type="submit" - disabled={!form.formState.isDirty} + disabled={!form.formState.isDirty || !form.formState.isValid} isLoading={form.formState.isSubmitting || isCreatingProviderKey || isUpdatingProviderKey}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/views/providerKeyForm.tsx` around lines 131 - 139, The Save Button currently disables only when !form.formState.isDirty, allowing submissions of dirty but invalid forms; update the disabled condition on the Button (data-testid "key-save-btn") to also check form.formState.isValid (e.g., disable when !form.formState.isDirty || !form.formState.isValid) so the button remains disabled for invalid forms while keeping existing isLoading checks (form.formState.isSubmitting, isCreatingProviderKey, isUpdatingProviderKey) intact.
♻️ Duplicate comments (1)
ui/lib/types/schemas.ts (1)
65-69:⚠️ Potential issue | 🟡 MinorThe
trim()calls create a semantic mismatch with the backendIsSet().The Go
EnvVar.IsSet()method checkse.Val != "" || e.EnvVar != ""without trimming whitespace. This helper trims before checking, meaning whitespace-only values are treated as "not set" in the UI but "set" in the backend. This can cause the form to reject or clear fields thatBeforeSaveconsiders configured.🛠️ Suggested fix to match backend semantics
function isEnvVarSet(v: { value?: string; env_var?: string } | undefined): boolean { if (!v) return false; - return !!v.value?.trim() || !!v.env_var?.trim(); + return (v.value ?? "") !== "" || (v.env_var ?? "") !== ""; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/lib/types/schemas.ts` around lines 65 - 69, The helper isEnvVarSet currently trims values which differs from backend EnvVar.IsSet(); change isEnvVarSet to match backend semantics by checking presence without trimming: return false if v is undefined/null, otherwise treat a value as set when v.value !== "" or v.env_var !== "" (i.e., use direct empty-string comparisons on the { value?: string; env_var?: string } fields instead of .trim()) so UI and backend agree on whitespace-only values being "set".
🧹 Nitpick comments (1)
core/schemas/envvar.go (1)
316-324: Consider documenting the distinction betweenIsSet()andIsDefined().Both methods exist with overlapping but different semantics:
IsSet()(new): returns true if eitherValorEnvVaris non-emptyIsDefined(): returns true ifEnvVaris non-empty whenFromEnv=true, else ifValis non-emptyThis subtle difference could confuse callers. A brief doc comment on
IsDefined()clarifying when to use each would help maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/envvar.go` around lines 316 - 324, Add a clarifying doc comment on EnvVar.IsDefined() that explains the difference from IsSet(): state that IsSet() returns true if either Val or EnvVar is non-empty, whereas IsDefined() respects the FromEnv flag (when IsFromEnv() is true it only considers EnvVar non-empty, otherwise it only considers Val non-empty); mention when callers should prefer IsDefined() (respecting explicit source) versus IsSet() (any set value) and reference the fields EnvVar, Val and the helper IsFromEnv() in the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@ui/app/workspace/providers/views/providerKeyForm.tsx`:
- Around line 131-139: The Save Button currently disables only when
!form.formState.isDirty, allowing submissions of dirty but invalid forms; update
the disabled condition on the Button (data-testid "key-save-btn") to also check
form.formState.isValid (e.g., disable when !form.formState.isDirty ||
!form.formState.isValid) so the button remains disabled for invalid forms while
keeping existing isLoading checks (form.formState.isSubmitting,
isCreatingProviderKey, isUpdatingProviderKey) intact.
---
Duplicate comments:
In `@ui/lib/types/schemas.ts`:
- Around line 65-69: The helper isEnvVarSet currently trims values which differs
from backend EnvVar.IsSet(); change isEnvVarSet to match backend semantics by
checking presence without trimming: return false if v is undefined/null,
otherwise treat a value as set when v.value !== "" or v.env_var !== "" (i.e.,
use direct empty-string comparisons on the { value?: string; env_var?: string }
fields instead of .trim()) so UI and backend agree on whitespace-only values
being "set".
---
Nitpick comments:
In `@core/schemas/envvar.go`:
- Around line 316-324: Add a clarifying doc comment on EnvVar.IsDefined() that
explains the difference from IsSet(): state that IsSet() returns true if either
Val or EnvVar is non-empty, whereas IsDefined() respects the FromEnv flag (when
IsFromEnv() is true it only considers EnvVar non-empty, otherwise it only
considers Val non-empty); mention when callers should prefer IsDefined()
(respecting explicit source) versus IsSet() (any set value) and reference the
fields EnvVar, Val and the helper IsFromEnv() in the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd83ff19-2596-436e-aded-1bf72c94067f
📒 Files selected for processing (8)
core/schemas/envvar.gocore/schemas/envvar_test.goframework/configstore/tables/encryption_test.goframework/configstore/tables/key.goui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/app/workspace/providers/views/providerKeyForm.tsxui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- framework/configstore/tables/key.go
- ui/app/workspace/providers/views/modelProviderKeysTableView.tsx
- ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx
- framework/configstore/tables/encryption_test.go
impoiler
force-pushed
the
04-06-fix_validation_in_all_provider_configuration_forms
branch
from
bf2c06c to
2c064ab
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
ui/lib/types/schemas.ts (1)
65-69:⚠️ Potential issue | 🟠 MajorMatch backend
EnvVar.IsSet()semantics in this helper.This still trims before checking presence. The Go side now treats any non-empty raw
value/env_varas configured, so this reintroduces a frontend/backend mismatch and can make the form reject or clear fields that persistence would keep.Suggested fix
function isEnvVarSet(v: { value?: string; env_var?: string } | undefined): boolean { if (!v) return false; - return !!v.value?.trim() || !!v.env_var?.trim(); + return (v.value ?? "") !== "" || (v.env_var ?? "") !== ""; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/lib/types/schemas.ts` around lines 65 - 69, The helper isEnvVarSet currently trims fields before checking which mismatches backend semantics; update it to consider any non-empty raw string as set (including whitespace). In function isEnvVarSet, stop using .trim() and instead return true if v is defined and (v.value !== undefined && v.value !== '') or (v.env_var !== undefined && v.env_var !== ''), so presence is based on raw non-empty strings to match backend EnvVar.IsSet().ui/app/workspace/providers/views/modelProviderKeysTableView.tsx (1)
178-184:⚠️ Potential issue | 🟡 MinorExpand
hasEnvVarConfigto the rest of the auth fields.This still misses env-backed inputs added by the new auth modes—e.g. Azure
api_version/Entra fields, Vertexproject_number/auth_credentials, Bedrock credential or role fields, and Ollama/SGL URLs—so those resolution failures still fall back to the generic error icon.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/views/modelProviderKeysTableView.tsx` around lines 178 - 184, The current hasEnvVarConfig boolean in modelProviderKeysTableView.tsx only checks a subset of env-backed auth inputs and needs to be expanded to include all new auth-mode fields so the UI detects env-resolved keys correctly; update the expression that defines hasEnvVarConfig (the constant inside the component rendering the keys table) to also OR-in checks for Azure's api_version and any Entra-related fields (e.g., tenant/client id/secret flags), Vertex's project_number and auth_credentials.from_env, Bedrock's credential and role fields, and Ollama/SGL url fields (and any other new provider URL/credential flags added) so the boolean reflects any env-backed source across all provider configs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/workspace/providers/views/modelProviderKeysTableView.tsx`:
- Around line 185-195: The view currently infers env-var resolution failures by
regexing key.description (the isEnvResolutionError variable) which is brittle;
instead update the API/type to include a typed failure reason (e.g.,
key.failureReason or key.resolutionStatus enum with values like "EnvVarNotSet")
and change the component logic in modelProviderKeysTableView.tsx to check that
typed field (e.g., key.failureReason === 'EnvVarNotSet') before rendering the
Tooltip/AlertCircle and use key.description only as human text inside the
tooltip; also update the TypeScript types/interfaces for Key to include the new
field and make the UI conservative (do not show env-var warning if the typed
field is absent).
In `@ui/lib/types/schemas.ts`:
- Around line 191-199: The current refine for vllmKeyConfigSchema (and similarly
for the Ollama/SGL schemas around the other block) only checks presence via
isEnvVarSet, which lets raw strings like "localhost" pass; update the refine to
branch on whether url matches envVarSchema (use the same logic as isEnvVarSet
for env-var-backed values) and for direct string values attempt to
parse/validate them as real URLs (e.g., using the URL constructor or a strict
URL validation) and return false when parsing fails so invalid raw endpoints are
rejected; apply the same change to the corresponding schemas (the Ollama and SGL
config schemas) so env-var URLs remain presence-only but direct string URLs are
properly validated.
---
Duplicate comments:
In `@ui/app/workspace/providers/views/modelProviderKeysTableView.tsx`:
- Around line 178-184: The current hasEnvVarConfig boolean in
modelProviderKeysTableView.tsx only checks a subset of env-backed auth inputs
and needs to be expanded to include all new auth-mode fields so the UI detects
env-resolved keys correctly; update the expression that defines hasEnvVarConfig
(the constant inside the component rendering the keys table) to also OR-in
checks for Azure's api_version and any Entra-related fields (e.g., tenant/client
id/secret flags), Vertex's project_number and auth_credentials.from_env,
Bedrock's credential and role fields, and Ollama/SGL url fields (and any other
new provider URL/credential flags added) so the boolean reflects any env-backed
source across all provider configs.
In `@ui/lib/types/schemas.ts`:
- Around line 65-69: The helper isEnvVarSet currently trims fields before
checking which mismatches backend semantics; update it to consider any non-empty
raw string as set (including whitespace). In function isEnvVarSet, stop using
.trim() and instead return true if v is defined and (v.value !== undefined &&
v.value !== '') or (v.env_var !== undefined && v.env_var !== ''), so presence is
based on raw non-empty strings to match backend EnvVar.IsSet().
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3cd8bee-91c4-433f-a157-7bb7d491eca5
📒 Files selected for processing (8)
core/schemas/envvar.gocore/schemas/envvar_test.goframework/configstore/tables/encryption_test.goframework/configstore/tables/key.goui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/app/workspace/providers/views/providerKeyForm.tsxui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (1)
- framework/configstore/tables/encryption_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- ui/app/workspace/providers/views/providerKeyForm.tsx
- core/schemas/envvar.go
- framework/configstore/tables/key.go
- core/schemas/envvar_test.go
- ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx
sammaji
reviewed
Apr 8, 2026
Comment on lines
+265
to
+274
| // IsSet returns true if the EnvVar has a resolved value or an environment variable reference. | ||
| // This should be used instead of GetValue() != "" when checking whether a field was configured, | ||
| // because env var references may have an empty Val before resolution (e.g., when the env var | ||
| // is not available in the current environment). | ||
| func (e *EnvVar) IsSet() bool { | ||
| if e == nil { | ||
| return false | ||
| } | ||
| return e.Val != "" || e.EnvVar != "" | ||
| } |
Member
There was a problem hiding this comment.
@impoiler there is a utility function for this already - IsDefined
Contributor
Merge activity
|
akshaydeo
deleted the
04-06-fix_validation_in_all_provider_configuration_forms
branch
2 tasks
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
4 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.
Summary
Improves environment variable handling for provider configurations by adding a new
IsSet()method to check if an EnvVar has a value or environment variable reference, and enhances the UI with better authentication method selection and validation for Azure, Vertex AI, and Bedrock providers.Changes
IsSet()method toEnvVarstruct in Go to properly detect configured fields even when environment variables are unresolvedGetValue() != ""checks withIsSet()calls in database persistence layer to avoid incorrectly treating unresolved env vars as emptyType of change
Affected areas
How to test
Test environment variable handling and provider authentication methods:
Test scenarios:
Screenshots/Recordings
UI changes include new tabbed interface for selecting authentication methods for cloud providers, with contextual field visibility and improved validation messaging.
Breaking changes
Related issues
Improves provider configuration UX and fixes issues with environment variable detection in provider key persistence.
#2490
Security considerations
_auth_typefields are stripped before sending to API to avoid exposing UI stateChecklist
docs/contributing/README.mdand followed the guidelines