4ac613d fix(favicon): address follow-up review findings · dumber

M internal/application/usecase/favicon.go +3 -0


@@ -386,6 +386,9 @@ func (uc *FaviconUseCase) EnsureSized(ctx context.Context, key favicon.Key, size
386	386		if err != nil {
387	387		return err
388	388		}
	389	+	if uc.converter == nil {
	390	+	return ErrFaviconMiss
	391	+	}
389	392		converted, err := uc.converter.Convert(ctx, orig, ct, []int{size})
390	393		if err != nil {
391	394		return err


@@ -386,6 +386,9 @@ func (uc *FaviconUseCase) EnsureSized(ctx context.Context, key favicon.Key, size
386	if err != nil {	386		if err != nil {
387	return err	387		return err
388	}	388		}
		389	+	if uc.converter == nil {
		390	+	return ErrFaviconMiss
		391	+	}
389	converted, err := uc.converter.Convert(ctx, orig, ct, []int{size})	392		converted, err := uc.converter.Convert(ctx, orig, ct, []int{size})
390	if err != nil {	393		if err != nil {
391	return err	394		return err

M internal/application/usecase/favicon_test.go +12 -0


@@ -55,6 +55,18 @@ func TestFaviconResolveRepairsOrphanCanonicalBlobMetadata(t *testing.T) {
55	55		}
56	56		}
57	57
	58	+	func TestFaviconResolveRepairOrphanWithoutConverterReturnsMiss(t *testing.T) {
	59	+	fx := newFaviconFixture()
	60	+	fx.uc.converter = nil
	61	+	fx.blobs.original["example.com"] = []byte("original")
	62	+	fx.blobs.contentTypes["example.com"] = "image/png"
	63	+
	64	+	_, err := fx.uc.ResolveSystemviewIcon(context.Background(), "example.com", SystemviewIconSize)
	65	+	if !errors.Is(err, ErrFaviconMiss) {
	66	+	t.Fatalf("expected favicon miss, got %v", err)
	67	+	}
	68	+	}
	69	+
58	70		func TestFaviconResolveStaleSchedulesDedupedBackgroundRefresh(t *testing.T) {
59	71		fx := newFaviconFixture()
60	72		fx.seed("example.com", []byte("old"), true)


@@ -55,6 +55,18 @@ func TestFaviconResolveRepairsOrphanCanonicalBlobMetadata(t *testing.T) {
55	}	55		}
56	}	56		}
57		57
		58	+	func TestFaviconResolveRepairOrphanWithoutConverterReturnsMiss(t *testing.T) {
		59	+	fx := newFaviconFixture()
		60	+	fx.uc.converter = nil
		61	+	fx.blobs.original["example.com"] = []byte("original")
		62	+	fx.blobs.contentTypes["example.com"] = "image/png"
		63	+
		64	+	_, err := fx.uc.ResolveSystemviewIcon(context.Background(), "example.com", SystemviewIconSize)
		65	+	if !errors.Is(err, ErrFaviconMiss) {
		66	+	t.Fatalf("expected favicon miss, got %v", err)
		67	+	}
		68	+	}
		69	+
58	func TestFaviconResolveStaleSchedulesDedupedBackgroundRefresh(t *testing.T) {	70		func TestFaviconResolveStaleSchedulesDedupedBackgroundRefresh(t *testing.T) {
59	fx := newFaviconFixture()	71		fx := newFaviconFixture()
60	fx.seed("example.com", []byte("old"), true)	72		fx.seed("example.com", []byte("old"), true)

M internal/infrastructure/cef/webview_handlers.go +7 -4


@@ -387,11 +387,14 @@ func (wv *WebView) retainFaviconSourceVisitor(visitor purecef.StringVisitor) fun
387	387		wv.faviconSourceVisitorsMu.Lock()
388	388		defer wv.faviconSourceVisitorsMu.Unlock()
389	389		for i, candidate := range wv.faviconSourceVisitors {
390		-	if candidate.id == id {
391		-	wv.faviconSourceVisitors[i].visitor = nil
392		-	wv.faviconSourceVisitors = append(wv.faviconSourceVisitors[:i], wv.faviconSourceVisitors[i+1:]...)
393		-	return
	390	+	if candidate.id != id {
	391	+	continue
394	392		}
	393	+	last := len(wv.faviconSourceVisitors) - 1
	394	+	copy(wv.faviconSourceVisitors[i:], wv.faviconSourceVisitors[i+1:])
	395	+	wv.faviconSourceVisitors[last] = faviconSourceVisitorRetention{}
	396	+	wv.faviconSourceVisitors = wv.faviconSourceVisitors[:last]
	397	+	return
395	398		}
396	399		}
397	400		}

 		wv.faviconSourceVisitorsMu.Lock()
 		defer wv.faviconSourceVisitorsMu.Unlock()
 		for i, candidate := range wv.faviconSourceVisitors {
-			if candidate.id == id {
+			if candidate.id != id {
-				wv.faviconSourceVisitors[i].visitor = nil
+				continue
-				wv.faviconSourceVisitors = append(wv.faviconSourceVisitors[:i], wv.faviconSourceVisitors[i+1:]...)
-				return
 			}
+			last := len(wv.faviconSourceVisitors) - 1
+			copy(wv.faviconSourceVisitors[i:], wv.faviconSourceVisitors[i+1:])
+			wv.faviconSourceVisitors[last] = faviconSourceVisitorRetention{}
+			wv.faviconSourceVisitors = wv.faviconSourceVisitors[:last]
+			return
 		}
 	}
 }

M internal/ui/adapter/favicon.go +34 -14


@@ -23,6 +23,7 @@ type FaviconAdapter struct {
23	23		resolver port.FaviconResolver
24	24		faviconDB port.FaviconDatabase
25	25		textureCache map[string]*gdk.Texture
	26	+	aliasesByKey map[favicon.Key]map[string]struct{}
26	27		mu sync.RWMutex
27	28		warnMu sync.Mutex
28	29		warnCounts map[string]int

@@ -74,6 +75,7 @@ func NewFaviconAdapterWithResolver(
74	75		resolver: resolver,
75	76		faviconDB: faviconDB,
76	77		textureCache: make(map[string]*gdk.Texture),
	78	+	aliasesByKey: make(map[favicon.Key]map[string]struct{}),
77	79		warnCounts: make(map[string]int),
78	80		isInternalURL: cfg.IsInternalURL,
79	81		internalDomain: cfg.InternalDomain,

@@ -512,19 +514,15 @@ func (a *FaviconAdapter) Close() {
512	514		}
513	515		}
514	516
515		-	// Invalidate clears one resolved-key texture cache entry and any domain aliases
516		-	// pointing at the same texture.
	517	+	// Invalidate clears one resolved-key texture cache entry and all domain aliases
	518	+	// explicitly bound to that canonical favicon key.
517	519		func (a *FaviconAdapter) Invalidate(_ context.Context, key favicon.Key) error {
518	520		a.mu.Lock()
519		-	texture := a.textureCache[string(key)]
520	521		delete(a.textureCache, string(key))
521		-	if texture != nil {
522		-	for cacheKey, cached := range a.textureCache {
523		-	if cached == texture {
524		-	delete(a.textureCache, cacheKey)
525		-	}
526		-	}
	522	+	for alias := range a.aliasesByKey[key] {
	523	+	delete(a.textureCache, alias)
527	524		}
	525	+	delete(a.aliasesByKey, key)
528	526		a.mu.Unlock()
529	527		return nil
530	528		}

@@ -533,6 +531,7 @@ func (a *FaviconAdapter) Invalidate(_ context.Context, key favicon.Key) error {
533	531		func (a *FaviconAdapter) Clear() {
534	532		a.mu.Lock()
535	533		a.textureCache = make(map[string]*gdk.Texture)
	534	+	a.aliasesByKey = make(map[favicon.Key]map[string]struct{})
536	535		a.mu.Unlock()
537	536		}
538	537

@@ -545,12 +544,33 @@ func (a *FaviconAdapter) Size() int {
545	544		}
546	545
547	546		// setResolvedTexture stores resolver results under both the resolved favicon key
548		-	// and the current page domain. The domain alias keeps UI lookups fast while
549		-	// Invalidate removes aliases sharing the same texture during the migration path.
	547	+	// and the current page domain. Aliases are indexed by canonical key so
	548	+	// invalidation does not depend on pointer equality with the latest texture.
550	549		func (a FaviconAdapter) setResolvedTexture(pageURL string, key favicon.Key, texture gdk.Texture) {
551		-	a.setTexture(string(key), texture)
552		-	if domain := domainurl.ExtractDomain(pageURL); domain != "" {
553		-	a.setTexture(domain, texture)
	550	+	if key == "" \|\| texture == nil {
	551	+	return
	552	+	}
	553	+	domain := domainurl.ExtractDomain(pageURL)
	554	+	a.mu.Lock()
	555	+	defer a.mu.Unlock()
	556	+	a.textureCache[string(key)] = texture
	557	+	if domain == "" {
	558	+	return
	559	+	}
	560	+	a.removeAliasLocked(domain)
	561	+	a.textureCache[domain] = texture
	562	+	if a.aliasesByKey[key] == nil {
	563	+	a.aliasesByKey[key] = make(map[string]struct{})
	564	+	}
	565	+	a.aliasesByKey[key][domain] = struct{}{}
	566	+	}
	567	+
	568	+	func (a *FaviconAdapter) removeAliasLocked(alias string) {
	569	+	for key, aliases := range a.aliasesByKey {
	570	+	delete(aliases, alias)
	571	+	if len(aliases) == 0 {
	572	+	delete(a.aliasesByKey, key)
	573	+	}
554	574		}
555	575		}
556	576

 	resolver     port.FaviconResolver
 	faviconDB    port.FaviconDatabase
 	textureCache map[string]*gdk.Texture
+	aliasesByKey map[favicon.Key]map[string]struct{}
 	mu           sync.RWMutex
 	warnMu       sync.Mutex
 	warnCounts   map[string]int
 		resolver:           resolver,
 		faviconDB:          faviconDB,
 		textureCache:       make(map[string]*gdk.Texture),
+		aliasesByKey:       make(map[favicon.Key]map[string]struct{}),
 		warnCounts:         make(map[string]int),
 		isInternalURL:      cfg.IsInternalURL,
 		internalDomain:     cfg.InternalDomain,
 	}
 }
-// Invalidate clears one resolved-key texture cache entry and any domain aliases
+// Invalidate clears one resolved-key texture cache entry and all domain aliases
-// pointing at the same texture.
+// explicitly bound to that canonical favicon key.
 func (a *FaviconAdapter) Invalidate(_ context.Context, key favicon.Key) error {
 	a.mu.Lock()
-	texture := a.textureCache[string(key)]
 	delete(a.textureCache, string(key))
-	if texture != nil {
+	for alias := range a.aliasesByKey[key] {
-		for cacheKey, cached := range a.textureCache {
+		delete(a.textureCache, alias)
-			if cached == texture {
-				delete(a.textureCache, cacheKey)
-			}
-		}
 	}
+	delete(a.aliasesByKey, key)
 	a.mu.Unlock()
 	return nil
 }
 func (a *FaviconAdapter) Clear() {
 	a.mu.Lock()
 	a.textureCache = make(map[string]*gdk.Texture)
+	a.aliasesByKey = make(map[favicon.Key]map[string]struct{})
 	a.mu.Unlock()
 }
 }
 // setResolvedTexture stores resolver results under both the resolved favicon key
-// and the current page domain. The domain alias keeps UI lookups fast while
+// and the current page domain. Aliases are indexed by canonical key so
-// Invalidate removes aliases sharing the same texture during the migration path.
+// invalidation does not depend on pointer equality with the latest texture.
 func (a *FaviconAdapter) setResolvedTexture(pageURL string, key favicon.Key, texture *gdk.Texture) {
-	a.setTexture(string(key), texture)
+	if key == "" || texture == nil {
-	if domain := domainurl.ExtractDomain(pageURL); domain != "" {
+		return
-		a.setTexture(domain, texture)
+	}
+	domain := domainurl.ExtractDomain(pageURL)
+	a.mu.Lock()
+	defer a.mu.Unlock()
+	a.textureCache[string(key)] = texture
+	if domain == "" {
+		return
+	}
+	a.removeAliasLocked(domain)
+	a.textureCache[domain] = texture
+	if a.aliasesByKey[key] == nil {
+		a.aliasesByKey[key] = make(map[string]struct{})
+	}
+	a.aliasesByKey[key][domain] = struct{}{}
+}
+func (a *FaviconAdapter) removeAliasLocked(alias string) {
+	for key, aliases := range a.aliasesByKey {
+		delete(aliases, alias)
+		if len(aliases) == 0 {
+			delete(a.aliasesByKey, key)
+		}
 	}
 }

M internal/ui/adapter/favicon_test.go +13 -0


@@ -26,6 +26,19 @@ func TestFaviconAdapterInvalidateRemovesResolvedTextureAliases(t *testing.T) {
26	26		}
27	27		}
28	28
	29	+	func TestFaviconAdapterInvalidateRemovesOlderAliasesAfterKeyRebind(t *testing.T) {
	30	+	adapter := NewFaviconAdapter(nil, nil, FaviconAdapterConfig{})
	31	+	adapter.setResolvedTexture("https://docs.example.com/page", favicon.Key("example.com"), &gdk.Texture{})
	32	+	adapter.setResolvedTexture("https://blog.example.com/page", favicon.Key("example.com"), &gdk.Texture{})
	33	+
	34	+	if err := adapter.Invalidate(context.Background(), "example.com"); err != nil {
	35	+	t.Fatal(err)
	36	+	}
	37	+	if adapter.GetTexture("example.com") != nil \|\| adapter.GetTexture("docs.example.com") != nil \|\| adapter.GetTexture("blog.example.com") != nil {
	38	+	t.Fatal("expected invalidation to remove canonical key and all aliases")
	39	+	}
	40	+	}
	41	+
29	42		func TestFaviconWarningDedup_FirstAndRepeated(t *testing.T) {
30	43		adapter := NewFaviconAdapter(nil, nil, FaviconAdapterConfig{})
31	44


@@ -26,6 +26,19 @@ func TestFaviconAdapterInvalidateRemovesResolvedTextureAliases(t *testing.T) {
26	}	26		}
27	}	27		}
28		28
		29	+	func TestFaviconAdapterInvalidateRemovesOlderAliasesAfterKeyRebind(t *testing.T) {
		30	+	adapter := NewFaviconAdapter(nil, nil, FaviconAdapterConfig{})
		31	+	adapter.setResolvedTexture("https://docs.example.com/page", favicon.Key("example.com"), &gdk.Texture{})
		32	+	adapter.setResolvedTexture("https://blog.example.com/page", favicon.Key("example.com"), &gdk.Texture{})
		33	+
		34	+	if err := adapter.Invalidate(context.Background(), "example.com"); err != nil {
		35	+	t.Fatal(err)
		36	+	}
		37	+	if adapter.GetTexture("example.com") != nil \|\| adapter.GetTexture("docs.example.com") != nil \|\| adapter.GetTexture("blog.example.com") != nil {
		38	+	t.Fatal("expected invalidation to remove canonical key and all aliases")
		39	+	}
		40	+	}
		41	+
29	func TestFaviconWarningDedup_FirstAndRepeated(t *testing.T) {	42		func TestFaviconWarningDedup_FirstAndRepeated(t *testing.T) {
30	adapter := NewFaviconAdapter(nil, nil, FaviconAdapterConfig{})	43		adapter := NewFaviconAdapter(nil, nil, FaviconAdapterConfig{})
31		44