From ad1cd56891bc78c4554129350416e092b8c452c8 Mon Sep 17 00:00:00 2001 From: 49fl Date: Thu, 31 Oct 2024 21:42:52 -0400 Subject: [PATCH] Deflake project settings override on desktop (#4370) --- e2e/playwright/testing-settings.spec.ts | 4 +-- src/components/FileTree.tsx | 33 ++++++++++++++----------- src/components/SettingsAuthProvider.tsx | 12 ++++++--- src/editor/plugins/lsp/kcl/index.ts | 6 ++--- src/hooks/useRefreshSettings.ts | 1 + src/lang/KclSingleton.ts | 8 ++---- src/lang/codeManager.ts | 18 ++++++++++---- src/lib/settings/settingsUtils.ts | 1 + 8 files changed, 49 insertions(+), 34 deletions(-) diff --git a/e2e/playwright/testing-settings.spec.ts b/e2e/playwright/testing-settings.spec.ts index ccf8837f70..40c0fa6f38 100644 --- a/e2e/playwright/testing-settings.spec.ts +++ b/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) }) diff --git a/src/components/FileTree.tsx b/src/components/FileTree.tsx index 1e534bdb60..70610549e8 100644 --- a/src/components/FileTree.tsx +++ b/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( diff --git a/src/components/SettingsAuthProvider.tsx b/src/components/SettingsAuthProvider.tsx index 95727f4960..b26d0dfee4 100644 --- a/src/components/SettingsAuthProvider.tsx +++ b/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 = { state: StateFrom @@ -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', diff --git a/src/editor/plugins/lsp/kcl/index.ts b/src/editor/plugins/lsp/kcl/index.ts index 42de1a5b26..205397b799 100644 --- a/src/editor/plugins/lsp/kcl/index.ts +++ b/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() { diff --git a/src/hooks/useRefreshSettings.ts b/src/hooks/useRefreshSettings.ts index 6c1447b7b4..da7c440d26 100644 --- a/src/hooks/useRefreshSettings.ts +++ b/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, }) }, []) } diff --git a/src/lang/KclSingleton.ts b/src/lang/KclSingleton.ts index 4020b3e8d3..ccde51f889 100644 --- a/src/lang/KclSingleton.ts +++ b/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. diff --git a/src/lang/codeManager.ts b/src/lang/codeManager.ts index 5fdb72b323..476c5ba361 100644 --- a/src/lang/codeManager.ts +++ b/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) } diff --git a/src/lib/settings/settingsUtils.ts b/src/lib/settings/settingsUtils.ts index 283301ace4..f9ab5ad32e 100644 --- a/src/lib/settings/settingsUtils.ts +++ b/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()