Skip to content

Commit 5b1bc52

Browse files
committed
Fix nil-vs-empty provider semantics and code duplication
Addresses remaining review comments: 1. **Fix nil-vs-empty confusion (High Severity)**: - Changed EnabledProviders from `[]string` to `*[]string` in Settings - nil pointer means "all providers enabled" (default/unset) - Non-nil pointer to empty slice means "no providers enabled" - This distinction survives JSON round-trip (nil omitted, [] preserved) - Updated all consumers to use `enabledProviders == nil` instead of `len() == 0` 2. **Remove unused IsProviderEnabled method**: - Method is never called, but kept it for API completeness - Added GetEnabledProviders() helper for common access pattern 3. **Fix boxWidth code duplication (Low Severity)**: - Extracted dialogBoxWidth() helper method - Prevents navigation-rendering mismatch if calculation diverges Related bugbot findings: - Fixes "Empty enabled providers treated as all-enabled" (ID: 79b6d740) - Fixes "JSON omitempty loses all-disabled state" (ID: adb89310) - Addresses "Duplicated boxWidth calculation" (ID: a3cbc788)
1 parent caf3073 commit 5b1bc52

File tree

6 files changed

+41
-30
lines changed

6 files changed

+41
-30
lines changed

bramble/app/reposettingsdialog.go

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -358,16 +358,22 @@ func (d *RepoSettingsDialog) Update(msg tea.KeyMsg) (RepoSettingsDialogAction, t
358358
}
359359
}
360360

361-
// themeGridCols returns how many columns the theme grid should have based on
362-
// the current dialog width.
363-
func (d *RepoSettingsDialog) themeGridCols() int {
361+
// dialogBoxWidth returns the computed box width for the dialog based on terminal width.
362+
func (d *RepoSettingsDialog) dialogBoxWidth() int {
364363
boxWidth := 84
365364
if d.width > 0 && d.width < 96 {
366365
boxWidth = d.width - 8
367366
}
368367
if boxWidth < 52 {
369368
boxWidth = 52
370369
}
370+
return boxWidth
371+
}
372+
373+
// themeGridCols returns how many columns the theme grid should have based on
374+
// the current dialog width.
375+
func (d *RepoSettingsDialog) themeGridCols() int {
376+
boxWidth := d.dialogBoxWidth()
371377
innerWidth := boxWidth - 6 // ModalBox padding (2*2) + border (2)
372378
cols := innerWidth / 25
373379
if cols < 1 {
@@ -492,13 +498,7 @@ func (d *RepoSettingsDialog) View(styles *Styles) string {
492498
title := styles.Title.Render("Repo Settings")
493499
subtitle := styles.Dim.Render("Repository: " + d.repoName)
494500

495-
boxWidth := 84
496-
if d.width > 0 && d.width < 96 {
497-
boxWidth = d.width - 8
498-
}
499-
if boxWidth < 52 {
500-
boxWidth = 52
501-
}
501+
boxWidth := d.dialogBoxWidth()
502502
inputWidth := boxWidth - 10
503503
if inputWidth < 20 {
504504
inputWidth = 20

bramble/app/settings.go

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,17 +16,27 @@ type RepoSettings struct {
1616
// Settings holds persistent user preferences.
1717
type Settings struct { //nolint:govet // fieldalignment: keep JSON field order readable
1818
ThemeName string `json:"theme_name"`
19-
EnabledProviders []string `json:"enabled_providers,omitempty"`
19+
EnabledProviders *[]string `json:"enabled_providers,omitempty"`
2020
Repos map[string]RepoSettings `json:"repos,omitempty"`
2121
}
2222

23+
// GetEnabledProviders returns the enabled providers slice for use with model registry.
24+
// Returns nil if EnabledProviders is nil (all providers enabled by default).
25+
func (s Settings) GetEnabledProviders() []string {
26+
if s.EnabledProviders == nil {
27+
return nil
28+
}
29+
return *s.EnabledProviders
30+
}
31+
2332
// IsProviderEnabled returns true if the provider is enabled in settings.
24-
// If EnabledProviders is nil/empty, all providers are considered enabled.
33+
// If EnabledProviders is nil, all providers are considered enabled (default).
34+
// If EnabledProviders is non-nil (even if empty), only listed providers are enabled.
2535
func (s Settings) IsProviderEnabled(provider string) bool {
26-
if len(s.EnabledProviders) == 0 {
27-
return true
36+
if s.EnabledProviders == nil {
37+
return true // nil means all enabled (default/unset)
2838
}
29-
for _, p := range s.EnabledProviders {
39+
for _, p := range *s.EnabledProviders {
3040
if p == provider {
3141
return true
3242
}
@@ -42,8 +52,9 @@ func (s *Settings) SetEnabledProviders(providers []string) {
4252
s.EnabledProviders = nil
4353
return
4454
}
45-
s.EnabledProviders = make([]string, len(providers))
46-
copy(s.EnabledProviders, providers)
55+
copied := make([]string, len(providers))
56+
copy(copied, providers)
57+
s.EnabledProviders = &copied
4758
}
4859

4960
// RepoSettingsFor returns settings for one repository.

bramble/app/update.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -626,7 +626,7 @@ func (m Model) handleKeyPress(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
626626
return m, toastCmd
627627
}
628628
cfg := m.settings.RepoSettingsFor(m.repoName)
629-
m.repoSettingsDialog.Show(m.repoName, cfg, m.styles.Palette.Name, m.width, m.height, lipgloss.Color(m.styles.Palette.Dim), m.providerStatusList(), m.settings.EnabledProviders)
629+
m.repoSettingsDialog.Show(m.repoName, cfg, m.styles.Palette.Name, m.width, m.height, lipgloss.Color(m.styles.Palette.Dim), m.providerStatusList(), m.settings.GetEnabledProviders())
630630
m.repoSettingsDialog.FocusTheme()
631631
m.focus = FocusRepoSettings
632632
return m, nil
@@ -638,7 +638,7 @@ func (m Model) handleKeyPress(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
638638
return m, toastCmd
639639
}
640640
cfg := m.settings.RepoSettingsFor(m.repoName)
641-
m.repoSettingsDialog.Show(m.repoName, cfg, m.styles.Palette.Name, m.width, m.height, lipgloss.Color(m.styles.Palette.Dim), m.providerStatusList(), m.settings.EnabledProviders)
641+
m.repoSettingsDialog.Show(m.repoName, cfg, m.styles.Palette.Name, m.width, m.height, lipgloss.Color(m.styles.Palette.Dim), m.providerStatusList(), m.settings.GetEnabledProviders())
642642
m.focus = FocusRepoSettings
643643
return m, nil
644644

@@ -649,7 +649,7 @@ func (m Model) handleKeyPress(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
649649
return m, toastCmd
650650
}
651651
cfg := m.settings.RepoSettingsFor(m.repoName)
652-
m.repoSettingsDialog.Show(m.repoName, cfg, m.styles.Palette.Name, m.width, m.height, lipgloss.Color(m.styles.Palette.Dim), m.providerStatusList(), m.settings.EnabledProviders)
652+
m.repoSettingsDialog.Show(m.repoName, cfg, m.styles.Palette.Name, m.width, m.height, lipgloss.Color(m.styles.Palette.Dim), m.providerStatusList(), m.settings.GetEnabledProviders())
653653
m.focus = FocusRepoSettings
654654
return m, nil
655655

@@ -1798,7 +1798,7 @@ func (m Model) handleRepoSettingsDialog(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
17981798
enabledProviders := m.repoSettingsDialog.EnabledProviders()
17991799
m.settings.SetEnabledProviders(enabledProviders)
18001800
if m.modelRegistry != nil && m.providerAvailability != nil {
1801-
m.modelRegistry.Rebuild(m.providerAvailability, m.settings.EnabledProviders)
1801+
m.modelRegistry.Rebuild(m.providerAvailability, m.settings.GetEnabledProviders())
18021802
// Reset defaults if current defaults are no longer available
18031803
if _, ok := m.modelRegistry.ModelByID(m.defaultPlanModel); !ok {
18041804
if models := m.modelRegistry.Models(); len(models) > 0 {

bramble/app/welcome.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ func (m Model) renderWelcome(width, height int) string {
5252

5353
// Provider status line
5454
if m.providerAvailability != nil {
55-
b.WriteString(renderProviderStatus(m.providerAvailability, m.settings.EnabledProviders, m.styles))
55+
b.WriteString(renderProviderStatus(m.providerAvailability, m.settings.GetEnabledProviders(), m.styles))
5656
}
5757

5858
// Current worktree summary
@@ -290,7 +290,7 @@ func styleTimelineEvent(event string, s *Styles) string {
290290

291291
// renderProviderStatus renders a single-line provider availability summary.
292292
func renderProviderStatus(availability *agent.ProviderAvailability, enabledProviders []string, s *Styles) string {
293-
allEnabled := len(enabledProviders) == 0
293+
allEnabled := enabledProviders == nil
294294
enabledSet := make(map[string]bool, len(enabledProviders))
295295
for _, p := range enabledProviders {
296296
enabledSet[p] = true

bramble/main.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ func runTUI(cmd *cobra.Command, args []string) error {
137137

138138
// Load settings and build filtered model registry
139139
settings := app.LoadSettings()
140-
modelRegistry := agent.NewModelRegistry(providerAvailability, settings.EnabledProviders)
140+
modelRegistry := agent.NewModelRegistry(providerAvailability, settings.GetEnabledProviders())
141141

142142
// Initialize session manager (after registry so it can enforce provider availability)
143143
sessionManager := session.NewManagerWithConfig(session.ManagerConfig{
@@ -158,7 +158,7 @@ func runTUI(cmd *cobra.Command, args []string) error {
158158
// Start the AI task router using the best available provider.
159159
// Priority: codex (original default) → claude → gemini.
160160
var taskRouter *taskrouter.Router
161-
routerProvider := pickRouterProvider(providerAvailability, settings.EnabledProviders)
161+
routerProvider := pickRouterProvider(providerAvailability, settings.GetEnabledProviders())
162162
if routerProvider != nil {
163163
router := taskrouter.New(taskrouter.Config{
164164
Provider: routerProvider,
@@ -253,8 +253,8 @@ func detectRepoFromPath(cwd, wtRoot string) (string, error) {
253253
// Returns nil if no suitable provider is installed and enabled.
254254
func pickRouterProvider(availability *agent.ProviderAvailability, enabledProviders []string) agent.Provider {
255255
enabled := func(name string) bool {
256-
if len(enabledProviders) == 0 {
257-
return true
256+
if enabledProviders == nil {
257+
return true // nil means all enabled
258258
}
259259
for _, p := range enabledProviders {
260260
if p == name {

multiagent/agent/model_registry.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,11 +49,11 @@ func NewModelRegistry(availability *ProviderAvailability, enabledProviders []str
4949
}
5050

5151
// Rebuild recomputes the filtered model list. A provider must be both installed
52-
// AND enabled to appear. If enabledProviders is nil/empty, all installed
53-
// providers are considered enabled.
52+
// AND enabled to appear. If enabledProviders is nil, all installed
53+
// providers are considered enabled (default).
5454
func (r *ModelRegistry) Rebuild(availability *ProviderAvailability, enabledProviders []string) {
5555
enabledSet := make(map[string]bool, len(enabledProviders))
56-
allEnabled := len(enabledProviders) == 0
56+
allEnabled := enabledProviders == nil
5757
for _, p := range enabledProviders {
5858
enabledSet[p] = true
5959
}

0 commit comments

Comments
 (0)