Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Deflake project settings override on desktop (#4370)" #4450

Merged
merged 2 commits into from
Nov 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions e2e/playwright/testing-settings.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,6 @@ 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 () => {
Expand All @@ -345,13 +344,14 @@ 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 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)
})

Expand Down
33 changes: 14 additions & 19 deletions src/components/FileTree.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -189,15 +189,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)
Expand Down Expand Up @@ -242,11 +242,11 @@ const FileTreeItem = ({
// Show the renaming form
addCurrentItemToRenaming()
} else if (e.code === 'Space') {
void handleClick()
handleClick()
}
}

async function handleClick() {
function handleClick() {
setTreeSelection(fileOrDir)

if (fileOrDir.children !== null) return // Don't open directories
Expand All @@ -258,10 +258,12 @@ const FileTreeItem = ({
`import("${fileOrDir.path.replace(project.path, '.')}")\n` +
codeManager.code
)
await codeManager.writeToFile()
// eslint-disable-next-line @typescript-eslint/no-floating-promises
codeManager.writeToFile()

// Prevent seeing the model built one piece at a time when changing files
await kclManager.executeCode(true)
// eslint-disable-next-line @typescript-eslint/no-floating-promises
kclManager.executeCode(true)
} else {
// Let the lsp servers know we closed a file.
onFileClose(currentFile?.path || null, project?.path || null)
Expand Down Expand Up @@ -293,7 +295,7 @@ const FileTreeItem = ({
style={{ paddingInlineStart: getIndentationCSS(level) }}
onClick={(e) => {
e.currentTarget.focus()
void handleClick()
handleClick()
}}
onKeyUp={handleKeyUp}
>
Expand Down Expand Up @@ -653,13 +655,6 @@ 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(
Expand Down
12 changes: 4 additions & 8 deletions src/components/SettingsAuthProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ 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'
import { createRouteCommands } from 'lib/commandBarConfigs/routeCommandConfig'

type MachineContext<T extends AnyStateMachine> = {
Expand Down Expand Up @@ -202,13 +201,13 @@ export const SettingsAuthProviderBase = ({
console.error('Error executing AST after settings change', e)
}
},
async persistSettings({ context, event }) {
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

codeManager.writeCausedByAppCheckedInFileTreeFileSystemWatcher = true
return saveSettings(context, loadedProject?.project?.path)
// eslint-disable-next-line @typescript-eslint/no-floating-promises
saveSettings(context, loadedProject?.project?.path)
},
},
}),
Expand All @@ -222,7 +221,7 @@ export const SettingsAuthProviderBase = ({
}, [])

useFileSystemWatcher(
async (eventType: string) => {
async () => {
// 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
Expand All @@ -236,9 +235,6 @@ 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',
Expand Down
6 changes: 3 additions & 3 deletions src/editor/plugins/lsp/kcl/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()

void codeManager.writeToFile().then(() => {
this.scheduleUpdateDoc()
})
this.scheduleUpdateDoc()
}

scheduleUpdateDoc() {
Expand Down
1 change: 0 additions & 1 deletion src/hooks/useRefreshSettings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ export function useRefreshSettings(routeId: string = PATHS.INDEX) {
ctx.settings.send({
type: 'Set all settings',
settings: routeData,
doNotPersist: true,
})
}, [])
}
8 changes: 6 additions & 2 deletions src/lang/KclSingleton.ts
Original file line number Diff line number Diff line change
Expand Up @@ -434,9 +434,13 @@ export class KclManager {

// Update the code state and the editor.
codeManager.updateCodeStateEditor(code)

// Write back to the file system.
void codeManager.writeToFile().then(() => this.executeCode())
// 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()
}
// 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.
Expand Down
18 changes: 5 additions & 13 deletions src/lang/codeManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,28 +121,20 @@ 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

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
this.timeoutWriter = setTimeout(() => {
// Wait one event loop to give a chance for params to be set
// Save the file to disk
this._currentFilePath &&
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)
}
Expand Down
1 change: 0 additions & 1 deletion src/lib/settings/settingsUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,6 @@ 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()
Expand Down
Loading