feat: enabled a key without deleting #992
Conversation
📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds an optional Enabled flag for provider API keys across schema, storage, transport, core selection logic, migrations, and UI; disabled keys are persisted, shown in redacted outputs, can be toggled in the UI, and are excluded during provider key selection. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User (Browser)
participant UI as Frontend
participant API as Bifrost HTTP
participant DB as Config Store
participant Core as Core (selection)
User->>UI: Toggle key Enabled switch
UI->>API: PATCH provider keys (enabled flag)
API->>DB: Persist updated TableKey.enabled
DB-->>API: OK
API-->>UI: Success response
UI-->>User: Show success toast
note over Core,DB: Request-time key selection
API->>Core: Request needing provider key
Core->>DB: List provider keys for provider
DB-->>Core: Return keys (include Enabled)
Core->>Core: Filter out keys where Enabled == false
Core-->>API: Return selected key or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (2)framework/configstore/migrations.go (2)
ui/app/workspace/providers/views/modelProviderKeysTableView.tsx (2)
🔇 Additional comments (6)
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}
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/views/modelProviderKeysTableView.tsx (1)
106-170: Treatundefinedas enabled in the UI to match backend default semantics.Right now:
checked={!!key.enabled}means keys with noenabledfield (backend omits it whenEnabled == nil) render as unchecked/disabled.disabled={!key.enabled}on the Edit action means those same keys have Edit disabled (!undefined === true).But the backend treats
Enabled == nilas “enabled” (default true), so existing/config‑file keys will show up as disabled+non‑editable in the UI while still being usable at runtime.You can fix this by normalizing the enabled state in the row:
{provider.keys.map((key, index) => { const isEnabled = key.enabled !== false; // nil/undefined => true return ( <TableRow key={index} /* ... */> {/* ... */} <TableCell> <Switch checked={isEnabled} disabled={!hasUpdateProviderAccess} onCheckedChange={(checked) => { updateProvider({ ...provider, keys: provider.keys.map((k, i) => i === index ? { ...k, enabled: checked } : k, ), }) .unwrap() .then(() => { toast.success(`Key ${checked ? "enabled" : "disabled"} successfully`); }) .catch((err) => { toast.error("Failed to update key", { description: getErrorMessage(err), }); }); }} /> </TableCell> {/* ... */} <DropdownMenuItem onClick={() => setShowAddNewKeyDialog({ show: true, keyIndex: index })} disabled={!isEnabled} > <PencilIcon className="mr-1 h-4 w-4" /> Edit </DropdownMenuItem> {/* ... */} </TableRow> ); })}This keeps defaults consistent: keys without an explicit flag stay enabled in the UI, and only explicitly disabled keys block editing.
🧹 Nitpick comments (3)
core/schemas/account.go (1)
9-18: Ensure persistence ↔ schema mapping correctly setsEnabledfor disabled keys.Treating
Enabled *boolwithomitemptyand “nil means enabled” is a good fit for backward compatibility and for not emitting the field when it isn’t explicitly set.Just make sure all conversions into
schemas.KeysetEnabledwhen a key is disabled (e.g., when reading fromconfigstore/TableKey.Enabledor other storage structs). If those mappings don’t populateEnabledyet, disabled state could be silently dropped and the key would behave as enabled at runtime even if the DB says otherwise.Worth double‑checking the provider-config →
schemas.Keyconversions (e.g., intransports/bifrost-http/lib/config.goand any configstore adapters) to confirmEnabledis wired through.core/bifrost.go (1)
2465-2539: Disabled-key filtering looks correct, but ListModels path may still use disabled keys.The added checks:
if requestType == schemas.ListModelsRequest { for _, k := range keys { // Skip disabled keys if k.Enabled != nil && !*k.Enabled { continue } // existing value check... } } else { for _, key := range keys { // Skip disabled keys if key.Enabled != nil && !*key.Enabled { continue } // existing model/deployment checks... } }are consistent with
Enabled *boolsemantics (nil== enabled) and correctly prevent disabled keys from being chosen for non‑ListModels requests.However,
ListModelsRequestcurrently usesgetAllSupportedKeys:if providerRequiresKey(baseProvider, config.CustomProviderConfig) { keys, err = bifrost.getAllSupportedKeys(&ctx, req.Provider, baseProvider) // ... }and
getAllSupportedKeysdoes not filter byEnabledat all. That means disabled keys can still be used when listing models, even though they’re excluded from actual inference traffic.Two follow‑ups to consider:
Decide whether ListModels should also honor the Enabled flag. If yes, either:
- Filter in
getAllSupportedKeyssimilarly, or- Reuse
selectKeyFromProviderForModel(or a shared helper) for ListModels key selection.Double‑check call sites to ensure the
ListModelsRequestbranch inselectKeyFromProviderForModelis actually reachable; if ListModels never uses this helper, that branch is effectively dead code.Aligning the ListModels code path with the new Enabled semantics will avoid surprises where a “disabled” key is still exercised for list‑models calls.
transports/bifrost-http/lib/config.go (1)
1145-1151: IncludingEnabledin redacted provider keys aligns HTTP output with core/UISchema.Adding:
redactedConfig.Keys[i] = schemas.Key{ ID: key.ID, Name: key.Name, Models: key.Models, Weight: key.Weight, Enabled: key.Enabled, }ensures the redacted provider config exposes the Enabled flag end‑to‑end, matching
schemas.Keyand the frontendModelProviderKey.enabledfield.Because
Enabledis a*boolwithomitempty, existing configs without an explicit flag will still omit the field from JSON (backward compatible). Combined with the UI change to treatundefinedas “enabled” (see table view comment), this gives consistent semantics across storage, core, and transport.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/bifrost.go(1 hunks)core/schemas/account.go(1 hunks)framework/configstore/tables/key.go(1 hunks)transports/bifrost-http/lib/config.go(2 hunks)ui/app/workspace/providers/views/modelProviderKeysTableView.tsx(4 hunks)ui/lib/types/config.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
transports/bifrost-http/lib/config.go (1)
framework/configstore/tables/key.go (2)
TableKey(13-51)TableKey(54-54)
🔇 Additional comments (2)
ui/lib/types/config.ts (1)
72-93:ModelProviderKey.enabledshape and default align with Go schema.The optional
enabled?: booleanplusenabled: trueinDefaultModelProviderKeycleanly mirrorEnabled *bool "omitempty"on the Go side and ensure newly created keys are enabled by default. The remaining behavior questions are in how callers interpretundefinedvsfalse(covered in the view component).transports/bifrost-http/lib/config.go (1)
775-797: Virtual key key‑lookup behavior is reasonable and failure modes are explicit.The new lookup:
if keyRef.KeyID != "" { var existingKey configstoreTables.TableKey if err := tx.Where("key_id = ?", keyRef.KeyID).First(&existingKey).Error; err != nil { if errors.Is(err, gorm.ErrRecordNotFound) { logger.Warn("referenced key %s not found for virtual key %s", keyRef.KeyID, virtualKey.ID) continue } return fmt.Errorf("failed to lookup key %s for virtual key %s: %w", keyRef.KeyID, virtualKey.ID, err) } existingKeys = append(existingKeys, existingKey) }gives a good balance:
- Missing referenced keys are logged and skipped without killing the whole governance bootstrap.
- Unexpected DB errors abort the transaction with a clear error.
This should make virtual‑key initialization robust in the presence of stale references.
slyang08
force-pushed
the
11-17-feat-option-disable-key
branch
from
dce3e0c to
a128370
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/bifrost.go(1 hunks)core/schemas/account.go(1 hunks)framework/configstore/tables/key.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/schemas/account.go
🔇 Additional comments (2)
core/bifrost.go (2)
2507-2510: LGTM: Disabled keys correctly skipped in ListModels path.The logic properly checks for explicitly disabled keys (
Enabled != nil && !*Enabled) and treats unset values as enabled by default, which aligns with the feature objectives.
2517-2521: LGTM: Disabled keys correctly skipped in model selection path.The implementation consistently filters out disabled keys across both the ListModels path and the regular model selection path, correctly treating
nilas enabled by default.
|
@slyang08 thanks for the PR ❤️ . Have a look at CodeRabbit comments. Ill review as soon as these comments are resolved |
slyang08
force-pushed
the
11-17-feat-option-disable-key
branch
from
a128370 to
713254a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/bifrost.go (2)
2505-2515: Enabled flag handling for ListModelsRequest looks correct; consider consistency withgetAllSupportedKeysThe
Enabled != nil && !*Enabledguard is nil‑safe and preserves “nil means enabled” semantics, which matches the new field’s documented default. That part looks good.One thing to double‑check: List‑models requests that go through
getAllSupportedKeys(used inListModelsRequest) still ignore theEnabledflag, while this ListModels branch inselectKeyFromProviderForModelskips disabled keys. If the product expectation is “disabled keys are never used anywhere”, you may also want to filter them out ingetAllSupportedKeysfor consistency; if list‑models is intentionally allowed to use disabled keys, a short comment here or ongetAllSupportedKeysexplaining the distinction would help future readers.
2517-2523: Skipping disabled keys in main selection path is correct; small DRY helper could reduce duplicationThis guard correctly excludes disabled keys from consideration for normal requests while treating
Enabled == nilas enabled. That aligns with the new flag’s semantics.Given the same check now appears in both branches of this function (and may appear elsewhere later), consider extracting a tiny helper like
func isKeyEnabled(k schemas.Key) booland using it in both loops to keep the logic in one place. Not required, but it would reduce duplication and make future tweaks to enablement behavior safer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/bifrost.go(1 hunks)core/schemas/account.go(1 hunks)framework/configstore/tables/key.go(1 hunks)transports/bifrost-http/lib/config.go(1 hunks)ui/app/workspace/providers/views/modelProviderKeysTableView.tsx(4 hunks)ui/lib/types/config.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- framework/configstore/tables/key.go
- ui/app/workspace/providers/views/modelProviderKeysTableView.tsx
- ui/lib/types/config.ts
- core/schemas/account.go
- transports/bifrost-http/lib/config.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
core/bifrost.go
slyang08
force-pushed
the
11-17-feat-option-disable-key
branch
from
713254a to
ff082f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
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/modelProviderKeysTableView.tsx (1)
105-118: UpdatecolSpanto account for the new Enabled columnThe table now has four columns (
API Key,Weight,Enabled, actions), but the “No keys found.” row still usescolSpan={3}, which will misalign the empty state with the header.You can fix this by updating the
colSpan:- <TableRow> - <TableCell colSpan={3} className="py-6 text-center"> + <TableRow> + <TableCell colSpan={4} className="py-6 text-center"> No keys found. </TableCell> </TableRow>
🧹 Nitpick comments (1)
ui/app/workspace/providers/views/modelProviderKeysTableView.tsx (1)
133-153: Normalize enabled state once per row and reuse it for Switch and Edit gatingRight now
!!key.enabledis used directly, anddisabled={!key.enabled}on the Edit action. Ifkey.enabledis everundefinedwhile the backend treats “missing” as defaulttrue, this would render the key as disabled in the UI and also block editing.Consider deriving an
isKeyEnabledper row with a default-true semantic, and reusing it for both the Switch and the Edit menu item (and optionally RBAC gating), e.g.:- {provider.keys.map((key, index) => { - return ( + {provider.keys.map((key, index) => { + const isKeyEnabled = key.enabled ?? true; + return ( <TableRow key={index} className="text-sm transition-colors hover:bg-white" onClick={() => {}}> @@ - <TableCell> - <Switch - checked={!!key.enabled} - disabled={!hasUpdateProviderAccess} + <TableCell> + <Switch + checked={isKeyEnabled} + disabled={!hasUpdateProviderAccess} onCheckedChange={(checked) => { updateProvider({ ...provider, keys: provider.keys.map((k, i) => i === index ? { ...k, enabled: checked } : k ), }) @@ - <DropdownMenuItem + <DropdownMenuItem onClick={() => { setShowAddNewKeyDialog({ show: true, keyIndex: index }); }} - disabled={!key.enabled} + disabled={!hasUpdateProviderAccess || !isKeyEnabled} >(Optionally, you could also add
|| isUpdatingProviderto theSwitch’sdisabledprop to prevent rapid re-toggles during an in‑flight update.)Also applies to: 167-167
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/bifrost.go(1 hunks)core/schemas/account.go(1 hunks)framework/configstore/tables/key.go(3 hunks)transports/bifrost-http/lib/config.go(1 hunks)ui/app/workspace/providers/views/modelProviderKeysTableView.tsx(4 hunks)ui/lib/types/config.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- framework/configstore/tables/key.go
- core/bifrost.go
- core/schemas/account.go
- transports/bifrost-http/lib/config.go
- ui/lib/types/config.ts
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
ui/app/workspace/providers/views/modelProviderKeysTableView.tsx
🔇 Additional comments (1)
ui/app/workspace/providers/views/modelProviderKeysTableView.tsx (1)
17-17: Switch import is correct and appropriately scopedThe
Switchimport from your shared UI library is consistent with its usage below; no issues here.
slyang08
force-pushed
the
11-17-feat-option-disable-key
branch
from
ff082f8 to
4cd551e
Compare
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/views/modelProviderKeysTableView.tsx (1)
163-171: Add permission check to Edit menu item.The Edit menu item is only disabled based on
!key.enabledbut doesn't checkhasUpdateProviderAccess. A user without update permission could see the Edit option enabled (if the key is enabled) and potentially click it.Apply this diff to also gate the Edit option by user permissions:
<DropdownMenuItem onClick={() => { setShowAddNewKeyDialog({ show: true, keyIndex: index }); }} - disabled={!key.enabled} + disabled={!key.enabled || !hasUpdateProviderAccess} > <PencilIcon className="mr-1 h-4 w-4" /> Edit </DropdownMenuItem>This ensures consistency with other UI elements that check
hasUpdateProviderAccess.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/bifrost.go(1 hunks)core/schemas/account.go(1 hunks)framework/configstore/tables/key.go(3 hunks)transports/bifrost-http/lib/config.go(1 hunks)ui/app/workspace/providers/views/modelProviderKeysTableView.tsx(4 hunks)ui/lib/types/config.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- ui/lib/types/config.ts
- core/bifrost.go
- core/schemas/account.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
framework/configstore/tables/key.gotransports/bifrost-http/lib/config.goui/app/workspace/providers/views/modelProviderKeysTableView.tsx
🔇 Additional comments (5)
framework/configstore/tables/key.go (2)
68-71: LGTM!The defaulting logic correctly ensures that
Enabledis always set totruewhennilbefore saving to the database, maintaining consistency with the GORM default and preventing any ambiguity in the persisted state.
177-180: LGTM!The
AfterFindhook correctly applies the defaulttruevalue whenEnabledisnilafter loading from the database, providing a safety net and ensuring consistent in-memory representation.ui/app/workspace/providers/views/modelProviderKeysTableView.tsx (1)
115-115: LGTM!The
colSpancorrectly updated to4to match the new column count (API Key, Weight, Enabled, Actions).transports/bifrost-http/lib/config.go (2)
1386-1390: LGTM!Correctly includes the
Enabledfield in the redacted provider key output. The boolean/pointer value is not sensitive and does not require redaction.
342-352: Critical:Enabledfield not copied during database-to-schema conversion.When loading provider configurations from the database, the
Enabledfield is not copied fromdbKey(typeTableKey) to theschemas.Keystruct. This causes enabled keys to lose their enabled state in memory, and they will be treated as disabled (nil) throughout the system.Apply this diff to include the
Enabledfield in the conversion:keys[i] = schemas.Key{ ID: dbKey.ID, Name: dbKey.Name, Value: dbKey.Value, Models: dbKey.Models, Weight: dbKey.Weight, + Enabled: dbKey.Enabled, AzureKeyConfig: dbKey.AzureKeyConfig, VertexKeyConfig: dbKey.VertexKeyConfig, BedrockKeyConfig: dbKey.BedrockKeyConfig, }
slyang08
force-pushed
the
11-17-feat-option-disable-key
branch
from
4cd551e to
5a8c168
Compare
Hi akshaydeo, thank you for the response, I think I have followed the comments from CodeRabbit and fixed the problems. |
|
Hey @slyang08, the PR looks good - only one piece remaining in adding a migration in |
…usage (maximhq#983) Fixed inconsistencies in stream response handling for Anthropic and Bedrock providers by standardizing index values and removing unnecessary conditional checks. - Removed conditional check for `usage` in Anthropic response handling, ensuring usage is always assigned - Changed dynamic index values to fixed `0` index in stream responses for both Anthropic and Bedrock providers - Removed unnecessary content block index handling in Bedrock provider - [x] Bug fix - [ ] Feature - [x] Refactor - [ ] Documentation - [ ] Chore/CI - [x] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (Next.js) - [ ] Docs Test streaming responses from Anthropic and Bedrock providers to ensure proper index values and usage information: ```sh go version go test ./core/providers/anthropic/... go test ./core/providers/bedrock/... ``` - [ ] Yes - [x] No Fixes inconsistencies in stream response handling across providers. No security implications. - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable
slyang08
force-pushed
the
11-17-feat-option-disable-key
branch
from
5a8c168 to
78c4962
Compare
Hi @Pratham-Mishra04, I followed what you mentioned and tried to fix the problem, I think it is okay. However, if there is anything that I should improve, please let me know, thank you. |
3 participants
Summary
Implemented a feature that user can disable or enable a key without deleting the key on the website.
Type of change
Affected areas
If adding new configs or environment variables, document them here.
Added
Enabledincore/schemas/account.goBefore
After
Breaking changes
Related issues
Closes #831
Checklist
docs/contributing/README.mdand followed the guidelines