Deflake project settings override on desktop (#4370)

e2e/playwright/testing-settings.spec.ts

@@ -318,6 +318,7 @@ test.describe('Testing settings', () => {
             timeout: 5_000,
           })
           .toContain(`themeColor = "${userThemeColor}"`)
+        // Only close the button after we've confirmed
       })
 
       await test.step('Set project theme color', async () => {
@@ -344,14 +345,13 @@ test.describe('Testing settings', () => {
       await test.step('Refresh the application and see project setting applied', async () => {
         // Make sure we're done navigating before we reload
         await expect(settingsCloseButton).not.toBeVisible()
-        await page.reload({ waitUntil: 'domcontentloaded' })
 
+        await page.reload({ waitUntil: 'domcontentloaded' })
         await expect(logoLink).toHaveCSS('--primary-hue', projectThemeColor)
       })
 
       await test.step(`Navigate back to the home view and see user setting applied`, async () => {
         await logoLink.click()
-        await page.screenshot({ path: 'out.png' })
         await expect(logoLink).toHaveCSS('--primary-hue', userThemeColor)
       })
 

src/components/FileTree.tsx

@@ -138,15 +138,15 @@ const FileTreeItem = ({
   // the ReactNodes are destroyed, so is this listener :)
   useFileSystemWatcher(
     async (eventType, path) => {
+      // Prevents a cyclic read / write causing editor problems such as
+      // misplaced cursor positions.
+      if (codeManager.writeCausedByAppCheckedInFileTreeFileSystemWatcher) {
+        codeManager.writeCausedByAppCheckedInFileTreeFileSystemWatcher = false
+        return
+      }
+
       // Don't try to read a file that was removed.
       if (isCurrentFile && eventType !== 'unlink') {
-        // Prevents a cyclic read / write causing editor problems such as
-        // misplaced cursor positions.
-        if (codeManager.writeCausedByAppCheckedInFileTreeFileSystemWatcher) {
-          codeManager.writeCausedByAppCheckedInFileTreeFileSystemWatcher = false
-          return
-        }
-
         let code = await window.electron.readFile(path, { encoding: 'utf-8' })
         code = normalizeLineEndings(code)
         codeManager.updateCodeStateEditor(code)
@@ -194,11 +194,11 @@ const FileTreeItem = ({
       // Show the renaming form
       addCurrentItemToRenaming()
     } else if (e.code === 'Space') {
-      handleClick()
+      void handleClick()
     }
   }
 
-  function handleClick() {
+  async function handleClick() {
     if (fileOrDir.children !== null) return // Don't open directories
 
     if (fileOrDir.name?.endsWith(FILE_EXT) === false && project?.path) {
@@ -208,12 +208,10 @@ const FileTreeItem = ({
         `import("${fileOrDir.path.replace(project.path, '.')}")\n` +
           codeManager.code
       )
-      // eslint-disable-next-line @typescript-eslint/no-floating-promises
-      codeManager.writeToFile()
+      await codeManager.writeToFile()
 
       // Prevent seeing the model built one piece at a time when changing files
-      // eslint-disable-next-line @typescript-eslint/no-floating-promises
-      kclManager.executeCode(true)
+      await kclManager.executeCode(true)
     } else {
       // Let the lsp servers know we closed a file.
       onFileClose(currentFile?.path || null, project?.path || null)
@@ -242,7 +240,7 @@ const FileTreeItem = ({
               style={{ paddingInlineStart: getIndentationCSS(level) }}
               onClick={(e) => {
                 e.currentTarget.focus()
-                handleClick()
+                void handleClick()
               }}
               onKeyUp={handleKeyUp}
             >
@@ -501,6 +499,13 @@ export const FileTreeInner = ({
       const isCurrentFile = loaderData.file?.path === path
       const hasChanged = eventType === 'change'
       if (isCurrentFile && hasChanged) return
+
+      // If it's a settings file we wrote to already from the app ignore it.
+      if (codeManager.writeCausedByAppCheckedInFileTreeFileSystemWatcher) {
+        codeManager.writeCausedByAppCheckedInFileTreeFileSystemWatcher = false
+        return
+      }
+
       fileSend({ type: 'Refresh' })
     },
     [loaderData?.project?.path, fileContext.selectedDirectory.path].filter(

src/components/SettingsAuthProvider.tsx

@@ -41,6 +41,7 @@ import { reportRejection } from 'lib/trap'
 import { getAppSettingsFilePath } from 'lib/desktop'
 import { isDesktop } from 'lib/isDesktop'
 import { useFileSystemWatcher } from 'hooks/useFileSystemWatcher'
+import { codeManager } from 'lib/singletons'
 
 type MachineContext<T extends AnyStateMachine> = {
   state: StateFrom<T>
@@ -200,13 +201,13 @@ export const SettingsAuthProviderBase = ({
             console.error('Error executing AST after settings change', e)
           }
         },
-        persistSettings: ({ context, event }) => {
+        async persistSettings({ context, event }) {
           // Without this, when a user changes the file, it'd
           // create a detection loop with the file-system watcher.
           if (event.doNotPersist) return
 
-          // eslint-disable-next-line @typescript-eslint/no-floating-promises
-          saveSettings(context, loadedProject?.project?.path)
+          codeManager.writeCausedByAppCheckedInFileTreeFileSystemWatcher = true
+          return saveSettings(context, loadedProject?.project?.path)
         },
       },
     }),
@@ -220,7 +221,7 @@ export const SettingsAuthProviderBase = ({
   }, [])
 
   useFileSystemWatcher(
-    async () => {
+    async (eventType: string) => {
       // If there is a projectPath but it no longer exists it means
       // it was exterally removed. If we let the code past this condition
       // execute it will recreate the directory due to code in
@@ -234,6 +235,9 @@ export const SettingsAuthProviderBase = ({
         }
       }
 
+      // Only reload if there are changes. Ignore everything else.
+      if (eventType !== 'change') return
+
       const data = await loadAndValidateSettings(loadedProject?.project?.path)
       settingsSend({
         type: 'Set all settings',

src/editor/plugins/lsp/kcl/index.ts

@@ -96,10 +96,10 @@ export class KclPlugin implements PluginValue {
 
     const newCode = viewUpdate.state.doc.toString()
     codeManager.code = newCode
-    // eslint-disable-next-line @typescript-eslint/no-floating-promises
-    codeManager.writeToFile()
 
-    this.scheduleUpdateDoc()
+    void codeManager.writeToFile().then(() => {
+      this.scheduleUpdateDoc()
+    })
   }
 
   scheduleUpdateDoc() {

src/hooks/useRefreshSettings.ts

@@ -26,6 +26,7 @@ export function useRefreshSettings(routeId: string = PATHS.INDEX) {
     ctx.settings.send({
       type: 'Set all settings',
       settings: routeData,
+      doNotPersist: true,
     })
   }, [])
 }

src/lang/KclSingleton.ts

@@ -429,13 +429,9 @@ export class KclManager {
 
     // Update the code state and the editor.
     codeManager.updateCodeStateEditor(code)
-    // Write back to the file system.
-    // eslint-disable-next-line @typescript-eslint/no-floating-promises
-    codeManager.writeToFile()
 
-    // execute the code.
-    // eslint-disable-next-line @typescript-eslint/no-floating-promises
-    this.executeCode()
+    // Write back to the file system.
+    void codeManager.writeToFile().then(() => this.executeCode())
   }
   // There's overlapping responsibility between updateAst and executeAst.
   // updateAst was added as it was used a lot before xState migration so makes the port easier.

src/lang/codeManager.ts

@@ -121,20 +121,28 @@ export default class CodeManager {
       // Only write our buffer contents to file once per second. Any faster
       // and file-system watchers which read, will receive empty data during
       // writes.
+
       clearTimeout(this.timeoutWriter)
       this.writeCausedByAppCheckedInFileTreeFileSystemWatcher = true
-      this.timeoutWriter = setTimeout(() => {
-        // Wait one event loop to give a chance for params to be set
-        // Save the file to disk
-        this._currentFilePath &&
+
+      return new Promise((resolve, reject) => {
+        this.timeoutWriter = setTimeout(() => {
+          if (!this._currentFilePath)
+            return reject(new Error('currentFilePath not set'))
+
+          // Wait one event loop to give a chance for params to be set
+          // Save the file to disk
           window.electron
             .writeFile(this._currentFilePath, this.code ?? '')
+            .then(resolve)
             .catch((err: Error) => {
               // TODO: add tracing per GH issue #254 (https://github.com/KittyCAD/modeling-app/issues/254)
               console.error('error saving file', err)
               toast.error('Error saving file, please check file permissions')
+              reject(err)
             })
-      }, 1000)
+        }, 1000)
+      })
     } else {
       safeLSSetItem(PERSIST_CODE_KEY, this.code)
     }

src/lib/settings/settingsUtils.ts

@@ -178,6 +178,7 @@ export async function loadAndValidateSettings(
   if (err(appSettingsPayload)) return Promise.reject(appSettingsPayload)
 
   let settingsNext = createSettings()
+
   // Because getting the default directory is async, we need to set it after
   if (onDesktop) {
     settings.app.projectDirectory.default = await getInitialDefaultDir()