diff --git a/apps/desktop/main/datastore.test.ts b/apps/desktop/main/datastore.test.ts index 6f14d7b9..d098b2d9 100644 --- a/apps/desktop/main/datastore.test.ts +++ b/apps/desktop/main/datastore.test.ts @@ -338,6 +338,46 @@ describe('Desktop Datastore Tests', () => { }); }); + describe('getFaviconForUrl (loading-card icon lookup)', () => { + it('returns the stored favicon for an exact URL', () => { + datastore.addItem('url', { content: 'https://fav-exact.example/page' }); + datastore.updateItemFavicon('https://fav-exact.example/page', 'https://fav-exact.example/icon.png'); + assert.strictEqual( + datastore.getFaviconForUrl('https://fav-exact.example/page'), + 'https://fav-exact.example/icon.png', + ); + }); + + it('finds the favicon via normalized match (trailing slash)', () => { + datastore.addItem('url', { content: 'https://fav-norm.example/page' }); + datastore.updateItemFavicon('https://fav-norm.example/page', 'https://fav-norm.example/icon.png'); + // Active URL has a trailing slash; stored content does not. + assert.strictEqual( + datastore.getFaviconForUrl('https://fav-norm.example/page/'), + 'https://fav-norm.example/icon.png', + ); + }); + + it('falls back to a same-domain favicon when the exact URL has none', () => { + datastore.addItem('url', { content: 'https://fav-domain.example/home' }); + datastore.updateItemFavicon('https://fav-domain.example/home', 'https://fav-domain.example/icon.png'); + // A different path on the same domain, never stored itself. + assert.strictEqual( + datastore.getFaviconForUrl('https://fav-domain.example/some/other/path'), + 'https://fav-domain.example/icon.png', + ); + }); + + it("returns '' (not null) when no favicon is known", () => { + assert.strictEqual(datastore.getFaviconForUrl('https://no-fav-anywhere.example/'), ''); + }); + + it("returns '' for an item that exists but has an empty favicon", () => { + datastore.addItem('url', { content: 'https://fav-empty.example/page' }); + assert.strictEqual(datastore.getFaviconForUrl('https://fav-empty.example/page'), ''); + }); + }); + describe('Item deletion', () => { it('should soft delete an item', () => { const { id } = datastore.addItem('url', { content: 'https://delete-me.com' }); diff --git a/apps/desktop/main/datastore.ts b/apps/desktop/main/datastore.ts index 5906f43b..d9bf34c8 100644 --- a/apps/desktop/main/datastore.ts +++ b/apps/desktop/main/datastore.ts @@ -3669,6 +3669,43 @@ export function updateItemFavicon(url: string, faviconUrl: string): boolean { return false; } +/** + * Read the stored favicon for a URL item, or '' if none is known. The READ + * counterpart of `updateItemFavicon` — mirrors its lookup order (exact + * normalized match → raw match → most-recent same-domain item) so a favicon + * written under any of those keys is found again. Used by the hybrid page-host + * loading card to paint the last-known favicon the instant a load starts + * (before the live `page-favicon-updated` arrives). Returns '' — not null — so + * callers can pass it straight into a payload field. + */ +export function getFaviconForUrl(url: string): string { + const normalizedUri = normalizeUrl(url); + const d = getDb(); + + let existing = d.prepare( + 'SELECT favicon FROM items WHERE type = ? AND content = ? AND deletedAt = 0' + ).get('url', normalizedUri) as { favicon: string | null } | undefined; + + if ((!existing || !existing.favicon) && url !== normalizedUri) { + existing = d.prepare( + 'SELECT favicon FROM items WHERE type = ? AND content = ? AND deletedAt = 0' + ).get('url', url) as { favicon: string | null } | undefined; + } + + if (!existing || !existing.favicon) { + try { + const domain = new URL(url).hostname; + existing = d.prepare( + `SELECT favicon FROM items WHERE type = ? AND domain = ? AND deletedAt = 0 + AND favicon != '' AND favicon IS NOT NULL + ORDER BY updatedAt DESC LIMIT 1` + ).get('url', domain) as { favicon: string | null } | undefined; + } catch { /* invalid URL, skip domain phase */ } + } + + return existing?.favicon || ''; +} + /** * Soft delete an item (sets deletedAt timestamp) */ diff --git a/apps/desktop/main/hybrid-overlay.ts b/apps/desktop/main/hybrid-overlay.ts index 73f3c60f..eb5bcca2 100644 --- a/apps/desktop/main/hybrid-overlay.ts +++ b/apps/desktop/main/hybrid-overlay.ts @@ -69,6 +69,9 @@ import { // Stage B: per-host load-error state + timeout (lives in registry to avoid circular dep) registerLoadErrorListener, getHybridLoadError, + hasHybridPainted, + hasHybridOutstandingFailure, + getHybridRetryAttempts, getHybridLoadingTimeoutMs, setHybridLoadingTimeoutMs as _setHybridLoadingTimeoutMs, // Hybrid teardown → deny pending web-permission prompts (see initHybridOverlay) @@ -90,6 +93,7 @@ import { publish, subscribe, unsubscribe, getSystemAddress } from './pubsub.js'; // became a NEW hybrid host. ipc.ts/main.ts do NOT import hybrid-overlay, so // this is a one-way (acyclic) dependency. import { wireHybridContentEvents } from './ipc.js'; +import { getFaviconForUrl } from './datastore.js'; import { getWindowInfo, registerWindow } from './main.js'; import { getWindowPresentation } from './window-presenter.js'; import { hybridShowIntent, showHybridHost, closeFocusedWindow, closeOrHideWindow, handleHybridEscape } from './windows.js'; @@ -414,10 +418,24 @@ function sendActiveHandoff(id: number | null): void { } catch { /* baseWin gone */ } + // Last-known favicon for this URL (from the datastore), so the loading card + // can paint the site's icon the instant it's active — before the live + // `page-favicon-updated` arrives. '' when unknown (spinner-only card). + let favicon = ''; + try { + const u = entry.url(); + if (u) favicon = getFaviconForUrl(u); + } catch { + /* datastore unavailable / bad url — spinner-only */ + } publish(getSystemAddress(), 'page-overlay:active', { windowId: id, url: entry.url(), title: entry.title(), + favicon, + // The overlay shows the favicon+spinner loading card only for a window that + // has never painted (fresh/restore/deferred) — see registry perHostPainted. + firstLoad: !hasHybridPainted(id), canGoBack, canGoForward, // Stage B: include per-host load-error so the overlay paints the error card @@ -1235,6 +1253,18 @@ function installTestBridge(): void { return null; } }, + /** Test: has this window painted a frame yet (firstLoad gate for the loading card). */ + hybridHasPainted(id: number): boolean { + return hasHybridPainted(id); + }, + /** Test: current same-URL Retry attempt count for the window (retry-cap spec). */ + hybridRetryAttempts(id: number): number { + return getHybridRetryAttempts(id); + }, + /** Test: does the window currently have an outstanding load failure (retry-cap spec). */ + hybridHasOutstandingFailure(id: number): boolean { + return hasHybridOutstandingFailure(id); + }, /** * P1.4b-3: the work area of the display the given host currently sits on. * Maximize snaps the host to exactly this rect, so the maximize test can diff --git a/apps/desktop/main/hybrid-page-host-registry.test.ts b/apps/desktop/main/hybrid-page-host-registry.test.ts index 1b692b25..7cb403c0 100644 --- a/apps/desktop/main/hybrid-page-host-registry.test.ts +++ b/apps/desktop/main/hybrid-page-host-registry.test.ts @@ -34,6 +34,15 @@ import { setOverlayFocusProvider, setOverlayWindowIdProvider, isHybridOverlayWindowId, + markHybridPainted, + hasHybridPainted, + HYBRID_SAME_URL_RETRY_CAP, + noteHybridLoadFailure, + hasHybridOutstandingFailure, + noteHybridRetryAttempt, + getHybridRetryAttempts, + clearHybridRetry, + unregisterHybridWindow, } from './hybrid-page-host-registry.js'; type Handler = (...args: unknown[]) => void; @@ -516,3 +525,96 @@ describe('hybrid-page-host-registry: overlay window id (cmd+W overlay redirect)' assert.strictEqual(isHybridOverlayWindowId(7), false); }); }); + +// ── Loading-card first-paint tracking ──────────────────────────────────────── +describe('hybrid-page-host-registry: first-paint (loading card gate)', () => { + beforeEach(() => { + _resetForTests(); + nextId = 1; + }); + + it('a fresh window has not painted (firstLoad = true)', () => { + assert.strictEqual(hasHybridPainted(1), false); + }); + + it('markHybridPainted flips it — a subsequent load is no longer firstLoad', () => { + markHybridPainted(1); + assert.strictEqual(hasHybridPainted(1), true); + assert.strictEqual(hasHybridPainted(2), false); // per-window + }); + + it('teardown clears the painted flag (a reused id starts unpainted)', () => { + const f = mk({ url: 'https://a.test/' }); + markHybridPainted(f.id); + assert.strictEqual(hasHybridPainted(f.id), true); + unregisterHybridWindow(f.id); + assert.strictEqual(hasHybridPainted(f.id), false); + }); +}); + +// ── Same-URL Retry cap ─────────────────────────────────────────────────────── +describe('hybrid-page-host-registry: same-URL retry cap', () => { + beforeEach(() => { + _resetForTests(); + nextId = 1; + }); + + it('no outstanding failure by default; a normal reload is never throttled', () => { + assert.strictEqual(hasHybridOutstandingFailure(1), false); + assert.strictEqual(getHybridRetryAttempts(1), 0); + // noteHybridRetryAttempt is a no-op without an outstanding failure. + assert.strictEqual(noteHybridRetryAttempt(1), 0); + }); + + it('records a failure and accumulates same-URL retries up to the cap', () => { + noteHybridLoadFailure(1, 'https://bad.test/'); + assert.strictEqual(hasHybridOutstandingFailure(1), true); + assert.strictEqual(getHybridRetryAttempts(1), 0); + + // Simulate the page:reload gate: allowed while attempts < cap. + const reloads: number[] = []; + for (let i = 0; i < HYBRID_SAME_URL_RETRY_CAP + 3; i++) { + if (getHybridRetryAttempts(1) >= HYBRID_SAME_URL_RETRY_CAP) continue; // capped: blocked + noteHybridRetryAttempt(1); + reloads.push(i); + } + // Exactly CAP reloads are allowed; the rest are blocked. + assert.strictEqual(reloads.length, HYBRID_SAME_URL_RETRY_CAP); + assert.strictEqual(getHybridRetryAttempts(1), HYBRID_SAME_URL_RETRY_CAP); + }); + + it('a re-failure of the SAME url keeps the accumulated attempts', () => { + noteHybridLoadFailure(1, 'https://bad.test/'); + noteHybridRetryAttempt(1); + noteHybridRetryAttempt(1); + assert.strictEqual(getHybridRetryAttempts(1), 2); + // did-fail-load fires again for the same url after a retry reload. + noteHybridLoadFailure(1, 'https://bad.test/'); + assert.strictEqual(getHybridRetryAttempts(1), 2); // NOT reset + }); + + it('a failure of a DIFFERENT url resets the attempt counter', () => { + noteHybridLoadFailure(1, 'https://bad.test/'); + noteHybridRetryAttempt(1); + noteHybridRetryAttempt(1); + assert.strictEqual(getHybridRetryAttempts(1), 2); + noteHybridLoadFailure(1, 'https://other-bad.test/'); + assert.strictEqual(getHybridRetryAttempts(1), 0); // fresh url, fresh budget + }); + + it('clearHybridRetry (genuine success) drops the outstanding failure + budget', () => { + noteHybridLoadFailure(1, 'https://bad.test/'); + noteHybridRetryAttempt(1); + clearHybridRetry(1); + assert.strictEqual(hasHybridOutstandingFailure(1), false); + assert.strictEqual(getHybridRetryAttempts(1), 0); + }); + + it('teardown clears the retry record', () => { + const f = mk({ url: 'https://bad.test/' }); + noteHybridLoadFailure(f.id, 'https://bad.test/'); + noteHybridRetryAttempt(f.id); + unregisterHybridWindow(f.id); + assert.strictEqual(hasHybridOutstandingFailure(f.id), false); + }); +}); diff --git a/apps/desktop/main/hybrid-page-host-registry.ts b/apps/desktop/main/hybrid-page-host-registry.ts index d4d108e3..85d54d15 100644 --- a/apps/desktop/main/hybrid-page-host-registry.ts +++ b/apps/desktop/main/hybrid-page-host-registry.ts @@ -55,6 +55,78 @@ export type HybridLoadError = { const perHostLoadError = new Map(); +// ── First-paint tracking (loading card) ────────────────────────────────────── +// A hybrid window's content view shows nothing but its theme background fill +// until its FIRST load settles (fresh open, session restore, deferred-restore +// on focus) — the "black/blank screen while restoring" the user hit. The overlay +// paints a favicon+spinner loading card ONLY for that first-load window; once a +// window has painted a page, an in-page navigation keeps the old page visible, +// so covering it with a card would be a regression. `did-stop-loading` marks the +// window painted (first settle). Cleared on teardown. +const perHostPainted = new Set(); + +/** Mark that `windowId` has settled at least one load (its view has painted). */ +export function markHybridPainted(windowId: number): void { + perHostPainted.add(windowId); +} + +/** True once `windowId` has settled a load — i.e. it has a page to show, so a + * subsequent navigation should NOT get the opaque loading card. */ +export function hasHybridPainted(windowId: number): boolean { + return perHostPainted.has(windowId); +} + +// ── Same-URL retry cap (load-error "Retry" hammering) ───────────────────────── +// Mashing Retry on a permanently-failing URL used to reload it with zero backoff +// (read as an "infinite loop"). We cap the number of consecutive Retry reloads +// of the SAME failing URL. The record is intentionally SEPARATE from the +// load-error state above, which flickers (cleared on did-start-loading, re-set +// on did-fail-load) — so a fast Retry mash that lands mid-reload can't reset the +// counter. It is cleared only by a genuine load SUCCESS (did-finish-load) or a +// DIFFERENT failing URL, and by teardown. fsm-pilled: attempt-count driven off +// the load lifecycle, no wall-clock timer. +export const HYBRID_SAME_URL_RETRY_CAP = 3; + +type HybridRetryRecord = { failedUrl: string; attempts: number }; +const perHostRetry = new Map(); + +/** + * Record that `failedUrl` failed to load for `windowId` (did-fail-load / + * loading-timeout). Resets the attempt counter when the failing URL CHANGES; + * leaves it intact when the SAME url fails again so retries keep accumulating. + */ +export function noteHybridLoadFailure(windowId: number, failedUrl: string): void { + const rec = perHostRetry.get(windowId); + if (!rec || rec.failedUrl !== failedUrl) { + perHostRetry.set(windowId, { failedUrl, attempts: 0 }); + } +} + +/** True when `windowId` currently has an outstanding (unresolved) load failure — + * i.e. a `page:reload` is a Retry of a failed URL, subject to the cap. */ +export function hasHybridOutstandingFailure(windowId: number): boolean { + return perHostRetry.has(windowId); +} + +/** Increment and return the same-URL Retry attempt count for `windowId`. Callers + * guard with `hasHybridOutstandingFailure` first (returns 0 if none). */ +export function noteHybridRetryAttempt(windowId: number): number { + const rec = perHostRetry.get(windowId); + if (!rec) return 0; + rec.attempts += 1; + return rec.attempts; +} + +/** Current same-URL Retry attempt count (0 if no outstanding failure). */ +export function getHybridRetryAttempts(windowId: number): number { + return perHostRetry.get(windowId)?.attempts ?? 0; +} + +/** Clear the retry record — genuine load success, or navigation elsewhere. */ +export function clearHybridRetry(windowId: number): void { + perHostRetry.delete(windowId); +} + // Per-host pending navigation URL. Set by installHybridNavbarActionRouting's // page:navigate subscriber BEFORE calling loadURL(url), so wireHybridContentEvents // can read the target URL in did-start-loading (where contentWC.getURL() still @@ -630,6 +702,9 @@ export function unregisterHybridWindow(windowId: number): void { // Stage B: clean up per-host load-error and pending-nav state. perHostLoadError.delete(windowId); perHostPendingNavUrl.delete(windowId); + // Loading-card first-paint + retry-cap per-host state. + perHostPainted.delete(windowId); + perHostRetry.delete(windowId); // Announce the teardown (e.g. deny pending permission prompts). notifyWindowClosed(windowId); if (activeHybridWindowId === windowId) { @@ -787,6 +862,8 @@ export function _resetForTests(): void { activeHybridWindowId = null; isOverlayFocused = null; overlayWindowIdProvider = null; + perHostPainted.clear(); + perHostRetry.clear(); } /** diff --git a/apps/desktop/main/ipc.ts b/apps/desktop/main/ipc.ts index 9061596c..0e7dd57a 100644 --- a/apps/desktop/main/ipc.ts +++ b/apps/desktop/main/ipc.ts @@ -18,6 +18,7 @@ import { updateItem, updateItemTitle, updateItemFavicon, + getFaviconForUrl, updateItemThumbnail, hardDeleteItem, trackWindowLoad, @@ -139,9 +140,20 @@ import { claimHybridDeferredLoad, // Stage B: per-host load-error state + timeout (lives in registry to avoid circular dep) setHybridLoadError, + getHybridLoadError, getHybridLoadingTimeoutMs, setHybridPendingNavUrl, takeHybridPendingNavUrl, + // Loading-card first-paint tracking + markHybridPainted, + hasHybridPainted, + // Same-URL Retry cap + HYBRID_SAME_URL_RETRY_CAP, + noteHybridLoadFailure, + hasHybridOutstandingFailure, + noteHybridRetryAttempt, + getHybridRetryAttempts, + clearHybridRetry, } from './hybrid-page-host-registry.js'; /** @@ -907,6 +919,18 @@ function assembleHybridPageHost( baseWin.contentView.addChildView(view); + // Give the content WebContentsView the same theme fill as the host BaseWindow + // (ipc.ts window-open BaseWindow ctor). Without this, the GPU-backed content + // view paints opaque macOS black over the host's theme background until its + // first frame commits — the "black screen while restoring" the user saw. The + // BaseWindow backgroundColor is the outer belt; this is the inner one for the + // view that actually overlaps the content rect. + try { + view.setBackgroundColor(getSystemThemeBackgroundColor()); + } catch { + /* setBackgroundColor unsupported in some envs */ + } + // Fill the window; keep it filled on resize. const fillView = () => { const [w, h] = baseWin.getContentSize(); @@ -1374,9 +1398,22 @@ export function wireHybridContentEvents( errorCode: 'TIMEOUT', errorDescription: 'The page took too long to load.', }); + noteHybridLoadFailure(hostWindowId, capturedUrl); try { contentWC.stop(); } catch { /* already stopped */ } }, timeoutMs); - publish(getSystemAddress(), 'page:loading', { windowId: hostWindowId, loading: true }); + // Include the last-known favicon so a window that starts loading while + // ALREADY active (no re-handoff) still paints its icon on the loading card. + // '' when unknown → spinner-only. Rides the existing page:loading path — no + // extra timer or IPC. + let favicon = ''; + try { if (isHttp(capturedUrl)) favicon = getFaviconForUrl(capturedUrl); } catch { /* ds unavailable */ } + publish(getSystemAddress(), 'page:loading', { + windowId: hostWindowId, + loading: true, + favicon, + // Card shows only for a never-painted window (fresh/restore/deferred). + firstLoad: !hasHybridPainted(hostWindowId), + }); }); contentWC.on('did-stop-loading', () => { if (contentWC.isDestroyed()) return; @@ -1384,6 +1421,10 @@ export function wireHybridContentEvents( // Cancel the loading-timeout safety net so it can't fire after the WC // has already stopped (prevents a stale error overlay on a clean load). if (loadingTimeoutHandle) { clearTimeout(loadingTimeoutHandle); loadingTimeoutHandle = null; } + // The view has now painted a frame (page, error page, or abort) — so any + // FUTURE load is an in-page navigation that must NOT get the opaque loading + // card. Marks this window painted for the firstLoad gate. + markHybridPainted(hostWindowId); publish(getSystemAddress(), 'page:loading', { windowId: hostWindowId, loading: false }); // Thumbnail capture, matching the canvas guest's did-stop-loading (ipc.ts ~2102). @@ -1433,6 +1474,7 @@ export function wireHybridContentEvents( errorCode: String(errorCode), errorDescription: errorDescription || 'Load failed', }); + noteHybridLoadFailure(hostWindowId, failedUrl); try { contentWC.stop(); } catch { /* already stopped */ } }, ); @@ -2024,6 +2066,14 @@ export function wireHybridContentEvents( const pageUrl = contentWC.getURL(); if (!isHttp(pageUrl)) return; + // Genuine load success — reset the same-URL Retry budget so a URL that later + // fails starts fresh (and a recovered URL isn't stuck at the cap). Gate on + // there being NO active load-error: a FAILED main-frame load also fires + // did-finish-load for the committed error page, and getURL() still returns + // the (http) failed URL, so an unguarded clear would wipe the record + // did-fail-load just set (which fires first, so the error is already stored). + if (!getHybridLoadError(hostWindowId)) clearHybridRetry(hostWindowId); + let opensearchUrl: string | null = null; try { const detectedUrl = await contentWC.executeJavaScript(` @@ -2121,6 +2171,21 @@ export function installHybridNavbarActionRouting(): void { subscribe(getSystemAddress(), 'page:reload', (raw) => { const r = wcFor(raw); if (!r) return; + // page:reload is multi-purpose — Cmd+R, the error card's Retry button, and + // the same-URL-window-reuse auto-reload all publish it. Only Retry-of-a- + // failed-URL is capped: gate on there being an OUTSTANDING load failure for + // this window (survives the reload's own error-flicker, unlike the load- + // error state). A normal reload (no outstanding failure) is never throttled. + if (hasHybridOutstandingFailure(r.windowId)) { + if (getHybridRetryAttempts(r.windowId) >= HYBRID_SAME_URL_RETRY_CAP) { + // Cap reached — refuse to hammer the failing URL. Tell the overlay so it + // can disable Retry instead of silently swallowing the click. The error + // card stays up (no reload → no did-start-loading to clear it). + publish(getSystemAddress(), 'page:retry-capped', { windowId: r.windowId }); + return; + } + noteHybridRetryAttempt(r.windowId); + } try { r.wc.reload(); } catch { /* wc gone */ } }); diff --git a/apps/desktop/renderer/page/overlay.html b/apps/desktop/renderer/page/overlay.html index a05bd4e9..1648afe3 100644 --- a/apps/desktop/renderer/page/overlay.html +++ b/apps/desktop/renderer/page/overlay.html @@ -883,6 +883,49 @@ opacity: 0.9; } + /* ── Loading card (favicon + spinner) ────────────────────────────────── + * Dynamically created/removed by overlay.js (showLoadingOverlay / + * clearLoadingOverlay), positioned over the content rect by + * updateOverlayPositions() (same geometry as .load-error-overlay). Covers + * the content view's un-painted fill on restore/open so the user never sees + * a black flash. Same z-index (25) as the error card — the two are mutually + * exclusive (error supersedes loading), so they never stack. */ + .loading-overlay { + position: fixed; + z-index: 25; + display: flex; + align-items: center; + justify-content: center; + background: var(--theme-bg, #1e1e1e); + /* left / top / width / height set dynamically in updateOverlayPositions(). */ + } + body.no-active .loading-overlay { + display: none; + } + .loading-card { + display: flex; + flex-direction: column; + align-items: center; + gap: 16px; + } + .loading-favicon { + width: 32px; + height: 32px; + border-radius: 6px; + object-fit: contain; + } + .loading-spinner { + width: 28px; + height: 28px; + border-radius: 50%; + border: 3px solid color-mix(in srgb, var(--theme-text, #e0e0e0) 20%, transparent); + border-top-color: var(--theme-accent, #007aff); + animation: loading-spin 800ms linear infinite; + } + @keyframes loading-spin { + to { transform: rotate(360deg); } + } + /* ── Permission prompt (web permission requests — geolocation, etc.) ──── * Ported from the retired canvas page-host chrome (renderer/page/index.html, * see git history 2b14032d^) with overlay adaptations: the element is diff --git a/apps/desktop/renderer/page/overlay.js b/apps/desktop/renderer/page/overlay.js index c6683829..c6e2e12d 100644 --- a/apps/desktop/renderer/page/overlay.js +++ b/apps/desktop/renderer/page/overlay.js @@ -239,6 +239,16 @@ function updateOverlayPositions() { errorOverlay.style.height = `${h}px`; } + // Loading card: same content-rect geometry as the error card (dynamically + // created, so its bounds are set here each time positions change). + const loadingOverlay = document.querySelector('.loading-overlay'); + if (loadingOverlay) { + loadingOverlay.style.left = `${left}px`; + loadingOverlay.style.top = `${top}px`; + loadingOverlay.style.width = `${w}px`; + loadingOverlay.style.height = `${h}px`; + } + // Permission prompt: pinned top-center of the content rect (the canvas chrome // used window-relative top:16px/left:50%; the overlay frames content + gutters, // so anchor to the content instead). CSS translateX(-50%) centers on `left`. @@ -299,6 +309,12 @@ let activeWindowId = null; // or the active URL is not http(s). let activeUrl = null; +// The active window's last-known favicon (seeded from the handoff snapshot, +// updated by page:favicon / page:loading). Painted on the loading card so a +// restore/open shows the site's icon the instant the load starts — before the +// live favicon arrives. '' → spinner-only card. +let activeFavicon = ''; + // P1.4b-3: the active host window's current OUTER bounds, seeded from the active // handoff and kept in sync as the user resizes/moves through the overlay chrome. // The overlay frames the content window's full outer rect, so these ARE the @@ -373,7 +389,11 @@ function showLoadErrorOverlay(failedUrl, errorCode, errorDescription) { retry.textContent = 'Retry'; retry.addEventListener('click', () => { if (activeWindowId == null) return; - clearLoadErrorOverlay(); + // Don't optimistically clear the card: main is the single source of truth. + // A reload that actually happens clears it via did-start-loading → + // page:load-error(null); a Retry that hits the same-URL cap does NOT reload, + // and main answers with page:retry-capped so the card stays (and Retry + // disables) instead of vanishing into a blank page. api.publish('page:reload', { windowId: activeWindowId }); }); @@ -409,6 +429,87 @@ function showLoadErrorOverlay(failedUrl, errorCode, errorDescription) { updateOverlayPositions(); } +// ── Loading card (favicon + spinner over the content rect) ─────────────────── +// Kills the "black screen while restoring" the user saw: a deferred-restore or +// fresh open shows nothing but the content view's un-painted fill until the +// first frame commits. This card covers the content rect with a theme fill, the +// site's last-known favicon, and a spinner the instant the load STARTS, and is +// removed the instant it SETTLES. FSM alignment: driven purely by the existing +// deterministic `loading` signal (did-start-loading → page:loading:true / +// did-stop-loading → page:loading:false; snap.loading on retarget) — no timer, +// no polling. The branded error card SUPERSEDES it (error > loading). + +/** Remove any existing loading card. */ +function clearLoadingOverlay() { + const existing = document.querySelector('.loading-overlay'); + if (existing) existing.remove(); +} + +/** Update the favicon on a currently-shown loading card (live favicon arrived + * while still loading). No-op if no card is shown. */ +function updateLoadingOverlayFavicon(favicon) { + const card = document.querySelector('.loading-overlay .loading-card'); + if (!card) return; + let img = card.querySelector('.loading-favicon'); + if (favicon) { + if (!img) { + img = document.createElement('img'); + img.className = 'loading-favicon'; + img.alt = ''; + img.onerror = () => { img.remove(); }; + card.insertBefore(img, card.firstChild); + } + img.src = favicon; + } else if (img) { + img.remove(); + } +} + +/** + * Show the loading card over the content rect. Idempotent: if a card is already + * up, just refreshes its favicon (so the spinner animation isn't restarted on a + * favicon update). The error card wins — never show a loading card over an + * active error. + */ +function showLoadingOverlay(favicon) { + if (document.querySelector('.load-error-overlay')) return; // error supersedes + const existing = document.querySelector('.loading-overlay'); + if (existing) { + updateLoadingOverlayFavicon(favicon); + return; + } + + const overlay = document.createElement('div'); + overlay.className = 'loading-overlay'; + + const card = document.createElement('div'); + card.className = 'loading-card'; + + if (favicon) { + const img = document.createElement('img'); + img.className = 'loading-favicon'; + img.alt = ''; + img.src = favicon; + img.onerror = () => { img.remove(); }; + card.appendChild(img); + } + + const spinner = document.createElement('div'); + spinner.className = 'loading-spinner'; + card.appendChild(spinner); + + overlay.appendChild(card); + document.body.appendChild(overlay); + + // Position over the current content rect now that it's in the DOM. + updateOverlayPositions(); +} + +/** Hide the loading card (load settled or window went inactive). */ +function hideLoadingOverlay() { + clearLoadingOverlay(); +} + /** Reset the navbar to empty (active→null or before painting a new window). */ function clearChrome() { navbar.setUrl(''); @@ -422,6 +523,8 @@ function clearChrome() { document.body.classList.add('no-active'); hideFindBar(); clearLoadErrorOverlay(); + clearLoadingOverlay(); + activeFavicon = ''; clearPanels(); setCapture(false); } @@ -437,14 +540,23 @@ function paintFromSnapshot(snap) { // the previous window's title/favicon never bleeds across a retarget. navbar.pageTitle = snap.title || ''; navbar.favicon = snap.favicon || ''; + activeFavicon = snap.favicon || ''; // Stage B: restore the per-host error state for the newly-active window so the // error card shows (or stays gone) instantly on retarget — no flash of the // wrong window's state. Rides the same deterministic handoff path as // navbar.loading — no extra timer or separate IPC. if (snap.loadError) { showLoadErrorOverlay(snap.loadError.failedUrl, snap.loadError.errorCode, snap.loadError.errorDescription); + hideLoadingOverlay(); } else { clearLoadErrorOverlay(); + // Show the favicon+spinner loading card if the newly-active window is mid + // FIRST load (deferred restore loading on focus, or a fresh open still in + // flight) — a never-painted view would otherwise show a blank theme fill. + // firstLoad gates out in-page navigations of already-painted windows (they + // keep the old page visible). Same deterministic `loading` signal, no timer. + if (snap.loading && snap.firstLoad) showLoadingOverlay(activeFavicon); + else hideLoadingOverlay(); } } @@ -514,11 +626,22 @@ api.subscribe('page:favicon', (msg) => { if (!isActive(msg)) return; // P1.4b-4: surface the favicon in the navbar title chip. navbar.favicon = msg.favicon || ''; + // Keep the loading-card icon fresh if a card is currently up. + if (msg.favicon) activeFavicon = msg.favicon; + updateLoadingOverlayFavicon(activeFavicon); }); api.subscribe('page:loading', (msg) => { if (!isActive(msg)) return; navbar.loading = !!msg.loading; + // A favicon may ride the loading payload (window that started loading while + // already active — no re-handoff). Seed it for the card. + if (msg.favicon) activeFavicon = msg.favicon; + // Show the favicon+spinner card on FIRST load-start (never-painted window), + // remove it on settle. firstLoad gates out in-page navigations. The error card + // supersedes (showLoadingOverlay early-returns while an error is shown). + if (msg.loading && msg.firstLoad) showLoadingOverlay(activeFavicon); + else hideLoadingOverlay(); }); // Stage B: live error update for the currently-active window. Main publishes @@ -529,12 +652,35 @@ api.subscribe('page:loading', (msg) => { api.subscribe('page:load-error', (msg) => { if (!isActive(msg)) return; if (msg.error) { + // Error supersedes the loading card — take it down and show the error card. + hideLoadingOverlay(); showLoadErrorOverlay(msg.error.failedUrl, msg.error.errorCode, msg.error.errorDescription); } else { clearLoadErrorOverlay(); } }); +// Same-URL Retry cap reached: main refused another reload of the permanently- +// failing URL. Reflect it on the current error card — disable Retry and tell the +// user to edit the address or close — instead of leaving a dead button. The card +// is still up (main didn't reload), so mutate it in place. +api.subscribe('page:retry-capped', (msg) => { + if (!isActive(msg)) return; + const card = document.querySelector('.load-error-overlay .load-error-card'); + if (!card) return; + const retry = card.querySelector('.load-error-retry'); + if (retry) { + retry.textContent = 'Too many attempts'; + retry.disabled = true; + retry.style.opacity = '0.5'; + retry.style.cursor = 'default'; + } + const reason = card.querySelector('.load-error-reason'); + if (reason) { + reason.textContent = 'This page keeps failing. Edit the address or close the window.'; + } +}); + api.subscribe('page:nav-state', (msg) => { if (!isActive(msg)) return; if (msg.url) navbar.setUrl(msg.url); diff --git a/apps/desktop/tests/desktop/hybrid-loading-card.spec.ts b/apps/desktop/tests/desktop/hybrid-loading-card.spec.ts new file mode 100644 index 00000000..915c94b4 --- /dev/null +++ b/apps/desktop/tests/desktop/hybrid-loading-card.spec.ts @@ -0,0 +1,187 @@ +/** + * Hybrid page-host: favicon + spinner LOADING CARD (issue #3, black-screen fix). + * + * A fresh / session-restored / deferred-restore-on-focus hybrid window shows + * nothing but its content view's un-painted fill until the first frame commits — + * the "black screen while restoring" the user reported. Two-part fix: + * (A) the content WebContentsView gets the theme background (kills the BLACK), + * (B) the shared overlay paints a favicon+spinner card over the content rect + * while the FIRST load is in flight, removed the instant it settles. + * + * The card is gated on `firstLoad` (a never-painted window): an in-page + * navigation of an already-painted window keeps the old page visible, so it must + * NOT get an opaque card. This spec pins both: the card appears on first load and + * is removed on settle, and it does NOT appear on a subsequent navigation. + * + * Observability: real pixels are unobservable in macOS-headless, but the card is + * a real overlay DOM element, so we assert its presence/absence in the overlay + * window (same approach as the branded error card in page-load-failure.spec). + * + * Run with: + * yarn test:grep "Hybrid Loading Card" + */ + +import { test, expect, DesktopApp, getSharedApp, closeSharedApp } from '../fixtures/desktop-app'; +import { Page } from '@playwright/test'; +import { waitForExtensionsReady } from '../helpers/window-utils'; +import http from 'http'; + +let sharedApp: DesktopApp; +let sharedBgWindow: Page; +let server: http.Server; +let serverPort: number; +// Withheld responses released in afterAll so nothing dangles mid-load. +const pending = new Set(); + +test.beforeAll(async () => { + sharedApp = await getSharedApp(); + sharedBgWindow = await sharedApp.getBackgroundWindow(); + await waitForExtensionsReady(sharedBgWindow); + + await new Promise((resolve) => { + server = http.createServer((req, res) => { + if (req.url && req.url.startsWith('/slow')) { + // Hold the response so the content WC stays in the LOADING state long + // enough for the test to observe the card, then answer. + pending.add(res); + setTimeout(() => { + if (pending.delete(res)) { + res.writeHead(200, { 'Content-Type': 'text/html' }); + res.end(`${req.url}${req.url}`); + } + }, 2000); + return; + } + res.writeHead(200, { 'Content-Type': 'text/html' }); + res.end(`${req.url}${req.url}`); + }); + server.listen(0, '127.0.0.1', () => { + const addr = server.address(); + serverPort = typeof addr === 'object' && addr ? addr.port : 0; + resolve(); + }); + }); +}); + +test.afterAll(async () => { + for (const res of pending) { + try { + res.writeHead(200, { 'Content-Type': 'text/html' }); + res.end('draineddrained'); + } catch { /* socket gone */ } + } + pending.clear(); + if (server) server.close(); + await closeSharedApp(); +}); + +async function openHybridHost(): Promise<{ id: number; overlayWindow: Page }> { + const id = await sharedApp.evaluateMain!(() => { + const b = (globalThis as any).__peekHybridOverlayTest; + return b.makeWiredHybridHost({ x: 100, y: 100, width: 800, height: 600 }) as number; + }); + await sharedApp.evaluateMain!(((_e: unknown, arg: { id: number }) => { + const b = (globalThis as any).__peekHybridOverlayTest; + b.setActive(arg.id); + }) as any, { id }); + const overlayWindow = await sharedApp.getWindow('page/overlay.html', 20000); + await overlayWindow.waitForFunction( + () => (window as any).__overlayReady === true, + undefined, + { timeout: 10000 }, + ); + return { id, overlayWindow }; +} + +async function navigateHybridHost(id: number, url: string): Promise { + await sharedApp.evaluateMain!(((_e: unknown, arg: { id: number; url: string }) => { + const b = (globalThis as any).__peekHybridOverlayTest; + b.publishTopic('page:navigate', { windowId: arg.id, url: arg.url }); + }) as any, { id, url }); +} + +async function destroyHybridHost(id: number): Promise { + await sharedApp.evaluateMain!(((_e: unknown, arg: { id: number }) => { + const b = (globalThis as any).__peekHybridOverlayTest; + b.destroyFakeHybridWindow(arg.id); + }) as any, { id }); +} + +async function overlayLoading(): Promise { + return sharedApp.evaluateMain!(async () => { + const b = (globalThis as any).__peekHybridOverlayTest; + const s = await b.overlayChromeState(); + return s ? !!s.loading : false; + }); +} + +test.describe('Hybrid Loading Card @desktop', () => { + test('first load: favicon+spinner card appears while loading, removed on settle', async () => { + const slowUrl = `http://127.0.0.1:${serverPort}/slow/first`; + const { id, overlayWindow } = await openHybridHost(); + try { + // Never painted yet → firstLoad is true. + const paintedBefore = await sharedApp.evaluateMain!(((_e: unknown, i: number) => { + const b = (globalThis as any).__peekHybridOverlayTest; + return b.hybridHasPainted(i) as boolean; + }) as any, id) as boolean; + expect(paintedBefore).toBe(false); + + await navigateHybridHost(id, slowUrl); + + // The loading card (with spinner) appears while the first load is in flight. + await overlayWindow.waitForFunction(() => { + const card = document.querySelector('.loading-overlay'); + return !!card && !!card.querySelector('.loading-spinner'); + }, undefined, { timeout: 10000 }); + + // No favicon is stored for this test URL, so the card is spinner-only. + const hasFavicon = await overlayWindow.evaluate( + () => !!document.querySelector('.loading-overlay .loading-favicon'), + ); + expect(hasFavicon).toBe(false); + + // Once the withheld response arrives, the load settles → card removed. + await overlayWindow.waitForFunction( + () => !document.querySelector('.loading-overlay'), + undefined, + { timeout: 10000 }, + ); + } finally { + await destroyHybridHost(id); + } + }); + + test('second navigation (already painted) does NOT show the loading card', async () => { + const goodUrl = `http://127.0.0.1:${serverPort}/ok`; + const slowUrl = `http://127.0.0.1:${serverPort}/slow/second`; + const { id, overlayWindow } = await openHybridHost(); + try { + // First load settles → the window is now painted. + await navigateHybridHost(id, goodUrl); + await expect + .poll(() => sharedApp.evaluateMain!(((_e: unknown, i: number) => { + const b = (globalThis as any).__peekHybridOverlayTest; + return b.hybridHasPainted(i) as boolean; + }) as any, id), { timeout: 15000 }) + .toBe(true); + // Card gone after settle. + await overlayWindow.waitForFunction( + () => !document.querySelector('.loading-overlay'), + undefined, + { timeout: 10000 }, + ); + + // Second navigation to a slow URL: loading goes true, but firstLoad is now + // false → the opaque card must NOT be created (old page stays visible). + await navigateHybridHost(id, slowUrl); + await expect.poll(() => overlayLoading(), { timeout: 10000 }).toBe(true); + const cardDuringSecond = await overlayWindow.evaluate( + () => !!document.querySelector('.loading-overlay'), + ); + expect(cardDuringSecond).toBe(false); + } finally { + await destroyHybridHost(id); + } + }); +}); diff --git a/apps/desktop/tests/desktop/hybrid-retry-cap.spec.ts b/apps/desktop/tests/desktop/hybrid-retry-cap.spec.ts new file mode 100644 index 00000000..b7b9b303 --- /dev/null +++ b/apps/desktop/tests/desktop/hybrid-retry-cap.spec.ts @@ -0,0 +1,176 @@ +/** + * Hybrid page-host: same-URL Retry CAP (issue #1). + * + * The branded load-error card's "Retry" button publishes `page:reload`, which + * reloaded the same failing URL with zero backoff — so mashing Retry on a + * permanently-broken URL hammered it (the user read this as an "infinite loop"). + * + * Fix: main caps the number of consecutive Retry reloads of the SAME failing URL + * (HYBRID_SAME_URL_RETRY_CAP). The cap is gated on there being an OUTSTANDING + * load failure — a normal Cmd+R / same-URL-window-reuse reload is never + * throttled. When the cap is hit, main refuses the reload and publishes + * `page:retry-capped`, which the overlay reflects by disabling the Retry button. + * + * The retry record is deterministic (attempt count off the load lifecycle, no + * wall-clock timer) and separate from the flickering load-error state, so a fast + * mash that lands mid-reload can't reset the counter. We drive it via the + * `__peekHybridOverlayTest` bridge and assert both the main-side count and the + * overlay's disabled Retry button. + * + * Run with: + * yarn test:grep "Hybrid Retry Cap" + */ + +import { test, expect, DesktopApp, getSharedApp, closeSharedApp } from '../fixtures/desktop-app'; +import { Page } from '@playwright/test'; +import { waitForExtensionsReady } from '../helpers/window-utils'; +import http from 'http'; + +let sharedApp: DesktopApp; +let sharedBgWindow: Page; +let server: http.Server; +let serverPort: number; + +const RETRY_CAP = 3; // must match HYBRID_SAME_URL_RETRY_CAP + +test.beforeAll(async () => { + sharedApp = await getSharedApp(); + sharedBgWindow = await sharedApp.getBackgroundWindow(); + await waitForExtensionsReady(sharedBgWindow); + + await new Promise((resolve) => { + server = http.createServer((req, res) => { + res.writeHead(200, { 'Content-Type': 'text/html' }); + res.end(`${req.url}${req.url}`); + }); + server.listen(0, '127.0.0.1', () => { + const addr = server.address(); + serverPort = typeof addr === 'object' && addr ? addr.port : 0; + resolve(); + }); + }); +}); + +test.afterAll(async () => { + if (server) server.close(); + await closeSharedApp(); +}); + +async function openHybridHost(): Promise<{ id: number; overlayWindow: Page }> { + const id = await sharedApp.evaluateMain!(() => { + const b = (globalThis as any).__peekHybridOverlayTest; + return b.makeWiredHybridHost({ x: 100, y: 100, width: 800, height: 600 }) as number; + }); + await sharedApp.evaluateMain!(((_e: unknown, arg: { id: number }) => { + const b = (globalThis as any).__peekHybridOverlayTest; + b.setActive(arg.id); + }) as any, { id }); + const overlayWindow = await sharedApp.getWindow('page/overlay.html', 20000); + await overlayWindow.waitForFunction( + () => (window as any).__overlayReady === true, + undefined, + { timeout: 10000 }, + ); + return { id, overlayWindow }; +} + +async function navigateHybridHost(id: number, url: string): Promise { + await sharedApp.evaluateMain!(((_e: unknown, arg: { id: number; url: string }) => { + const b = (globalThis as any).__peekHybridOverlayTest; + b.publishTopic('page:navigate', { windowId: arg.id, url: arg.url }); + }) as any, { id, url }); +} + +async function publishReload(id: number): Promise { + await sharedApp.evaluateMain!(((_e: unknown, i: number) => { + const b = (globalThis as any).__peekHybridOverlayTest; + b.publishTopic('page:reload', { windowId: i }); + }) as any, id); +} + +async function retryAttempts(id: number): Promise { + return sharedApp.evaluateMain!(((_e: unknown, i: number) => { + const b = (globalThis as any).__peekHybridOverlayTest; + return b.hybridRetryAttempts(i) as number; + }) as any, id) as Promise; +} + +async function hasOutstandingFailure(id: number): Promise { + return sharedApp.evaluateMain!(((_e: unknown, i: number) => { + const b = (globalThis as any).__peekHybridOverlayTest; + return b.hybridHasOutstandingFailure(i) as boolean; + }) as any, id) as Promise; +} + +async function destroyHybridHost(id: number): Promise { + await sharedApp.evaluateMain!(((_e: unknown, arg: { id: number }) => { + const b = (globalThis as any).__peekHybridOverlayTest; + b.destroyFakeHybridWindow(arg.id); + }) as any, { id }); +} + +async function waitForErrorOverlay(overlayWindow: Page, timeout = 20000): Promise { + await overlayWindow.waitForFunction( + () => !!document.querySelector('.load-error-overlay'), + undefined, + { timeout }, + ); +} + +test.describe('Hybrid Retry Cap @desktop', () => { + test('mashing Retry on a permanently-failing URL is capped; Retry disables', async () => { + const badUrl = 'http://nonexistent-retrycap-xyz.invalid/'; + const { id, overlayWindow } = await openHybridHost(); + try { + await navigateHybridHost(id, badUrl); + await waitForErrorOverlay(overlayWindow); + // Failure recorded; no retry yet. + expect(await hasOutstandingFailure(id)).toBe(true); + expect(await retryAttempts(id)).toBe(0); + + // Fire exactly CAP Retry reloads. Each is allowed (attempts < cap), reloads, + // re-fails, and re-shows the error card. Poll the deterministic main-side + // attempt count between presses so the sequence can't race. + for (let i = 1; i <= RETRY_CAP; i++) { + await publishReload(id); + await expect.poll(() => retryAttempts(id), { timeout: 20000 }).toBe(i); + await waitForErrorOverlay(overlayWindow); // reload re-failed → card back + } + expect(await retryAttempts(id)).toBe(RETRY_CAP); + + // One more Retry — now at the cap. Main refuses the reload (attempt count + // stays at CAP) and publishes page:retry-capped → overlay disables Retry. + await publishReload(id); + await overlayWindow.waitForFunction(() => { + const btn = document.querySelector('.load-error-retry') as HTMLButtonElement | null; + return !!btn && btn.disabled && /too many/i.test(btn.textContent || ''); + }, undefined, { timeout: 10000 }); + + // The reload was blocked — attempt count did not grow past the cap, and the + // error card is STILL present (not blanked into an empty page). + expect(await retryAttempts(id)).toBe(RETRY_CAP); + const cardPresent = await overlayWindow.evaluate( + () => !!document.querySelector('.load-error-overlay'), + ); + expect(cardPresent).toBe(true); + } finally { + await destroyHybridHost(id); + } + }); + + test('a successful load has no outstanding failure — a reload is not throttled', async () => { + const goodUrl = `http://127.0.0.1:${serverPort}/ok`; + const { id } = await openHybridHost(); + try { + await navigateHybridHost(id, goodUrl); + // After a genuine success there is no outstanding failure, so the reload + // cap gate is never entered (a normal Cmd+R reloads freely). + await expect.poll(() => hasOutstandingFailure(id), { timeout: 15000 }).toBe(false); + await publishReload(id); + expect(await retryAttempts(id)).toBe(0); + expect(await hasOutstandingFailure(id)).toBe(false); + } finally { + await destroyHybridHost(id); + } + }); +});