Connect: Show config file errors (#22728)

* Add a function that returns error that might happen during loading the file

* Return validation and file loading errors from `ConfigService`

* Discard file storage updates when the file was not loaded correctly

* Do not show usage reporting dialog when the file was not loaded correctly

* Make title of notifications a little bit smaller, so longer titles do not look weird

* Get rid of sync fs functions wherever possible in file storage

* Move error handling code to `createFileStorage`

* Improve `getConfigError` comment

* Rename `discardUpdatesWhenLoadingFileFailed` to `discardUpdatesOnLoadError`

* Fix the error message in `notifyAboutConfigErrors`

* Revert "Make title of notifications a little bit smaller, so longer titles do not look weird"

* Make `writeSync` async too

* Return `unknown` from `FileStorage.get`

* Add a TODO comment about createFileStorage type

* Add "the" to "Using default config instead"

* Remove `toString()`

---------

Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com>

web/packages/teleterm/src/main.ts

@@ -39,22 +39,15 @@ if (app.requestSingleInstanceLock()) {
   app.exit(1);
 }
 
-function initializeApp(): void {
+async function initializeApp(): Promise<void> {
   let devRelaunchScheduled = false;
   const settings = getRuntimeSettings();
   const logger = initMainLogger(settings);
-  const appStateFileStorage = createFileStorage({
-    filePath: path.join(settings.userDataDir, 'app_state.json'),
-    debounceWrites: true,
-  });
-  const configFileStorage = createFileStorage({
-    filePath: path.join(settings.userDataDir, 'app_config.json'),
-    debounceWrites: false,
-  });
-  const configJsonSchemaFileStorage = createFileStorage({
-    filePath: path.join(settings.userDataDir, 'schema_app_config.json'),
-    debounceWrites: false,
-  });
+  const {
+    appStateFileStorage,
+    configFileStorage,
+    configJsonSchemaFileStorage,
+  } = await createFileStorages(settings.userDataDir);
 
   const configService = createConfigService({
     configFile: configFileStorage,
@@ -97,16 +90,17 @@ function initializeApp(): void {
 
   app.on('will-quit', async event => {
     event.preventDefault();
+    const disposeMainProcess = async () => {
+      try {
+        await mainProcess.dispose();
+      } catch (e) {
+        logger.error('Failed to gracefully dispose of main process', e);
+      }
+    };
 
-    appStateFileStorage.writeSync();
     globalShortcut.unregisterAll();
-    try {
-      await mainProcess.dispose();
-    } catch (e) {
-      logger.error('Failed to gracefully dispose of main process', e);
-    } finally {
-      app.exit();
-    }
+    await Promise.all([appStateFileStorage.write(), disposeMainProcess()]); // none of them can throw
+    app.exit();
   });
 
   app.on('quit', () => {
@@ -204,3 +198,25 @@ function initMainLogger(settings: types.RuntimeSettings) {
 
   return new Logger('Main');
 }
+
+function createFileStorages(userDataDir: string) {
+  return Promise.all([
+    createFileStorage({
+      filePath: path.join(userDataDir, 'app_state.json'),
+      debounceWrites: true,
+    }),
+    createFileStorage({
+      filePath: path.join(userDataDir, 'app_config.json'),
+      debounceWrites: false,
+      discardUpdatesOnLoadError: true,
+    }),
+    createFileStorage({
+      filePath: path.join(userDataDir, 'schema_app_config.json'),
+      debounceWrites: false,
+    }),
+  ]).then(storages => ({
+    appStateFileStorage: storages[0],
+    configFileStorage: storages[1],
+    configJsonSchemaFileStorage: storages[2],
+  }));
+}

web/packages/teleterm/src/mainProcess/types.ts

@@ -125,13 +125,14 @@ export enum TabContextMenuEventType {
 export enum ConfigServiceEventType {
   Get = 'Get',
   Set = 'Set',
-  GetStoredConfigErrors = 'GetStoredConfigErrors',
+  GetConfigError = 'GetConfigError',
 }
 
 export enum FileStorageEventType {
   Get = 'Get',
   Put = 'Put',
-  WriteSync = 'WriteSync',
+  Write = 'Write',
   Replace = 'Replace',
   GetFilePath = 'GetFilePath',
+  GetFileLoadingError = 'GetFileLoadingError',
 }

web/packages/teleterm/src/mainProcess/windowsManager.ts

@@ -197,7 +197,7 @@ export class WindowsManager {
   }
 
   private getWindowState(): WindowState {
-    const windowState = this.fileStorage.get<WindowState>(this.storageKey);
+    const windowState = this.fileStorage.get(this.storageKey) as WindowState;
     const getDefaults = () => ({
       height: 720,
       width: 1280,

web/packages/teleterm/src/services/config/configService.test.ts

@@ -32,7 +32,7 @@ test('stored and default values are combined', () => {
     platform: 'darwin',
   });
 
-  expect(configService.getStoredConfigErrors()).toBeUndefined();
+  expect(configService.getConfigError()).toBeUndefined();
 
   const usageReportingEnabled = configService.get('usageReporting.enabled');
   expect(usageReportingEnabled.value).toBe(true);
@@ -52,15 +52,18 @@ test('in case of invalid value a default one is returned', () => {
     platform: 'darwin',
   });
 
-  expect(configService.getStoredConfigErrors()).toStrictEqual([
-    {
-      code: 'invalid_type',
-      expected: 'boolean',
-      received: 'string',
-      message: 'Expected boolean, received string',
-      path: ['usageReporting.enabled'],
-    },
-  ]);
+  expect(configService.getConfigError()).toStrictEqual({
+    source: 'validation',
+    errors: [
+      {
+        code: 'invalid_type',
+        expected: 'boolean',
+        received: 'string',
+        message: 'Expected boolean, received string',
+        path: ['usageReporting.enabled'],
+      },
+    ],
+  });
 
   const usageReportingEnabled = configService.get('usageReporting.enabled');
   expect(usageReportingEnabled.value).toBe(false);
@@ -71,6 +74,23 @@ test('in case of invalid value a default one is returned', () => {
   expect(terminalFontSize.metadata.isStored).toBe(false);
 });
 
+test('if config file failed to load correctly the error is returned', () => {
+  const configFile = createMockFileStorage();
+  const error = new Error('Failed to read');
+  jest.spyOn(configFile, 'getFileLoadingError').mockReturnValue(error);
+
+  const configService = createConfigService({
+    configFile,
+    jsonSchemaFile: createMockFileStorage(),
+    platform: 'darwin',
+  });
+
+  expect(configService.getConfigError()).toStrictEqual({
+    source: 'file-loading',
+    error,
+  });
+});
+
 test('calling set updated the value in store', () => {
   const configFile = createMockFileStorage();
   const configService = createConfigService({

web/packages/teleterm/src/services/config/configService.ts

@@ -31,14 +31,35 @@ import {
 
 const logger = new Logger('ConfigService');
 
+type FileLoadingError = {
+  source: 'file-loading';
+  error: Error;
+};
+
+type ValidationError = {
+  source: 'validation';
+  errors: ZodIssue[];
+};
+
+type ConfigError = FileLoadingError | ValidationError;
+
 export interface ConfigService {
   get<K extends keyof AppConfig>(
     key: K
   ): { value: AppConfig[K]; metadata: { isStored: boolean } };
 
   set<K extends keyof AppConfig>(key: K, value: AppConfig[K]): void;
 
-  getStoredConfigErrors(): ZodIssue[] | undefined;
+  /**
+   * Returns validation errors or an error that occurred during loading the config file (this means IO and syntax errors).
+   * This error has to be checked during the initialization of the app.
+   *
+   * The reason we have a getter for this error instead of making `createConfigService` fail with an error
+   * is that in the presence of this error we want to notify about it and then continue with default values:
+   * - If validation errors occur, the incorrect values are replaced with the defaults.
+   * - In case of an error coming from loading the file, all values are replaced with the defaults.
+   * */
+  getConfigError(): ConfigError | undefined;
 }
 
 export function createConfigService({
@@ -53,10 +74,11 @@ export function createConfigService({
   const schema = createAppConfigSchema(platform);
   updateJsonSchema({ schema, configFile, jsonSchemaFile });
 
-  const { storedConfig, configWithDefaults, errors } = validateStoredConfig(
-    schema,
-    configFile
-  );
+  const {
+    storedConfig,
+    configWithDefaults,
+    errors: validationErrors,
+  } = validateStoredConfig(schema, configFile);
 
   return {
     get: key => ({
@@ -68,7 +90,21 @@ export function createConfigService({
       configWithDefaults[key] = value;
       storedConfig[key] = value;
     },
-    getStoredConfigErrors: () => errors,
+    getConfigError: () => {
+      const fileLoadingError = configFile.getFileLoadingError();
+      if (fileLoadingError) {
+        return {
+          source: 'file-loading',
+          error: fileLoadingError,
+        };
+      }
+      if (validationErrors) {
+        return {
+          source: 'validation',
+          errors: validationErrors,
+        };
+      }
+    },
   };
 }
 
@@ -96,8 +132,6 @@ function updateJsonSchema({
   }
 }
 
-//TODO (gzdunek): syntax errors of the JSON file are silently ignored, report
-// them to the user too
 function validateStoredConfig(
   schema: AppConfigSchema,
   configFile: FileStorage
@@ -108,7 +142,7 @@ function validateStoredConfig(
 } {
   const parse = (data: Partial<AppConfig>) => schema.safeParse(data);
 
-  const storedConfig = configFile.get<Partial<AppConfig>>();
+  const storedConfig = configFile.get() as Partial<AppConfig>;
   const parsed = parse(storedConfig);
   if (parsed.success === true) {
     return {

web/packages/teleterm/src/services/config/configServiceClient.ts

@@ -33,8 +33,8 @@ export function subscribeToConfigServiceEvents(
           return (event.returnValue = configService.get(item.path));
         case ConfigServiceEventType.Set:
           return configService.set(item.path, item.value);
-        case ConfigServiceEventType.GetStoredConfigErrors:
-          return (event.returnValue = configService.getStoredConfigErrors());
+        case ConfigServiceEventType.GetConfigError:
+          return (event.returnValue = configService.getConfigError());
       }
     }
   );
@@ -54,10 +54,10 @@ export function createConfigServiceClient(): ConfigService {
         value,
       });
     },
-    getStoredConfigErrors: () => {
+    getConfigError: () => {
       return ipcRenderer.sendSync(
         ConfigServiceEventChannel,
-        ConfigServiceEventType.GetStoredConfigErrors
+        ConfigServiceEventType.GetConfigError
       );
     },
   };

web/packages/teleterm/src/services/config/fixtures/mocks.ts

@@ -27,8 +27,8 @@ export function createMockConfigService(
     set(key, value) {
       values[key] = value;
     },
-    getStoredConfigErrors() {
-      return [];
+    getConfigError() {
+      return undefined;
     },
   };
 }