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.
This commit is contained in:
Merge Sim
2026-09-12 00:11:33 -07:00
parent ff6f0be3d4
commit cda685e89f
2 changed files with 83 additions and 1 deletions
@@ -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)
}
}
@@ -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 = {