Reland #362 - improved startup error handling (#446)

* Reapply "Settings store (#362)" (#440)

This reverts commit 81320e2.

* 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

@@ -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}`);

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<string, string>;
 
+/** @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<string | null> {
+  /**
+   * 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<BasePathReadResult> {
     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;
     }
   }
 }

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<T, K extends keyof T> = Omit<T, K> & Required<Pick<T, K>>;
+
+type MessageBoxOptions = RequireProperties<Electron.MessageBoxOptions, 'buttons' | 'defaultId' | 'cancelId'>;
+
+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<void> {
+    const defaults: Partial<Electron.MessageBoxOptions> = {
+      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(() => {});
+  }
+}

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<StoreType>;
+  private store: Store<AppWindowSettings>;
   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<StoreType> {
+  private loadWindowStore(): Store<AppWindowSettings> {
     try {
       // Separate file for non-critical convenience settings - just resets itself if invalid
-      return new Store<StoreType>({
+      return new Store<AppWindowSettings>({
         clearInvalidConfig: true,
         name: 'window',
       });

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<string>((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<ComfyDesktopApp> {
-    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<string | undefined> {
+    // 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<string | null> {
+    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);
   }