feat: improve config provider TUI interaction (#213)

* feat: improve config provider TUI interaction
- Support wrap-around navigation in provider, custom provider, and model lists
  (up at first item jumps to last, down at last item jumps to first)
- Add d key to delete custom models in model selection step with confirmation prompt
- Add protocol selection (anthropic/openai) to manual configuration flow
  and respect UseAnthropic setting in saved config
- Preserve manual protocol selection when re-entering the form
- Fix manual form input fields losing focus after returning from next step
- Use safe slice removal in removeFromSlice to avoid backing array corruption

* fix: address code review feedback
- Restore Blur() calls in handleManualFormEnter to prevent visual focus artifacts
  when transitioning between manual form steps
- Add empty-providers guard in handleUp/handleDown to avoid negative index
- Consolidate redundant protocolIdx checks into single if/else in viewManualTab
  and viewCustomProviderForm for clarity

* fix: persist custom model deletions and support masked auth token editing
- Track deleted custom models in TUI and apply them on exit
  (both confirm and cancel paths) to keep config.json in sync
- Clear custom provider's active Model field when that model is deleted,
  preventing stale "model" reference in the provider list label
- Add masked display for manual config Auth Token to allow easy re-entry,
  matching the official provider flow (any key clears and starts fresh)

* test: update manual form tests for masked auth token behavior
Update TestProviderTUI_ManualFormPrefilledValues,
TestProviderTUI_ManualFormEscRestoresOriginalValues, and
TestProviderTUI_ManualFormPrefilledWhenProviderSet to assert
the new masked display state (manualTokenMasked + manualTokenOriginal)
instead of the raw token value.

* feat: refine config provider TUI flows and session persistence
Custom provider interactions:
- Simplify create/edit form to Name → Protocol → URL → API Key → Auth Header
- After create or edit save, jump straight into the model list for that provider
- Support comma-separated model names in the custom model input
- Show masked API key in edit form; empty Auth Header defaults to (Authorization)
- Populate edit form from existingCfg to avoid stale list data after in-session saves
Model selection and highlight:
- Green highlight follows the persisted active model, not the cursor position
- Prefer provider entry.model over global cfg.model when resolving active model
- Deleting a non-active model keeps the current green highlight unchanged
Delete and navigation:
- Delete custom providers (d) and custom models (d) with confirmation prompts
- Persist create/edit/model-select/add/delete changes to disk during the session
- Fix custom provider deletion not surviving Esc exe-entry
Tests:
- Add coverage for model highlight, delete-model behavior, and create→model-list flow
- Update manual/custom form tests for masked token and session save behavior

* refactor: address provider TUI code review feedback
- Remove dead helper removeFromSlice (superseded by removeModels)
- Move misplaced doc comment to applyEditCustomProviderSave
- Drop redundant applyProviderDeletions post-TUI call so provider
  deletions rely solely on the in-session save
- Fix brace/indent drift in updateDeleteModelConfirm and cache the
  model list instead of recomputing m.models() twice

* feat: refine provider TUI flows and manual config form
Custom provider flow:
- Default protocol to anthropic on the new-provider form
- Single-name model input; reject duplicates with inline error and
  preserve the typed value so the user can edit instead of re-typing
- Drop the global green highlight; cursor/blue is the only selection cue
Manual configuration form:
- Add Auth Header step (URL → Protocol → Model → Auth Token → Auth Header)
  and persist it to Llm.AuthHeader on confirm
- Reorder so Auth Header is always entered last
- Make Auth Token required; empty Enter stays on the field
- Show every field's label on every render, even when empty, matching
  the custom-provider form style
Tests:
- Cover custom model input add / duplicate paths
- Cover manual form prefill of Llm.AuthHeader
- Cover that deleting a non-active model keeps the active model intact

* Improve provider TUI validation, persistence, and code clarity.

Address code review feedback: align custom Auth Header validation with manual mode; fix savedInSession after model deletion; refactor applyEditCustomProviderSave to return error; remove dead applyModelDeletions/deletedModels; document ExtraBody shallow-copy limit. Also fix manual/custom form UX (token skip on edit, k key input, formError scoping, switch indentation).

* Fix provider/model TUI list ordering and add test coverage.

Address review and UX feedback: remove model list sorting in provider and config model TUIs; preserve Models list order when selecting active model (ensureModelInList); add test for duplicate rename on custom provider edit. Includes prior review fixes for savedInSession, applyEditCustomProviderSave error return, and dead code removal.

* Normalize AuthHeader at apply layer and simplify UseAnthropic assignment.
Call NormalizeAuthHeader in applyManualConfig and applyCustomProviderConfig before save; simplify UseAnthropic assignment in manual config; add unit tests.

* Address review: lowercase error strings and newProviderTUI signature.

Use lowercase "failed to save" errors; replace variadic configPath with string and remove configPathFromArgs; pass "" in tests when no path; gofmt provider_tui.go.

Author: 5wxwxwxw5

cmd/opencodereview/config_cmd.go

@@ -394,6 +394,16 @@ func parseModelListValue(value string) ([]string, error) {
 	return normalizeModelList(strings.Split(value, ",")), nil
 }
 
+func activeModelForProvider(cfg *Config, providerName string, entry ProviderEntry) string {
+	if entry.Model != "" {
+		return entry.Model
+	}
+	if cfg != nil && cfg.Provider == providerName && cfg.Model != "" {
+		return cfg.Model
+	}
+	return ""
+}
+
 func normalizeModelList(models []string) []string {
 	out := make([]string, 0, len(models))
 	seen := make(map[string]struct{}, len(models))
@@ -419,6 +429,19 @@ func mergeModelLists(lists ...[]string) []string {
 	return normalizeModelList(merged)
 }
 
+// ensureModelInList appends model to the end when missing; never reorders existing entries.
+func ensureModelInList(models []string, model string) []string {
+	model = strings.TrimSpace(model)
+	if model == "" {
+		return models
+	}
+	if modelListContains(models, model) {
+		return models
+	}
+	out := append([]string(nil), models...)
+	return append(out, model)
+}
+
 func modelListContains(models []string, target string) bool {
 	target = strings.TrimSpace(target)
 	for _, model := range models {

cmd/opencodereview/config_cmd_test.go

@@ -416,3 +416,23 @@ func TestUnsetInvalidKey(t *testing.T) {
 		})
 	}
 }
+
+func TestEnsureModelInList(t *testing.T) {
+	models := []string{"test-model", "test-model-2", "bbb", "aaa", "test-model-3"}
+
+	got := ensureModelInList(models, "test-model-3")
+	if len(got) != len(models) {
+		t.Fatalf("existing model should not reorder: got %v", got)
+	}
+	for i := range models {
+		if got[i] != models[i] {
+			t.Errorf("models[%d] = %q, want %q", i, got[i], models[i])
+		}
+	}
+
+	got = ensureModelInList(models, "new-model")
+	want := append(append([]string(nil), models...), "new-model")
+	if len(got) != len(want) || got[len(got)-1] != "new-model" {
+		t.Errorf("new model should append: got %v, want %v", got, want)
+	}
+}

cmd/opencodereview/provider_cmd.go

@@ -22,7 +22,7 @@ func runConfigProvider() error {
 		return fmt.Errorf("load config: %w", err)
 	}
 
-	m := newProviderTUI(cfg)
+	m := newProviderTUI(cfg, configPath)
 	p := tea.NewProgram(m)
 	finalModel, err := p.Run()
 	if err != nil {
@@ -31,19 +31,11 @@ func runConfigProvider() error {
 
 	final := finalModel.(providerTUIModel)
 
-	if len(final.deletedProviders) > 0 {
-		clearedActive, err := applyProviderDeletions(configPath, cfg, final.deletedProviders)
-		if err != nil {
-			return err
-		}
-		if clearedActive && !final.confirmed {
-			fmt.Fprintf(os.Stderr, "[ocr] WARNING: active provider was deleted; 'provider' and 'model' have been cleared.\n")
-			fmt.Fprintf(os.Stderr, "[ocr] Run 'ocr config provider' to select a new provider.\n")
-		}
-	}
-
 	if !final.confirmed {
-		if len(final.deletedProviders) > 0 {
+		// TUI persists changes (create/edit/model/add/delete) directly to disk
+		// during the session, so the on-disk file is already up to date for any
+		// savedInSession operation. No additional post-TUI apply step is needed.
+		if final.savedInSession {
 			return nil
 		}
 		fmt.Println("Cancelled.")
@@ -82,6 +74,21 @@ func applyProviderDeletions(configPath string, cfg *Config, names []string) (boo
 	return clearedActive, nil
 }
 
+func removeModels(existing, toRemove []string) []string {
+	removeSet := make(map[string]struct{}, len(toRemove))
+	for _, m := range toRemove {
+		removeSet[m] = struct{}{}
+	}
+	result := make([]string, 0, len(existing))
+	for _, m := range existing {
+		if _, found := removeSet[m]; found {
+			continue
+		}
+		result = append(result, m)
+	}
+	return result
+}
+
 func applyManualConfig(configPath string, cfg *Config, result providerTUIResult) error {
 	if result.url == "" {
 		return fmt.Errorf("URL is required for manual configuration")
@@ -95,13 +102,21 @@ func applyManualConfig(configPath string, cfg *Config, result providerTUIResult)
 	cfg.Llm.URL = result.url
 	cfg.Llm.Model = result.model
 	cfg.Llm.AuthToken = result.apiKey
+	authHeader, err := llm.NormalizeAuthHeader(result.authHeader)
+	if err != nil {
+		return fmt.Errorf("invalid auth_header: %w", err)
+	}
+	cfg.Llm.AuthHeader = authHeader
+	useAnthropic := result.protocol == "anthropic"
+	cfg.Llm.UseAnthropic = &useAnthropic
 
 	if err := saveConfig(configPath, cfg); err != nil {
 		return err
 	}
 
 	fmt.Println("\nManual configuration saved.")
 	fmt.Printf("URL: %s\n", result.url)
+	fmt.Printf("Protocol: %s\n", result.protocol)
 	fmt.Printf("Model: %s\n", result.model)
 
 	fmt.Println("\nTesting connection...")
@@ -129,31 +144,49 @@ func applyCustomProviderConfig(configPath string, cfg *Config, result providerTU
 	entry := cfg.CustomProviders[result.provider]
 	entry.Model = result.model
 	if len(result.models) > 0 {
-		entry.Models = mergeModelLists([]string{result.model}, result.models)
+		entry.Models = append([]string(nil), result.models...)
 	}
+	entry.Models = ensureModelInList(entry.Models, result.model)
 	if result.url != "" {
 		entry.URL = result.url
 	}
 	if result.protocol != "" {
 		entry.Protocol = result.protocol
 	}
 	if result.authHeader != "" {
-		entry.AuthHeader = result.authHeader
+		authHeader, err := llm.NormalizeAuthHeader(result.authHeader)
+		if err != nil {
+			return fmt.Errorf("invalid auth_header: %w", err)
+		}
+		entry.AuthHeader = authHeader
 	}
 	if result.apiKey != "" {
 		entry.APIKey = result.apiKey
 	}
 	cfg.CustomProviders[result.provider] = entry
 
-	if cfg.Provider != result.provider {
-		cfg.Model = ""
+	if !result.isEdit {
+		cfg.Provider = result.provider
+		cfg.Model = result.model
+	} else if cfg.Provider == result.provider {
+		cfg.Model = result.model
 	}
-	cfg.Provider = result.provider
 
 	if err := saveConfig(configPath, cfg); err != nil {
 		return err
 	}
 
+	if result.isEdit {
+		if cfg.Provider == result.provider {
+			fmt.Printf("\nActive provider %q updated.\n", result.provider)
+		} else {
+			fmt.Printf("\nCustom provider %q updated (not currently active).\n", result.provider)
+		}
+		fmt.Printf("Model: %s\n", result.model)
+		fmt.Println("\nTip: run 'ocr config model' to switch model later.")
+		return nil
+	}
+
 	fmt.Printf("\nProvider set to: %s (custom)\n", result.provider)
 	fmt.Printf("Model: %s\n", result.model)
 
@@ -203,6 +236,7 @@ func applyOfficialProviderConfig(configPath string, cfg *Config, result provider
 		cfg.Model = ""
 	}
 	cfg.Provider = result.provider
+	cfg.Model = result.model
 
 	if err := saveConfig(configPath, cfg); err != nil {
 		return err
@@ -243,7 +277,7 @@ func runConfigModel() error {
 	if preset, isPreset := llm.LookupProvider(cfg.Provider); isPreset {
 		provider = preset
 		if entry, ok := cfg.Providers[cfg.Provider]; ok {
-			currentModel = entry.Model
+			currentModel = activeModelForProvider(cfg, cfg.Provider, entry)
 			provider.Models = mergeModelLists(provider.Models, entry.Models)
 		}
 	} else {
@@ -252,15 +286,12 @@ func runConfigModel() error {
 		if !ok {
 			return fmt.Errorf("provider %q is not configured in custom_providers", cfg.Provider)
 		}
-		currentModel = entry.Model
+		currentModel = activeModelForProvider(cfg, cfg.Provider, entry)
 		provider.DisplayName = cfg.Provider + " (custom)"
 		provider.Protocol = entry.Protocol
 		provider.BaseURL = entry.URL
 		provider.Models = mergeModelLists(entry.Models)
 	}
-	if currentModel == "" {
-		currentModel = cfg.Model
-	}
 
 	m := newModelTUI(provider, currentModel)
 	p := tea.NewProgram(m)
@@ -286,7 +317,7 @@ func runConfigModel() error {
 		}
 		entry := cfg.CustomProviders[cfg.Provider]
 		entry.Model = selectedModel
-		entry.Models = mergeModelLists([]string{selectedModel}, entry.Models)
+		entry.Models = ensureModelInList(entry.Models, selectedModel)
 		cfg.CustomProviders[cfg.Provider] = entry
 	} else {
 		if cfg.Providers == nil {
@@ -295,10 +326,11 @@ func runConfigModel() error {
 		entry := cfg.Providers[cfg.Provider]
 		entry.Model = selectedModel
 		if !modelListContains(provider.Models, selectedModel) {
-			entry.Models = mergeModelLists([]string{selectedModel}, entry.Models)
+			entry.Models = ensureModelInList(entry.Models, selectedModel)
 		}
 		cfg.Providers[cfg.Provider] = entry
 	}
+	cfg.Model = selectedModel
 
 	if err := saveConfig(configPath, cfg); err != nil {
 		return err