Fix favicon retention across same-origin navigations (#18879)

* fix: retain favicons across same-origin navigations

Move favicon clearing from did-start-loading to did-start-navigation and
only clear when origin changes. Chromium re-announces favicons only when
the icon URL list changes, so clearing on every load orphans same-origin
navigations. Extract favicon URL validation into a shared module.

* fix: drop favicon on cross-origin redirects

When a same-origin navigation redirects to a different origin, the favicon should be cleared to prevent stale icons from displaying the wrong site's identity.

src/renderer/src/components/browser-favicon.test.tsx

@@ -0,0 +1,57 @@
+// @vitest-environment happy-dom
+import { createElement } from 'react'
+import { cleanup, fireEvent, render } from '@testing-library/react'
+import { afterEach, expect, it } from 'vitest'
+import { BrowserFavicon } from './browser-favicon'
+
+afterEach(cleanup)
+
+const faviconUrl = 'https://example.test/favicon.ico'
+const icon = (loading = false, url: string | null = faviconUrl) =>
+  createElement(BrowserFavicon, { faviconUrl: url, loading })
+
+it('retries a failed icon after a same-origin reload completes', () => {
+  const view = render(icon())
+  fireEvent.error(view.container.querySelector('img')!)
+  expect(view.container.querySelector('img')).toBeNull()
+  view.rerender(icon(true))
+  expect(view.container.querySelector('img')).toBeNull()
+  view.rerender(icon(false))
+  expect(view.container.querySelector('img')?.getAttribute('src')).toBe(faviconUrl)
+
+  fireEvent.error(view.container.querySelector('img')!)
+  view.rerender(icon(false))
+  expect(view.container.querySelector('img')).toBeNull()
+  view.rerender(icon(true))
+  view.rerender(icon(false))
+  expect(view.container.querySelector('img')).not.toBeNull()
+})
+
+it('keeps a working image mounted throughout a reload', () => {
+  const view = render(icon())
+  const image = view.container.querySelector('img')
+  view.rerender(icon(true))
+  expect(view.container.querySelector('img')).toBe(image)
+  view.rerender(icon(false))
+  expect(view.container.querySelector('img')).toBe(image)
+})
+
+it('retries an image that failed during initial loading when loading finishes', () => {
+  const view = render(icon(true))
+  fireEvent.error(view.container.querySelector('img')!)
+  view.rerender(icon(false))
+  expect(view.container.querySelector('img')).not.toBeNull()
+})
+
+it('still resets failures when the favicon URL changes or clears', () => {
+  const view = render(icon())
+  fireEvent.error(view.container.querySelector('img')!)
+  view.rerender(icon(false, null))
+  view.rerender(icon())
+  expect(view.container.querySelector('img')).not.toBeNull()
+  fireEvent.error(view.container.querySelector('img')!)
+  view.rerender(icon(false, 'https://other.test/favicon.ico'))
+  expect(view.container.querySelector('img')?.getAttribute('src')).toBe(
+    'https://other.test/favicon.ico'
+  )
+})

src/renderer/src/components/browser-favicon.tsx

@@ -1,34 +1,30 @@
 import { useState } from 'react'
 import { Globe } from 'lucide-react'
 import { cn } from '@/lib/utils'
-
-function displayableFaviconUrl(faviconUrl: string | null | undefined): string | null {
-  const trimmed = faviconUrl?.trim()
-  if (!trimmed) {
-    return null
-  }
-  if (trimmed.startsWith('data:image/')) {
-    return trimmed
-  }
-  try {
-    const url = new URL(trimmed)
-    return url.protocol === 'http:' || url.protocol === 'https:' ? trimmed : null
-  } catch {
-    return null
-  }
-}
+import { displayableFaviconUrl } from './browser-pane/describe-page/browser-favicon-url'
 
 export function BrowserFavicon({
   faviconUrl,
+  loading = false,
   className,
   fallbackClassName
 }: {
   faviconUrl: string | null | undefined
+  loading?: boolean
   className?: string
   fallbackClassName?: string
 }): React.JSX.Element {
   const displayUrl = displayableFaviconUrl(faviconUrl)
   const [failedUrl, setFailedUrl] = useState<string | null>(null)
+  const [previousLoading, setPreviousLoading] = useState(loading)
+
+  // Retry after navigation settles, when cookies and connectivity may have recovered.
+  if (previousLoading !== loading) {
+    setPreviousLoading(loading)
+    if (!loading) {
+      setFailedUrl(null)
+    }
+  }
 
   // Why: reset during render on any favicon identity change — including a clear to null while
   // a page loads — so navigating back to the same url retries instead of keeping the fallback.

src/renderer/src/components/browser-pane/describe-page/browser-favicon-url.test.ts

@@ -0,0 +1,90 @@
+import { describe, expect, it } from 'vitest'
+import {
+  browserNavigationLeavesFaviconOrigin,
+  displayableFaviconUrl,
+  pickDisplayableFaviconUrl
+} from './browser-favicon-url'
+
+describe('displayableFaviconUrl', () => {
+  it('accepts http, https and image data urls', () => {
+    expect(displayableFaviconUrl('https://github.com/favicon.ico')).toBe(
+      'https://github.com/favicon.ico'
+    )
+    expect(displayableFaviconUrl('http://127.0.0.1:8765/favicon.ico')).toBe(
+      'http://127.0.0.1:8765/favicon.ico'
+    )
+    expect(displayableFaviconUrl('  data:image/png;base64,AAAA  ')).toBe(
+      'data:image/png;base64,AAAA'
+    )
+  })
+
+  it('rejects the empty-icon sentinel and non-web schemes', () => {
+    expect(displayableFaviconUrl('data:,')).toBeNull()
+    expect(displayableFaviconUrl('chrome-extension://abc/icon.png')).toBeNull()
+    expect(displayableFaviconUrl('file:///tmp/icon.png')).toBeNull()
+    expect(displayableFaviconUrl('not a url')).toBeNull()
+    expect(displayableFaviconUrl(null)).toBeNull()
+    expect(displayableFaviconUrl('   ')).toBeNull()
+  })
+})
+
+describe('pickDisplayableFaviconUrl', () => {
+  it('skips leading entries that cannot render', () => {
+    expect(pickDisplayableFaviconUrl(['data:,', 'https://example.com/icon.png'])).toBe(
+      'https://example.com/icon.png'
+    )
+  })
+
+  it('keeps the declaration order among usable entries', () => {
+    expect(
+      pickDisplayableFaviconUrl([
+        'https://github.githubassets.com/favicons/favicon.png',
+        'https://github.githubassets.com/favicons/favicon.svg'
+      ])
+    ).toBe('https://github.githubassets.com/favicons/favicon.png')
+  })
+
+  it('reports nothing for an absent or unusable list', () => {
+    expect(pickDisplayableFaviconUrl(undefined)).toBeNull()
+    expect(pickDisplayableFaviconUrl([])).toBeNull()
+    expect(pickDisplayableFaviconUrl(['data:,'])).toBeNull()
+  })
+})
+
+describe('browserNavigationLeavesFaviconOrigin', () => {
+  it('keeps the icon across a same-origin navigation', () => {
+    expect(
+      browserNavigationLeavesFaviconOrigin(
+        'https://github.com/alibaba/jvm-sandbox',
+        'https://github.com/btraceio/btrace'
+      )
+    ).toBe(false)
+  })
+
+  it('drops the icon when the origin changes', () => {
+    expect(
+      browserNavigationLeavesFaviconOrigin('https://github.com/nodejs/node', 'https://x.com/home')
+    ).toBe(true)
+  })
+
+  it('treats scheme and port as part of the origin', () => {
+    expect(
+      browserNavigationLeavesFaviconOrigin('http://localhost:3000/', 'http://localhost:4000/')
+    ).toBe(true)
+    expect(
+      browserNavigationLeavesFaviconOrigin('http://example.com/', 'https://example.com/')
+    ).toBe(true)
+  })
+
+  it('drops the icon when the destination cannot carry one', () => {
+    expect(browserNavigationLeavesFaviconOrigin('https://github.com/', 'about:blank')).toBe(true)
+    expect(
+      browserNavigationLeavesFaviconOrigin('https://github.com/', 'file:///tmp/report.html')
+    ).toBe(true)
+  })
+
+  it('keeps the icon when the document being left is unknown', () => {
+    expect(browserNavigationLeavesFaviconOrigin(null, 'https://github.com/nodejs/node')).toBe(false)
+    expect(browserNavigationLeavesFaviconOrigin('about:blank', 'https://github.com/')).toBe(false)
+  })
+})

src/renderer/src/components/browser-pane/describe-page/browser-favicon-url.ts

@@ -0,0 +1,63 @@
+// Why this lives apart from the <img>: Chromium only emits `page-favicon-updated` when a document's
+// icon URL list *changes*, so both the chrome that renders an icon and the guest listeners that
+// decide when to drop one have to agree on what counts as a usable icon and as a new site.
+
+export function displayableFaviconUrl(faviconUrl: string | null | undefined): string | null {
+  const trimmed = faviconUrl?.trim()
+  if (!trimmed) {
+    return null
+  }
+  // Why not a plain `data:` check: Chromium reports `data:,` for a page that declares no icon.
+  if (trimmed.startsWith('data:image/')) {
+    return trimmed
+  }
+  try {
+    const url = new URL(trimmed)
+    return url.protocol === 'http:' || url.protocol === 'https:' ? trimmed : null
+  } catch {
+    return null
+  }
+}
+
+export function pickDisplayableFaviconUrl(favicons: readonly string[] | undefined): string | null {
+  // Why not favicons[0]: the first entry can be a `data:,` sentinel or a non-web scheme while a
+  // later entry is a real icon.
+  for (const candidate of favicons ?? []) {
+    const displayable = displayableFaviconUrl(candidate)
+    if (displayable) {
+      return displayable
+    }
+  }
+  return null
+}
+
+function faviconOrigin(rawUrl: string | null | undefined): string | null {
+  if (!rawUrl) {
+    return null
+  }
+  try {
+    const url = new URL(rawUrl)
+    return url.protocol === 'http:' || url.protocol === 'https:' ? url.origin : null
+  } catch {
+    return null
+  }
+}
+
+// Why the two sides are treated asymmetrically: a destination with no icon of its own (about:blank,
+// file://, a doc preview) must drop the previous site's icon, but an unknown *origin* — a freshly
+// attached guest that hasn't committed a document yet — is not evidence the icon is stale, and
+// clearing there would strand a restored tab on the globe until its first paint.
+export function browserNavigationLeavesFaviconOrigin(
+  fromUrl: string | null | undefined,
+  toUrl: string | null | undefined
+): boolean {
+  const to = faviconOrigin(toUrl)
+  if (to === null) {
+    return true
+  }
+  const from = faviconOrigin(fromUrl)
+  if (from === null) {
+    return false
+  }
+  return from !== to
+}

src/renderer/src/components/browser-pane/host-guest/bind-browser-page-webview-listeners.ts

@@ -116,6 +116,7 @@ export function bindBrowserPageWebviewListeners({
 
   const {
     handleDidStartNavigation,
+    handleDidRedirectNavigation,
     handleFullDidNavigate,
     handleDidNavigateInPage,
     handleTitleUpdate,
@@ -149,6 +150,7 @@ export function bindBrowserPageWebviewListeners({
   webview.addEventListener('focus', dismissAddressBarSuggestions)
   webview.addEventListener('did-start-loading', handleDidStartLoading)
   webview.addEventListener('did-start-navigation', handleDidStartNavigation)
+  webview.addEventListener('did-redirect-navigation', handleDidRedirectNavigation)
   webview.addEventListener('did-stop-loading', handleDidStopLoading)
   // Why: close find only on full 'did-navigate', not the shared handler, which also fires on SPA in-page hash/pushState changes.
   const handleFindCloseOnNavigate = (): void => {
@@ -186,6 +188,7 @@ export function bindBrowserPageWebviewListeners({
     webview.removeEventListener('focus', dismissAddressBarSuggestions)
     webview.removeEventListener('did-start-loading', handleDidStartLoading)
     webview.removeEventListener('did-start-navigation', handleDidStartNavigation)
+    webview.removeEventListener('did-redirect-navigation', handleDidRedirectNavigation)
     webview.removeEventListener('did-stop-loading', handleDidStopLoading)
     webview.removeEventListener('did-navigate', handleFullDidNavigate)
     webview.removeEventListener('did-navigate', handleFindCloseOnNavigate)

src/renderer/src/components/browser-pane/host-guest/browser-page-favicon-retention.test.ts

@@ -0,0 +1,149 @@
+import { describe, expect, it, vi } from 'vitest'
+import { createBrowserPageWebviewNavigationHandlers } from './browser-page-webview-navigation-handlers'
+import { createBrowserPageWebviewLoadingHandlers } from './browser-page-webview-loading-handlers'
+import type { BrowserTabPageState } from '../describe-page/browser-page-types'
+
+const TAB_ID = 'tab-1'
+const GITHUB_ICON = 'https://github.githubassets.com/favicons/favicon.png'
+
+function createHarness(startUrl: string) {
+  const updates: BrowserTabPageState[] = []
+  const committedUrl = { current: startUrl }
+  const webview = {
+    getURL: () => committedUrl.current,
+    getTitle: () => 'title',
+    canGoBack: () => false,
+    canGoForward: () => false,
+    src: startUrl
+  } as unknown as Electron.WebviewTag
+  const faviconUrlRef = { current: null as string | null }
+  const onUpdatePageStateRef = {
+    current: (_tabId: string, next: BrowserTabPageState) => {
+      updates.push(next)
+    }
+  }
+  const ref = <T>(value: T) => ({ current: value })
+  const navigation = createBrowserPageWebviewNavigationHandlers({
+    webview,
+    browserTabId: TAB_ID,
+    browserTabUrl: startUrl,
+    recoveryNavigationValidationRef: ref(null),
+    activeLoadFailureRef: ref(null),
+    // Why the destination, not the current document: Orca-driven navigations set this ref before
+    // assigning src, which is exactly the case the origin check must not read it for.
+    lastKnownWebviewUrlRef: ref<string | null>(startUrl),
+    addressBarInputRef: ref(null),
+    onSetUrlRef: ref(vi.fn()),
+    onUpdatePageStateRef,
+    addBrowserHistoryEntryRef: ref(vi.fn()),
+    faviconUrlRef,
+    setAddressBarValue: vi.fn(),
+    annotationViewportBridgeTokenRef: ref('token'),
+    setBrowserOverlayViewport: vi.fn()
+  })
+  const loading = createBrowserPageWebviewLoadingHandlers({
+    webview,
+    browserTabId: TAB_ID,
+    faviconUrlRef,
+    browserTabUrlRef: ref(startUrl),
+    addressBarValueRef: ref(startUrl),
+    addressBarInputRef: ref(null),
+    activeLoadFailureRef: ref(null),
+    lastKnownWebviewUrlRef: ref<string | null>(startUrl),
+    trackNextLoadingEventRef: ref(true),
+    keepAddressBarFocusRef: ref(false),
+    recoveryNavigationValidationRef: ref(null),
+    clearBrowserPageAnnotationsRef: ref(vi.fn()),
+    onUpdatePageStateRef,
+    onSetUrlRef: ref(vi.fn()),
+    setPendingAnnotationPayload: vi.fn(),
+    setBrowserOverlayViewport: vi.fn(),
+    setAddressBarValue: vi.fn(),
+    focusAddressBarNow: () => false
+  })
+
+  const navigateTo = (url: string): void => {
+    loading.handleDidStartLoading()
+    navigation.handleDidStartNavigation({
+      isMainFrame: true,
+      isInPlace: false,
+      url
+    } as Electron.DidStartNavigationEvent)
+    committedUrl.current = url
+  }
+
+  return { faviconUrlRef, updates, navigation, navigateTo, committedUrl }
+}
+
+describe('favicon retention across navigations', () => {
+  it('keeps the icon when Chromium will not re-announce it for a same-origin load', () => {
+    const harness = createHarness('https://github.com/alibaba/jvm-sandbox')
+    harness.navigation.handleFaviconUpdate({ favicons: [GITHUB_ICON] })
+    expect(harness.faviconUrlRef.current).toBe(GITHUB_ICON)
+
+    // Chromium emits no page-favicon-updated here: the icon URL list is unchanged.
+    harness.navigateTo('https://github.com/btraceio/btrace')
+
+    expect(harness.faviconUrlRef.current).toBe(GITHUB_ICON)
+    expect(harness.updates.some((update) => update.faviconUrl === null)).toBe(false)
+  })
+
+  it('drops the icon when the navigation leaves the origin', () => {
+    const harness = createHarness('https://github.com/nodejs/node')
+    harness.navigation.handleFaviconUpdate({ favicons: [GITHUB_ICON] })
+
+    harness.navigateTo('https://x.com/home')
+
+    expect(harness.faviconUrlRef.current).toBeNull()
+    expect(harness.updates.at(-1)).toEqual({ faviconUrl: null })
+  })
+
+  it('drops the icon when a same-origin navigation redirects to another origin', () => {
+    const harness = createHarness('https://github.com/nodejs/node')
+    harness.navigation.handleFaviconUpdate({ favicons: [GITHUB_ICON] })
+    harness.navigateTo('https://github.com/login')
+
+    harness.navigation.handleDidRedirectNavigation({
+      isMainFrame: true,
+      isInPlace: false,
+      url: 'https://example.com/after-login'
+    } as Electron.DidRedirectNavigationEvent)
+
+    expect(harness.faviconUrlRef.current).toBeNull()
+    expect(harness.updates.at(-1)).toEqual({ faviconUrl: null })
+  })
+
+  it('does not clear on a same-document navigation', () => {
+    const harness = createHarness('https://github.com/nodejs/node')
+    harness.navigation.handleFaviconUpdate({ favicons: [GITHUB_ICON] })
+
+    harness.navigation.handleDidStartNavigation({
+      isMainFrame: true,
+      isInPlace: true,
+      url: 'https://example.com/'
+    } as Electron.DidStartNavigationEvent)
+
+    expect(harness.faviconUrlRef.current).toBe(GITHUB_ICON)
+  })
+
+  it('reports loading without touching the icon on did-start-loading', () => {
+    const harness = createHarness('https://github.com/nodejs/node')
+    harness.navigation.handleFaviconUpdate({ favicons: [GITHUB_ICON] })
+    harness.updates.length = 0
+
+    harness.navigateTo('https://github.com/nodejs/undici')
+
+    expect(harness.updates).toEqual([{ loading: true }])
+  })
+
+  it('takes the first renderable icon rather than the first declared one', () => {
+    const harness = createHarness('https://example.com/')
+    harness.navigation.handleFaviconUpdate({
+      favicons: ['data:,', 'https://example.com/icon.png']
+    })
+    expect(harness.faviconUrlRef.current).toBe('https://example.com/icon.png')
+
+    harness.navigation.handleFaviconUpdate({ favicons: ['data:,'] })
+    expect(harness.faviconUrlRef.current).toBeNull()
+  })
+})

src/renderer/src/components/browser-pane/host-guest/browser-page-webview-loading-handlers.ts

@@ -78,10 +78,10 @@ export function createBrowserPageWebviewLoadingHandlers({
     if (!trackNextLoadingEventRef.current) {
       return
     }
-    faviconUrlRef.current = null
+    // Why the favicon isn't cleared here: it is dropped on the cross-origin did-start-navigation
+    // instead, because Chromium won't re-announce an unchanged icon for a same-origin load.
     onUpdatePageStateRef.current(browserTabId, {
-      loading: true,
-      faviconUrl: null
+      loading: true
     })
   }
 

src/renderer/src/components/browser-pane/host-guest/browser-page-webview-navigation-handlers.ts

@@ -13,6 +13,10 @@ import {
   isChromiumErrorPage,
   toDisplayUrl
 } from '../describe-page/browser-page-url-display'
+import {
+  browserNavigationLeavesFaviconOrigin,
+  pickDisplayableFaviconUrl
+} from '../describe-page/browser-favicon-url'
 import type {
   BrowserPageNavigateEvent,
   BrowserPageRecoveryNavigationValidation,
@@ -41,6 +45,7 @@ export type BrowserPageWebviewNavigationHandlersArgs = {
 
 export type BrowserPageWebviewNavigationHandlers = {
   handleDidStartNavigation: (event: Electron.DidStartNavigationEvent) => void
+  handleDidRedirectNavigation: (event: Electron.DidRedirectNavigationEvent) => void
   handleFullDidNavigate: (event: BrowserPageNavigateEvent) => void
   handleDidNavigateInPage: (event: BrowserPageNavigateEvent) => void
   handleTitleUpdate: (event: { title?: string }) => void
@@ -64,6 +69,28 @@ export function createBrowserPageWebviewNavigationHandlers({
   annotationViewportBridgeTokenRef,
   setBrowserOverlayViewport
 }: BrowserPageWebviewNavigationHandlersArgs): BrowserPageWebviewNavigationHandlers {
+  const clearFaviconIfOriginChanges = (
+    event: Electron.DidStartNavigationEvent | Electron.DidRedirectNavigationEvent
+  ): void => {
+    if (!event.isMainFrame || event.isInPlace || !event.url) {
+      return
+    }
+    const browserStartedUrl = redactKagiSessionToken(event.url)
+    const startedUrl = normalizeBrowserNavigationUrl(browserStartedUrl) ?? browserStartedUrl
+    // Why getURL() and not lastKnownWebviewUrlRef: Orca-driven navigations point that ref at the
+    // destination before assigning src, so it can't identify the document being left.
+    let committedUrl: string | null = null
+    try {
+      committedUrl = webview.getURL() || null
+    } catch {
+      // Why: a guest that hasn't attached yet rejects getURL(); an unknown origin keeps the icon.
+    }
+    if (browserNavigationLeavesFaviconOrigin(committedUrl, startedUrl)) {
+      faviconUrlRef.current = null
+      onUpdatePageStateRef.current(browserTabId, { faviconUrl: null })
+    }
+  }
+
   const handleDidStartNavigation = (event: Electron.DidStartNavigationEvent): void => {
     if (!event.isMainFrame || event.isInPlace || !event.url) {
       return
@@ -74,6 +101,14 @@ export function createBrowserPageWebviewNavigationHandlers({
     if (pendingRecoveryNavigation?.targetUrl === startedUrl) {
       pendingRecoveryNavigation.started = true
     }
+    // Why here and not on did-start-loading: Chromium re-announces a favicon only when the icon URL
+    // list changes, so clearing on every load strands same-origin navigations with no icon and no
+    // event that would ever restore one.
+    clearFaviconIfOriginChanges(event)
+  }
+
+  const handleDidRedirectNavigation = (event: Electron.DidRedirectNavigationEvent): void => {
+    clearFaviconIfOriginChanges(event)
   }
 
   const handleDidNavigate = (
@@ -136,14 +171,7 @@ export function createBrowserPageWebviewNavigationHandlers({
   }
 
   const handleFaviconUpdate = (event: { favicons?: string[] }): void => {
-    const faviconUrl = event.favicons?.[0] ?? null
-    faviconUrlRef.current =
-      faviconUrl &&
-      (faviconUrl.startsWith('https://') ||
-        faviconUrl.startsWith('http://') ||
-        faviconUrl.startsWith('data:image/'))
-        ? faviconUrl
-        : null
+    faviconUrlRef.current = pickDisplayableFaviconUrl(event.favicons)
     onUpdatePageStateRef.current(browserTabId, { faviconUrl: faviconUrlRef.current })
   }
 
@@ -175,6 +203,7 @@ export function createBrowserPageWebviewNavigationHandlers({
 
   return {
     handleDidStartNavigation,
+    handleDidRedirectNavigation,
     handleFullDidNavigate,
     handleDidNavigateInPage,
     handleTitleUpdate,

src/renderer/src/components/tab-bar/BrowserTab.tsx

@@ -191,6 +191,7 @@ export default function BrowserTab({
           muted-foreground made the icon read as "disabled" in practice. */}
       <BrowserFavicon
         faviconUrl={tab.faviconUrl}
+        loading={tab.loading}
         className="size-3 mr-1"
         fallbackClassName="text-blue-500"
       />