diff --git a/src/renderer/src/components/browser-favicon.test.tsx b/src/renderer/src/components/browser-favicon.test.tsx new file mode 100644 index 00000000000..362cf15b46f --- /dev/null +++ b/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' + ) +}) diff --git a/src/renderer/src/components/browser-favicon.tsx b/src/renderer/src/components/browser-favicon.tsx index ecde4065f03..92b14a6ad64 100644 --- a/src/renderer/src/components/browser-favicon.tsx +++ b/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(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. diff --git a/src/renderer/src/components/browser-pane/describe-page/browser-favicon-url.test.ts b/src/renderer/src/components/browser-pane/describe-page/browser-favicon-url.test.ts new file mode 100644 index 00000000000..4c295309174 --- /dev/null +++ b/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) + }) +}) diff --git a/src/renderer/src/components/browser-pane/describe-page/browser-favicon-url.ts b/src/renderer/src/components/browser-pane/describe-page/browser-favicon-url.ts new file mode 100644 index 00000000000..8e1c442068d --- /dev/null +++ b/src/renderer/src/components/browser-pane/describe-page/browser-favicon-url.ts @@ -0,0 +1,63 @@ +// Why this lives apart from the : 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 +} diff --git a/src/renderer/src/components/browser-pane/host-guest/bind-browser-page-webview-listeners.ts b/src/renderer/src/components/browser-pane/host-guest/bind-browser-page-webview-listeners.ts index e80409fa821..f8699324889 100644 --- a/src/renderer/src/components/browser-pane/host-guest/bind-browser-page-webview-listeners.ts +++ b/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) diff --git a/src/renderer/src/components/browser-pane/host-guest/browser-page-favicon-retention.test.ts b/src/renderer/src/components/browser-pane/host-guest/browser-page-favicon-retention.test.ts new file mode 100644 index 00000000000..924e7834319 --- /dev/null +++ b/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 = (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(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(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() + }) +}) diff --git a/src/renderer/src/components/browser-pane/host-guest/browser-page-webview-loading-handlers.ts b/src/renderer/src/components/browser-pane/host-guest/browser-page-webview-loading-handlers.ts index f263354e8c5..b6887f119eb 100644 --- a/src/renderer/src/components/browser-pane/host-guest/browser-page-webview-loading-handlers.ts +++ b/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 }) } diff --git a/src/renderer/src/components/browser-pane/host-guest/browser-page-webview-navigation-handlers.ts b/src/renderer/src/components/browser-pane/host-guest/browser-page-webview-navigation-handlers.ts index dbb47ae4acb..240e763cf63 100644 --- a/src/renderer/src/components/browser-pane/host-guest/browser-page-webview-navigation-handlers.ts +++ b/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, diff --git a/src/renderer/src/components/tab-bar/BrowserTab.tsx b/src/renderer/src/components/tab-bar/BrowserTab.tsx index b72f27287f9..507b738318a 100644 --- a/src/renderer/src/components/tab-bar/BrowserTab.tsx +++ b/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. */}