From 2e612c2ab71dd181398c714c3364f19fd181f347 Mon Sep 17 00:00:00 2001 From: Christopher Loverich <1010084+cloverich@users.noreply.github.com> Date: Mon, 3 Feb 2025 12:21:32 -0800 Subject: [PATCH] improve document error handling - add better error display and ability to navigate when a document errors unexpectedly - refactor Alert to simplify interface - add better behavior when document not found (navigates back to homepage with toast); clean-up invalid error handling Closes #260 --- src/components/Alert.tsx | 56 ++++++++++- src/error.tsx | 4 + src/views/edit/EditorErrorBoundary.tsx | 134 +++++++++++++++++++++++++ src/views/edit/index.tsx | 40 ++++++-- src/views/edit/loading.tsx | 19 ++-- src/views/edit/useEditableDocument.ts | 26 ++--- src/views/layout.tsx | 2 +- 7 files changed, 244 insertions(+), 37 deletions(-) create mode 100644 src/views/edit/EditorErrorBoundary.tsx diff --git a/src/components/Alert.tsx b/src/components/Alert.tsx index 2cbf883..2fa2e9b 100644 --- a/src/components/Alert.tsx +++ b/src/components/Alert.tsx @@ -1,4 +1,5 @@ import { cva, type VariantProps } from "class-variance-authority"; +import { AngryIcon, InfoIcon, OctagonIcon, TriangleIcon } from "lucide-react"; import * as React from "react"; import { cn } from "@udecode/cn"; @@ -11,6 +12,8 @@ const alertVariants = cva( default: "bg-background text-foreground", destructive: "border-destructive/50 text-destructive dark:border-destructive [&>svg]:text-destructive", + warning: "border-warning/50 text-warning dark:border-warning", + error: "border-error/50 text-error dark:border-error", }, }, defaultVariants: { @@ -19,7 +22,51 @@ const alertVariants = cva( }, ); +const iconForVariant = { + default: InfoIcon, + destructive: OctagonIcon, + warning: TriangleIcon, + error: AngryIcon, +}; + +interface IAlert { + // variant: "default" | "destructive" | "warning" | "error"; + title: string; +} + const Alert = React.forwardRef< + HTMLDivElement, + React.HTMLAttributes & + VariantProps & + IAlert +>(({ className, variant, title, children }, ref) => { + const Icon = iconForVariant[variant || "default"]; + return ( + +
+ {/* This worked ok to display Icon but layout wasn't great */} + {/*
+ +
*/} +
+ {title} + {children} +
+
+ + ); +}); + +Alert.displayName = "Alert"; + +const AlertBase = React.forwardRef< HTMLDivElement, React.HTMLAttributes & VariantProps >(({ className, variant, ...props }, ref) => ( @@ -30,21 +77,22 @@ const Alert = React.forwardRef< {...props} /> )); -Alert.displayName = "Alert"; +AlertBase.displayName = "AlertBase"; const AlertTitle = React.forwardRef< HTMLParagraphElement, React.HTMLAttributes >(({ className, ...props }, ref) => ( -
)); + AlertTitle.displayName = "AlertTitle"; const AlertDescription = React.forwardRef< @@ -59,4 +107,4 @@ const AlertDescription = React.forwardRef< )); AlertDescription.displayName = "AlertDescription"; -export { Alert, AlertDescription, AlertTitle }; +export { Alert, AlertBase, AlertDescription, AlertTitle }; diff --git a/src/error.tsx b/src/error.tsx index 3d175a4..07be57f 100644 --- a/src/error.tsx +++ b/src/error.tsx @@ -6,6 +6,10 @@ interface State { error: any; } +interface Props { + children: React.ReactNode; +} + export default class ErrorBoundary extends React.Component { constructor(props: any) { super(props); diff --git a/src/views/edit/EditorErrorBoundary.tsx b/src/views/edit/EditorErrorBoundary.tsx new file mode 100644 index 0000000..3f0640e --- /dev/null +++ b/src/views/edit/EditorErrorBoundary.tsx @@ -0,0 +1,134 @@ +import { ChevronLeftIcon, IconButton } from "evergreen-ui"; +import React from "react"; +import { NavigateFunction } from "react-router-dom"; +import { Alert } from "../../components"; +import Titlebar from "../../titlebar/macos"; +import * as Base from "../layout"; +import { Separator } from "./editor/components/Separator"; + +interface State { + hasError: boolean; + error: any; +} + +interface Props { + children: React.ReactNode; + navigate: NavigateFunction; + documentId: string; + journal: string; +} + +/** + * Wraps the editor in an error boundary that catches and displays errors in an editor-friendly way. + */ +export default class EditorErrorBoundary extends React.Component { + constructor(props: any) { + super(props); + this.state = { hasError: false, error: null }; + } + + static getDerivedStateFromError(error: any) { + return { hasError: true, error }; + } + + componentDidCatch(error: any, errorInfo: any) { + console.error( + "EditorErrorBoundary error boundary reached:", + error, + errorInfo, + ); + } + + renderError() { + const { errStr, stack } = decomposeError(this.state.error); + + return ( + + + this.props.navigate("/documents")} + marginRight={8} + > + Back to documents + + + + + {/* This Ghost div is same height as titlebar, so pushes the main content below it -- necessary for the contents scrollbar to make sense */} + + + + + + ); + } + + render() { + if (this.state.hasError) { + return this.renderError(); + } + + return this.props.children; + } +} + +export function decomposeError(error: any) { + let errStr: string = "Unable to parse error for display. Check console? :|"; + let stack: string = ""; + try { + if (error instanceof Error) { + errStr = error.message; + stack = error.stack || ""; + } else if (typeof error === "string") { + errStr = error; + } else { + errStr = JSON.stringify(error, null, 2); + } + errStr = JSON.stringify(error, null, 2); + } catch (err) { + console.error("Error parsing error to string in top-level Error boundary"); + } + + return { errStr, stack }; +} + +export function ErrorContent({ + error, + journal, + documentId, +}: { + error: any; + journal?: string; + documentId?: string; +}) { + const { errStr, stack } = decomposeError(error); + + return ( + +
+

There was an error that crashed the editor

+ {documentId && ( +

+ Offending document:{" "} + {journal || "Unknown Journal"}/{documentId} + .md +

+ )} + {errStr && errStr != "{}" && 
{errStr}
} + 
{stack}
+
+ + ); +} diff --git a/src/views/edit/index.tsx b/src/views/edit/index.tsx index e2c6f43..d7fc2f1 100644 --- a/src/views/edit/index.tsx +++ b/src/views/edit/index.tsx @@ -10,6 +10,7 @@ import Titlebar from "../../titlebar/macos"; import { useSearchStore } from "../documents/SearchStore"; import * as Base from "../layout"; import { EditableDocument } from "./EditableDocument"; +import EditorErrorBoundary from "./EditorErrorBoundary"; import { EditorMode } from "./EditorMode"; import FrontMatter from "./FrontMatter"; import PlateContainer from "./PlateContainer"; @@ -42,8 +43,6 @@ const DocumentLoadingContainer = observer(() => { const journalsStore = useJournals(); const { document: documentId } = useParams(); - // todo: handle missing or invalid documentId; loadingError may be fine for this, but - // haven't done any UX / design thinking around it. const { document, error: loadingError, @@ -54,6 +53,8 @@ const DocumentLoadingContainer = observer(() => { // the current document's journal if its archived const [journals, setJournals] = useState(); + // Filter journals to non-archived ones, but must also add + // the current document's journal if its archived useEffect(() => { if (!document) return; @@ -69,13 +70,24 @@ const DocumentLoadingContainer = observer(() => { }, [document, loadingError]); if (loadingError) { - return ; + return ( + + ); } // `loading` acts as a break when navigating from one document to another, effectively // resetting the state of the editor if (!document || !journals || loading) { - return ; + return ( + + ); } return ; @@ -128,13 +140,19 @@ const DocumentEditView = observer((props: DocumentEditProps) => { selectedEditorMode={selectedViewMode} setSelectedEditorMode={setSelectedViewMode} > - + + + ); }); diff --git a/src/views/edit/loading.tsx b/src/views/edit/loading.tsx index 20103dd..fc28061 100644 --- a/src/views/edit/loading.tsx +++ b/src/views/edit/loading.tsx @@ -1,12 +1,15 @@ -import { ChevronLeftIcon, IconButton, Pane } from "evergreen-ui"; +import { ChevronLeftIcon, IconButton } from "evergreen-ui"; import { observer } from "mobx-react-lite"; import React from "react"; import { useNavigate } from "react-router-dom"; import Titlebar from "../../titlebar/macos"; +import { ErrorContent } from "./EditorErrorBoundary"; import { Separator } from "./editor/components/Separator"; export interface LoadingComponentProps { error?: Error | null; + documentId?: string; + journal?: string; } export const placeholderDate = new Date().toISOString().slice(0, 10); @@ -30,13 +33,13 @@ export const EditLoadingComponent = observer((props: LoadingComponentProps) => { - - - - {props.error && props.error?.message} - - - + {props.error && ( + + )} ); }); diff --git a/src/views/edit/useEditableDocument.ts b/src/views/edit/useEditableDocument.ts index a8fa964..bccac94 100644 --- a/src/views/edit/useEditableDocument.ts +++ b/src/views/edit/useEditableDocument.ts @@ -14,7 +14,7 @@ interface LoodingState { /** * Load a new or existing document into a view model */ -export function useEditableDocument(documentId: string) { +export function useEditableDocument(documentId?: string) { const navigate = useNavigate(); const client = useClient(); const [state, _] = React.useState(() => { @@ -33,24 +33,17 @@ export function useEditableDocument(documentId: string) { async function load() { state.error = null; + // NOTE: If this navigation removed, MUST review consumption of useEditableDocument + // to ensure missing documentId is handled correctly. if (!documentId) { - // Fail safe; this shouldn't happen. If scenarios come up where it could; maybe toast and navigate - // to documents list instead? - state.error = new Error( - "Called useEditableDocument without a documentId, unable to load document", - ); - + navigate("/documents"); + toast.warning(`Document ${documentId} not found`); return; } try { const doc = await client.documents.findById({ id: documentId }); if (!isEffectMounted) return; - if (!doc) { - navigate("/documents"); - toast.warning("Document not found, redirecting to documents list"); - return state; - } state.document = new EditableDocument(client, doc); @@ -60,7 +53,14 @@ export function useEditableDocument(documentId: string) { state.loading = false; }); } catch (err) { - state.error = err as Error; + const message = err instanceof Error ? err.message : String(err); + + if (message.match(/not found/)) { + navigate("/documents"); + toast.warning(`Document ${documentId} not found`); + } else { + state.error = err as Error; + } } } diff --git a/src/views/layout.tsx b/src/views/layout.tsx index 12b0f1c..4145a02 100644 --- a/src/views/layout.tsx +++ b/src/views/layout.tsx @@ -64,7 +64,7 @@ export const ScrollContainer: React.FC = ({ }) => { return (
{children}