refactor(Project): extract auto-save logic from startEditing (#906)

- Moved auto-save logic into dedicated methods. - Improved code organization and testability. - Fixed #880. Signed-off-by: Aofei Sheng <[email protected]>
goplus · Sep 14, 2024 · bb66c12 · bb66c12
1 parent b6bf51a
commit bb66c12
Show file tree

Hide file tree

Showing 2 changed files with 138 additions and 148 deletions.
diff --git a/spx-gui/src/models/project/index.test.ts b/spx-gui/src/models/project/index.test.ts
@@ -181,7 +181,7 @@ describe('Project', () => {
     await vi.advanceTimersByTimeAsync(500)
 
     const secondSavePromise = project.saveToCloud()
-    vi.runAllTimersAsync()
+    vi.runOnlyPendingTimersAsync()
 
     await expect(firstSavePromise).rejects.toThrow(Cancelled)
     await expect(secondSavePromise).resolves.not.toThrow()
@@ -207,40 +207,37 @@ describe('Project', () => {
   })
 
   // https://github.com/goplus/builder/pull/794#discussion_r1728120369
-  it('should handle failed auto-save correctly', async () => {
-    const project = makeProject()
-
+  it('should handle failed auto-save-to-cloud correctly', async () => {
     const cloudSaveMock = vi.spyOn(cloudHelper, 'save').mockRejectedValue(new Error('save failed'))
     const localSaveMock = vi.spyOn(localHelper, 'save').mockResolvedValue(undefined)
     const localClearMock = vi.spyOn(localHelper, 'clear').mockResolvedValue(undefined)
 
-    await project.startEditing('localCacheKey')
+    const project = makeProject()
+
     project.setAutoSaveMode(AutoSaveMode.Cloud)
+    project['startAutoSaveToCloud']('localCacheKey')
+    await flushPromises() // flush immediate watchers
+    const autoSaveToCloud = project['autoSaveToCloud']
+    expect(autoSaveToCloud).toBeTruthy()
+
+    project['filesHash'] = 'newHash'
+    project['lastSyncedFilesHash'] = 'hash'
+    expect(project.hasUnsyncedChanges).toBe(true)
 
-    const newSprite = new Sprite('newSprite')
-    project.addSprite(newSprite)
-    await flushPromises()
-    await vi.advanceTimersByTimeAsync(1000) // wait for changes to be picked up
-    await flushPromises()
+    autoSaveToCloud?.()
     expect(project.autoSaveToCloudState).toBe(AutoSaveToCloudState.Pending)
     expect(project.hasUnsyncedChanges).toBe(true)
 
-    await vi.advanceTimersByTimeAsync(1500) // wait for auto-save to trigger
-    await flushPromises()
+    await vi.runOnlyPendingTimersAsync()
     expect(project.autoSaveToCloudState).toBe(AutoSaveToCloudState.Failed)
     expect(project.hasUnsyncedChanges).toBe(true)
     expect(cloudSaveMock).toHaveBeenCalledTimes(1)
     expect(localSaveMock).toHaveBeenCalledTimes(1)
 
-    project.removeSprite(newSprite.id)
-    await flushPromises()
-    await vi.advanceTimersByTimeAsync(1000) // wait for changes to be picked up
-    await flushPromises()
-    expect(project.autoSaveToCloudState).toBe(AutoSaveToCloudState.Failed)
+    project['lastSyncedFilesHash'] = project['filesHash']
     expect(project.hasUnsyncedChanges).toBe(false)
 
-    await vi.advanceTimersByTimeAsync(5000) // wait for auto-retry to trigger
-    await flushPromises()
+    await vi.runOnlyPendingTimersAsync()
     expect(project.autoSaveToCloudState).toBe(AutoSaveToCloudState.Saved)
     expect(project.hasUnsyncedChanges).toBe(false)
     expect(cloudSaveMock).toHaveBeenCalledTimes(1)
@@ -249,50 +246,46 @@ describe('Project', () => {
   })
 
   it('should cancel pending auto-save-to-cloud when project is disposed', async () => {
-    const project = makeProject()
-
     const cloudSaveMock = vi.spyOn(cloudHelper, 'save').mockRejectedValue(undefined)
 
-    await project.startEditing('localCacheKey')
+    const project = makeProject()
+
     project.setAutoSaveMode(AutoSaveMode.Cloud)
+    project['startAutoSaveToCloud']('localCacheKey')
+    await flushPromises() // flush immediate watchers
+    const autoSaveToCloud = project['autoSaveToCloud']
+    expect(autoSaveToCloud).toBeTruthy()
 
-    const newSprite = new Sprite('newSprite')
-    project.addSprite(newSprite)
-    await flushPromises()
-    await vi.advanceTimersByTimeAsync(1000) // wait for changes to be picked up
-    await flushPromises()
+    project['filesHash'] = 'newHash'
+    project['lastSyncedFilesHash'] = 'hash'
+    expect(project.hasUnsyncedChanges).toBe(true)
+
+    autoSaveToCloud?.()
     expect(project.autoSaveToCloudState).toBe(AutoSaveToCloudState.Pending)
     expect(project.hasUnsyncedChanges).toBe(true)
 
     project.dispose()
-
-    await vi.advanceTimersByTimeAsync(1500 * 2) // wait longer to ensure auto-save does not trigger
-    await flushPromises()
+    await vi.runOnlyPendingTimersAsync()
     expect(project.autoSaveToCloudState).toBe(AutoSaveToCloudState.Pending)
     expect(project.hasUnsyncedChanges).toBe(true)
     expect(cloudSaveMock).toHaveBeenCalledTimes(0)
   })
 
   it('should cancel pending auto-save-to-local-cache when project is disposed', async () => {
-    const project = makeProject()
-
     const localSaveMock = vi.spyOn(localHelper, 'save').mockResolvedValue(undefined)
 
-    await project.startEditing('localCacheKey')
+    const project = makeProject()
+
     project.setAutoSaveMode(AutoSaveMode.LocalCache)
+    project['startAutoSaveToLocalCache']('localCacheKey')
+    await flushPromises() // flush immediate watchers
+    const autoSaveToLocalCache = project['autoSaveToLocalCache']
+    expect(autoSaveToLocalCache).toBeTruthy()
 
-    const newSprite = new Sprite('newSprite')
-    project.addSprite(newSprite)
-    await flushPromises()
-    await vi.advanceTimersByTimeAsync(1000) // wait for changes to be picked up
-    await flushPromises()
-    expect(project.hasUnsyncedChanges).toBe(true)
+    autoSaveToLocalCache?.()
 
     project.dispose()
-
-    await vi.advanceTimersByTimeAsync(1000 * 2) // wait longer to ensure auto-save does not trigger
-    await flushPromises()
-    expect(project.hasUnsyncedChanges).toBe(true)
+    await vi.runOnlyPendingTimersAsync()
     expect(localSaveMock).toHaveBeenCalledTimes(0)
   })
 })
diff --git a/spx-gui/src/models/project/index.ts b/spx-gui/src/models/project/index.ts
@@ -418,84 +418,70 @@ export class Project extends Disposable {
   autoSaveMode = AutoSaveMode.Off
   setAutoSaveMode(autoSaveMode: AutoSaveMode) {
     this.autoSaveMode = autoSaveMode
-  }
-  autoSaveToCloudState = AutoSaveToCloudState.Saved
-
-  /** Initialize editing features */
-  async startEditing(localCacheKey: string) {
-    if (this.lastSyncedFilesHash == null) {
-      this.lastSyncedFilesHash = await hashFiles(this.exportGameFiles())
+    switch (autoSaveMode) {
+      case AutoSaveMode.Cloud:
+        if (this.hasUnsyncedChanges) this.autoSaveToCloud?.()
+        break
+      case AutoSaveMode.LocalCache:
+        this.autoSaveToLocalCache?.()
+        break
     }
-    if (this.filesHash == null) {
-      this.filesHash = this.lastSyncedFilesHash
-    }
-
-    // watch for changes of game files, update filesHash, and auto save to cloud if hasUnsyncedChanges
-    const autoSaveToCloud = (() => {
-      const save = debounce(async () => {
-        if (this.autoSaveToCloudState !== AutoSaveToCloudState.Pending) return
-        this.autoSaveToCloudState = AutoSaveToCloudState.Saving
-
-        try {
-          if (this.isSavingToCloud) {
-            await untilConditionMet(
-              () => this.isSavingToCloud,
-              () => !this.isSavingToCloud
-            )
-          }
+  }
 
-          if (this.hasUnsyncedChanges) await this.saveToCloud()
-          this.autoSaveToCloudState = AutoSaveToCloudState.Saved
-        } catch (e) {
-          this.autoSaveToCloudState = AutoSaveToCloudState.Failed
-          if (e instanceof Cancelled) {
-            autoSaveToCloud()
-            save.flush()
-          } else {
-            retryAutoSave()
-            await this.saveToLocalCache(localCacheKey) // prevent data loss
-            console.error('failed to auto save to cloud', e)
-          }
-          return
+  /** watch for changes of game files, update filesHash, and auto save to cloud if hasUnsyncedChanges */
+  autoSaveToCloudState = AutoSaveToCloudState.Saved
+  private autoSaveToCloud: (() => void) | null = null
+  private startAutoSaveToCloud(localCacheKey: string) {
+    const retryAutoSaveToCloud = debounce(async () => {
+      if (this.autoSaveToCloudState !== AutoSaveToCloudState.Failed) return
+      if (this.hasUnsyncedChanges) {
+        this.autoSaveToCloud?.()
+      } else {
+        this.autoSaveToCloudState = AutoSaveToCloudState.Saved
+        await localHelper.clear(localCacheKey)
+      }
+    }, 5000)
+    this.addDisposer(retryAutoSaveToCloud.cancel)
+
+    const saveToCloud = debounce(async () => {
+      if (this.autoSaveToCloudState !== AutoSaveToCloudState.Pending) return
+      this.autoSaveToCloudState = AutoSaveToCloudState.Saving
+
+      try {
+        if (this.isSavingToCloud) {
+          await untilConditionMet(
+            () => this.isSavingToCloud,
+            () => !this.isSavingToCloud
+          )
         }
 
-        if (this.hasUnsyncedChanges) autoSaveToCloud()
-        else await localHelper.clear(localCacheKey)
-      }, 1500)
-      this.addDisposer(save.cancel)
-
-      const retryAutoSave = debounce(async () => {
-        if (this.autoSaveToCloudState !== AutoSaveToCloudState.Failed) return
-        if (this.hasUnsyncedChanges) {
-          autoSaveToCloud()
+        if (this.hasUnsyncedChanges) await this.saveToCloud()
+        this.autoSaveToCloudState = AutoSaveToCloudState.Saved
+      } catch (e) {
+        this.autoSaveToCloudState = AutoSaveToCloudState.Failed
+        if (e instanceof Cancelled) {
+          this.autoSaveToCloud?.()
+          saveToCloud.flush()
         } else {
-          this.autoSaveToCloudState = AutoSaveToCloudState.Saved
-          await localHelper.clear(localCacheKey)
+          retryAutoSaveToCloud()
+          await this.saveToLocalCache(localCacheKey) // prevent data loss
+          console.error('failed to auto save to cloud', e)
         }
-      }, 5000)
-      this.addDisposer(retryAutoSave.cancel)
-
-      // fire pending or retryable auto saves immediately when a new save occurs, making autoSaveToCloudState more responsive
-      this.addDisposer(
-        watch(
-          () => this.isSavingToCloud,
-          async () => {
-            if (this.isSavingToCloud) {
-              await retryAutoSave.flush()
-              save.flush()
-            }
-          },
-          { immediate: true }
-        )
-      )
-
-      return () => {
-        retryAutoSave.cancel()
-        if (this.autoSaveToCloudState !== AutoSaveToCloudState.Saving)
-          this.autoSaveToCloudState = AutoSaveToCloudState.Pending
-        if (this.autoSaveMode === AutoSaveMode.Cloud) save()
+        return
       }
-    })()
+
+      if (this.hasUnsyncedChanges) this.autoSaveToCloud?.()
+      else await localHelper.clear(localCacheKey)
+    }, 1500)
+    this.addDisposer(saveToCloud.cancel)
+
+    this.autoSaveToCloud = () => {
+      retryAutoSaveToCloud.cancel()
+      if (this.autoSaveToCloudState !== AutoSaveToCloudState.Saving)
+        this.autoSaveToCloudState = AutoSaveToCloudState.Pending
+      if (this.autoSaveMode === AutoSaveMode.Cloud) saveToCloud()
+    }
+
     this.addDisposer(
       watch(
         () => this.exportGameFiles(),
@@ -505,53 +491,64 @@ export class Project extends Disposable {
           const filesHash = await hashFiles(files)
           if (cancelled) return // avoid race condition and ensure filesHash accuracy
           this.filesHash = filesHash
-          if (this.hasUnsyncedChanges) autoSaveToCloud()
+          if (this.hasUnsyncedChanges) this.autoSaveToCloud?.()
         },
         { immediate: true }
       )
     )
 
-    // watch for all changes, auto save to local cache, or touch all game files to trigger lazy loading to ensure they are in memory
-    const autoSaveToLocalCache = (() => {
-      const save = debounce(() => this.saveToLocalCache(localCacheKey), 1000)
-      this.addDisposer(save.cancel)
-
-      const delazyLoadGameFiles = debounce(() => {
-        const files = this.exportGameFiles()
-        const fileList = Object.keys(files)
-        fileList.map((path) => files[path]!.arrayBuffer())
-      }, 1000)
-      this.addDisposer(delazyLoadGameFiles.cancel)
-
-      return () => {
-        if (this.autoSaveMode === AutoSaveMode.LocalCache) save()
-        else delazyLoadGameFiles()
-      }
-    })()
-    this.addDisposer(
-      watch(() => [this.exportMetadata(), this.exportGameFiles()], autoSaveToLocalCache, {
-        immediate: true
-      })
-    )
-
-    // watch for autoSaveMode switch, and trigger auto save accordingly
+    // fire pending or retryable auto saves immediately when a new save occurs, making autoSaveToCloudState more responsive
     this.addDisposer(
       watch(
-        () => this.autoSaveMode,
-        () => {
-          switch (this.autoSaveMode) {
-            case AutoSaveMode.Cloud:
-              if (this.hasUnsyncedChanges) autoSaveToCloud()
-              break
-            case AutoSaveMode.LocalCache:
-              autoSaveToLocalCache()
-              break
+        () => this.isSavingToCloud,
+        async () => {
+          if (this.isSavingToCloud) {
+            await retryAutoSaveToCloud.flush()
+            saveToCloud.flush()
           }
         },
         { immediate: true }
       )
     )
   }
+
+  /** watch for all changes, auto save to local cache, or touch all game files to trigger lazy loading to ensure they are in memory */
+  private autoSaveToLocalCache: (() => void) | null = null
+  private startAutoSaveToLocalCache(localCacheKey: string) {
+    const saveToLocalCache = debounce(() => this.saveToLocalCache(localCacheKey), 1000)
+    this.addDisposer(saveToLocalCache.cancel)
+
+    const touchGameFiles = debounce(() => {
+      const files = this.exportGameFiles()
+      Object.keys(files).map((path) => files[path]!.arrayBuffer())
+    }, 1000)
+    this.addDisposer(touchGameFiles.cancel)
+
+    this.autoSaveToLocalCache = () => {
+      if (this.autoSaveMode === AutoSaveMode.LocalCache) saveToLocalCache()
+      else touchGameFiles()
+    }
+
+    this.addDisposer(
+      watch(
+        () => [this.exportMetadata(), this.exportGameFiles()],
+        () => this.autoSaveToLocalCache?.(),
+        {
+          immediate: true
+        }
+      )
+    )
+  }
+
+  /** Initialize editing features */
+  async startEditing(localCacheKey: string) {
+    this.filesHash = await hashFiles(this.exportGameFiles())
+    if (this.lastSyncedFilesHash == null) {
+      this.lastSyncedFilesHash = this.filesHash
+    }
+    this.startAutoSaveToCloud(localCacheKey)
+    this.startAutoSaveToLocalCache(localCacheKey)
+  }
 }
 
 /** Get full name for project, which stands for a globally unique identifier for the project */