Skip to content

Commit 69e3f85

Browse files
jiveystaxly[bot]
andauthored
fix delete highlight crash when search term matches highlight (#2796)
* fix delete highlight crash when search term matches highlight * fix type * fix highlighter errors when highlighting inside search highlight * Revert "fix delete highlight crash when search term matches highlight" This reverts commit 52d285d. * recreate the highlighter if it crashes while erasing * add coverage * just use services --------- Co-authored-by: staxly[bot] <35789409+staxly[bot]@users.noreply.github.com>
1 parent 3f89054 commit 69e3f85

2 files changed

Lines changed: 50 additions & 7 deletions

File tree

src/app/content/components/Page/searchHighlightManager.spec.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,4 +107,29 @@ describe('searchHighlightManager', () => {
107107
options.formatMessage({ id: 'asdfg' });
108108
expect(intl.formatMessage).toHaveBeenCalledWith({ id: 'asdfg:search' });
109109
});
110+
111+
it('recreates highlighter when eraseAll throws', () => {
112+
const firstHighlighterInstance = Highlighter.mock.instances[0];
113+
firstHighlighterInstance.eraseAll.mockImplementation(() => { throw new Error('orphaned span'); });
114+
firstHighlighterInstance.unmount = jest.fn();
115+
116+
const newSearchResults = [
117+
makeSearchResultHit({book, page, highlights: ['highlight <strong>number</strong> 4']}),
118+
];
119+
120+
attachedManager.update(
121+
{searchResults, selectedResult: null},
122+
{searchResults: newSearchResults, selectedResult: null},
123+
{forceRedraw: false, onSelect: onHighlightSelect}
124+
);
125+
126+
expect(firstHighlighterInstance.unmount).toHaveBeenCalled();
127+
// A new Highlighter instance should have been created to replace the broken one
128+
expect(Highlighter).toHaveBeenCalledTimes(2);
129+
// The replacement highlighter should be used for subsequent highlightResults calls
130+
expect(utils.highlightResults).toHaveBeenCalledWith(
131+
Highlighter.mock.instances[1],
132+
newSearchResults
133+
);
134+
});
110135
});

src/app/content/components/Page/searchHighlightManager.ts

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import allImagesLoaded from '../utils/allImagesLoaded';
1212
interface Services {
1313
highlighter: Highlighter;
1414
container: HTMLElement;
15+
intl: IntlShape;
1516
searchResultMap: ReturnType<typeof highlightResults>;
1617
}
1718

@@ -33,6 +34,19 @@ export interface UpdateOptions {
3334
onSelect: (selectedHighlight?: Highlight) => void;
3435
}
3536

37+
const safeEraseAll = (services: Services) => {
38+
try {
39+
services.highlighter.eraseAll();
40+
} catch {
41+
// User highlight DOM mutations can orphan search highlight spans (parentNode
42+
// becomes null), making eraseAll crash on insertBefore. When that happens,
43+
// discard the broken instance and create a fresh one — the orphaned spans
44+
// are already detached from the DOM so there is nothing left to unwrap.
45+
services.highlighter.unmount();
46+
services.highlighter = createHighlighter(services.container, services.intl);
47+
}
48+
};
49+
3650
const updateResults = (
3751
services: Services,
3852
previous: HighlightProp | null,
@@ -42,7 +56,7 @@ const updateResults = (
4256
return;
4357
}
4458

45-
services.highlighter.eraseAll();
59+
safeEraseAll(services);
4660
services.searchResultMap = highlightResults(services.highlighter, current.searchResults);
4761
};
4862

@@ -99,14 +113,18 @@ const handleUpdate = (services: Services) => (
99113
selectResult(services, previous, current, options);
100114
};
101115

116+
const createHighlighter = (container: HTMLElement, intl: IntlShape) =>
117+
new Highlighter(container, {
118+
className: 'search-highlight',
119+
formatMessage: ({ id }) => intl.formatMessage({ id: `${id}:search` }),
120+
tabbable: false,
121+
});
122+
102123
const searchHighlightManager = (container: HTMLElement, intl: IntlShape) => {
103-
const services = {
124+
const services: Services = {
104125
container,
105-
highlighter: new Highlighter(container, {
106-
className: 'search-highlight',
107-
formatMessage: ({ id }) => intl.formatMessage({ id: `${id}:search` }),
108-
tabbable: false,
109-
}),
126+
highlighter: createHighlighter(container, intl),
127+
intl,
110128
searchResultMap: [],
111129
};
112130

0 commit comments

Comments
 (0)