Skip to content

refactor(provider-tui): share model-delete, form rendering and test setup code #1572

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sisantaChhatoi wants to merge 2 commits into
base: main
Choose a base branch
from
Open

refactor(provider-tui): share model-delete, form rendering and test setup code #1572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
867beba
refactor(provider-tui): share model-delete and form rendering code
sisantaChhatoi Sep 25, 2026
758a7f5
test(provider-tui): share setup helpers and merge mirrored tests
sisantaChhatoi Sep 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
251 changes: 89 additions & 162 deletions cmd/opencodereview/provider_tui.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,13 @@ func clearCfgActiveModelIfDeleted(cfg *Config, providerName, name string) {
}
}

func cfgActiveModelFor(cfg *Config, providerName string) string {
if cfg != nil && cfg.Provider == providerName {
return cfg.Model
}
return ""
}

func rollbackCfgActiveModel(cfg *Config, providerName, prevModel string) {
if cfg != nil && cfg.Provider == providerName {
cfg.Model = prevModel
Expand Down Expand Up @@ -1633,10 +1640,7 @@ func (m providerTUIModel) confirmDeleteCustomModel() (tea.Model, tea.Cmd) {
if m.modelIdx < len(models) {
cp := m.customProviders[m.customIdx]
prevEntry := cloneProviderEntry(cp.entry)
prevCfgModel := ""
if m.existingCfg != nil && m.existingCfg.Provider == cp.name {
prevCfgModel = m.existingCfg.Model
}
prevCfgModel := cfgActiveModelFor(m.existingCfg, cp.name)
cp.entry = applyModelDeleteToEntry(cp.entry, m.deleteModelName)
clearCfgActiveModelIfDeleted(m.existingCfg, cp.name, m.deleteModelName)
m.customProviders[m.customIdx] = cp
Expand All @@ -1646,24 +1650,14 @@ func (m providerTUIModel) confirmDeleteCustomModel() (tea.Model, tea.Cmd) {
}
m.existingCfg.CustomProviders[cp.name] = cp.entry
}
if m.configPath != "" {
if err := saveConfig(m.configPath, m.existingCfg); err != nil {
if !m.reloadConfigAfterSaveFailure() {
cp.entry = prevEntry
m.customProviders[m.customIdx] = cp
if m.existingCfg != nil {
m.existingCfg.CustomProviders[cp.name] = prevEntry
rollbackCfgActiveModel(m.existingCfg, cp.name, prevCfgModel)
}
}
m.formError = fmt.Sprintf("failed to save: %v", err)
m.adjustModelIdxAfterDelete()
m.confirmingDeleteModel = false
return m, nil
m.persistModelDelete(func() {
cp.entry = prevEntry
m.customProviders[m.customIdx] = cp
if m.existingCfg != nil {
m.existingCfg.CustomProviders[cp.name] = prevEntry
rollbackCfgActiveModel(m.existingCfg, cp.name, prevCfgModel)
}
}
m.adjustModelIdxAfterDelete()
m.savedInSession = true
})
}
m.confirmingDeleteModel = false
return m, nil
Expand All @@ -1683,30 +1677,34 @@ func (m providerTUIModel) confirmDeleteOfficialModel() (tea.Model, tea.Cmd) {
m.existingCfg.Providers = make(map[string]ProviderEntry)
}
prevEntry := cloneProviderEntry(m.existingCfg.Providers[provider.Name])
prevCfgModel := ""
if m.existingCfg.Provider == provider.Name {
prevCfgModel = m.existingCfg.Model
}
prevCfgModel := cfgActiveModelFor(m.existingCfg, provider.Name)
entry := applyModelDeleteToEntry(m.existingCfg.Providers[provider.Name], m.deleteModelName)
clearCfgActiveModelIfDeleted(m.existingCfg, provider.Name, m.deleteModelName)
m.existingCfg.Providers[provider.Name] = entry
m.persistModelDelete(func() {
m.existingCfg.Providers[provider.Name] = prevEntry
rollbackCfgActiveModel(m.existingCfg, provider.Name, prevCfgModel)
})
m.confirmingDeleteModel = false
return m, nil
}

// rollback runs only when the save fails and the on-disk config cannot be
// reloaded in its place.
func (m *providerTUIModel) persistModelDelete(rollback func()) {
if m.configPath != "" {
if err := saveConfig(m.configPath, m.existingCfg); err != nil {
if !m.reloadConfigAfterSaveFailure() {
m.existingCfg.Providers[provider.Name] = prevEntry
rollbackCfgActiveModel(m.existingCfg, provider.Name, prevCfgModel)
rollback()
}
m.formError = fmt.Sprintf("failed to save: %v", err)
m.adjustModelIdxAfterDelete()
m.confirmingDeleteModel = false
return m, nil
return
}
}
m.adjustModelIdxAfterDelete()
// In-memory delete succeeded; configPath may be empty in tests (no disk write).
m.savedInSession = true
m.confirmingDeleteModel = false
return m, nil
}

func (m *providerTUIModel) adjustModelIdxAfterDelete() {
Expand Down Expand Up @@ -2266,6 +2264,43 @@ func (m providerTUIModel) viewCustomTab(s *strings.Builder) {
}
}

type formField struct {
label string
value string
active bool
}

func writeInactiveFormField(s *strings.Builder, f formField) {
display := f.value
if display == "" && f.label == "Auth Header" {
display = "(Authorization)"
}
if display == "" {
s.WriteString(" " + tuiDimStyle.Render(f.label+":") + "\n")
} else {
s.WriteString(" " + tuiDimStyle.Render(f.label+": "+display) + "\n")
}
}

func writeOptionList(s *strings.Builder, options []string, selected int) {
for i, opt := range options {
if i == selected {
s.WriteString(" " + tuiCursorStyle.Render(tuiCursor) + " " + tuiSelectedItemStyle.Render(opt) + "\n")
} else {
s.WriteString(" " + tuiItemStyle.Render(opt) + "\n")
}
}
}

func writeFormError(s *strings.Builder, msg string) {
if msg == "" {
return
}
s.WriteString("\n")
s.WriteString(tuiErrorStyle.Render(" " + msg))
s.WriteString("\n")
}

func (m providerTUIModel) viewCustomProviderForm(s *strings.Builder) {
title := " Add Custom Provider"
if m.editingCustom {
Expand All @@ -2274,21 +2309,15 @@ func (m providerTUIModel) viewCustomProviderForm(s *strings.Builder) {
s.WriteString(tuiTitleStyle.Render(title))
s.WriteString("\n\n")

type field struct {
label string
value string
active bool
}

fields := []field{
fields := []formField{
{"Provider name", m.cpNameInput.Value(), m.cpStep == cpStepName},
{"Protocol", cpProtocols[m.cpProtocolIdx], m.cpStep == cpStepProtocol},
}
if !m.cpAmbientProtocol() {
fields = append(fields,
field{"Base URL", m.cpURLInput.Value(), m.cpStep == cpStepBaseURL},
field{"API Key", strings.Repeat("*", len(m.apiKeyInput.Value())), m.cpStep == cpStepAPIKey},
field{"Auth Header", m.cpAuthInput.Value(), m.cpStep == cpStepAuthHeader},
formField{"Base URL", m.cpURLInput.Value(), m.cpStep == cpStepBaseURL},
formField{"API Key", strings.Repeat("*", len(m.apiKeyInput.Value())), m.cpStep == cpStepAPIKey},
formField{"Auth Header", m.cpAuthInput.Value(), m.cpStep == cpStepAuthHeader},
)
}

Expand All @@ -2299,15 +2328,7 @@ func (m providerTUIModel) viewCustomProviderForm(s *strings.Builder) {
case cpStepName:
s.WriteString(" " + m.cpNameInput.View() + "\n")
case cpStepProtocol:
for i, proto := range cpProtocols {
if i == m.cpProtocolIdx {
cur := " " + tuiCursorStyle.Render(tuiCursor) + " "
s.WriteString(cur + tuiSelectedItemStyle.Render(proto) + "\n")
} else {
cur := " "
s.WriteString(cur + tuiItemStyle.Render(proto) + "\n")
}
}
writeOptionList(s, cpProtocols, m.cpProtocolIdx)
if m.cpAmbientProtocol() {
s.WriteString(tuiDimStyle.Render(" credentials come from the AWS chain; pin a region or profile with `ocr config set custom_providers."+m.cpNameInput.Value()+".aws_region <r>`") + "\n")
}
Expand All @@ -2322,23 +2343,11 @@ func (m providerTUIModel) viewCustomProviderForm(s *strings.Builder) {
s.WriteString(" " + m.cpAuthInput.View() + "\n")
}
} else {
display := f.value
if display == "" && f.label == "Auth Header" {
display = "(Authorization)"
}
if display == "" {
s.WriteString(" " + tuiDimStyle.Render(f.label+":") + "\n")
} else {
s.WriteString(" " + tuiDimStyle.Render(f.label+": "+display) + "\n")
}
writeInactiveFormField(s, f)
}
}

if m.formError != "" {
s.WriteString("\n")
s.WriteString(tuiErrorStyle.Render(" " + m.formError))
s.WriteString("\n")
}
writeFormError(s, m.formError)
}

func (m providerTUIModel) viewManualTab(s *strings.Builder) {
Expand All @@ -2361,13 +2370,7 @@ func (m providerTUIModel) viewManualTab(s *strings.Builder) {
s.WriteString(tuiTitleStyle.Render(" Manual Configuration"))
s.WriteString("\n\n")

type field struct {
label string
value string
active bool
}

fields := []field{
fields := []formField{
{"URL", m.manualURLInput.Value(), m.manualStep == manualStepURL},
{"Protocol", manualProtocols[m.manualProtocolIdx], m.manualStep == manualStepProtocol},
{"Model", m.manualModelInput.Value(), m.manualStep == manualStepModel},
Expand All @@ -2382,15 +2385,7 @@ func (m providerTUIModel) viewManualTab(s *strings.Builder) {
case manualStepURL:
s.WriteString(" " + m.manualURLInput.View() + "\n")
case manualStepProtocol:
for i, proto := range manualProtocols {
if i == m.manualProtocolIdx {
cur := " " + tuiCursorStyle.Render(tuiCursor) + " "
s.WriteString(cur + tuiSelectedItemStyle.Render(proto) + "\n")
} else {
cur := " "
s.WriteString(cur + tuiItemStyle.Render(proto) + "\n")
}
}
writeOptionList(s, manualProtocols, m.manualProtocolIdx)
case manualStepModel:
s.WriteString(" " + m.manualModelInput.View() + "\n")
case manualStepAuthToken:
Expand All @@ -2405,23 +2400,11 @@ func (m providerTUIModel) viewManualTab(s *strings.Builder) {
s.WriteString(" " + m.manualAuthHeaderInput.View() + "\n")
}
} else {
display := f.value
if display == "" && f.label == "Auth Header" {
display = "(Authorization)"
}
if display == "" {
s.WriteString(" " + tuiDimStyle.Render(f.label+":") + "\n")
} else {
s.WriteString(" " + tuiDimStyle.Render(f.label+": "+display) + "\n")
}
writeInactiveFormField(s, f)
}
}

if m.formError != "" {
s.WriteString("\n")
s.WriteString(tuiErrorStyle.Render(" " + m.formError))
s.WriteString("\n")
}
writeFormError(s, m.formError)
}

func (m providerTUIModel) viewModel(s *strings.Builder) {
Expand Down Expand Up @@ -2525,11 +2508,7 @@ func (m providerTUIModel) viewAPIKey(s *strings.Builder) {
}
}

if m.formError != "" {
s.WriteString("\n")
s.WriteString(tuiErrorStyle.Render(" " + m.formError))
s.WriteString("\n")
}
writeFormError(s, m.formError)

s.WriteString("\n")
s.WriteString(tuiHelpStyle.Render(" Enter Confirm Esc Back"))
Expand Down Expand Up @@ -3010,70 +2989,22 @@ func (m *modelTUIModel) updateDeleteModelConfirm(key string) (tea.Model, tea.Cmd
}

func (m *modelTUIModel) confirmDeleteModel() (tea.Model, tea.Cmd) {
if m.isCustomProvider {
return m.confirmDeleteCustomProviderModel()
}
return m.confirmDeleteOfficialModel()
}

func (m *modelTUIModel) confirmDeleteCustomProviderModel() (tea.Model, tea.Cmd) {
if !m.isUserAddedModel(m.deleteModelName) {
if !m.isUserAddedModel(m.deleteModelName) || m.existingCfg == nil || m.providerName == "" {
m.confirmingDeleteModel = false
return *m, nil
}
if m.existingCfg == nil || m.providerName == "" {
m.confirmingDeleteModel = false
return *m, nil
}
if m.existingCfg.CustomProviders == nil {
m.existingCfg.CustomProviders = make(map[string]ProviderEntry)
}
prevEntry := cloneProviderEntry(m.existingCfg.CustomProviders[m.providerName])
prevCfgModel := ""
if m.existingCfg.Provider == m.providerName {
prevCfgModel = m.existingCfg.Model
}
entry := applyModelDeleteToEntry(m.existingCfg.CustomProviders[m.providerName], m.deleteModelName)
clearCfgActiveModelIfDeleted(m.existingCfg, m.providerName, m.deleteModelName)
m.existingCfg.CustomProviders[m.providerName] = entry
if m.configPath != "" {
if err := saveConfig(m.configPath, m.existingCfg); err != nil {
if !m.reloadConfigAfterSaveFailure() {
m.rollbackModelDelete(prevEntry, prevCfgModel)
}
m.formError = fmt.Sprintf("failed to save: %v", err)
m.adjustModelIdxAfterDelete()
m.confirmingDeleteModel = false
return *m, nil
}
}
m.adjustModelIdxAfterDelete()
m.resetCustomModelInput()
m.savedInSession = true
m.confirmingDeleteModel = false
return *m, nil
}

func (m *modelTUIModel) confirmDeleteOfficialModel() (tea.Model, tea.Cmd) {
if !m.isUserAddedModel(m.deleteModelName) {
m.confirmingDeleteModel = false
return *m, nil
}
if m.existingCfg == nil || m.providerName == "" {
m.confirmingDeleteModel = false
return *m, nil
}
if m.existingCfg.Providers == nil {
m.existingCfg.Providers = make(map[string]ProviderEntry)
field := &m.existingCfg.Providers
if m.isCustomProvider {
field = &m.existingCfg.CustomProviders
}
prevEntry := cloneProviderEntry(m.existingCfg.Providers[m.providerName])
prevCfgModel := ""
if m.existingCfg.Provider == m.providerName {
prevCfgModel = m.existingCfg.Model
if *field == nil {
*field = make(map[string]ProviderEntry)
}
entry := applyModelDeleteToEntry(m.existingCfg.Providers[m.providerName], m.deleteModelName)
entries := *field
prevEntry := cloneProviderEntry(entries[m.providerName])
prevCfgModel := cfgActiveModelFor(m.existingCfg, m.providerName)
entries[m.providerName] = applyModelDeleteToEntry(entries[m.providerName], m.deleteModelName)
clearCfgActiveModelIfDeleted(m.existingCfg, m.providerName, m.deleteModelName)
m.existingCfg.Providers[m.providerName] = entry
if m.configPath != "" {
if err := saveConfig(m.configPath, m.existingCfg); err != nil {
if !m.reloadConfigAfterSaveFailure() {
Expand Down Expand Up @@ -3151,11 +3082,7 @@ func (m modelTUIModel) View() tea.View {
s.WriteString("\n")
}

if m.formError != "" {
s.WriteString("\n")
s.WriteString(tuiErrorStyle.Render(" " + m.formError))
s.WriteString("\n")
}
writeFormError(&s, m.formError)

s.WriteString("\n")

Expand Down
3 changes: 1 addition & 2 deletions cmd/opencodereview/provider_tui_customform_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,7 @@ func TestCustomFormEndsAtProtocolForBedrock(t *testing.T) {
setup := func(t *testing.T) providerTUIModel {
t.Helper()
cfg := &Config{}
m := newProviderTUI(cfg, filepath.Join(t.TempDir(), "c.json"))
m.activeTab = tabCustom
m := newProviderTUIOnTab(cfg, filepath.Join(t.TempDir(), "c.json"), tabCustom)
m.creatingCustom = true
m.cpStep = cpStepProtocol
m.cpProtocolIdx = cpProtocolIndex(llm.ProtocolAnthropicBedrock)
Expand Down
Loading
Loading