feat: add ability to delete custom providers from configuration#189
Conversation
|
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| switch key { | ||
| case "y", "Y": | ||
| m.deletedProviders = append(m.deletedProviders, m.deleteTargetName) | ||
| m.customProviders = append(m.customProviders[:m.deleteTargetIdx], m.customProviders[m.deleteTargetIdx+1:]...) |
There was a problem hiding this comment.
Potential index-out-of-range panic: deleteTargetIdx is captured when the user presses 'd', but by the time they press 'y' to confirm, the index is used without re-validation. While the Bubble Tea event loop is single-threaded making concurrent mutation unlikely, adding a defensive bounds check here would make the code more robust against future refactoring or unexpected state changes.
Also, the in-place slice deletion pattern append(s[:i], s[i+1:]...) modifies the underlying array, which can retain references to deleted elements (minor memory leak) and may cause subtle bugs if the slice is ever shared. Consider using an explicit copy instead.
Suggestion:
| m.customProviders = append(m.customProviders[:m.deleteTargetIdx], m.customProviders[m.deleteTargetIdx+1:]...) | |
| if m.deleteTargetIdx < 0 || m.deleteTargetIdx >= len(m.customProviders) { | |
| m.confirmingDelete = false | |
| return m, nil | |
| } | |
| // Use explicit copy to avoid retaining references in the underlying array | |
| newList := make([]customProviderListItem, 0, len(m.customProviders)-1) | |
| newList = append(newList, m.customProviders[:m.deleteTargetIdx]...) | |
| newList = append(newList, m.customProviders[m.deleteTargetIdx+1:]...) | |
| m.customProviders = newList |
|
🔍 OpenCodeReview found 2 issue(s) in this PR.
📊 Posting Statistics:
|
Implements GitHub issue alibaba#136. CLI: add 'ocr config unset custom_providers.<name>' command to delete a custom provider from config. If the deleted provider is the active one, clears 'provider' and 'model' fields and prompts the user. TUI: press 'd' on Custom tab to delete a provider with y/n confirmation. Shows warning when deleting the active provider. Deletions are persisted even if the user cancels provider selection afterward. - Add runConfigUnset() with key validation and active-provider handling - Add 'unset' case to parseConfigArgs() and printConfigUsage() - Add delete confirmation state machine to providerTUIModel - Add applyProviderDeletions() helper in provider_cmd.go - Add 10 new tests covering CLI parsing, deletion logic, and TUI flows - Update README with unset command documentation
yingjiexu2002
force-pushed
the
feat/delete-custom-provider
branch
from
da01ce8 to
10b403a
Compare
- Add defensive bounds check for deleteTargetIdx before deletion - Use explicit slice copy to avoid retaining references (memory leak) - Update subCmd comment to reflect 'set' and 'unset' values
lizhengfeng101
left a comment
lizhengfeng101
left a comment
There was a problem hiding this comment.
整体功能设计合理，TUI 交互考虑周全。主要问题在于 CLI 单元测试未调用实际函数，以及 CLI 和 TUI 之间存在删除逻辑重复。详见各行评论。
| cfg.CustomProviders = nil | ||
| } | ||
| if err := saveConfig(configPath, cfg); err != nil { | ||
| t.Fatalf("saveConfig: %v", err) |
There was a problem hiding this comment.
[Critical] Tests do not call the actual runConfigUnset function
This test manually inlines the delete + saveConfig logic instead of calling runConfigUnset("custom_providers.my-gateway"). This means if runConfigUnset has a bug (e.g., missing the wasActive check, defaultConfigPath error handling, etc.), the test would still pass.
Suggestion: Extract the core logic of runConfigUnset into an internal function that accepts a configPath parameter (e.g., unsetCustomProvider(configPath, key string) error), then call the actual function in tests using a t.TempDir() path. This way the tests will actually cover the validation logic, wasActive handling, and CustomProviders = nil cleanup.
The same issue applies to TestUnsetActiveCustomProvider and TestUnsetInvalidKey below.
| cfg.Provider = "" | ||
| cfg.Model = "" | ||
| } | ||
| delete(cfg.CustomProviders, name) |
There was a problem hiding this comment.
[Critical] Same as above — manually implements deletion logic instead of calling runConfigUnset
This manually executes if cfg.Provider == name { cfg.Provider = ""; cfg.Model = "" } + delete instead of calling the function under test. If the order of operations in runConfigUnset is accidentally changed (e.g., deleting before checking wasActive), this test would not catch the bug.
| {"providers.anthropic", true}, | ||
| {"custom_providers.", true}, | ||
| {"custom_providers", true}, | ||
| {"something", true}, |
There was a problem hiding this comment.
[Critical] Same as above — re-implements validation logic
This manually replicates the SplitN + condition check validation logic instead of calling runConfigUnset(tt.key) and checking the returned error. Should be rewritten to call the actual function:
for _, tt := range tests {
err := unsetCustomProvider(configPath, tt.key)
if (err != nil) != tt.wantErr {
t.Errorf("key %q: err=%v, wantErr=%v", tt.key, err, tt.wantErr)
}
}| cfg.Model = "" | ||
| clearedActive = true | ||
| } | ||
| fmt.Printf("Deleted custom provider %q.\n", name) |
There was a problem hiding this comment.
[Medium] Prints deletion message without checking if the provider actually existed
delete(cfg.CustomProviders, name) is a silent no-op for non-existent keys, but this line prints Deleted custom provider %q regardless of whether the provider was actually present. While the current TUI flow only populates deletedProviders with names that exist, as a standalone function it should validate existence. Suggestion:
if _, exists := cfg.CustomProviders[name]; !exists {
continue
}| } | ||
|
|
||
| func runConfigUnset(key string) error { | ||
| parts := strings.SplitN(key, ".", 2) |
There was a problem hiding this comment.
[Minor] Duplicated logic with applyProviderDeletions
runConfigUnset and applyProviderDeletions in provider_cmd.go implement the same core logic: delete custom provider → check if active → clear Provider/Model → nil out empty map. Consider extracting a shared internal function (e.g., deleteCustomProvider(cfg *Config, name string) bool) called by both, to avoid the risk of the two implementations diverging in future changes.
Address PR review feedback from lizhengfeng101: - Extract deleteCustomProvider() as a pure function shared by both runConfigUnset (CLI) and applyProviderDeletions (TUI) - Extract unsetCustomProvider() accepting configPath for testability - Add existence check in applyProviderDeletions (skip non-existent providers) - Rewrite 3 tests to call actual functions instead of inlining logic - Remove unused strings import from config_cmd_test.go
Align with project convention (output.go, flags.go): warnings use '[ocr] WARNING' prefix and write to stderr instead of stdout.
lizhengfeng101
left a comment
lizhengfeng101
left a comment
There was a problem hiding this comment.
Overall this is a high-quality PR — clean separation of concerns, solid test coverage, and good UX details. Left a few inline comments on specific areas worth addressing.
| name string | ||
| wantErr bool | ||
| }{ | ||
| {"my-gateway", false}, |
There was a problem hiding this comment.
Bug: test pollution across sub-cases. The first case {"my-gateway", false} successfully deletes the provider and modifies the config file on disk, which changes the state for the second case. While the result happens to be correct today (because "nonexistent" would fail regardless), this test is fragile:
- Reversing the order would break the test.
- Adding a case that depends on
my-gatewaystill existing would silently fail. - The test is named
TestUnsetInvalidKeybut the first case is a valid deletion — the intent is unclear.
Suggestion: use t.Run sub-tests with independent config files per case, or split into separate test functions.
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
dir := t.TempDir()
configPath := dir + "/config.json"
cfg := &Config{CustomProviders: map[string]ProviderEntry{
"my-gateway": {URL: "https://gw.example.com/v1"},
}}
saveConfig(configPath, cfg)
err := unsetCustomProvider(configPath, tt.name)
// ...
})
}| } | ||
|
|
||
| if !final.confirmed { | ||
| if len(final.deletedProviders) > 0 { |
There was a problem hiding this comment.
Design: deletions persist even when the user cancels provider selection. This is explicitly documented in the PR description, so it's clearly intentional. However, from a UX perspective it may surprise users — pressing Esc typically means "discard all changes", but here deletions have already been committed to disk.
Worth confirming this is the desired team UX convention. If so, consider adding a brief note in the TUI (e.g., "Provider deleted. Press Esc to exit without selecting a new provider.") so the user is aware the deletion was already applied.
| for _, name := range names { | ||
| wasActive, err := deleteCustomProvider(cfg, name) | ||
| if err != nil { | ||
| continue |
There was a problem hiding this comment.
Nit: silently swallowing errors. If deleteCustomProvider returns an error (e.g., the provider was already removed from memory by a duplicate entry in deletedProviders), this continue drops the error without any signal to the user.
While unlikely in practice, consider at least logging it:
if err != nil {
fmt.Fprintf(os.Stderr, "[ocr] skip delete %q: %v\n", name, err)
continue
}| if m.confirmingDelete { | ||
| s.WriteString("\n") | ||
| prompt := fmt.Sprintf(" Delete %q?", m.deleteTargetName) | ||
| if m.existingCfg != nil && m.existingCfg.Provider == m.deleteTargetName { |
There was a problem hiding this comment.
Minor: active-provider warning uses initial config snapshot. This checks m.existingCfg (the config at TUI startup) to decide whether to show the "active provider" warning. If the user deletes provider A (which was active), then tries to delete provider B, the warning correctly won't fire for B.
However, this won't reflect any provider selection made during the same TUI session. Not a bug in the current flow (selection happens after deletion), but worth a comment noting the assumption so future refactors don't break it.
| func runConfigUnset(key string) error { | ||
| parts := strings.SplitN(key, ".", 2) | ||
| if len(parts) != 2 || parts[0] != "custom_providers" || parts[1] == "" { | ||
| return fmt.Errorf("unset only supports custom_providers.<name>\nUsage: ocr config unset custom_providers.<name>") |
There was a problem hiding this comment.
Nit: error message style inconsistency. This error includes a multi-line usage hint with \n, while runConfigSet and other error paths in the codebase return short single-line errors. Consider keeping it consistent:
return fmt.Errorf("unset only supports custom_providers.<name>")The full usage is already shown by printConfigUsage() when the user runs ocr config without args.
- Fix test state pollution in TestUnsetInvalidKey: use t.Run sub-tests with independent config files per case (alibaba#7) - Log warning instead of silently swallowing errors in applyProviderDeletions (alibaba#9) - Add comment explaining existingCfg snapshot assumption in viewCustomTab (alibaba#10) - Simplify runConfigUnset error message to single line for consistency with project style (alibaba#11)
lizhengfeng101
left a comment
lizhengfeng101
left a comment
There was a problem hiding this comment.
Re-review: all previous feedback addressed ✓
Verified against latest commit b0a6960e ("fix: address second round of PR review feedback"). All 5 issues from the previous round have been resolved:
| # | Issue | Status |
|---|---|---|
| 1 | TestUnsetInvalidKey test pollution — sub-cases shared config file on disk |
Fixed — now uses t.Run with independent t.TempDir() per sub-case |
| 2 | Deletions persist on cancel (design question) | Acknowledged — intentional design; no code change needed |
| 3 | applyProviderDeletions silently swallowed errors |
Fixed — now logs [ocr] skip delete %q: %v to stderr |
| 4 | Active-provider warning used existingCfg snapshot without explanation |
Fixed — added clarifying comment noting this is a startup snapshot |
| 5 | Error message style inconsistency (multi-line vs single-line) | Fixed — simplified to single-line "unset only supports custom_providers.<name>" |
Code quality summary
- Architecture: Clean separation —
deleteCustomProvider(in-memory logic) is shared by both CLI (unsetCustomProvider) and TUI (applyProviderDeletions) paths. No duplication. - Tests: 10 new tests with proper isolation. CLI and TUI paths, happy paths and edge cases all covered.
- TUI UX: Delete confirmation, active-provider warning, cursor adjustment after deletion,
don "Add" option ignored — all edge cases handled. - Documentation: README command table and examples updated.
LGTM — ready to merge.
|
Hey @yingjiexu2002, great work on this PR! One request: @Ackberry had previously claimed this issue (#136) on Jun 15 and was working on it as part of a coursework project. It seems the efforts overlapped since there was no draft PR to signal work-in-progress. Would you mind adding a co-author credit when merging? You can append this to the final commit message: This way both contributors get recognized. Thanks! |
3 participants
Description
Add the ability to delete custom providers from configuration, addressing issue #136.
CLI: New
ocr config unset custom_providers.<name>command that validates the key format, removes the provider from config, and clearsprovider/modelfields if the deleted provider was active.TUI: Press
don any custom provider in the Custom tab to trigger ay/nconfirmation prompt. Shows a warning when targeting the active provider. Deletions persist even if the user cancels provider selection afterward. Cursor position adjusts automatically after deletion.Type of Change
How Has This Been Tested?
10 new unit tests added (5 CLI + 5 TUI):
TestParseConfigArgsUnset,TestParseConfigArgsUnsetMissingKey,TestUnsetCustomProvider,TestUnsetActiveCustomProvider,TestUnsetInvalidKeyTestProviderTUI_DeleteCustomProvider,TestProviderTUI_DeleteCustomProviderCancel,TestProviderTUI_DeleteOnAddOptionIgnored,TestProviderTUI_DeleteActiveCustomProvider,TestProviderTUI_DeleteEscCancelsFull test suite passes with zero regressions (52 tests in
cmd/opencodereview).make testpasses locallyManually verified:
ocr config unset custom_providers.my-gatewaydeletes the provider and outputs confirmationocr config unset custom_providers.active-providerclears active provider/model and prompts reconfigurationocr config unset llm.urlcorrectly rejects non-custom-provider keysd→yflow deletes and adjusts cursor;d→n/Esccancels;don "Add" option is ignoredChecklist
go fmt,go vet)Related Issues
Closes #136