From e7014492c5e32ae595b1bfb256398385bc662d9c Mon Sep 17 00:00:00 2001 From: Henry Heino <46334387+personalizedrefrigerator@users.noreply.github.com> Date: Fri, 18 Aug 2023 01:31:45 -0700 Subject: [PATCH] Desktop: Fixes #8661: Fix note editor blank after syncing an encrypted note with remote changes (#8666) --- .eslintignore | 1 + .gitignore | 1 + .../app-desktop/gui/NoteEditor/NoteEditor.tsx | 2 + .../app-desktop/gui/NoteEditor/utils/types.ts | 1 + .../gui/NoteEditor/utils/useFormNote.test.ts | 73 +++++++++++++++++++ .../gui/NoteEditor/utils/useFormNote.ts | 36 ++++++--- packages/app-desktop/jest.setup.js | 38 ++++++---- packages/lib/testing/test-utils.ts | 9 ++- 8 files changed, 136 insertions(+), 25 deletions(-) create mode 100644 packages/app-desktop/gui/NoteEditor/utils/useFormNote.test.ts diff --git a/.eslintignore b/.eslintignore index 0d0a1776b..417fd73f6 100644 --- a/.eslintignore +++ b/.eslintignore @@ -260,6 +260,7 @@ packages/app-desktop/gui/NoteEditor/utils/types.js packages/app-desktop/gui/NoteEditor/utils/useDropHandler.js packages/app-desktop/gui/NoteEditor/utils/useEffectiveNoteId.js packages/app-desktop/gui/NoteEditor/utils/useFolder.js +packages/app-desktop/gui/NoteEditor/utils/useFormNote.test.js packages/app-desktop/gui/NoteEditor/utils/useFormNote.js packages/app-desktop/gui/NoteEditor/utils/useMarkupToHtml.js packages/app-desktop/gui/NoteEditor/utils/useMessageHandler.js diff --git a/.gitignore b/.gitignore index 08b45a60b..3c88e2abd 100644 --- a/.gitignore +++ b/.gitignore @@ -246,6 +246,7 @@ packages/app-desktop/gui/NoteEditor/utils/types.js packages/app-desktop/gui/NoteEditor/utils/useDropHandler.js packages/app-desktop/gui/NoteEditor/utils/useEffectiveNoteId.js packages/app-desktop/gui/NoteEditor/utils/useFolder.js +packages/app-desktop/gui/NoteEditor/utils/useFormNote.test.js packages/app-desktop/gui/NoteEditor/utils/useFormNote.js packages/app-desktop/gui/NoteEditor/utils/useMarkupToHtml.js packages/app-desktop/gui/NoteEditor/utils/useMessageHandler.js diff --git a/packages/app-desktop/gui/NoteEditor/NoteEditor.tsx b/packages/app-desktop/gui/NoteEditor/NoteEditor.tsx index 3feeffafa..890f1865a 100644 --- a/packages/app-desktop/gui/NoteEditor/NoteEditor.tsx +++ b/packages/app-desktop/gui/NoteEditor/NoteEditor.tsx @@ -78,6 +78,7 @@ function NoteEditor(props: NoteEditorProps) { const { formNote, setFormNote, isNewNote, resourceInfos } = useFormNote({ syncStarted: props.syncStarted, + decryptionStarted: props.decryptionStarted, noteId: effectiveNoteId, isProvisional: props.isProvisional, titleInputRef: titleInputRef, @@ -633,6 +634,7 @@ const mapStateToProps = (state: AppState) => { isProvisional: state.provisionalNoteIds.includes(noteId), editorNoteStatuses: state.editorNoteStatuses, syncStarted: state.syncStarted, + decryptionStarted: state.decryptionWorker?.state !== 'idle', themeId: state.settings.theme, richTextBannerDismissed: state.settings.richTextBannerDismissed, watchedNoteFiles: state.watchedNoteFiles, diff --git a/packages/app-desktop/gui/NoteEditor/utils/types.ts b/packages/app-desktop/gui/NoteEditor/utils/types.ts index be4f83313..7634331a3 100644 --- a/packages/app-desktop/gui/NoteEditor/utils/types.ts +++ b/packages/app-desktop/gui/NoteEditor/utils/types.ts @@ -27,6 +27,7 @@ export interface NoteEditorProps { isProvisional: boolean; editorNoteStatuses: any; syncStarted: boolean; + decryptionStarted: boolean; bodyEditor: string; notesParentType: string; selectedNoteTags: any[]; diff --git a/packages/app-desktop/gui/NoteEditor/utils/useFormNote.test.ts b/packages/app-desktop/gui/NoteEditor/utils/useFormNote.test.ts new file mode 100644 index 000000000..bb6220939 --- /dev/null +++ b/packages/app-desktop/gui/NoteEditor/utils/useFormNote.test.ts @@ -0,0 +1,73 @@ +import Note from '@joplin/lib/models/Note'; +import { setupDatabaseAndSynchronizer, switchClient } from '@joplin/lib/testing/test-utils'; +import { renderHook } from '@testing-library/react-hooks'; +import useFormNote, { HookDependencies } from './useFormNote'; + + +describe('useFormNote', () => { + beforeEach(async () => { + await setupDatabaseAndSynchronizer(1); + await switchClient(1); + }); + + it('should update note when decryption completes', async () => { + const testNote = await Note.save({ title: 'Test Note!' }); + + const makeFormNoteProps = (syncStarted: boolean, decryptionStarted: boolean): HookDependencies => { + return { + syncStarted, + decryptionStarted, + noteId: testNote.id, + isProvisional: false, + titleInputRef: null, + editorRef: null, + onBeforeLoad: ()=>{}, + onAfterLoad: ()=>{}, + }; + }; + + const formNote = renderHook(props => useFormNote(props), { + initialProps: makeFormNoteProps(true, false), + }); + await formNote.waitFor(() => { + expect(formNote.result.current.formNote).toMatchObject({ + encryption_applied: 0, + title: testNote.title, + }); + }); + + await Note.save({ + id: testNote.id, + encryption_cipher_text: 'cipher_text', + encryption_applied: 1, + }); + + // Sync starting should cause a re-render + formNote.rerender(makeFormNoteProps(false, false)); + + await formNote.waitFor(() => { + expect(formNote.result.current.formNote).toMatchObject({ + encryption_applied: 1, + }); + }); + + + formNote.rerender(makeFormNoteProps(false, true)); + + await Note.save({ + id: testNote.id, + encryption_applied: 0, + title: 'Test Note!', + }); + + // Ending decryption should also cause a re-render + formNote.rerender(makeFormNoteProps(false, false)); + + await formNote.waitFor(() => { + expect(formNote.result.current.formNote).toMatchObject({ + encryption_applied: 0, + title: 'Test Note!', + }); + }); + }); +}); diff --git a/packages/app-desktop/gui/NoteEditor/utils/useFormNote.ts b/packages/app-desktop/gui/NoteEditor/utils/useFormNote.ts index 53eb717a9..96a330dc8 100644 --- a/packages/app-desktop/gui/NoteEditor/utils/useFormNote.ts +++ b/packages/app-desktop/gui/NoteEditor/utils/useFormNote.ts @@ -18,8 +18,9 @@ export interface OnLoadEvent { formNote: FormNote; } -interface HookDependencies { +export interface HookDependencies { syncStarted: boolean; + decryptionStarted: boolean; noteId: string; isProvisional: boolean; titleInputRef: any; @@ -61,15 +62,21 @@ function resourceInfosChanged(a: ResourceInfos, b: ResourceInfos): boolean { } export default function useFormNote(dependencies: HookDependencies) { - const { syncStarted, noteId, isProvisional, titleInputRef, editorRef, onBeforeLoad, onAfterLoad } = dependencies; + const { + syncStarted, decryptionStarted, noteId, isProvisional, titleInputRef, editorRef, onBeforeLoad, onAfterLoad, + } = dependencies; const [formNote, setFormNote] = useState(defaultFormNote()); - const [formNoteRefeshScheduled, setFormNoteRefreshScheduled] = useState(false); const [isNewNote, setIsNewNote] = useState(false); const prevSyncStarted = usePrevious(syncStarted); + const prevDecryptionStarted = usePrevious(decryptionStarted); const previousNoteId = usePrevious(formNote.id); const [resourceInfos, setResourceInfos] = useState({}); + // Increasing the value of this counter cancels any ongoing note refreshes and starts + // a new refresh. + const [formNoteRefeshScheduled, setFormNoteRefreshScheduled] = useState(0); + async function initNoteState(n: any) { let originalCss = ''; @@ -107,7 +114,7 @@ export default function useFormNote(dependencies: HookDependencies) { } useEffect(() => { - if (!formNoteRefeshScheduled) return () => {}; + if (formNoteRefeshScheduled <= 0) return () => {}; reg.logger().info('Sync has finished and note has never been changed - reloading it'); @@ -126,7 +133,7 @@ export default function useFormNote(dependencies: HookDependencies) { } await initNoteState(n); - setFormNoteRefreshScheduled(false); + setFormNoteRefreshScheduled(0); }; void loadNote(); @@ -136,21 +143,32 @@ export default function useFormNote(dependencies: HookDependencies) { }; }, [formNoteRefeshScheduled, noteId]); + const refreshFormNote = useCallback(() => { + // Increase the counter to cancel any ongoing refresh attempts + // and start a new one. + setFormNoteRefreshScheduled(formNoteRefeshScheduled + 1); + }, [formNoteRefeshScheduled]); + useEffect(() => { // Check that synchronisation has just finished - and // if the note has never been changed, we reload it. // If the note has already been changed, it's a conflict // that's already been handled by the synchronizer. + const decryptionJustEnded = prevDecryptionStarted && !decryptionStarted; + const syncJustEnded = prevSyncStarted && !syncStarted; - if (!prevSyncStarted) return; - if (syncStarted) return; + if (!decryptionJustEnded && !syncJustEnded) return; if (formNote.hasChanged) return; // Refresh the form note. // This is kept separate from the above logic so that when prevSyncStarted is changed // from true to false, it doesn't cancel the note from loading. - setFormNoteRefreshScheduled(true); - }, [prevSyncStarted, syncStarted, formNote.hasChanged]); + refreshFormNote(); + }, [ + prevSyncStarted, syncStarted, + prevDecryptionStarted, decryptionStarted, + formNote.hasChanged, refreshFormNote, + ]); useEffect(() => { if (!noteId) { diff --git a/packages/app-desktop/jest.setup.js b/packages/app-desktop/jest.setup.js index 16b07e902..7f4c8e23c 100644 --- a/packages/app-desktop/jest.setup.js +++ b/packages/app-desktop/jest.setup.js @@ -1,19 +1,14 @@ +/* eslint-disable jest/require-top-level-describe */ -const { default: Logger, TargetType } = require('@joplin/utils/Logger'); -const initLib = require('@joplin/lib/initLib').default; - -// TODO: Some libraries required by test-utils.js seem to fail to import with the -// jsdom environment. -// -// Thus, require('@joplin/lib/testing/test-utils.js') fails and some setup must be -// copied. - -const logger = new Logger(); -logger.addTarget(TargetType.Console); -logger.setLevel(Logger.LEVEL_WARN); -Logger.initializeGlobalLogger(logger); -initLib(logger); +const { shimInit } = require('@joplin/lib/shim-init-node'); +const sqlite3 = require('sqlite3'); +const SyncTargetNone = require('@joplin/lib/SyncTargetNone').default; +// Mock the S3 sync target -- the @aws-s3 libraries depend on an old version +// of uuid that doesn't work with jest without additional configuration. +jest.doMock('@joplin/lib/SyncTargetAmazonS3', () => { + return SyncTargetNone; +}); // @electron/remote requires electron to be running. Mock it. jest.mock('@electron/remote', () => { @@ -25,3 +20,18 @@ jest.mock('@electron/remote', () => { }, }; }); + +// Import after mocking problematic libraries +const { afterEachCleanUp, afterAllCleanUp } = require('@joplin/lib/testing/test-utils.js'); + + +shimInit({ nodeSqlite: sqlite3 }); + +afterEach(async () => { + await afterEachCleanUp(); +}); + +afterAll(async () => { + await afterAllCleanUp(); +}); + diff --git a/packages/lib/testing/test-utils.ts b/packages/lib/testing/test-utils.ts index 00c285dc2..732fd6c9f 100644 --- a/packages/lib/testing/test-utils.ts +++ b/packages/lib/testing/test-utils.ts @@ -34,7 +34,6 @@ const { FileApiDriverLocal } = require('../file-api-driver-local'); const { FileApiDriverWebDav } = require('../file-api-driver-webdav.js'); const { FileApiDriverDropbox } = require('../file-api-driver-dropbox.js'); const { FileApiDriverOneDrive } = require('../file-api-driver-onedrive.js'); -const { FileApiDriverAmazonS3 } = require('../file-api-driver-amazon-s3.js'); import SyncTargetRegistry from '../SyncTargetRegistry'; const SyncTargetMemory = require('../SyncTargetMemory.js'); const SyncTargetFilesystem = require('../SyncTargetFilesystem.js'); @@ -60,7 +59,6 @@ import Synchronizer from '../Synchronizer'; import SyncTargetNone from '../SyncTargetNone'; import { setRSA } from '../services/e2ee/ppk'; const md5 = require('md5'); -const { S3Client } = require('@aws-sdk/client-s3'); const { Dirnames } = require('../services/synchronizer/utils/types'); import RSA from '../services/e2ee/RSA.node'; import { State as ShareState } from '../services/share/reducer'; @@ -627,6 +625,13 @@ async function initFileApi() { const appDir = await api.appDirectory(); fileApi = new FileApi(appDir, new FileApiDriverOneDrive(api)); } else if (syncTargetId_ === SyncTargetRegistry.nameToId('amazon_s3')) { + // (Most of?) the @aws-sdk libraries depend on an old version of uuid + // that doesn't work with jest (without converting ES6 exports to CommonJS). + // + // Require it dynamically so that this doesn't break test environments that + // aren't configured to do this conversion. + const { FileApiDriverAmazonS3 } = require('../file-api-driver-amazon-s3.js'); + const { S3Client } = require('@aws-sdk/client-s3'); // We make sure for S3 tests run in band because tests // share the same directory which will cause locking errors.