From c856c5f6db02be20fe1fb3ab97887723a481b618 Mon Sep 17 00:00:00 2001 From: Chen Jin Date: Wed, 12 Aug 2026 11:22:25 +0800 Subject: [PATCH] fix(desktop): keep find-in-page highlights consistent across React re-renders (#81726) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scoped find walker wraps transcript text nodes in elements that React does not own. Assistant responses stream through markdown-text.tsx, which rebuilds the markdown DOM on every delta, and a new message is appended whenever the assistant answers — so a re-render of a changed region detaches the marks we inserted, dropping the user's highlights while the bar stays open. Watch the captured scope with a MutationObserver and re-wrap only when an unmarked occurrence of the active query actually reappears. The observer is gated behind a re-entrancy flag while the walker is mutating, coalesced to one re-apply per microtask, torn down when the bar closes or the query clears, and restores the active ordinal so a mid-stream re-render doesn't reset the user's place to match #1. An append that adds no matching text is a no-op; re-wrapping only fires when highlights genuinely went stale. Co-Authored-By: Claude Fable 5 --- .../src/lib/find-in-page-scope.test.ts | 106 +++++++++++ apps/desktop/src/lib/find-in-page-scope.ts | 170 +++++++++++++++++- 2 files changed, 274 insertions(+), 2 deletions(-) diff --git a/apps/desktop/src/lib/find-in-page-scope.test.ts b/apps/desktop/src/lib/find-in-page-scope.test.ts index 334d510bc2..32328e9195 100644 --- a/apps/desktop/src/lib/find-in-page-scope.test.ts +++ b/apps/desktop/src/lib/find-in-page-scope.test.ts @@ -319,4 +319,110 @@ describe('scope lifecycle', () => { const result = performScopedFind(foreground, 'needle', { forward: true, findNext: false }) expect(result.count).toBe(1) }) +}) + +describe('scoped find survives React re-render', () => { + // The transcript is React-owned: assistant answers stream, and a new message + // is appended whenever the assistant replies. React doesn't know about the + // ``s we insert, so a render that re-allocates a region detaches its + // marks. The scope watcher (MutationObserver) must re-wrap after the render + // commits. jsdom delivers observer callbacks + our queueMicrotask re-apply + // as microtasks, so awaiting a macrotask drains them all. + async function flushAll(): Promise { + await new Promise(resolve => setTimeout(resolve, 0)) + } + + it('re-wraps content React re-allocated, keeping the active position', async () => { + const surface = plantSurface('surface', '

needle hay needle

') + captureFindScope() + performScopedFind(surface, 'needle', { forward: true, findNext: false }) + expect(surface.querySelectorAll('mark.find-hit').length).toBe(2) + + // Step onto match #2, then mock a React re-render that rebuilds the + // paragraph (destroying our marks) while the matching text remains. + performScopedFind(surface, 'needle', { forward: true, findNext: true }) + expect(surface.querySelectorAll('mark.find-hit[data-find-active]')[0]).toBe( + surface.querySelectorAll('mark.find-hit')[1] + ) + + surface.querySelector('p')!.innerHTML = 'needle hay needle' + await flushAll() + + // Marks restored, and the active mark is still the second match. + expect(surface.querySelectorAll('mark.find-hit').length).toBe(2) + expect(surface.querySelectorAll('mark.find-hit[data-find-active]').length).toBe(1) + expect(surface.querySelectorAll('mark.find-hit[data-find-active]')[0]).toBe( + surface.querySelectorAll('mark.find-hit')[1] + ) + expect(surface.querySelector('p')!.textContent).toBe('needle hay needle') + }) + + it('keeps existing highlights and wraps a newly appended message', async () => { + // The task's target scenario: the bar is open with highlights, then the + // assistant appends a NEW message while the list stays mounted. Existing + // highlights must persist and the appended content must not be broken. + const surface = plantSurface('surface', '

needle in the first message

') + captureFindScope() + performScopedFind(surface, 'needle', { forward: true, findNext: false }) + expect(surface.querySelectorAll('mark.find-hit').length).toBe(1) + + // React appends a new message block. + const next = document.createElement('div') + next.innerHTML = 'second message needle' + surface.appendChild(next) + await flushAll() + + // The first highlight survived the append AND the new match was wrapped + // too — nothing clobbered, nothing lost. + const marks = surface.querySelectorAll('mark.find-hit') + expect(marks.length).toBe(2) + expect(surface.querySelector('p')!.textContent).toBe('needle in the first message') + expect(next.textContent).toBe('second message needle') + }) + + it('does NOT re-wrap when React touched a region with no unmarked match', async () => { + const surface = plantSurface('surface', '

needle needle

') + captureFindScope() + performScopedFind(surface, 'needle', { forward: true, findNext: false }) + expect(surface.querySelectorAll('mark.find-hit').length).toBe(2) + + const before = [...surface.querySelectorAll('mark.find-hit')] + // React re-renders a paragraph that doesn't match the query — the marks + // are untouched and must not be rebuilt. + surface.insertAdjacentHTML('beforeend', '

irrelevant

') + await flushAll() + + const after = [...surface.querySelectorAll('mark.find-hit')] + expect(after.length).toBe(before.length) + expect(after.every(mark => before.includes(mark))).toBe(true) + }) + + it('searches a typeahead that clears and re-runs still tears the watcher down', async () => { + const surface = plantSurface('surface', '

needle

') + captureFindScope() + performScopedFind(surface, 'needle', { forward: true, findNext: false }) + expect(surface.querySelectorAll('mark.find-hit').length).toBe(1) + + // Clearing the query tears down the watcher; a later React write that + // would normally re-wrap must NOT resurrect highlights (the bar closed). + performScopedFind(surface, '', { forward: true, findNext: false }) + surface.querySelector('p')!.innerHTML = 'needle again' + await flushAll() + + expect(surface.querySelectorAll('mark.find-hit').length).toBe(0) + expect(surface.querySelector('p')!.textContent).toBe('needle again') + }) + + it('releaseFindScope detaches the watcher so no write can re-highlight', async () => { + const surface = plantSurface('surface', '

needle

') + captureFindScope() + performScopedFind(surface, 'needle', { forward: true, findNext: false }) + expect(surface.querySelectorAll('mark.find-hit').length).toBe(1) + + releaseFindScope() + surface.querySelector('p')!.innerHTML = 'needle needle' + await flushAll() + + expect(surface.querySelectorAll('mark.find-hit').length).toBe(0) + }) }) \ No newline at end of file diff --git a/apps/desktop/src/lib/find-in-page-scope.ts b/apps/desktop/src/lib/find-in-page-scope.ts index 8b2578cde2..a5b0170e58 100644 --- a/apps/desktop/src/lib/find-in-page-scope.ts +++ b/apps/desktop/src/lib/find-in-page-scope.ts @@ -30,6 +30,19 @@ * `` carries `data-active`. We re-derive it on every step instead of * keeping a counter, so a stale count after a DOM mutation self-heals on * the next search. + * - The transcript is React-owned and re-renders while the bar is open — + * assistant responses stream through `markdown-text.tsx`, which rebuilds + * the markdown DOM on every delta, and a new message is appended whenever + * the assistant answers. React does not know about the ``s we insert, + * so a re-render of a region whose text or structure changed detaches our + * marks (React re-allocates those nodes) while leaving marks in untouched + * regions intact. To keep highlights consistent across those renders we + * watch the captured scope with a MutationObserver and re-apply the wrap + * whenever an unmarked occurrence of the query reappears — but ONLY then, + * so a responsive render that left every mark in place costs nothing and an + * append that adds no matching text is a no-op. The observer is gated off + * while WE are mutating (a re-entrancy flag), coalesced to one re-apply per + * microtask, and torn down when the bar closes (#81726). * - We do NOT call `webContents.findInPage` from here. That bridge still * exists for multi-window secondary sessions (each window searches its * own webContents), but for the primary window it cannot be scoped, so @@ -43,6 +56,25 @@ const HIGHLIGHT_CLASS = 'find-hit' const ACTIVE_ATTR = 'data-find-active' const ROOT_ATTR = 'data-find-root' +// ── Re-render watch ───────────────────────────────────────────────────────── +// The transcript is React-owned and re-renders under us (streaming markdown, +// message append). React doesn't know about our ``s, so a render that +// re-allocates a region detaches the marks in that region. We watch the scope +// and re-wrap when an unmarked occurrence of the active query appears. +// +// `applying` is the re-entrancy guard: it's set for the whole synchronous +// re-apply, and any observer callback that fires during it flips `pending` +// so we take one more look afterwards instead of recursing infinitely. All +// observer work is coalesced to a single microtask so a burst of mutations +// (one streaming delta) lands as one re-apply. +let observer: MutationObserver | null = null +let scopeRoot: HTMLElement | null = null +let activeQuery = '' +let lastActiveOrdinal = 0 +let applying = false +let pending = false +let scheduled = false + /** * The element the open FindBar should search. Captured at bar-open time and * reused for every query / step until the bar closes. Reading the visible @@ -63,9 +95,25 @@ export function captureFindScope(): HTMLElement | null { root.setAttribute(ROOT_ATTR, '') } + resetScopeState() + scopeRoot = root + return root } +/** Forget a past scope: stop watching it and drop any queued re-apply. Called + * whenever the bar re-opens or closes so state never leaks across searches. */ +function resetScopeState(): void { + observer?.disconnect() + observer = null + scopeRoot = null + activeQuery = '' + lastActiveOrdinal = 0 + applying = false + pending = false + scheduled = false +} + /** The scope captured by the open bar, or null if none is active. */ export function currentFindScope(): HTMLElement | null { const roots = document.querySelectorAll(`[${ROOT_ATTR}]`) @@ -348,6 +396,10 @@ export function performScopedFind( if (!query) { clearHighlights(root) setActiveMark(null) + // No query, no marks to maintain — stop watching and forget the query. + activeQuery = '' + lastActiveOrdinal = 0 + stopObserver() return DEFAULT_RESULT } @@ -375,12 +427,24 @@ export function performScopedFind( let marks = existingMarks if (!sameQuery) { - clearHighlights(root) - marks = highlightMatches(root, query) + // Mutate under the re-entrancy guard so the re-render watcher doesn't + // fire on the marks WE are replacing (it has nothing to repair yet). + applying = true + + try { + clearHighlights(root) + marks = highlightMatches(root, query) + } finally { + applying = false + } } if (marks.length === 0) { setActiveMark(null) + // A query that matches nothing has nothing to maintain. + activeQuery = '' + lastActiveOrdinal = 0 + stopObserver() return DEFAULT_RESULT } @@ -404,9 +468,110 @@ export function performScopedFind( setActiveMark(marks[nextIndex] ?? null) + // A live search must survive React re-rendering the transcript under us. + // Remember the query + active position and start watching the scope; the + // observer re-wraps when a render detaches our marks and re-activates the + // same ordinal so the user's place doesn't reset to match #1 on the next + // streamed token. + activeQuery = query + lastActiveOrdinal = nextIndex + 1 + ensureObserver(root) + return { count: marks.length, activeOrdinal: nextIndex + 1 } } +// ── Re-apply on React re-render ───────────────────────────────────────────── +/** + * Attach the scope watcher, if it isn't already attached. Only observes the + * current scope; a fresh `captureFindScope` (or close) detaches it. + */ +function ensureObserver(root: Element): void { + if (observer && scopeRoot === root) { + return + } + + stopObserver() + observer = new MutationObserver(() => scheduleReapply()) + observer.observe(root, { childList: true, subtree: true }) +} + +function stopObserver(): void { + observer?.disconnect() + observer = null +} + +/** + * Coalesce a mutation burst (one streaming delta) into a single re-apply that + * runs on the next microtask. A no-op when the marks still cover the query — + * React reusing an untouched region doesn't need any work. + */ +function scheduleReapply(): void { + if (applying) { + // A mutation landed while we were mutating the tree. We'll look again + // once this apply finishes rather than recursing now. + pending = true + + return + } + + if (scheduled || !scopeRoot || !activeQuery) { + return + } + + scheduled = true + + queueMicrotask(() => { + scheduled = false + + if (applying || !scopeRoot || !activeQuery) { + return + } + + // Nothing to repair: every occurrence is still wrapped (React only + // touched a region that doesn't match the query). + if (!hasUnmarkedMatch(scopeRoot, activeQuery)) { + return + } + + reapplying(scopeRoot) + }) +} + +/** + * Re-wrap every occurrence in the scope after React detached our previous + * marks, re-activating the position the user was on. Runs with `applying` set + * so the observer ignores the mutations WE make, then drains any `pending` + * mutation that arrived during the apply. + */ +function reapplying(root: HTMLElement): void { + const restoreOrdinal = lastActiveOrdinal + + applying = true + + try { + clearHighlights(root) + const marks = highlightMatches(root, activeQuery) + + if (marks.length === 0) { + return + } + + // Re-activate the same ordinal the user last stood on, clamped to the + // re-wrapped set, so a mid-stream re-render doesn't bounce their place + // back to match #1. + const nextIndex = Math.min(restoreOrdinal - 1, marks.length - 1) + setActiveMark(marks[nextIndex] ?? null) + lastActiveOrdinal = nextIndex + 1 + } finally { + applying = false + } + + if (pending) { + pending = false + scheduleReapply() + } +} + /** Tear down highlights and the scope marker — called when the bar closes. */ export function releaseFindScope(): void { const roots = document.querySelectorAll(`[${ROOT_ATTR}]`) @@ -417,4 +582,5 @@ export function releaseFindScope(): void { } setActiveMark(null) + resetScopeState() } \ No newline at end of file