diff --git a/src/renderer/src/assets/main.css b/src/renderer/src/assets/main.css index 1d4cbac0572..90549525afe 100644 --- a/src/renderer/src/assets/main.css +++ b/src/renderer/src/assets/main.css @@ -204,6 +204,8 @@ --git-graph-lane-4: #40b0a6; --git-graph-lane-5: #b66dff; --annotation-highlight: #f59e0b; + --markdown-search-match: #facc15; + --markdown-search-match-active: #fb923c; /* Why: tab-group splits use a dedicated divider — `--border` is too faint on card/editor surfaces when panes are side-by-side or stacked. Default must meet >=3:1 against `--card`, not only the hover/drag strong state. */ @@ -292,6 +294,8 @@ --git-graph-lane-4: #40b0a6; --git-graph-lane-5: #b66dff; --annotation-highlight: #fbbf24; + --markdown-search-match: #facc15; + --markdown-search-match-active: #fb923c; /* Brighter than terminal defaults so the always-visible line clears 3:1 on `--card`. */ --tab-group-split-divider: #71717a; --tab-group-split-divider-strong: #a1a1aa; diff --git a/src/renderer/src/assets/markdown-preview.css b/src/renderer/src/assets/markdown-preview.css index d043482689b..0b153d77cb1 100644 --- a/src/renderer/src/assets/markdown-preview.css +++ b/src/renderer/src/assets/markdown-preview.css @@ -237,15 +237,17 @@ } } -.markdown-preview-search-match { - padding: 0; - border-radius: 2px; - background: color-mix(in srgb, #facc15 50%, transparent); +/* Why: search hits are painted with the CSS Custom Highlight API (Ranges, no + DOM mutation) instead of injected elements — see markdown-preview-search.ts. + ::highlight() supports color/background only, not border-radius/padding. */ +::highlight(markdown-preview-search-match) { + background-color: color-mix(in srgb, var(--markdown-search-match) 50%, transparent); color: inherit; } -.markdown-preview-search-match[data-active] { - background: color-mix(in srgb, #fb923c 60%, transparent); +::highlight(markdown-preview-search-active-match) { + background-color: color-mix(in srgb, var(--markdown-search-match-active) 60%, transparent); + color: inherit; } .markdown-review-toolbar { diff --git a/src/renderer/src/components/editor/MarkdownPreview.tsx b/src/renderer/src/components/editor/MarkdownPreview.tsx index f47b3610f3c..331d683e715 100644 --- a/src/renderer/src/components/editor/MarkdownPreview.tsx +++ b/src/renderer/src/components/editor/MarkdownPreview.tsx @@ -484,12 +484,18 @@ export default function MarkdownPreview({ input.focus() input.select() }, []) - const matchesRef = useRef([]) + const matchesRef = useRef([]) + // Stable token identifying this preview in the document-global highlight + // registry, so split/floating previews don't clobber each other's Find paint. + const searchInstanceRef = useRef({}) const lastAppliedInitialAnchorRef = useRef(null) const pendingEditorRevealFrameIdsRef = useRef([]) const [isSearchOpen, setIsSearchOpen] = useState(false) const [query, setQuery] = useState('') const [matchCount, setMatchCount] = useState(0) + // Bumps whenever the match ranges are recomputed, so the active-highlight + // effect re-runs even when a streamed rerender yields the same count/index. + const [searchRevision, setSearchRevision] = useState(0) const [activeMatchIndex, setActiveMatchIndex] = useState(-1) const isMac = navigator.userAgent.includes('Mac') const openFile = useAppStore((s) => s.openFile) @@ -835,29 +841,36 @@ export default function MarkdownPreview({ return } + const instanceId = searchInstanceRef.current + if (!isSearchOpen) { matchesRef.current = [] setMatchCount(0) - clearMarkdownPreviewSearchHighlights(body) + clearMarkdownPreviewSearchHighlights(instanceId) return } - // Search decorations are applied imperatively because the rendered preview is - // already owned by react-markdown. Rewriting the markdown AST for transient - // find state would make navigation and link rendering much harder to reason about. - const matches = applyMarkdownPreviewSearchHighlights(body, query) + // Search decorations are painted via the CSS Custom Highlight API (Ranges, + // no DOM mutation) because the rendered preview is owned by react-markdown; + // splitting its nodes to inject corrupted react's tree (crash 237acef1). + const matches = applyMarkdownPreviewSearchHighlights(instanceId, body, query) matchesRef.current = matches setMatchCount(matches.length) + setSearchRevision((v) => v + 1) setActiveMatchIndex((cur) => matches.length === 0 ? -1 : cur >= 0 && cur < matches.length ? cur : 0 ) - return () => clearMarkdownPreviewSearchHighlights(body) + return () => clearMarkdownPreviewSearchHighlights(instanceId) }, [renderedContent, isSearchOpen, query]) useEffect(() => { - setActiveMarkdownPreviewSearchMatch(matchesRef.current, activeMatchIndex) - }, [activeMatchIndex, matchCount]) + setActiveMarkdownPreviewSearchMatch( + searchInstanceRef.current, + matchesRef.current, + activeMatchIndex + ) + }, [activeMatchIndex, matchCount, searchRevision]) useLayoutEffect(() => { if (!initialAnchor || initialAnchor === lastAppliedInitialAnchorRef.current) { @@ -1919,7 +1932,10 @@ export default function MarkdownPreview({ ) : null} ) : null} -
+ {/* Why: translate="no" keeps browser/OS page-translation from swapping + text nodes react owns, which otherwise triggers the same + insertBefore/removeChild reconciliation crash (237acef1). */} +
{/* Why: remarkFrontmatter strips front matter from normal markdown output. When the user opts in from the preview actions menu, render the raw metadata as a compact read-only block above the document body. */} diff --git a/src/renderer/src/components/editor/markdown-preview-search-crash.test.tsx b/src/renderer/src/components/editor/markdown-preview-search-crash.test.tsx new file mode 100644 index 00000000000..9bbe4a91824 --- /dev/null +++ b/src/renderer/src/components/editor/markdown-preview-search-crash.test.tsx @@ -0,0 +1,193 @@ +// @vitest-environment happy-dom +// +// Regression guards for crash 237acef1: search highlighting must not mutate the +// DOM react-markdown owns. Injecting by splitting react's text nodes (and +// normalize()-merging them on clear) left react with stale child pointers, so +// the next streamed-content commit threw +// NotFoundError: Failed to execute 'insertBefore' on 'Node': The node before +// which the new node is to be inserted is not a child of this node. +import { act } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +import { + applyMarkdownPreviewSearchHighlights, + clearMarkdownPreviewSearchHighlights, + setActiveMarkdownPreviewSearchMatch +} from './markdown-preview-search' + +const SEARCH_HIGHLIGHT_NAME = 'markdown-preview-search-match' +const ACTIVE_SEARCH_HIGHLIGHT_NAME = 'markdown-preview-search-active-match' + +function MarkdownBody({ parts }: { parts: readonly string[] }): React.JSX.Element { + // Mirror react-markdown: a

whose children are text nodes react owns. + return ( +

+

{parts.map((part) => part)}

+
+ ) +} + +describe('markdown preview search highlighting keeps react-owned DOM intact (crash 237acef1)', () => { + let container: HTMLDivElement + let root: Root + const instance = {} + + beforeEach(() => { + container = document.createElement('div') + document.body.append(container) + root = createRoot(container) + }) + + afterEach(() => { + act(() => root.unmount()) + container.remove() + clearMarkdownPreviewSearchHighlights(instance) + }) + + function render(parts: readonly string[]): HTMLElement { + act(() => root.render()) + const body = container.querySelector('.markdown-body') + if (!body) { + throw new Error('missing markdown body') + } + return body + } + + it('applies highlights without mutating the react-owned subtree', () => { + const body = render(['hello world']) + const matches = applyMarkdownPreviewSearchHighlights(instance, body, 'lo') + expect(matches.length).toBe(1) + // No is injected; the text react owns is byte-for-byte intact. + expect(body.querySelector('mark')).toBeNull() + expect(body.textContent).toBe('hello world') + }) + + it('survives a streamed re-render while highlights are active', () => { + const body = render(['alpha ', 'beta']) + applyMarkdownPreviewSearchHighlights(instance, body, 'beta') + // Highlights stay registered while streamed content rewrites the paragraph. + // When they mutated react's text nodes this commit threw NotFoundError; with + // no DOM mutation it is clean. (afterEach clears the instance.) + expect(() => render(['alpha ', 'gamma', ' delta'])).not.toThrow() + }) +}) + +// The paint path only runs where the CSS Custom Highlight API exists (prod +// Electron), which jsdom/happy-dom lack — so stub a faithful, Electron-shaped +// registry + Highlight to exercise it. The stub Highlight throws if handed any +// constructor argument, catching any accidental `new Highlight(...ranges)` +// spread (which overflows V8's arg stack on large docs). +class StubHighlight { + readonly ranges = new Set() + constructor(...args: Range[]) { + if (args.length > 0) { + throw new RangeError('Maximum call stack size exceeded') + } + } + add(range: Range): void { + this.ranges.add(range) + } +} + +class StubHighlightRegistry { + readonly entries = new Map() + set(name: string, highlight: StubHighlight): void { + this.entries.set(name, highlight) + } + delete(name: string): void { + this.entries.delete(name) + } +} + +describe('markdown preview search painting with the CSS Custom Highlight API', () => { + let registry: StubHighlightRegistry + + beforeEach(() => { + registry = new StubHighlightRegistry() + vi.stubGlobal('Highlight', StubHighlight) + vi.stubGlobal('CSS', { highlights: registry }) + }) + + afterEach(() => { + vi.unstubAllGlobals() + }) + + function bodyWithText(text: string): HTMLElement { + const div = document.createElement('div') + div.className = 'markdown-body' + div.textContent = text + document.body.append(div) + return div + } + + it('paints many matches without a constructor-arg spread', () => { + const instance = {} + const body = bodyWithText('a a a a a a a a') + // A spread paint (new Highlight(...ranges)) would throw via the stub. + expect(() => applyMarkdownPreviewSearchHighlights(instance, body, 'a')).not.toThrow() + expect(registry.entries.get(SEARCH_HIGHLIGHT_NAME)?.ranges.size).toBe(8) + clearMarkdownPreviewSearchHighlights(instance) + body.remove() + }) + + it('unions highlights across previews so a second Find does not clobber the first', () => { + const a = {} + const b = {} + const bodyA = bodyWithText('alpha alpha') + const bodyB = bodyWithText('alpha') + + const matchesA = applyMarkdownPreviewSearchHighlights(a, bodyA, 'alpha') + const matchesB = applyMarkdownPreviewSearchHighlights(b, bodyB, 'alpha') + expect(matchesA.length).toBe(2) + expect(matchesB.length).toBe(1) + // The registry paints the union (A's 2 + B's 1), not just the last writer's. + expect(registry.entries.get(SEARCH_HIGHLIGHT_NAME)?.ranges.size).toBe(3) + + // Closing B keeps A's highlights. + clearMarkdownPreviewSearchHighlights(b) + expect(registry.entries.get(SEARCH_HIGHLIGHT_NAME)?.ranges.size).toBe(2) + + clearMarkdownPreviewSearchHighlights(a) + expect(registry.entries.has(SEARCH_HIGHLIGHT_NAME)).toBe(false) + setActiveMarkdownPreviewSearchMatch(a, matchesA, -1) + bodyA.remove() + bodyB.remove() + }) + + it('navigation repaints only the active highlight, not the full match set', () => { + const instance = {} + const body = bodyWithText('a a a a') + const matches = applyMarkdownPreviewSearchHighlights(instance, body, 'a') + const paintedAfterApply = registry.entries.get(SEARCH_HIGHLIGHT_NAME) + setActiveMarkdownPreviewSearchMatch(instance, matches, 0) + setActiveMarkdownPreviewSearchMatch(instance, matches, 1) + // The SEARCH highlight object is untouched by navigation (not rebuilt each Next/Prev)... + expect(registry.entries.get(SEARCH_HIGHLIGHT_NAME)).toBe(paintedAfterApply) + // ...while the ACTIVE highlight tracks the current match. + expect(registry.entries.get(ACTIVE_SEARCH_HIGHLIGHT_NAME)?.ranges.has(matches[1])).toBe(true) + clearMarkdownPreviewSearchHighlights(instance) + body.remove() + }) + + it('re-apply drops the active highlight until the caller repaints it (same-count rerender)', () => { + const instance = {} + const body = bodyWithText('one two one two') + const first = applyMarkdownPreviewSearchHighlights(instance, body, 'one') + expect(first.length).toBe(2) + setActiveMarkdownPreviewSearchMatch(instance, first, 0) + expect(registry.entries.has(ACTIVE_SEARCH_HIGHLIGHT_NAME)).toBe(true) + + // A streamed rerender / new query re-applies and clears the active range even + // when the match count is unchanged (regression for the vanishing active mark). + const second = applyMarkdownPreviewSearchHighlights(instance, body, 'two') + expect(second.length).toBe(first.length) + expect(registry.entries.has(ACTIVE_SEARCH_HIGHLIGHT_NAME)).toBe(false) + + // MarkdownPreview repaints via its searchRevision effect; the module restores it. + setActiveMarkdownPreviewSearchMatch(instance, second, 0) + expect(registry.entries.get(ACTIVE_SEARCH_HIGHLIGHT_NAME)?.ranges.has(second[0])).toBe(true) + clearMarkdownPreviewSearchHighlights(instance) + body.remove() + }) +}) diff --git a/src/renderer/src/components/editor/markdown-preview-search.ts b/src/renderer/src/components/editor/markdown-preview-search.ts index aeaa55df256..1b92f958fa8 100644 --- a/src/renderer/src/components/editor/markdown-preview-search.ts +++ b/src/renderer/src/components/editor/markdown-preview-search.ts @@ -166,92 +166,170 @@ function buildLocaleLowercaseIndex(text: string): { return { text: normalized, originalStartByNormalizedOffset, originalEndByNormalizedOffset } } -export function clearMarkdownPreviewSearchHighlights(root: HTMLElement): void { - const highlights = root.querySelectorAll('[data-markdown-preview-search-match]') - for (const highlight of highlights) { - const textNode = document.createTextNode(highlight.textContent ?? '') - highlight.replaceWith(textNode) +// Why: react-markdown owns the preview DOM. Injecting by splitting its +// text nodes (and normalize()-merging them on clear) left react holding stale +// child pointers, so the next streamed-content commit threw NotFoundError +// ("insertBefore ... not a child of this node"; crash 237acef1). Paint matches +// with the CSS Custom Highlight API instead — it highlights Ranges without +// mutating the DOM react manages. The static names below must match the +// ::highlight() selectors in markdown-preview.css. +const SEARCH_HIGHLIGHT_NAME = 'markdown-preview-search-match' +const ACTIVE_SEARCH_HIGHLIGHT_NAME = 'markdown-preview-search-active-match' + +type HighlightLike = { add(range: Range): void } +type HighlightRegistryLike = { + set(name: string, highlight: HighlightLike): void + delete(name: string): void +} + +// Accessed via globalThis so the code degrades to a no-op where the API is +// absent (older Chromium, jsdom/happy-dom in tests) — match counting and +// navigation still work off the returned Ranges; only the paint is skipped. +function getHighlightApi(): { + registry: HighlightRegistryLike + create: (ranges: readonly Range[]) => HighlightLike +} | null { + const scope = globalThis as { + CSS?: { highlights?: HighlightRegistryLike } + Highlight?: new () => HighlightLike + } + const registry = scope.CSS?.highlights + const HighlightCtor = scope.Highlight + if (!registry || typeof HighlightCtor !== 'function') { + return null + } + return { + registry, + // Why: build with .add() rather than new Highlight(...ranges). A big doc + + // short query yields 100k+ ranges, and spreading that many constructor + // args overflows V8's argument stack (RangeError) — the same large-content + // regime as the bug this file fixes. + create: (ranges) => { + const highlight = new HighlightCtor() + for (const range of ranges) { + highlight.add(range) + } + return highlight + } + } +} + +// Why: CSS.highlights is a document-global registry keyed by a static name, but +// several MarkdownPreview instances can be open at once (split panes, floating +// window). Track each instance's ranges by its own token and paint the UNION, +// so a second preview's Find does not clobber the first's highlights. Ranges +// live in each instance's own subtree, so the union paints every pane correctly. +const searchRangesByInstance = new Map() +const activeRangeByInstance = new Map() + +// Avoid array spread when collecting union ranges — a large doc can produce +// 100k+ ranges and create()/registry writes must not build variadic arg lists. +function paintMatchHighlight(api: NonNullable>): void { + const matchRanges: Range[] = [] + for (const ranges of searchRangesByInstance.values()) { + for (const range of ranges) { + matchRanges.push(range) + } + } + if (matchRanges.length > 0) { + api.registry.set(SEARCH_HIGHLIGHT_NAME, api.create(matchRanges)) + } else { + api.registry.delete(SEARCH_HIGHLIGHT_NAME) + } +} + +function paintActiveHighlight(api: NonNullable>): void { + const activeRanges: Range[] = [] + for (const range of activeRangeByInstance.values()) { + activeRanges.push(range) + } + if (activeRanges.length > 0) { + api.registry.set(ACTIVE_SEARCH_HIGHLIGHT_NAME, api.create(activeRanges)) + } else { + api.registry.delete(ACTIVE_SEARCH_HIGHLIGHT_NAME) + } +} + +export function clearMarkdownPreviewSearchHighlights(instanceId: object): void { + searchRangesByInstance.delete(instanceId) + activeRangeByInstance.delete(instanceId) + const api = getHighlightApi() + if (api) { + paintMatchHighlight(api) + paintActiveHighlight(api) } - root.normalize() } export function applyMarkdownPreviewSearchHighlights( + instanceId: object, root: HTMLElement, query: string -): HTMLElement[] { - clearMarkdownPreviewSearchHighlights(root) +): Range[] { + const ranges: Range[] = [] - if (!query || isMarkdownPreviewSearchQueryTooLarge(query)) { - return [] + if (query && !isMarkdownPreviewSearchQueryTooLarge(query)) { + const walker = document.createTreeWalker(root, NodeFilter.SHOW_TEXT, { + acceptNode(node) { + if (!(node.parentElement instanceof HTMLElement)) { + return NodeFilter.FILTER_REJECT + } + if (!node.textContent?.trim()) { + return NodeFilter.FILTER_REJECT + } + return NodeFilter.FILTER_ACCEPT + } + }) + + let currentNode = walker.nextNode() + while (currentNode) { + if (currentNode instanceof Text) { + const text = currentNode.textContent ?? '' + // findTextMatchRanges returns offsets into the original text, so they + // map straight onto this Text node without any DOM rewrite. + for (const { start, end } of findTextMatchRanges(text, query)) { + const range = document.createRange() + range.setStart(currentNode, start) + range.setEnd(currentNode, end) + ranges.push(range) + } + } + currentNode = walker.nextNode() + } } - const walker = document.createTreeWalker(root, NodeFilter.SHOW_TEXT, { - acceptNode(node) { - if (!(node.parentElement instanceof HTMLElement)) { - return NodeFilter.FILTER_REJECT - } - if (node.parentElement.closest('[data-markdown-preview-search-match]')) { - return NodeFilter.FILTER_REJECT - } - if (!node.textContent?.trim()) { - return NodeFilter.FILTER_REJECT - } - return NodeFilter.FILTER_ACCEPT - } - }) - - const textNodes: Text[] = [] - let currentNode = walker.nextNode() - while (currentNode) { - if (currentNode instanceof Text) { - textNodes.push(currentNode) - } - currentNode = walker.nextNode() + searchRangesByInstance.set(instanceId, ranges) + activeRangeByInstance.delete(instanceId) + const api = getHighlightApi() + if (api) { + paintMatchHighlight(api) + paintActiveHighlight(api) } - const matches: HTMLElement[] = [] - for (const textNode of textNodes) { - const text = textNode.textContent ?? '' - const ranges = findTextMatchRanges(text, query) - if (ranges.length === 0) { - continue - } - - const fragment = document.createDocumentFragment() - let cursor = 0 - for (const range of ranges) { - if (range.start > cursor) { - fragment.append(document.createTextNode(text.slice(cursor, range.start))) - } - - const highlight = document.createElement('mark') - highlight.dataset.markdownPreviewSearchMatch = 'true' - highlight.className = 'markdown-preview-search-match' - highlight.textContent = text.slice(range.start, range.end) - fragment.append(highlight) - matches.push(highlight) - cursor = range.end - } - - if (cursor < text.length) { - fragment.append(document.createTextNode(text.slice(cursor))) - } - - textNode.replaceWith(fragment) - } - - return matches + return ranges } export function setActiveMarkdownPreviewSearchMatch( - matches: readonly HTMLElement[], + instanceId: object, + matches: readonly Range[], activeIndex: number ): void { - for (const [index, match] of matches.entries()) { - const isActive = index === activeIndex - match.toggleAttribute('data-active', isActive) - if (isActive) { - match.scrollIntoView({ block: 'center', inline: 'nearest' }) - } + const active = activeIndex >= 0 ? matches[activeIndex] : undefined + + if (active) { + activeRangeByInstance.set(instanceId, active) + } else { + activeRangeByInstance.delete(instanceId) + } + + const api = getHighlightApi() + if (api) { + // Only the active range changed — don't rebuild the (potentially 100k-range) + // match highlight on every Next/Prev navigation. + paintActiveHighlight(api) + } + + if (active) { + // The Range's start container is a Text node; scroll its element into view. + active.startContainer.parentElement?.scrollIntoView({ block: 'center', inline: 'nearest' }) } }