Connect: Show config file errors #22728
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ravicious
reviewed
Mar 8, 2023
ravicious
reviewed
Mar 8, 2023
…itles do not look weird"
gzdunek
commented
Mar 9, 2023
ravicious
approved these changes
Mar 9, 2023
avatus
approved these changes
Mar 13, 2023
gzdunek
added a commit
that referenced
this pull request
Mar 16, 2023
* 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> (cherry picked from commit 17ff6d6)
gzdunek
added a commit
that referenced
this pull request
Mar 17, 2023
* Connect: Prepare keyboard shortcuts configuration (#22193) * Make naming around keyboard shortcuts more consistent Previously, we used words like: "key", "type", "shortcut" which were really confusing (and I wrote this code), because it was hard to tell what exactly they describe. From now, we will use only "accelerator" (borrowed from Electron) and "action" words. For example, in a shortcut "Command+1": "tab-1": - "Command+1" is "accelerator" - "tab-1" is "action" * Use "+" instead of "-" as a separator With "-" we are not able to use this symbol as a valid key code. * Rename some config keys to follow the same naming pattern * Rename keyboard shortcuts to match config keys * Extract `mapAcceleratorsToActions` to a function * Rename other "keyboard shortcuts" to "accelerators" and "actions" (cherry picked from commit 717c4fa) * Connect: Enable keyboard shortcuts configuration (#22194) * Use event.code for non A-z keys Without this fix, shortcuts like "Shift+1" can't work, because the reported character is "!". * Add Zod schema to validate keyboard shortcuts * Render better formatted error notification, add a link to docs * Notify about duplicated accelerators * Improve the error message for re-parsing failure (it can happen when a default value does not pass validation) * A bunch of renames * Allow spaces in accelerators * Rename `isContentAnObject` * Allow `Notification` to render links and lists * Split by optional whitespace and "+" * Allow lowercase letters * Require a modifier for non-function keys * Move VALID_SHORTCUT_MESSAGE to `initUi` * Always return from `getDuplicateAccelerators` * Use 'Cmd' and 'Ctrl' for mac * Add comments * Fix notification not rendering content * Add more comments, rename `getKeyCode` to `getKeyName` * Fix incorrect size of <Link> text * Remove "expected" prefix * Revert `typeof content === 'object'` in `Notification` * Remove a comment about disabled keys in `ConfigService`, add a note about `keymap.` prefix * Improve `getKeyName` comment * Extract an inline object to a variable in `getKeyName` * Fix notification list padding * Change text for doc link & description for config error * Improve comment for `getKeyName` * Remove special formatting for list === 1 in `Notification` * Print valid modifiers * Call `getKeyboardShortcutSchema()` once * Collect issues from all validations, run `invalidModifiers` through a set * Change error message for `missingModifierIssue` * Convert duplicates warning to error * Define ALLOWED_KEY_CODES in a more concise way * Support both `IntlBackslash` and `Backquote` * Restore modifiers for mac to full spelling, sort them in order recommended by platform guidelines * Fix old comment about the shortcuts order --------- Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com> (cherry picked from commit 4830ea1) * Connect: Generate json schema for app config (#22538) * Rename `putAllSync` to `writeSync` * Extend `FileStorage` with functions to replace an entire state and return file path used to create it * Add `updateJsonSchema` function * Generate schema for the app config * Always return a string from `createMockFileStorage().getFilePath()` * Add missing license headers * Rename 'teleport_connect_config_schema.json' to 'schema_app_config.json' * Rename `getKeyboardShortcutDescription` to `getShortcutDesc` * Rename `keyboardShortcutSchema` to `shortcutSchema` * Update a valid shortcut message * Rename `configJsonSchemaFile` to `jsonSchema` * Simplify `updateJsonSchema` * Set `$refStrategy` to none * Add missing description for `usageReporting.enabled` * Bump zod to the latest (improves TS performance) * Move `configService` implementation to `configStore` * Rename `configStore` to `configService` * Move `updateJsonSchema` to `createConfigService` * Move `validateStoredConfig` outside `createConfigService` * Rename `createAppConfigSchema.ts` to `appConfigSchema.ts`, `getKeyboardShortcutSchema.ts` to `keyboardShortcutSchema.ts` * Export `createKeyboardShortcutSchema` * Add license header (cherry picked from commit 9967149) * 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> (cherry picked from commit 17ff6d6) * Connect: Add "Open Config File" item to menu (#22730) * Expose `openConfigFile` IPC * Add "Open Config File" to `NavigationMenu` * Make `NavigationMenu` more concise * Remove unused imports * Call "three dots" menu "More Options" to make it easier to refer to in the docs * Show a notification with a config file path * Render the icon component only in `NavigationItem` * Render the separator only with access request items * Use just `separator` string instead of an object * Render `StyledListItem` as a button * Shorten the notification Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com> --------- Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com> (cherry picked from commit af9e0ed) * Add docs for Connect config (#22898) * Rename old "quick input" to the current "command bar" feature name * Remove 'Droid Sans Fallback' from Linux fonts * Add docs for config * Link to the proper documentation section * Apply suggestions from @ravicious and @ibeckermayer Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com> Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com> * Adjust property descriptions with the names used in User Interface * Drop `Courier New` * Simplify `$schema` description * Fix lint issues * Add a break line after each heading Co-authored-by: Paul Gottschling <paul.gottschling@goteleport.com> * Schema should not be modified * Config file is created on first launch --------- Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com> Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com> Co-authored-by: Paul Gottschling <paul.gottschling@goteleport.com> (cherry picked from commit 2d387e1)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #21914
I changed
getStoredConfigErrors(nowgetConfigError) to return a union of two types of errors:validation(Zod errors) andfile-loading(IO and JSON syntax errors).I also noticed that when the config file is corrupted, updating its contents (for example by linking to the JSON schema) removes all values from it. That is because when we can’t read the file, our logic creates an empty object in memory, which is then written to disk with the first update. To remedy this, I created a new flag:
discardUpdatesWhenLoadingFileFailed(a better name is welcome) for theFileStorage. Enabling it discards all updates to the file. We don't want to enable it for all the files; for app state or json schema it is okay to overwrite them.Technically, thanks to this flag, we don't have to hide the usage reporting dialog (normally a response from the user would overwrite the file), but I decided to do it because:
If you think that hiding the dialog is not needed then I am happy to discuss it.
Other than that, I refactored
FileStorageto use async FS functions (suggested by @ravicious).Example notification:
