[sisantaChhatoi]

Description

Removes duplication in cmd/opencodereview/provider_tui.go and its tests. The behavior should not change. There are two commits, so the production change can be reviewed separately from the test churn.

1. refactor(provider-tui): production code

• modelTUIModel.confirmDeleteCustomProviderModel and confirmDeleteOfficialModel were identical apart from the config map they edited. They are merged into the existing confirmDeleteModel, which picks CustomProviders or Providers by isCustomProvider, the same way rollbackModelDelete already did.
• providerTUIModel.confirmDeleteCustomModel and confirmDeleteOfficialModel differ in their guards and in where the entry lives (the custom path also updates the local customProviders copy and tolerates a nil config), so they are not merged. Their shared tail (save, roll back if the save fails and the config cannot be reloaded, set the error, adjust the index, mark saved) moves into persistModelDelete(rollback func()), and each passes its own rollback.
• cfgActiveModelFor replaces the "remember the active model before delete" snippet repeated in three places.
• Form rendering: formField, writeInactiveFormField, writeOptionList and writeFormError replace blocks copied between viewCustomProviderForm and viewManualTab; the form-error block appeared four times, including once in modelTUIModel.View. The rendered strings are unchanged.
• New TestModelTUIConfirmDeleteModel_SaveFailureRollback covers the merged function's rollback path for both official and custom providers. That path had no test before.

2. test(provider-tui): tests only

• Mirrored scenarios are merged into table-driven tests:
  • Seven delete-flow tests existed once for providerTUIModel (ocr config provider) and again for modelTUIModel (ocr config model). They now run from one deleteFlowTUI table in provider_tui_deleteconfirm_test.go, still covering both models (14 subtests).
  • The custom-model-input add and duplicate-reject tests existed once per tab. They now share a customModelInputTabs table, and the official tab gains the assertions only the custom tab had.
  • TestCollectCustomProviders_NilConfig was a literal copy of TestCollectCustomProviders_Nil and is removed.
• New helpers replace repeated setup and assertions:
  • newProviderTUIOnTab (81 sites)
  • selectOfficialProvider and selectCustomProvider, which also fail the test when the name is missing instead of silently leaving index 0
  • firstRegistryProvider, activeDeepseekConfig, cpConfig
  • the custom-provider form walk (openCustomProviderForm, fillCustomFormToAuthHeader, sendKey)
  • loadDiskConfig, assertSaveFailed, currentProviderOrSkip
  • assertModels, which compares whole model lists with slices.Equal instead of partial length and index checks

Net diff: +688/−1211.

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Refactoring (no functional changes)
• Documentation update
• CI / Build / Tooling

How Has This Been Tested?

• make test passes locally
• Manual testing (describe below)

Each commit passes make check and make test on its own, go test -race ./cmd/opencodereview passes, and make coverage reports 92.1%. The new rollback test fails if the merged function edits the wrong config map. No previously running test is now skipped: the only skip in the package, TestInterruptChildHarness, is skipped on main as well.

Checklist

• My code follows the project's coding style (go fmt, go vet)
• I have performed a self-review of my code
• I have added tests that prove my fix is effective or my feature works
• New and existing unit tests pass locally with my changes
• I have updated the documentation accordingly (if applicable)
• I have signed the CLA
• I did not use AI/LLM to create this PR, or I disclosed the tool/model below and reviewed its output; I did not attribute commits to AI and will answer maintainer questions and review comments myself without AI/LLM.

AI disclosure: prepared with Claude Code (Claude Opus 5.5). I reviewed every line and can explain each change.

Related Issues

None.

🤖 Generated with Claude Code

[github-actions]

🔍 OpenCodeReview found 1 issue(s) in this PR.

• ✅ Successfully posted inline: 0 comment(s)
• ❌ Failed to post inline: 1 comment(s)

---

📄 cmd/opencodereview/provider_tui.go (L3029-L3031)

⚠️ GitHub could not post this as an inline comment: Lines 3029-3031 could not be resolved (outside PR diff hunks)

Behavioral change: resetCustomModelInput() is now called unconditionally on the success path for both custom and official provider deletions. Previously, only confirmDeleteCustomProviderModel called it; confirmDeleteOfficialModel did not. When deleting an official provider's model, this will unexpectedly reset m.customModel to false and clear/blur the model input, which could disrupt UI state for users who had a custom model input in progress.

💡 Suggested Change

Before:

	m.adjustModelIdxAfterDelete()
	m.resetCustomModelInput()
	m.savedInSession = true

After:

	m.adjustModelIdxAfterDelete()
	if m.isCustomProvider {
		m.resetCustomModelInput()
	}
	m.savedInSession = true

[sisantaChhatoi]

Checked this one, and I think it's a false positive. The official path was already resetting the custom-model input before this change. On main, modelTUIModel.confirmDeleteOfficialModel calls m.resetCustomModelInput() right before setting savedInSession (provider_tui.go#L3068), same as the custom variant at L3030. The merged confirmDeleteModel just keeps that one call, so nothing changes for either provider kind.

I'll leave the if m.isCustomProvider guard out. Adding it would actually change behavior, since the custom-model input would then stay open after deleting a model from an official provider.

[Qiyuanqiii]

I repeated the comparison against the pre-refactor TUI code, including rendered text, model-delete state transitions, rollback behavior, and the documentation impact.

The earlier automated finding about resetCustomModelInput() is a false positive: the previous official-provider deletion path already called it on success, just like the custom-provider path. The merged implementation therefore preserves that behavior.

I also rechecked the extracted rendering helpers. The labels, indentation, (Authorization) fallback, option rendering, and form-error spacing are preserved.

This is an internal refactor with no user-facing workflow or configuration change, so no documentation update is required.

The new helper and comment wording is clear enough, and I don't have any remaining blocking findings.

Approved.

[sisantaChhatoi]

Heads up on the latest push: it's just a rebase onto current main. No conflicts and no code changes. The red test/cross-compile checks stop at "Verify generated provider presets" on the self-hosted runners, same as other PRs right now; they pass locally.