fix(browser): keep annotation badges when a page updates its URL fragment while scrolling (#34267)

Scroll-spy pages rewrite the URL fragment with history.replaceState as
you scroll. The webview navigation handlers and setBrowserPageUrl treated
those same-document updates as a new document, retiring saved-note
badges and cancelling an open comment card. Fragment-only in-place
updates now keep the geometry; document navigations and in-place route
changes (path or query) still retire it, and the store no longer guesses
document boundaries from URL changes.
This commit is contained in:
Kelvin Amoaba
2026-10-27 13:00:54 +00:00
committed by GitHub
parent d2cc9e72a2
commit 62d9eb635b
5 changed files with 12 additions and 44 deletions
@@ -157,13 +157,16 @@ describe('browser annotation document boundaries', () => {
)
})
it('retires geometry for full and same-document main-frame navigation but not subframes', async () => {
it('retires geometry for document and in-place route changes but not subframes or fragment updates', async () => {
const h = await createHarness()
h.navigation.handleDidStartNavigation(
navigationEvent('https://example.com/frame', false, false)
)
expect(h.cancelCapture).not.toHaveBeenCalled()
h.navigation.handleDidStartNavigation(navigationEvent('https://example.com/#new', true))
h.navigation.handleDidNavigateInPage({ url: 'https://example.com/#new', isMainFrame: true })
expect(h.cancelCapture).not.toHaveBeenCalled()
expect(h.store.getState().browserAnnotationMarkerIdsByPageId[h.pageId]).toHaveLength(1)
h.navigation.handleDidNavigateInPage({ url: 'https://example.com/?tab=2', isMainFrame: true })
expect(h.cancelCapture).toHaveBeenCalledOnce()
h.store.getState().addBrowserPageAnnotation(makeAnnotation(h.pageId, 'fresh'))
h.navigation.handleDidStartNavigation(navigationEvent('https://example.com/next'))
@@ -180,8 +183,5 @@ describe('browser annotation document boundaries', () => {
h.navigation.handleFullDidNavigate({ url: 'https://example.com/', isMainFrame: true })
expect(h.store.getState().browserAnnotationsByPageId[h.pageId]).toBe(saved)
expect(h.store.getState().browserAnnotationMarkerIdsByPageId[h.pageId]).toBeUndefined()
h.store.getState().addBrowserPageAnnotation(makeAnnotation(h.pageId, 'fresh'))
h.navigation.handleDidNavigateInPage({ url: 'https://example.com/#another', isMainFrame: true })
expect(h.store.getState().browserAnnotationMarkerIdsByPageId[h.pageId]).toBeUndefined()
})
})
@@ -101,14 +101,12 @@ export function createBrowserPageWebviewNavigationHandlers({
}
const handleDidStartNavigation = (event: Electron.DidStartNavigationEvent): void => {
// Main-frame starts include same-URL reloads; did-start-loading also fires for iframe loads.
if (event.isMainFrame && event.url) {
invalidateBrowserAnnotationDocumentRef.current()
setBrowserOverlayViewport({ scrollX: 0, scrollY: 0, version: 0 })
}
if (!isPageDocumentNavigationStart(event) || !event.url) {
return
}
// Same-URL reloads replace the document too; in-place updates are judged once they commit.
invalidateBrowserAnnotationDocumentRef.current()
setBrowserOverlayViewport({ scrollX: 0, scrollY: 0, version: 0 })
onUpdatePageStateRef.current(browserTabId, { loading: true })
const pendingRecoveryNavigation = recoveryNavigationValidationRef.current
const browserStartedUrl = redactKagiSessionToken(event.url)
@@ -139,7 +137,8 @@ export function createBrowserPageWebviewNavigationHandlers({
const browserModelUrl = redactKagiSessionToken(currentUrl)
const normalizedBrowserModelUrl =
normalizeBrowserNavigationUrl(browserModelUrl) ?? browserModelUrl
if (lastKnownWebviewUrlRef.current !== normalizedBrowserModelUrl) {
// In-place route changes swap content, but fragment-only updates (scroll spies, anchors) keep the layout.
if (lastKnownWebviewUrlRef.current?.split('#')[0] !== normalizedBrowserModelUrl.split('#')[0]) {
invalidateBrowserAnnotationDocumentRef.current()
}
lastKnownWebviewUrlRef.current = normalizedBrowserModelUrl
@@ -65,6 +65,7 @@ describe('saved browser note lifetime and marker eligibility', () => {
for (let index = 0; index < GRAB_BUDGET.annotationsMaxPerPage + 3; index++) {
const url = `https://example.com/page-${index % 2}`
store.getState().setBrowserPageUrl(pageId, url)
store.getState().invalidateBrowserPageAnnotationGeometry(pageId)
const note = makeAnnotation(pageId, `note-${index}`)
note.payload.page.sanitizedUrl = url
store.getState().addBrowserPageAnnotation(note)
@@ -126,27 +126,6 @@ describe('createBrowserSlice annotations', () => {
})
})
it('retains saved page annotations while retiring geometry when the URL changes', async () => {
const { store, main } = await createTestStoreDrawnFromMain()
const tab = store.getState().createBrowserTab('wt-1', 'https://example.com')
await main.settle()
const pageId = tab.activePageId
if (!pageId) {
throw new Error('Expected a new browser page')
}
store.getState().addBrowserPageAnnotation(makeAnnotation(pageId))
expect(store.getState().browserAnnotationsByPageId[pageId]).toHaveLength(1)
const saved = store.getState().browserAnnotationsByPageId[pageId]
store.getState().setBrowserPageUrl(pageId, 'https://example.com/next')
store.getState().setBrowserPageUrl(pageId, 'https://example.com')
expect(store.getState().browserAnnotationsByPageId[pageId]).toBe(saved)
expect(saved?.[0]?.payload.page.sanitizedUrl).toBe('https://example.com')
expect(store.getState().browserAnnotationMarkerIdsByPageId[pageId]).toBeUndefined()
})
it('can commit a navigation URL without hiding an active recovery error', async () => {
const { store, main } = await createTestStoreDrawnFromMain()
const tab = store.getState().createBrowserTab('wt-1', 'https://example.com')
@@ -185,8 +185,6 @@ export function createBrowserPageStateActions(
}
// Keep document URL blank at navigation sink too; committing grants here leaks to persistence, publication and address bar.
const nextPageUrl = page.docLocation ? ORCA_BROWSER_BLANK_URL : nextUrl
// Saved feedback survives navigation; its old document coordinates do not.
const shouldInvalidateGeometry = normalizeUrl(page.url) !== nextPageUrl
const nextPages = (s.browserPagesByWorkspace[workspace.id] ?? []).map((entry) =>
entry.id === pageId
? {
@@ -200,12 +198,6 @@ export function createBrowserPageStateActions(
: entry
)
const nextWorkspace = mirrorWorkspaceFromActivePage(workspace, nextPages)
const nextBrowserAnnotationMarkerIdsByPageId = shouldInvalidateGeometry
? { ...s.browserAnnotationMarkerIdsByPageId }
: s.browserAnnotationMarkerIdsByPageId
if (shouldInvalidateGeometry) {
delete nextBrowserAnnotationMarkerIdsByPageId[pageId]
}
return {
browserPagesByWorkspace: {
...s.browserPagesByWorkspace,
@@ -216,10 +208,7 @@ export function createBrowserPageStateActions(
[workspace.worktreeId]: (s.browserTabsByWorktree[workspace.worktreeId] ?? []).map(
(tab) => (tab.id === workspace.id ? nextWorkspace : tab)
)
},
...(shouldInvalidateGeometry
? { browserAnnotationMarkerIdsByPageId: nextBrowserAnnotationMarkerIdsByPageId }
: {})
}
}
})
get().setBrowserPageCertificateFailure(pageId, null)