From eda2c69334a84438ffc4ea8a06a55bc90c1ca78a Mon Sep 17 00:00:00 2001 From: Henry Heino <46334387+personalizedrefrigerator@users.noreply.github.com> Date: Fri, 27 Sep 2024 07:25:55 -0700 Subject: [PATCH] Desktop: Fixes #11129: Improve performance by allowing note list background timers to be cancelled (#11133) --- .eslintignore | 1 + .gitignore | 1 + .../NoteListItem/utils/useRootElement.test.ts | 55 +++++++++++++++++++ .../gui/NoteListItem/utils/useRootElement.ts | 2 +- packages/app-desktop/jest.setup.js | 4 +- packages/lib/dom.ts | 6 +- 6 files changed, 64 insertions(+), 5 deletions(-) create mode 100644 packages/app-desktop/gui/NoteListItem/utils/useRootElement.test.ts diff --git a/.eslintignore b/.eslintignore index 69e7bd397..a4fa65b1b 100644 --- a/.eslintignore +++ b/.eslintignore @@ -374,6 +374,7 @@ packages/app-desktop/gui/NoteListItem/utils/useItemElement.js packages/app-desktop/gui/NoteListItem/utils/useItemEventHandlers.js packages/app-desktop/gui/NoteListItem/utils/useOnContextMenu.js packages/app-desktop/gui/NoteListItem/utils/useRenderedNote.js +packages/app-desktop/gui/NoteListItem/utils/useRootElement.test.js packages/app-desktop/gui/NoteListItem/utils/useRootElement.js packages/app-desktop/gui/NoteListWrapper/NoteListWrapper.js packages/app-desktop/gui/NotePropertiesDialog.js diff --git a/.gitignore b/.gitignore index cba79acff..3698426c8 100644 --- a/.gitignore +++ b/.gitignore @@ -351,6 +351,7 @@ packages/app-desktop/gui/NoteListItem/utils/useItemElement.js packages/app-desktop/gui/NoteListItem/utils/useItemEventHandlers.js packages/app-desktop/gui/NoteListItem/utils/useOnContextMenu.js packages/app-desktop/gui/NoteListItem/utils/useRenderedNote.js +packages/app-desktop/gui/NoteListItem/utils/useRootElement.test.js packages/app-desktop/gui/NoteListItem/utils/useRootElement.js packages/app-desktop/gui/NoteListWrapper/NoteListWrapper.js packages/app-desktop/gui/NotePropertiesDialog.js diff --git a/packages/app-desktop/gui/NoteListItem/utils/useRootElement.test.ts b/packages/app-desktop/gui/NoteListItem/utils/useRootElement.test.ts new file mode 100644 index 000000000..e6b99053f --- /dev/null +++ b/packages/app-desktop/gui/NoteListItem/utils/useRootElement.test.ts @@ -0,0 +1,55 @@ +import { act, renderHook } from '@testing-library/react-hooks'; +import useRootElement from './useRootElement'; + +describe('useRootElement', () => { + beforeEach(() => { + jest.useFakeTimers({ advanceTimers: true }); + }); + + test('should find an element with a matching ID', async () => { + const testElement = document.createElement('div'); + testElement.id = 'test-element-id'; + document.body.appendChild(testElement); + + const { result } = renderHook(useRootElement, { + initialProps: testElement.id, + }); + await act(async () => { + await jest.advanceTimersByTimeAsync(100); + }); + expect(result.current).toBe(testElement); + + testElement.remove(); + }); + + test('should redo the element search when the elementId prop changes', async () => { + const testElement = document.createElement('div'); + document.body.appendChild(testElement); + + const { rerender, result } = renderHook(useRootElement, { + initialProps: 'some-id-here', + }); + await jest.advanceTimersByTimeAsync(100); + expect(result.current).toBe(null); + + // Searching for another non-existent ID: Should not match + rerender('updated-id'); + await jest.advanceTimersByTimeAsync(100); + expect(result.current).toBe(null); + + // Should not match the first element if its ID is set to the original (search + // should be cancelled). + testElement.id = 'some-id-here'; + await jest.advanceTimersByTimeAsync(100); + expect(result.current).toBe(null); + + // Should match if the element ID changes to the updated ID. + await act(async () => { + testElement.id = 'updated-id'; + await jest.advanceTimersByTimeAsync(100); + }); + expect(result.current).toBe(testElement); + + testElement.remove(); + }); +}); diff --git a/packages/app-desktop/gui/NoteListItem/utils/useRootElement.ts b/packages/app-desktop/gui/NoteListItem/utils/useRootElement.ts index 878b0b3ca..6683cd585 100644 --- a/packages/app-desktop/gui/NoteListItem/utils/useRootElement.ts +++ b/packages/app-desktop/gui/NoteListItem/utils/useRootElement.ts @@ -6,7 +6,7 @@ const useRootElement = (elementId: string) => { const [rootElement, setRootElement] = useState(null); useAsyncEffect(async (event) => { - const element = await waitForElement(document, elementId); + const element = await waitForElement(document, elementId, event); if (event.cancelled) return; setRootElement(element); }, [document, elementId]); diff --git a/packages/app-desktop/jest.setup.js b/packages/app-desktop/jest.setup.js index 9446f6dde..bafc0cc6f 100644 --- a/packages/app-desktop/jest.setup.js +++ b/packages/app-desktop/jest.setup.js @@ -24,9 +24,9 @@ jest.mock('@electron/remote', () => { // Import after mocking problematic libraries const { afterEachCleanUp, afterAllCleanUp } = require('@joplin/lib/testing/test-utils.js'); +const React = require('react'); - -shimInit({ nodeSqlite: sqlite3 }); +shimInit({ nodeSqlite: sqlite3, React }); afterEach(async () => { await afterEachCleanUp(); diff --git a/packages/lib/dom.ts b/packages/lib/dom.ts index 75a939f82..e05547d00 100644 --- a/packages/lib/dom.ts +++ b/packages/lib/dom.ts @@ -7,13 +7,15 @@ export const isInsideContainer = (node: any, className: string): boolean => { return false; }; +interface CancelEvent { cancelled: boolean } + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied -export const waitForElement = async (parent: any, id: string): Promise => { +export const waitForElement = async (parent: any, id: string, cancelEvent?: CancelEvent): Promise => { return new Promise((resolve, reject) => { const iid = setInterval(() => { try { const element = parent.getElementById(id); - if (element) { + if (element || cancelEvent?.cancelled) { clearInterval(iid); resolve(element); }