From f5e108df86b58912cf37ff029fc9ea9336b1ab3c Mon Sep 17 00:00:00 2001 From: filtered <176114999+webfiltered@users.noreply.github.com> Date: Sat, 7 Dec 2024 02:20:48 +1100 Subject: [PATCH] Reland #362 - improved startup error handling (#446) * Reapply "Settings store (#362)" (#440) This reverts commit 81320e2c7290e665cebfd18dddfb00e3bfdc35ee. * Allow user to see result of YAML config read - Nature of the failure is returned - Invalid values read are included if available - Retains inferred types * Fix cannot start app if invalid install state Current requirement is that a missing YAML file triggers the install screen. * Detect YAML parse exceptions * Update test expectations --- scripts/resetInstall.js | 54 ++++++++++---- src/config/comfyServerConfig.ts | 57 +++++++++++---- src/install/installationValidator.ts | 60 ++++++++++++++++ src/main-process/appWindow.ts | 8 +-- src/main-process/comfyDesktopApp.ts | 77 +++++++++++++++++--- src/main.ts | 17 ++++- src/store/desktopConfig.ts | 104 +++++++++++++++++++++++++++ src/store/index.ts | 14 +++- tests/unit/comfyServerConfig.test.ts | 13 ++-- 9 files changed, 359 insertions(+), 45 deletions(-) create mode 100644 src/install/installationValidator.ts create mode 100644 src/store/desktopConfig.ts diff --git a/scripts/resetInstall.js b/scripts/resetInstall.js index 4d25605c..4d82b0e3 100644 --- a/scripts/resetInstall.js +++ b/scripts/resetInstall.js @@ -6,15 +6,16 @@ const readline = require('readline'); /** * Get the path to the extra_models_config.yaml file based on the platform. + * @param {string} filename The name of the file to find in the user data folder * @returns The path to the extra_models_config.yaml file. */ -function getConfigPath() { +function getConfigPath(filename) { switch (process.platform) { case 'darwin': // macOS - return path.join(os.homedir(), 'Library', 'Application Support', 'ComfyUI', 'extra_models_config.yaml'); + return path.join(os.homedir(), 'Library', 'Application Support', 'ComfyUI', filename); case 'win32': // Windows - return path.join(process.env.APPDATA, 'ComfyUI', 'extra_models_config.yaml'); + return path.join(process.env.APPDATA, 'ComfyUI', filename); default: console.log('Platform not supported for this operation'); process.exit(1); @@ -37,27 +38,54 @@ async function askForConfirmation(question) { async function main() { try { - const configPath = getConfigPath(); + const configPath = getConfigPath('config.json'); + const windowStorePath = getConfigPath('window.json'); + const modelsConfigPath = getConfigPath('extra_models_config.yaml'); + let desktopBasePath = null; let basePath = null; - // Read base_path before deleting the config file + // Read basePath from desktop config if (fs.existsSync(configPath)) { const configContent = fs.readFileSync(configPath, 'utf8'); + const parsed = JSON.parse(configContent); + desktopBasePath = parsed?.basePath; + } + + // Read base_path before deleting the config file + if (fs.existsSync(modelsConfigPath)) { + const configContent = fs.readFileSync(modelsConfigPath, 'utf8'); const config = yaml.parse(configContent); basePath = config?.comfyui?.base_path; - - // Delete config file - fs.unlinkSync(configPath); - console.log(`Successfully removed ${configPath}`); } else { console.log('Config file not found, nothing to remove'); } - // If base_path exists, ask user if they want to delete it - if (basePath && fs.existsSync(basePath)) { - console.log(`Found ComfyUI installation directory at: ${basePath}`); + // Delete all config files + for (const file of [configPath, windowStorePath, modelsConfigPath]) { + if (fs.existsSync(file)) { + fs.unlinkSync(file); + console.log(`Successfully removed ${file}`); + } + } + + // If config.json basePath exists, ask user if they want to delete it + if (desktopBasePath && fs.existsSync(desktopBasePath)) { + console.log(`Found ComfyUI installation directory at: ${desktopBasePath}`); + const shouldDelete = await askForConfirmation('Would you like to delete this directory as well?'); + + if (shouldDelete) { + fs.rmSync(desktopBasePath, { recursive: true, force: true }); + console.log(`Successfully removed ComfyUI directory at ${desktopBasePath}`); + } else { + console.log('Skipping ComfyUI directory deletion'); + } + } + + // If base_path exists and does not match basePath, ask user if they want to delete it + if (basePath && basePath !== desktopBasePath && fs.existsSync(basePath)) { + console.log(`Found ComfyUI models directory at: ${basePath}`); const shouldDelete = await askForConfirmation('Would you like to delete this directory as well?'); - + if (shouldDelete) { fs.rmSync(basePath, { recursive: true, force: true }); console.log(`Successfully removed ComfyUI directory at ${basePath}`); diff --git a/src/config/comfyServerConfig.ts b/src/config/comfyServerConfig.ts index ea47c07a..21036005 100644 --- a/src/config/comfyServerConfig.ts +++ b/src/config/comfyServerConfig.ts @@ -1,7 +1,7 @@ import * as fs from 'node:fs'; import * as fsPromises from 'node:fs/promises'; import log from 'electron-log/main'; -import yaml from 'yaml'; +import yaml, { type YAMLParseError } from 'yaml'; import path from 'node:path'; import { app } from 'electron'; @@ -43,6 +43,29 @@ const knownModelKeys = [ export type ModelPaths = Record; +/** @see BasePathReadSuccess */ +type BasePathReadResult = BasePathReadSuccess | BasePathReadFailure; + +/** Result of a YAML config read attempt */ +interface BasePathReadSuccess { + /** + * Exactly what happened when trying to read the file. + * - `success`: All OK. Path is present. + * - `invalid`: File format invalid: `base_path` was not present or not a string. + * - `notFound`: The file does not exist. + * - `error`: Filesystem error (permissions, unresponsive disk, etc). + */ + status: 'success'; + /** The value of base_path from the YAML file */ + path: string; +} + +/** @see BasePathReadSuccess */ +interface BasePathReadFailure { + status: 'invalid' | 'notFound' | 'error'; + path?: unknown; +} + /** * The ComfyServerConfig class is used to manage the configuration for the ComfyUI server. */ @@ -154,29 +177,37 @@ export class ComfyServerConfig { } } - public static async readBasePathFromConfig(configPath: string): Promise { + /** + * Reads a YAML config file and attempts to return the base_path value. + * + * Attempts to read the new config path first, falling back to the original path if not. + * @param configPath The path to read + * @returns Status of the attempt and the value of base_path, if available + */ + public static async readBasePathFromConfig(configPath: string): Promise { try { const fileContent = await fsPromises.readFile(configPath, 'utf8'); const config = yaml.parse(fileContent); - if (config?.comfyui_desktop?.base_path) { - return config.comfyui_desktop.base_path; - } - - // Legacy yaml format, where we have everything under root 'comfyui'. - if (config?.comfyui?.base_path) { - return config.comfyui.base_path; + // Fallback to legacy yaml format, where we have everything under root 'comfyui'. + const base_path = config?.comfyui_desktop?.base_path ?? config?.comfyui?.base_path; + if (typeof base_path !== 'string') { + log.warn(`Base path in YAML config was invalid: [${ComfyServerConfig.configPath}]`); + return { status: 'invalid', path: base_path }; } - log.warn(`No base_path found in ${configPath}`); - return null; + return { status: 'success', path: base_path }; } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + if ((error as YAMLParseError)?.name === 'YAMLParseError') { + log.error(`Unable to parse config file [${configPath}]`, error); + return { status: 'invalid' }; + } else if ((error as NodeJS.ErrnoException)?.code === 'ENOENT') { log.info(`Config file not found at ${configPath}`); + return { status: 'notFound' }; } else { log.error(`Error reading config file ${configPath}:`, error); + return { status: 'error' }; } - return null; } } } diff --git a/src/install/installationValidator.ts b/src/install/installationValidator.ts new file mode 100644 index 00000000..2f7bd09d --- /dev/null +++ b/src/install/installationValidator.ts @@ -0,0 +1,60 @@ +import { app, dialog, shell } from 'electron'; +import fs from 'fs/promises'; +import log from 'electron-log/main'; +import path from 'node:path'; + +type RequireProperties = Omit & Required>; + +type MessageBoxOptions = RequireProperties; + +export class InstallationValidator { + /** + * Shows a dialog box with an option to open the problematic file in the native shell file viewer. + * @param options The options paramter of {@link dialog.showMessageBox}, filled with defaults for invalid config + * @returns + */ + static async showInvalidFileAndQuit(file: string, options: MessageBoxOptions): Promise { + const defaults: Partial = { + title: 'Invalid file', + type: 'error', + buttons: ['Open the &directory and quit', '&Quit'], + defaultId: 0, + cancelId: 1, + normalizeAccessKeys: true, + }; + const opt = Object.assign(defaults, options); + + const result = await dialog.showMessageBox(opt); + + // Try show the file in file manager + if (result.response === 0) { + try { + const parsed = path.parse(file); + log.debug(`Attempting to open containing directory: ${parsed.dir}`); + await fs.access(file); + shell.showItemInFolder(file); + } catch (error) { + log.warn(`Could not access file whilst attempting to exit gracefully after a critical error.`, file); + try { + // Failed - try the parent dir + const parsed = path.parse(file); + await fs.access(parsed.dir); + shell.openPath(parsed.dir); + } catch (error) { + // Nothing works. Log, notify, quit. + log.error( + `Could not read directory containing file, whilst attempting to exit gracefully after a critical error.` + ); + dialog.showErrorBox( + 'Unable to fine file', + `Unable to find the file. Please navigate to it manually:\n\n${file}` + ); + } + } + } + + app.quit(); + // Wait patiently for graceful termination. + await new Promise(() => {}); + } +} diff --git a/src/main-process/appWindow.ts b/src/main-process/appWindow.ts index 380d33d5..2d1089bc 100644 --- a/src/main-process/appWindow.ts +++ b/src/main-process/appWindow.ts @@ -1,7 +1,7 @@ import { BrowserWindow, screen, app, shell, ipcMain, Tray, Menu, dialog, MenuItem } from 'electron'; import path from 'node:path'; import Store from 'electron-store'; -import { StoreType } from '../store'; +import { AppWindowSettings } from '../store'; import log from 'electron-log/main'; import { IPC_CHANNELS, ProgressStatus, ServerArgs } from '../constants'; import { getAppResourcesPath } from '../install/resourcePaths'; @@ -12,7 +12,7 @@ import { getAppResourcesPath } from '../install/resourcePaths'; */ export class AppWindow { private window: BrowserWindow; - private store: Store; + private store: Store; private messageQueue: Array<{ channel: string; data: any }> = []; private rendererReady: boolean = false; @@ -142,10 +142,10 @@ export class AppWindow { * There are edge cases where this might not be a catastrophic failure, but inability * to write to our own datastore may result in unexpected user data loss. */ - private loadWindowStore(): Store { + private loadWindowStore(): Store { try { // Separate file for non-critical convenience settings - just resets itself if invalid - return new Store({ + return new Store({ clearInvalidConfig: true, name: 'window', }); diff --git a/src/main-process/comfyDesktopApp.ts b/src/main-process/comfyDesktopApp.ts index 5d13c5ff..26ba6910 100644 --- a/src/main-process/comfyDesktopApp.ts +++ b/src/main-process/comfyDesktopApp.ts @@ -16,8 +16,10 @@ import { DownloadManager } from '../models/DownloadManager'; import { VirtualEnvironment } from '../virtualEnvironment'; import { InstallWizard } from '../install/installWizard'; import { Terminal } from '../terminal'; +import { DesktopConfig } from '../store/desktopConfig'; +import { InstallationValidator } from '../install/installationValidator'; import { restoreCustomNodes } from '../services/backup'; -import Store from 'electron-store'; + export class ComfyDesktopApp { public comfyServer: ComfyServer | null = null; private terminal: Terminal | null = null; // Only created after server starts. @@ -146,7 +148,11 @@ export class ComfyDesktopApp { return new Promise((resolve) => { ipcMain.on(IPC_CHANNELS.INSTALL_COMFYUI, async (event, installOptions: InstallOptions) => { const installWizard = new InstallWizard(installOptions); + const { store } = DesktopConfig; + store.set('basePath', installWizard.basePath); + await installWizard.install(); + store.set('installState', 'installed'); resolve(installWizard.basePath); }); }); @@ -183,7 +189,7 @@ export class ComfyDesktopApp { this.appWindow.send(IPC_CHANNELS.LOG_MESSAGE, data); }, }); - const store = new Store(); + const { store } = DesktopConfig; if (!store.get('Comfy-Desktop.RestoredCustomNodes', false)) { try { await restoreCustomNodes(virtualEnvironment, this.appWindow); @@ -201,16 +207,71 @@ export class ComfyDesktopApp { } static async create(appWindow: AppWindow): Promise { - const basePath = ComfyServerConfig.exists() - ? await ComfyServerConfig.readBasePathFromConfig(ComfyServerConfig.configPath) - : await this.install(appWindow); + const { store } = DesktopConfig; + // Migrate settings from old version if required + const installState = store.get('installState') ?? (await ComfyDesktopApp.migrateInstallState()); + + // Fresh install + const loadedPath = installState === undefined ? undefined : await ComfyDesktopApp.loadBasePath(); + const basePath = loadedPath ?? (await ComfyDesktopApp.install(appWindow)); - if (!basePath) { - throw new Error(`Base path not found! ${ComfyServerConfig.configPath} is probably corrupted.`); - } return new ComfyDesktopApp(basePath, new ComfySettings(basePath), appWindow); } + /** + * Sets the ugpraded state if this is a version upgrade from <= 0.3.18 + * @returns 'upgraded' if this install has just been upgraded, or undefined for a fresh install + */ + static async migrateInstallState(): Promise { + // Fresh install + if (!ComfyServerConfig.exists()) return undefined; + + // Upgrade + const basePath = await ComfyDesktopApp.loadBasePath(); + + // Migrate config + const { store } = DesktopConfig; + const upgraded = 'upgraded'; + store.set('installState', upgraded); + store.set('basePath', basePath); + return upgraded; + } + + /** + * Loads the base_path value from the YAML config. + * + * Quits in the event of failure. + * @returns The base path of the ComfyUI data directory, if available + */ + static async loadBasePath(): Promise { + const basePath = await ComfyServerConfig.readBasePathFromConfig(ComfyServerConfig.configPath); + switch (basePath.status) { + case 'success': + return basePath.path; + case 'invalid': + // TODO: File was there, and was valid YAML. It just didn't have a valid base_path. + // Show path edit screen instead of reinstall. + return null; + case 'notFound': + return null; + case 'error': + default: + // Explain and quit + // TODO: Support link? Something? + await InstallationValidator.showInvalidFileAndQuit(ComfyServerConfig.configPath, { + message: `Unable to read the YAML configuration file. Please ensure this file is available and can be read: + +${ComfyServerConfig.configPath} + +If this problem persists, back up and delete the config file, then restart the app.`, + buttons: ['Open ComfyUI &directory and quit', '&Quit'], + defaultId: 0, + cancelId: 1, + }); + throw new Error(/* Unreachable. */); + } + } + uninstall(): void { fs.rmSync(ComfyServerConfig.configPath); } diff --git a/src/main.ts b/src/main.ts index fc5d8cea..9c186198 100644 --- a/src/main.ts +++ b/src/main.ts @@ -9,6 +9,7 @@ import { AppInfoHandlers } from './handlers/appInfoHandlers'; import { ComfyDesktopApp } from './main-process/comfyDesktopApp'; import { LevelOption } from 'electron-log'; import SentryLogging from './services/sentry'; +import { DesktopConfig } from './store/desktopConfig'; dotenv.config(); log.initialize(); @@ -40,6 +41,17 @@ if (!gotTheLock) { app.on('ready', async () => { log.debug('App ready'); + const store = await DesktopConfig.load(); + if (store) { + startApp(); + } else { + app.exit(20); + } + }); +} + +async function startApp() { + try { const appWindow = new AppWindow(); appWindow.onClose(() => { log.info('App window closed. Quitting application.'); @@ -79,5 +91,8 @@ if (!gotTheLock) { appWindow.sendServerStartProgress(ProgressStatus.ERROR); appWindow.send(IPC_CHANNELS.LOG_MESSAGE, error); } - }); + } catch (error) { + log.error('Fatal error occurred during app startup.', error); + app.exit(2024); + } } diff --git a/src/store/desktopConfig.ts b/src/store/desktopConfig.ts new file mode 100644 index 00000000..51ca9e2d --- /dev/null +++ b/src/store/desktopConfig.ts @@ -0,0 +1,104 @@ +import log from 'electron-log/main'; +import ElectronStore from 'electron-store'; +import { app, dialog, shell } from 'electron'; +import path from 'node:path'; +import fs from 'fs/promises'; +import type { DesktopSettings } from '.'; + +/** Handles loading of electron-store config, pre-window errors, and provides a non-null interface for the store. */ +export class DesktopConfig { + static #store: ElectronStore | undefined; + static get store(): ElectronStore { + const store = this.#store; + if (!store) throw new Error('Cannot access store before initialization.'); + return store; + } + + static async load( + options?: ConstructorParameters>[0] + ): Promise | undefined> { + try { + DesktopConfig.#store = new ElectronStore(options); + + return DesktopConfig.#store; + } catch (error) { + const configFilePath = path.join(getUserDataOrQuit(), `${options?.name ?? 'config'}.json`); + + if (error instanceof SyntaxError) { + // The .json file is invalid. Prompt user to reset. + const { response } = await showResetPrompt(configFilePath); + + if (response === 1) { + // Open dir with file selected + shell.showItemInFolder(configFilePath); + } else if (response === 0) { + // Reset - you sure? + const { response } = await showConfirmReset(configFilePath); + + if (response === 0) { + // Open dir with file selected + shell.showItemInFolder(configFilePath); + } else if (response === 1) { + // Delete all settings + await tryDeleteConfigFile(configFilePath); + + // Causing a stack overflow from this recursion would take immense patience. + return DesktopConfig.load(options); + } + } + + // User chose to exit + app.quit(); + } else { + // Crash: Unknown filesystem error, permission denied on user data folder, etc + log.error(`Unknown error whilst loading configuration file: ${configFilePath}`, error); + dialog.showErrorBox('User Data', `Unknown error whilst writing to user data folder:\n\n${configFilePath}`); + } + } + } +} + +function showResetPrompt(configFilePath: string): Promise { + return dialog.showMessageBox({ + title: 'Invalid configuration file', + type: 'error', + message: `Format of the configuration file below is invalid. It should be a JSON file containing only ComfyUI configuration options.\n\n${configFilePath}`, + buttons: ['&Reset desktop configuration', 'Show the &file (and quit)', '&Quit'], + defaultId: 0, + cancelId: 2, + normalizeAccessKeys: true, + }); +} + +function showConfirmReset(configFilePath: string): Promise { + return dialog.showMessageBox({ + title: 'Confirm reset settings', + type: 'warning', + message: `The configuration file below will be cleared and all settings will be reset. You should back this file up before deleting it.\n\n${configFilePath}`, + buttons: ['Show the &file (and quit)', '&Yes, delete all settings', '&Quit'], + defaultId: 0, + cancelId: 2, + normalizeAccessKeys: true, + }); +} + +async function tryDeleteConfigFile(configFilePath: string) { + try { + await fs.rm(configFilePath); + } catch (error) { + log.error(`Unable to delete configuration file: ${configFilePath}`, error); + dialog.showErrorBox('Delete Failed', `Unknown error whilst attempting to delete config file:\n\n${configFilePath}`); + } +} + +function getUserDataOrQuit() { + try { + return app.getPath('userData'); + } catch (error) { + // Crash: Can't even find the user userData folder + log.error('Cannot find user data folder.', error); + dialog.showErrorBox('User Data', 'Unknown error whilst attempting to determine user data folder.'); + app.quit(); + throw error; + } +} diff --git a/src/store/index.ts b/src/store/index.ts index 64997873..f0cabae9 100644 --- a/src/store/index.ts +++ b/src/store/index.ts @@ -1,7 +1,19 @@ -export type StoreType = { +export type AppWindowSettings = { windowWidth: number; windowHeight: number; windowX: number | undefined; windowY: number | undefined; windowMaximized?: boolean; }; + +export type DesktopSettings = { + basePath?: string; + /** + * The state of the installation. + * - `started`: The installation has started. + * - `installed`: A fresh installation. + * - `upgraded`: An upgrade from a previous version that stores the base path + * in the yaml config. + */ + installState?: 'started' | 'installed' | 'upgraded'; +}; diff --git a/tests/unit/comfyServerConfig.test.ts b/tests/unit/comfyServerConfig.test.ts index c3ad9c3a..6a53b493 100644 --- a/tests/unit/comfyServerConfig.test.ts +++ b/tests/unit/comfyServerConfig.test.ts @@ -54,20 +54,23 @@ comfyui: it('should read base_path from valid config file', async () => { const result = await ComfyServerConfig.readBasePathFromConfig(testConfigPath); - expect(result).toBe('~/test/comfyui'); + expect(result.status).toBe('success'); + expect(result.path).toBe('~/test/comfyui'); }); - it('should return null for non-existent file', async () => { + it('should detect non-existent file', async () => { const result = await ComfyServerConfig.readBasePathFromConfig('non_existent_file.yaml'); - expect(result).toBeNull(); + expect(result.status).toBe('notFound'); + expect(result.path).toBeUndefined(); }); - it('should return null for invalid config file', async () => { + it('should detect invalid config file', async () => { const invalidConfigPath = path.join(__dirname, 'invalid_config.yaml'); await fsPromises.writeFile(invalidConfigPath, 'invalid: yaml: content:', 'utf8'); const result = await ComfyServerConfig.readBasePathFromConfig(invalidConfigPath); - expect(result).toBeNull(); + expect(result.status).toBe('invalid'); + expect(result.path).toBeUndefined(); await fsPromises.rm(invalidConfigPath); });