From 6190c5c265cb4b593e7eefa5b0781202ff35ab20 Mon Sep 17 00:00:00 2001 From: Amolith Date: Sun, 29 Mar 2026 23:50:06 -0600 Subject: [PATCH] Fix theme ownership and propagation The session previously allocated *theme.Styles internally and exposed it via a Styles() accessor. This created a chicken-and-egg problem: screens needed the pointer at construction time, but they were passed to New() which hadn't been called yet. Tests worked around it by mutating s.screens directly, but external callers in cmd/ could not. Change New() to accept *theme.Styles from the caller, who allocates once and passes the same pointer to both screen constructors and the session. Remove the Styles() accessor since it is no longer needed. Additionally, the session was swallowing BackgroundColorMsg after updating the shared styles, preventing screens from reacting to theme changes. Screens that cache themed state (like huh forms) had stale themes if background detection arrived after construction. Forward BackgroundColorMsg to the active screen after updating styles so screens can rebuild cached state. --- internal/ui/session.go | 30 +++++---- internal/ui/session_test.go | 127 ++++++++++++++++++++++++++++++------ 2 files changed, 123 insertions(+), 34 deletions(-) diff --git a/internal/ui/session.go b/internal/ui/session.go index c8f78473f07b397f9dacef8e90c00ba307269f74..7644d631b328351f20e4662c5544ae72bfe73e1e 100644 --- a/internal/ui/session.go +++ b/internal/ui/session.go @@ -65,16 +65,17 @@ type Session struct { completed bool } -// New creates a session with the given screens. Screens that should be -// skipped (because their value is already resolved) should simply not -// be included in the slice — the caller is responsible for building -// only the screens that need user input. -func New(screens []Screen) Session { - // Default to dark until we hear from the terminal. - sty := theme.New(true) - +// New creates a session with the given screens and shared styles. +// The caller allocates a [theme.Styles] and passes the same pointer +// to both screen constructors and this function, so everyone shares +// a single theme that the session updates on background detection. +// +// Screens that should be skipped (because their value is already +// resolved) should simply not be included in the slice — the caller +// is responsible for building only the screens that need user input. +func New(screens []Screen, styles *theme.Styles) Session { h := help.New() - h.Styles = sty.Help + h.Styles = styles.Help // Copy the slice so mutations during forwardToScreen don't // affect the caller's original. @@ -83,7 +84,7 @@ func New(screens []Screen) Session { return Session{ screens: owned, - styles: &sty, + styles: styles, help: h, } } @@ -114,10 +115,11 @@ func (s Session) Init() tea.Cmd { func (s Session) Update(msg tea.Msg) (tea.Model, tea.Cmd) { switch msg := msg.(type) { case tea.BackgroundColorMsg: - sty := theme.New(msg.IsDark()) - s.styles = &sty - s.help.Styles = sty.Help - return s, nil + *s.styles = theme.New(msg.IsDark()) + s.help.Styles = s.styles.Help + // Forward to the active screen so it can rebuild any + // cached themed state (e.g. huh form themes). + return s.forwardToScreen(msg) case tea.WindowSizeMsg: s.width = msg.Width diff --git a/internal/ui/session_test.go b/internal/ui/session_test.go index cffe63dd479934337c2a3076f16ed76fb5d7b4ab..635a4d6f65b4dc533f3834bd6635025689277997 100644 --- a/internal/ui/session_test.go +++ b/internal/ui/session_test.go @@ -1,11 +1,14 @@ package ui import ( + "image/color" "strings" "testing" "charm.land/bubbles/v2/key" tea "charm.land/bubbletea/v2" + + "git.secluded.site/keld/internal/theme" ) // fakeScreen is a minimal Screen implementation for testing session @@ -100,6 +103,13 @@ func (s *sizeCaptureScreen) Update(msg tea.Msg) (Screen, tea.Cmd) { return s.fakeScreen.Update(msg) } +// defaultStyles returns a *theme.Styles for use in tests. Callers +// pass this to both New() and any screen constructors that need it. +func defaultStyles() *theme.Styles { + s := theme.New(true) + return &s +} + func escMsg() tea.Msg { return tea.KeyPressMsg{Code: tea.KeyEscape} } @@ -122,7 +132,7 @@ func TestSessionInit(t *testing.T) { a := newFake("first") b := newFake("second") - s := New([]Screen{a, b}) + s := New([]Screen{a, b}, defaultStyles()) s.Init() if a.initCalls != 1 { @@ -136,7 +146,7 @@ func TestSessionInit(t *testing.T) { func TestCtrlCExits(t *testing.T) { t.Parallel() - s := New([]Screen{newFake("one")}) + s := New([]Screen{newFake("one")}, defaultStyles()) s.Init() result, cmd := s.Update(ctrlCMsg()) @@ -153,7 +163,7 @@ func TestCtrlCExits(t *testing.T) { func TestEscOnFirstScreenExits(t *testing.T) { t.Parallel() - s := New([]Screen{newFake("one")}) + s := New([]Screen{newFake("one")}, defaultStyles()) s.Init() // Esc is forwarded to the screen, which returns BackCmd. @@ -177,7 +187,7 @@ func TestEscNavigatesBack(t *testing.T) { b := newFake("second") a.complete("picked") - s := New([]Screen{a, b}) + s := New([]Screen{a, b}, defaultStyles()) s.cursor = 1 // Esc → screen returns BackCmd → session receives BackMsg. @@ -197,7 +207,7 @@ func TestBackNavigationPreservesState(t *testing.T) { b := newFake("second") a.complete("picked") - s := New([]Screen{a, b}) + s := New([]Screen{a, b}, defaultStyles()) s.cursor = 1 result, _ := s.Update(escMsg()) @@ -217,7 +227,7 @@ func TestBackNavDoesNotReAdvance(t *testing.T) { b := newFake("second") a.complete("picked") - s := New([]Screen{a, b}) + s := New([]Screen{a, b}, defaultStyles()) s.cursor = 1 // Navigate back to the first screen. @@ -246,7 +256,7 @@ func TestScreenAdvancesOnDoneMsg(t *testing.T) { a := newSelecting("first") b := newFake("second") - s := New([]Screen{a, b}) + s := New([]Screen{a, b}, defaultStyles()) s.Init() // Send a key to trigger selection on the first screen. The @@ -274,7 +284,7 @@ func TestReAdvanceAfterBackAndNewSelection(t *testing.T) { a := newSelecting("first") b := newFake("second") - s := New([]Screen{a, b}) + s := New([]Screen{a, b}, defaultStyles()) s.Init() // Advance past the first screen. @@ -312,7 +322,7 @@ func TestEscConsumedByScreen(t *testing.T) { b := newEscConsuming("picker") a.complete("restore") - s := New([]Screen{a, b}) + s := New([]Screen{a, b}, defaultStyles()) s.cursor = 1 // Esc is forwarded to the screen, which handles it internally @@ -334,7 +344,7 @@ func TestEscConsumedByScreen(t *testing.T) { func TestBreadcrumbEmpty(t *testing.T) { t.Parallel() - s := New([]Screen{newFake("one")}) + s := New([]Screen{newFake("one")}, defaultStyles()) crumb := s.breadcrumb() if crumb != "" { @@ -352,7 +362,7 @@ func TestBreadcrumbShowsCompletedSelections(t *testing.T) { a.complete("restore") b.complete("home@cloud") - s := New([]Screen{a, b, c}) + s := New([]Screen{a, b, c}, defaultStyles()) s.cursor = 2 crumb := s.breadcrumb() @@ -374,7 +384,7 @@ func TestBreadcrumbUpdatesOnBack(t *testing.T) { a.complete("restore") b.complete("home@cloud") - s := New([]Screen{a, b, c}) + s := New([]Screen{a, b, c}, defaultStyles()) s.cursor = 2 result, _ := s.Update(escMsg()) @@ -399,7 +409,7 @@ func TestViewIncludesChrome(t *testing.T) { a.complete("restore") b := newFake("Second Step") - s := New([]Screen{a, b}) + s := New([]Screen{a, b}, defaultStyles()) s.cursor = 1 s.width = 80 s.height = 24 @@ -420,7 +430,7 @@ func TestViewIncludesChrome(t *testing.T) { func TestViewEmptyScreens(t *testing.T) { t.Parallel() - s := New([]Screen{}) + s := New([]Screen{}, defaultStyles()) view := s.View() if view.Content != "" { t.Errorf("view with no screens should be empty, got %q", view.Content) @@ -434,7 +444,7 @@ func TestBackNavReInitsScreen(t *testing.T) { b := newFake("second") a.complete("cmd") - s := New([]Screen{a, b}) + s := New([]Screen{a, b}, defaultStyles()) s.cursor = 1 result, _ := s.Update(escMsg()) @@ -450,7 +460,7 @@ func TestDoneOnLastScreenCompletes(t *testing.T) { t.Parallel() a := newSelecting("only") - s := New([]Screen{a}) + s := New([]Screen{a}, defaultStyles()) s.Init() // Select on the only screen — DoneMsg should complete the session. @@ -472,7 +482,7 @@ func TestDoneOnLastScreenCompletes(t *testing.T) { func TestCtrlCIsNotCompleted(t *testing.T) { t.Parallel() - s := New([]Screen{newFake("one")}) + s := New([]Screen{newFake("one")}, defaultStyles()) s.Init() result, _ := s.Update(ctrlCMsg()) @@ -489,7 +499,7 @@ func TestCtrlCIsNotCompleted(t *testing.T) { func TestEscExitIsNotCompleted(t *testing.T) { t.Parallel() - s := New([]Screen{newFake("one")}) + s := New([]Screen{newFake("one")}, defaultStyles()) s.Init() result, _ := s.Update(escMsg()) @@ -513,7 +523,7 @@ func TestWindowSizeAdjustedForChrome(t *testing.T) { onSize: func(h int) { receivedHeight = h }, } - s := New([]Screen{screen}) + s := New([]Screen{screen}, defaultStyles()) s.Init() s.Update(tea.WindowSizeMsg{Width: 80, Height: 30}) @@ -524,6 +534,83 @@ func TestWindowSizeAdjustedForChrome(t *testing.T) { } } +// themeAwareScreen holds a *theme.Styles pointer, as real screen +// implementations will. This lets us verify that the session's theme +// update propagates through the shared pointer. +type themeAwareScreen struct { + fakeScreen + styles *theme.Styles +} + +// bgCapturingScreen records whether it received a BackgroundColorMsg. +type bgCapturingScreen struct { + fakeScreen + onBg func() +} + +func (b *bgCapturingScreen) Update(msg tea.Msg) (Screen, tea.Cmd) { + if _, ok := msg.(tea.BackgroundColorMsg); ok { + if b.onBg != nil { + b.onBg() + } + return b, nil + } + return b.fakeScreen.Update(msg) +} + +func newThemeAware(title string, styles *theme.Styles) *themeAwareScreen { + return &themeAwareScreen{ + fakeScreen: fakeScreen{title: title}, + styles: styles, + } +} + +func TestThemeUpdatePropagesToScreens(t *testing.T) { + t.Parallel() + + // Caller owns the styles allocation and passes the same pointer + // to both screen constructors and the session. + styles := defaultStyles() + screen := newThemeAware("test", styles) + s := New([]Screen{screen}, styles) + + if !screen.styles.Dark { + t.Fatal("precondition: styles should default to dark") + } + + // Simulate a light terminal background detection. + result, _ := s.Update(tea.BackgroundColorMsg{Color: color.White}) + _ = result.(Session) + + // The screen's pointer should now see the updated (light) styles + // because the session mutated through the shared pointer. + if screen.styles.Dark { + t.Error("screen still sees dark styles after light BackgroundColorMsg; session replaced pointer instead of mutating in place") + } +} + +func TestBackgroundColorForwardedToScreen(t *testing.T) { + t.Parallel() + + // Screens that cache themed state need to see BackgroundColorMsg + // so they can rebuild. Verify the session forwards it. + var received bool + screen := &bgCapturingScreen{ + fakeScreen: fakeScreen{title: "test"}, + onBg: func() { received = true }, + } + + styles := defaultStyles() + s := New([]Screen{screen}, styles) + s.Init() + + s.Update(tea.BackgroundColorMsg{Color: color.White}) + + if !received { + t.Error("BackgroundColorMsg was not forwarded to the active screen") + } +} + func TestSizeReplayedOnAdvance(t *testing.T) { t.Parallel() @@ -534,7 +621,7 @@ func TestSizeReplayedOnAdvance(t *testing.T) { onSize: func(h int) { receivedHeight = h }, } - s := New([]Screen{a, b}) + s := New([]Screen{a, b}, defaultStyles()) s.Init() // Set terminal size so the session caches it.