From 8c3c8439ad0562ca0f0c903194c3ed84a14c1887 Mon Sep 17 00:00:00 2001 From: Mattias Granlund Date: Fri, 10 Jan 2025 15:18:24 +0100 Subject: [PATCH] Improve front end error handling - handles signing error in one place --- apps/desktop/src/lib/error/error.ts | 17 +++++++ apps/desktop/src/lib/error/knownErrors.ts | 7 +++ apps/desktop/src/lib/error/typeguards.ts | 24 ++++++++++ apps/desktop/src/lib/notifications/toasts.ts | 28 +++++------ .../src/lib/vbranches/branchController.ts | 48 ++----------------- 5 files changed, 65 insertions(+), 59 deletions(-) create mode 100644 apps/desktop/src/lib/error/error.ts create mode 100644 apps/desktop/src/lib/error/knownErrors.ts create mode 100644 apps/desktop/src/lib/error/typeguards.ts diff --git a/apps/desktop/src/lib/error/error.ts b/apps/desktop/src/lib/error/error.ts new file mode 100644 index 0000000000..bd5772799c --- /dev/null +++ b/apps/desktop/src/lib/error/error.ts @@ -0,0 +1,17 @@ +/** + * Error type that has both a message and a status. These errors are primarily + * thrown by AI services and the Octokit GitHub client. + */ +export interface HttpError { + message: string; + status: number; +} + +/** + * Error type that has both a message and a code. These errors can be thrown + * by the back end code. + */ +export interface BackendError { + message: string; + code: string; +} diff --git a/apps/desktop/src/lib/error/knownErrors.ts b/apps/desktop/src/lib/error/knownErrors.ts new file mode 100644 index 0000000000..e01725b75c --- /dev/null +++ b/apps/desktop/src/lib/error/knownErrors.ts @@ -0,0 +1,7 @@ +export const KNOWN_ERRORS: Record = { + 'errors.commit.signing_failed': ` + Commit signing failed and has now been disabled. You can configure commit signing in the project settings. + + Please check our [documentation](https://docs.gitbutler.com/features/virtual-branches/verifying-commits) on setting up commit signing and verification. + ` +}; diff --git a/apps/desktop/src/lib/error/typeguards.ts b/apps/desktop/src/lib/error/typeguards.ts new file mode 100644 index 0000000000..49778685f4 --- /dev/null +++ b/apps/desktop/src/lib/error/typeguards.ts @@ -0,0 +1,24 @@ +import type { HttpError, BackendError } from './error'; + +export function isBackendError(err: unknown): err is BackendError { + return typeof err === 'object' && + err && + 'message' in err && + typeof err.message === 'string' && + 'code' in err && + typeof err.code === 'string' + ? true + : false; +} + +export function isHttpError(err: unknown): err is HttpError { + return ( + (typeof err === 'object' && + err && + 'message' in err && + typeof err.message === 'string' && + 'code' in err && + typeof err.code === 'string') || + false + ); +} diff --git a/apps/desktop/src/lib/notifications/toasts.ts b/apps/desktop/src/lib/notifications/toasts.ts index a8a6ca433c..7b0161c389 100644 --- a/apps/desktop/src/lib/notifications/toasts.ts +++ b/apps/desktop/src/lib/notifications/toasts.ts @@ -1,3 +1,5 @@ +import { KNOWN_ERRORS } from '$lib/error/knownErrors'; +import { isBackendError, isHttpError } from '$lib/error/typeguards'; import { isStr } from '@gitbutler/ui/utils/string'; import posthog from 'posthog-js'; import { writable, type Writable } from 'svelte/store'; @@ -31,22 +33,16 @@ export function showToast(toast: Toast) { } export function showError(title: string, error: unknown) { - // Silence GitHub octokit.js when disconnected - // TODO: Fix this elsewhere. - if (error instanceof Object) { - if ( - 'status' in error && - 'message' in error && - error.status === 500 && - error.message === 'Load failed' - ) - return; - const message = 'message' in error ? error.message : String(error); - showToast({ title, error: message, style: 'error' }); - } - - if (isStr(error)) { - showToast({ title, error, style: 'error' }); + if (isBackendError(error) && error.code in KNOWN_ERRORS) { + showToast({ title, message: KNOWN_ERRORS[error.code], error }); + } else if (isHttpError(error)) { + // Silence GitHub octokit.js when disconnected. This should ideally be + // prevented using `navigator.onLine` to avoid making requests when + // working offline. + if (error.status === 500 && error.message === 'Load failed') return; + showToast({ title, error: error.message, style: 'error' }); + } else if (isStr(error)) { + showToast({ title, error: error.toString(), style: 'error' }); } } diff --git a/apps/desktop/src/lib/vbranches/branchController.ts b/apps/desktop/src/lib/vbranches/branchController.ts index 1024781a27..f93f4ec4c3 100644 --- a/apps/desktop/src/lib/vbranches/branchController.ts +++ b/apps/desktop/src/lib/vbranches/branchController.ts @@ -69,12 +69,7 @@ export class BranchController { }); this.posthog.capture('Commit Successful'); } catch (err: any) { - if (err.code === 'errors.commit.signing_failed') { - showSignError(err); - } else { - showError('Failed to commit changes', err); - throw err; - } + showError('Failed to commit changes', err); this.posthog.capture('Commit Failed', err); } } @@ -503,12 +498,7 @@ export class BranchController { targetCommitOid }); } catch (err: any) { - // TODO: Probably we wanna have error code checking in a more generic way - if (err.code === 'errors.commit.signing_failed') { - showSignError(err); - } else { - showError('Failed to squash commit', err); - } + showError('Failed to squash commit', err); } } @@ -565,12 +555,7 @@ export class BranchController { message }); } catch (err: any) { - // TODO: Probably we wanna have error code checking in a more generic way - if (err.code === 'errors.commit.signing_failed') { - showSignError(err); - } else { - showError('Failed to change commit message', err); - } + showError('Failed to change commit message', err); } } @@ -583,12 +568,7 @@ export class BranchController { offset }); } catch (err: any) { - // TODO: Probably we wanna have error code checking in a more generic way - if (err.code === 'errors.commit.signing_failed') { - showSignError(err); - } else { - showError('Failed to insert blank commit', err); - } + showError('Failed to insert blank commit', err); } } @@ -601,25 +581,7 @@ export class BranchController { sourceBranchId }); } catch (err: any) { - // TODO: Probably we wanna have error code checking in a more generic way - if (err.code === 'errors.commit.signing_failed') { - showSignError(err); - } else { - showError('Failed to move commit', err); - } + showError('Failed to move commit', err); } } } - -function showSignError(err: any) { - showToast({ - title: 'Failed to commit due to signing error', - message: ` -Signing is now disabled, so subsequent commits will not fail. You can configure commit signing in the project settings. - -Please check our [documentation](https://docs.gitbutler.com/features/virtual-branches/verifying-commits) on setting up commit signing and verification. - `, - error: err.message, - style: 'error' - }); -}