refactor(colorscheme): cache sorted detectors and fix Refresh deadlock

commit: c65109cccf6857ab1f0b2e88fd782b863c77b860
author: bnema · 9mo ago · 2025-12-30 04:11 UTC
parent: 1d91c2c
signature: GPG Unverified2B82207D370127A0 The key is unknown

2 files · +32 -36

M internal/infrastructure/colorscheme/resolver.go +32 -26

@@ -1,7 +1,8 @@
1	1		package colorscheme
2	2
3	3		import (
4		-	"sort"
	4	+	"cmp"
	5	+	"slices"
5	6		"strings"
6	7		"sync"
7	8

@@ -30,11 +31,12 @@ type callbackWrapper struct {
30	31		// Resolver implements port.ColorSchemeResolver.
31	32		// It manages multiple detectors and respects config overrides.
32	33		type Resolver struct {
33		-	mu sync.RWMutex
34		-	config ConfigProvider
35		-	detectors []port.ColorSchemeDetector
36		-	current port.ColorSchemePreference
37		-	callbacks []*callbackWrapper
	34	+	mu sync.RWMutex
	35	+	config ConfigProvider
	36	+	detectors []port.ColorSchemeDetector
	37	+	sortedDetectors []port.ColorSchemeDetector // cached sorted detectors
	38	+	current port.ColorSchemePreference
	39	+	callbacks []*callbackWrapper
38	40		}
39	41
40	42		// NewResolver creates a new color scheme resolver.

@@ -78,15 +80,8 @@ func (r *Resolver) resolveInternal() port.ColorSchemePreference {
78	80		}
79	81		}
80	82
81		-	// Sort detectors by priority (highest first)
82		-	sorted := make([]port.ColorSchemeDetector, len(r.detectors))
83		-	copy(sorted, r.detectors)
84		-	sort.Slice(sorted, func(i, j int) bool {
85		-	return sorted[i].Priority() > sorted[j].Priority()
86		-	})
87		-
88		-	// Try each detector in priority order
89		-	for _, detector := range sorted {
	83	+	// Try each detector in priority order (already sorted)
	84	+	for _, detector := range r.sortedDetectors {
90	85		if !detector.Available() {
91	86		continue
92	87		}

@@ -110,32 +105,43 @@ func (r *Resolver) RegisterDetector(detector port.ColorSchemeDetector) {
110	105		r.mu.Lock()
111	106		defer r.mu.Unlock()
112	107		r.detectors = append(r.detectors, detector)
	108	+	r.rebuildSortedDetectors()
	109	+	}
	110	+
	111	+	// rebuildSortedDetectors creates a sorted copy of detectors by priority (highest first).
	112	+	// Caller must hold the write lock.
	113	+	func (r *Resolver) rebuildSortedDetectors() {
	114	+	r.sortedDetectors = make([]port.ColorSchemeDetector, len(r.detectors))
	115	+	copy(r.sortedDetectors, r.detectors)
	116	+	slices.SortFunc(r.sortedDetectors, func(a, b port.ColorSchemeDetector) int {
	117	+	return cmp.Compare(b.Priority(), a.Priority()) // descending order
	118	+	})
113	119		}
114	120
115	121		// Refresh implements port.ColorSchemeResolver.
116	122		func (r *Resolver) Refresh() port.ColorSchemePreference {
117	123		r.mu.Lock()
118		-	defer r.mu.Unlock()
119	124
120	125		newPref := r.resolveInternal()
121	126
122		-	// Only notify if preference changed
	127	+	// Prepare callbacks to invoke if preference changed
	128	+	var callbacks []*callbackWrapper
123	129		if newPref.PrefersDark != r.current.PrefersDark {
124	130		r.current = newPref
125		-	// Copy callbacks to avoid holding lock during callback invocation
126		-	callbacks := make([]*callbackWrapper, len(r.callbacks))
	131	+	callbacks = make([]*callbackWrapper, len(r.callbacks))
127	132		copy(callbacks, r.callbacks)
128		-
129		-	// Invoke callbacks outside of lock
130		-	r.mu.Unlock()
131		-	for _, cb := range callbacks {
132		-	cb.fn(newPref)
133		-	}
134		-	r.mu.Lock()
135	133		} else {
136	134		r.current = newPref
137	135		}
138	136
	137	+	// Release lock before invoking callbacks to avoid deadlocks
	138	+	r.mu.Unlock()
	139	+
	140	+	// Invoke callbacks outside of lock
	141	+	for _, cb := range callbacks {
	142	+	cb.fn(newPref)
	143	+	}
	144	+
139	145		return newPref
140	146		}
141	147

 package colorscheme
 import (
-	"sort"
+	"cmp"
+	"slices"
 	"strings"
 	"sync"
 // Resolver implements port.ColorSchemeResolver.
 // It manages multiple detectors and respects config overrides.
 type Resolver struct {
-	mu        sync.RWMutex
+	mu              sync.RWMutex
-	config    ConfigProvider
+	config          ConfigProvider
-	detectors []port.ColorSchemeDetector
+	detectors       []port.ColorSchemeDetector
-	current   port.ColorSchemePreference
+	sortedDetectors []port.ColorSchemeDetector // cached sorted detectors
-	callbacks []*callbackWrapper
+	current         port.ColorSchemePreference
+	callbacks       []*callbackWrapper
 }
 // NewResolver creates a new color scheme resolver.
 		}
 	}
-	// Sort detectors by priority (highest first)
+	// Try each detector in priority order (already sorted)
-	sorted := make([]port.ColorSchemeDetector, len(r.detectors))
+	for _, detector := range r.sortedDetectors {
-	copy(sorted, r.detectors)
-	sort.Slice(sorted, func(i, j int) bool {
-		return sorted[i].Priority() > sorted[j].Priority()
-	})
-	// Try each detector in priority order
-	for _, detector := range sorted {
 		if !detector.Available() {
 			continue
 		}
 	r.mu.Lock()
 	defer r.mu.Unlock()
 	r.detectors = append(r.detectors, detector)
+	r.rebuildSortedDetectors()
+}
+// rebuildSortedDetectors creates a sorted copy of detectors by priority (highest first).
+// Caller must hold the write lock.
+func (r *Resolver) rebuildSortedDetectors() {
+	r.sortedDetectors = make([]port.ColorSchemeDetector, len(r.detectors))
+	copy(r.sortedDetectors, r.detectors)
+	slices.SortFunc(r.sortedDetectors, func(a, b port.ColorSchemeDetector) int {
+		return cmp.Compare(b.Priority(), a.Priority()) // descending order
+	})
 }
 // Refresh implements port.ColorSchemeResolver.
 func (r *Resolver) Refresh() port.ColorSchemePreference {
 	r.mu.Lock()
-	defer r.mu.Unlock()
 	newPref := r.resolveInternal()
-	// Only notify if preference changed
+	// Prepare callbacks to invoke if preference changed
+	var callbacks []*callbackWrapper
 	if newPref.PrefersDark != r.current.PrefersDark {
 		r.current = newPref
-		// Copy callbacks to avoid holding lock during callback invocation
+		callbacks = make([]*callbackWrapper, len(r.callbacks))
-		callbacks := make([]*callbackWrapper, len(r.callbacks))
 		copy(callbacks, r.callbacks)
-		// Invoke callbacks outside of lock
-		r.mu.Unlock()
-		for _, cb := range callbacks {
-			cb.fn(newPref)
-		}
-		r.mu.Lock()
 	} else {
 		r.current = newPref
 	}
+	// Release lock before invoking callbacks to avoid deadlocks
+	r.mu.Unlock()
+	// Invoke callbacks outside of lock
+	for _, cb := range callbacks {
+		cb.fn(newPref)
+	}
 	return newPref
 }

M internal/ui/theme/manager.go +0 -10


@@ -24,16 +24,6 @@ type Manager struct {
24	24		colorResolver port.ColorSchemeResolver // Resolver for dynamic detection
25	25		}
26	26
27		-	// ManagerOption is a functional option for configuring Manager.
28		-	type ManagerOption func(*Manager)
29		-
30		-	// WithResolver configures the Manager to use a ColorSchemeResolver.
31		-	func WithResolver(resolver port.ColorSchemeResolver) ManagerOption {
32		-	return func(m *Manager) {
33		-	m.colorResolver = resolver
34		-	}
35		-	}
36		-
37	27		// NewManager creates a new theme manager from configuration.
38	28		// The ColorSchemeResolver is required for proper color scheme detection.
39	29		func NewManager(ctx context.Context, cfg config.Config, resolver port.ColorSchemeResolver) Manager {


@@ -24,16 +24,6 @@ type Manager struct {
24		colorResolver port.ColorSchemeResolver // Resolver for dynamic detection	24	colorResolver port.ColorSchemeResolver // Resolver for dynamic detection
25		}	25	}
26			26
27	-	// ManagerOption is a functional option for configuring Manager.
28	-	type ManagerOption func(*Manager)
29	-
30	-	// WithResolver configures the Manager to use a ColorSchemeResolver.
31	-	func WithResolver(resolver port.ColorSchemeResolver) ManagerOption {
32	-	return func(m *Manager) {
33	-	m.colorResolver = resolver
34	-	}
35	-	}
36	-
37		// NewManager creates a new theme manager from configuration.	27	// NewManager creates a new theme manager from configuration.
38		// The ColorSchemeResolver is required for proper color scheme detection.	28	// The ColorSchemeResolver is required for proper color scheme detection.
39		func NewManager(ctx context.Context, cfg config.Config, resolver port.ColorSchemeResolver) Manager {	29	func NewManager(ctx context.Context, cfg config.Config, resolver port.ColorSchemeResolver) Manager {