From cda685e89f9a83f8bc403e1c7a0cbebe6a2e18eb Mon Sep 17 00:00:00 2001 From: Merge Sim Date: Sat, 12 Sep 2026 00:11:33 -0700 Subject: [PATCH] fix(browser): publish same-document offscreen navigation too Review of the previous commit found the fix was half a fix. Electron does not emit did-navigate for a same-document commit, so pushState, replaceState, hash and anchor navigation kept the exact stale-push behaviour the committed- navigation notify was added to remove: a pull reads the live url while nothing invalidates the pushed snapshot. Every comparable surface already listens to both events -- the local webview (bind-browser-page-webview-listeners.ts:160-162) and the client-hosted pane (ClientHostedBrowserPagePane.tsx:299-300). The offscreen policy listened to one. It now handles main-frame did-navigate-in-page as well, and detaches it with the rest of the policy; on/off are symmetric at 7 each. did-start-navigation also recorded a pending target for in-place navigations that no did-navigate would ever clear, leaking prior urls into supersededUrls across SPA route changes. In-place starts now return early and leave the same-document commit to its own handler. Both guards ablate: removing the on() registration reddens the publication test, removing only the off() reddens the detachment test. --- ...browser-manager-guest-navigation-policy.ts | 25 +++++++- ...rowser-manager-load-failure-replay.test.ts | 59 +++++++++++++++++++ 2 files changed, 83 insertions(+), 1 deletion(-) diff --git a/src/main/browser/browser-manager-guest-navigation-policy.ts b/src/main/browser/browser-manager-guest-navigation-policy.ts index 7c8f8c28e22..d34892206b1 100644 --- a/src/main/browser/browser-manager-guest-navigation-policy.ts +++ b/src/main/browser/browser-manager-guest-navigation-policy.ts @@ -91,12 +91,18 @@ export abstract class BrowserManagerGuestNavigationPolicy extends BrowserManager const didStartNavigationHandler = ( _event: Electron.Event, url: string, - _isInPlace: boolean, + isInPlace: boolean, isMainFrame: boolean ): void => { if (!isMainFrame || isChromiumInternalErrorUrl(url)) { return } + if (isInPlace) { + // Why: a same-document navigation never emits 'did-navigate', so a pending target recorded + // here would never be cleared and would leak into supersededUrls on the next real + // navigation. 'did-navigate-in-page' owns the same-document commit instead. + return + } // Why: getURL() still reports the previous committed URL until this navigation commits, so // every UA writer must read the in-flight target or they disagree about the tab's host. this.startPendingNavigation(guest.id, url) @@ -124,10 +130,26 @@ export abstract class BrowserManagerGuestNavigationPolicy extends BrowserManager this.notifyBrowserGuestStateChanged(guest.id) } + const didNavigateInPageHandler = ( + _event: Electron.Event, + _url: string, + isMainFrame: boolean + ): void => { + if (!isMainFrame) { + return + } + // Why: Electron does not emit 'did-navigate' for a same-document commit, so without this a + // pushed host snapshot keeps the pre-navigation url while a pull reads the new one — the same + // stale-push class this policy's committed-navigation notify fixes for full navigations. + this.pendingNavigationByGuestId.delete(guest.id) + this.notifyBrowserGuestStateChanged(guest.id) + } + guest.on('will-navigate', navigationGuard) guest.on('will-redirect', willRedirectHandler) guest.on('did-start-navigation', didStartNavigationHandler) guest.on('did-navigate', didNavigateHandler) + guest.on('did-navigate-in-page', didNavigateInPageHandler) guest.on('did-fail-load', didFailLoadHandler) const handleDestroyed = (): void => { // Why: guests can die before renderer registration, else attach-time closures leak until shutdown. @@ -146,6 +168,7 @@ export abstract class BrowserManagerGuestNavigationPolicy extends BrowserManager guest.off('will-redirect', willRedirectHandler) guest.off('did-start-navigation', didStartNavigationHandler) guest.off('did-navigate', didNavigateHandler) + guest.off('did-navigate-in-page', didNavigateInPageHandler) guest.off('did-fail-load', didFailLoadHandler) } } diff --git a/src/main/browser/browser-manager-load-failure-replay.test.ts b/src/main/browser/browser-manager-load-failure-replay.test.ts index 9155e2846df..9cb281a83e2 100644 --- a/src/main/browser/browser-manager-load-failure-replay.test.ts +++ b/src/main/browser/browser-manager-load-failure-replay.test.ts @@ -92,6 +92,65 @@ describe('browserManager', () => { expect(stateChanged).toHaveBeenCalledWith('remote-worktree') }) + it('publishes a same-document offscreen navigation, and only for the main frame', () => { + const stateChanged = vi.fn() + const guest = { + id: 607, + isDestroyed: vi.fn(() => false), + getType: vi.fn(() => 'window'), + setBackgroundThrottling: vi.fn(), + setWindowOpenHandler: vi.fn(), + on: vi.fn(), + off: vi.fn(), + getURL: vi.fn(() => 'https://remote.test/app#route') + } + webContentsFromIdMock.mockReturnValue(guest) + browserManager.setBrowserGuestStateChangedListener(stateChanged) + browserManager.registerOffscreenGuest({ + browserPageId: 'spa-page', + worktreeId: 'remote-worktree', + webContentsId: guest.id + }) + stateChanged.mockClear() + // Why this event and not 'did-navigate': Electron never emits a full commit for a + // same-document navigation, so pushState/replaceState/hash routes would otherwise leave the + // published snapshot on the pre-navigation url while a pull reads the new one. + const inPage = guest.on.mock.calls.find(([event]) => event === 'did-navigate-in-page')?.[1] + expect(inPage).toBeTypeOf('function') + inPage!(null, guest.getURL(), false) + expect(stateChanged).not.toHaveBeenCalled() + inPage!(null, guest.getURL(), true) + expect(stateChanged).toHaveBeenCalledWith('remote-worktree') + }) + + it('detaches the same-document navigation listener with the rest of the policy', () => { + const guest = { + id: 608, + isDestroyed: vi.fn(() => false), + getType: vi.fn(() => 'window'), + setBackgroundThrottling: vi.fn(), + setWindowOpenHandler: vi.fn(), + on: vi.fn(), + off: vi.fn(), + getURL: vi.fn(() => 'https://remote.test/app') + } + webContentsFromIdMock.mockReturnValue(guest) + browserManager.registerOffscreenGuest({ + browserPageId: 'detach-page', + worktreeId: 'remote-worktree', + webContentsId: guest.id + }) + const destroyed = guest.on.mock.calls.find(([event]) => event === 'destroyed')?.[1] + expect(destroyed).toBeTypeOf('function') + destroyed!() + // Why assert the pairing: a listener added without a matching off() outlives its guest. + const attached = guest.on.mock.calls.map(([event]) => event).filter((e) => e !== 'destroyed') + const detached = new Set(guest.off.mock.calls.map(([event]) => event)) + for (const event of attached) { + expect(detached.has(event), `${event} was attached but never detached`).toBe(true) + } + }) + it('tracks offscreen load failures for the owning worktree snapshot', () => { const stateChanged = vi.fn() const offscreenGuest = {