From bd5ccbfab6a4cc045b05e0730a1f5f04f7f65f8e Mon Sep 17 00:00:00 2001
From: Mo <mo@standardnotes.com>
Date: Mon, 5 Dec 2022 07:53:21 -0600
Subject: [PATCH] refactor: avoid deallocing note controller until local
 propagation complete

---
 .../Controller/NoteViewController.spec.ts     | 36 ++++++++++++++++++-
 .../NoteView/Controller/NoteViewController.ts | 31 ++++++++++++----
 2 files changed, 59 insertions(+), 8 deletions(-)

diff --git a/packages/web/src/javascripts/Components/NoteView/Controller/NoteViewController.spec.ts b/packages/web/src/javascripts/Components/NoteView/Controller/NoteViewController.spec.ts
index 3deb55ab9..6792ab92d 100644
--- a/packages/web/src/javascripts/Components/NoteView/Controller/NoteViewController.spec.ts
+++ b/packages/web/src/javascripts/Components/NoteView/Controller/NoteViewController.spec.ts
@@ -7,6 +7,8 @@ import {
   SNTag,
   ItemsClientInterface,
   SNNote,
+  Deferred,
+  SyncServiceInterface,
 } from '@standardnotes/snjs'
 import { FeatureIdentifier, NoteType } from '@standardnotes/features'
 import { NoteViewController } from './NoteViewController'
@@ -17,10 +19,16 @@ describe('note view controller', () => {
 
   beforeEach(() => {
     application = {} as jest.Mocked<WebApplication>
-    application.streamItems = jest.fn()
+    application.streamItems = jest.fn().mockReturnValue(() => {})
     application.getPreference = jest.fn().mockReturnValue(true)
+    application.noAccount = jest.fn().mockReturnValue(false)
+    application.isNativeMobileWeb = jest.fn().mockReturnValue(false)
+
     Object.defineProperty(application, 'items', { value: {} as jest.Mocked<ItemsClientInterface> })
 
+    Object.defineProperty(application, 'sync', { value: {} as jest.Mocked<SyncServiceInterface> })
+    application.sync.sync = jest.fn().mockReturnValue(Promise.resolve())
+
     componentManager = {} as jest.Mocked<SNComponentManager>
     componentManager.legacyGetDefaultEditor = jest.fn()
     Object.defineProperty(application, 'componentManager', { value: componentManager })
@@ -80,4 +88,30 @@ describe('note view controller', () => {
     expect(controller['defaultTag']).toEqual(tag)
     expect(application.items.addTagToNote).toHaveBeenCalledWith(expect.anything(), tag, expect.anything())
   })
+
+  it('should wait until item finishes saving locally before deiniting', async () => {
+    const note = {
+      uuid: 'note-uuid',
+    } as jest.Mocked<SNNote>
+
+    application.items.findItem = jest.fn().mockReturnValue(note)
+
+    const controller = new NoteViewController(application, note)
+    await controller.initialize()
+
+    const changePromise = Deferred()
+
+    application.mutator.changeItem = jest.fn().mockReturnValue(changePromise.promise)
+
+    const savePromise = controller.saveAndAwaitLocalPropagation({ isUserModified: true, bypassDebouncer: true })
+    controller.deinit()
+
+    expect(controller.dealloced).toEqual(false)
+
+    changePromise.resolve(true)
+    await changePromise.promise
+    await savePromise
+
+    expect(controller.dealloced).toEqual(true)
+  })
 })
diff --git a/packages/web/src/javascripts/Components/NoteView/Controller/NoteViewController.ts b/packages/web/src/javascripts/Components/NoteView/Controller/NoteViewController.ts
index 94138d18e..7f8fb83ad 100644
--- a/packages/web/src/javascripts/Components/NoteView/Controller/NoteViewController.ts
+++ b/packages/web/src/javascripts/Components/NoteView/Controller/NoteViewController.ts
@@ -11,7 +11,7 @@ import {
   PrefKey,
 } from '@standardnotes/models'
 import { UuidString } from '@standardnotes/snjs'
-import { removeFromArray } from '@standardnotes/utils'
+import { removeFromArray, Deferred } from '@standardnotes/utils'
 import { ContentType } from '@standardnotes/common'
 import { ItemViewControllerInterface } from './ItemViewControllerInterface'
 import { TemplateNoteViewControllerOptions } from './TemplateNoteViewControllerOptions'
@@ -29,14 +29,16 @@ const NotePreviewCharLimit = 160
 export class NoteViewController implements ItemViewControllerInterface {
   public item!: SNNote
   public dealloced = false
+  public isTemplateNote = false
+  public runtimeId = `${Math.random()}`
+  public needsInit = true
+
   private innerValueChangeObservers: ((note: SNNote, source: PayloadEmitSource) => void)[] = []
   private disposers: (() => void)[] = []
-  public isTemplateNote = false
   private saveTimeout?: ReturnType<typeof setTimeout>
   private defaultTagUuid: UuidString | undefined
   private defaultTag?: SNTag
-  public runtimeId = `${Math.random()}`
-  public needsInit = true
+  private savingLocallyPromise = Deferred<void>()
 
   constructor(
     private application: WebApplication,
@@ -57,7 +59,14 @@ export class NoteViewController implements ItemViewControllerInterface {
   }
 
   deinit(): void {
+    void this.savingLocallyPromise.promise.then(() => {
+      this.performDeinitSafely()
+    })
+  }
+
+  private performDeinitSafely(): void {
     this.dealloced = true
+
     for (const disposer of this.disposers) {
       disposer()
     }
@@ -184,6 +193,8 @@ export class NoteViewController implements ItemViewControllerInterface {
       throw Error('NoteViewController not initialized')
     }
 
+    this.savingLocallyPromise = Deferred<void>()
+
     if (this.saveTimeout) {
       clearTimeout(this.saveTimeout)
     }
@@ -198,7 +209,13 @@ export class NoteViewController implements ItemViewControllerInterface {
 
     return new Promise((resolve) => {
       this.saveTimeout = setTimeout(() => {
-        void this.undebouncedSave({ ...params, onLocalPropagationComplete: resolve })
+        void this.undebouncedSave({
+          ...params,
+          onLocalPropagationComplete: () => {
+            this.savingLocallyPromise.resolve()
+            resolve()
+          },
+        })
       }, syncDebouceMs)
     })
   }
@@ -262,10 +279,10 @@ export class NoteViewController implements ItemViewControllerInterface {
       params.isUserModified,
     )
 
-    params.onLocalPropagationComplete?.()
-
     void this.application.sync.sync().then(() => {
       params.onRemoteSyncComplete?.()
     })
+
+    params.onLocalPropagationComplete?.()
   }
 }