Fix theme ownership and propagation

Amolith created 1 month ago

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.

Change summary

internal/ui/session.go      |  30 ++++----
internal/ui/session_test.go | 127 ++++++++++++++++++++++++++++++++------
2 files changed, 123 insertions(+), 34 deletions(-)

Detailed changes

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

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.