Conversation
| import { SidebarFilter, type SidebarFilterGroup } from "@site/src/components/ui/sidebar-filter"; | ||
| import { Menu, X } from "lucide-react"; | ||
| import Link from '@docusaurus/Link'; | ||
| import { errorMessage } from '../../../ui/desktop/src/utils/conversionUtils'; |
There was a problem hiding this comment.
Are we sure we want to import from desktop what if we want to split out the documentation from this repo one day?
There was a problem hiding this comment.
Pull request overview
This pull request refactors error handling across the codebase to use a centralized errorMessage utility function instead of repetitive inline error type checking patterns. The utility function extracts error messages from Error instances or converts other types to strings with an optional default fallback.
Changes:
- Replaced inline
error instanceof Error ? error.message : 'default'patterns with calls to theerrorMessageutility function - Added imports of
errorMessagefromconversionUtilsacross multiple files in the desktop UI and documentation - Renamed local variables in some locations to avoid shadowing the imported function
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 25 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/desktop/src/utils/sessionCache.ts | Simplified error handling in loadSession |
| ui/desktop/src/utils/ollamaDetection.ts | Refactored error message extraction in checkOllamaStatus |
| ui/desktop/src/utils/githubUpdater.ts | Updated multiple error handling sites to use errorMessage utility |
| ui/desktop/src/utils/autoUpdater.ts | Consolidated error message extraction in update handlers |
| ui/desktop/src/sessionLinks.ts | Renamed local variable to avoid shadowing, uses errorMessage utility |
| ui/desktop/src/main.ts | Simplified error message extraction in multiple IPC handlers |
| ui/desktop/src/hooks/useWhisper.ts | Updated error handling in recording start logic |
| ui/desktop/src/hooks/useFileDrop.ts | Simplified file drop error handling |
| ui/desktop/src/components/settings/models/modelInterface.ts | Renamed import to avoid shadowing, proper usage |
| ui/desktop/src/components/settings/config/ConfigSettings.tsx | Added import (with syntax error) and incorrect usage |
| ui/desktop/src/components/settings/app/UpdateSection.tsx | Added import (incorrect path) and usage |
| ui/desktop/src/components/sessions/SessionsInsights.tsx | Simplified error handling in insights loading |
| ui/desktop/src/components/sessions/SessionListView.tsx | Renamed variables to avoid shadowing, uses utility |
| ui/desktop/src/components/sessions/SessionHistoryView.tsx | Updated error handling in session operations |
| ui/desktop/src/components/schedule/*.tsx | Multiple schedule-related files updated to use errorMessage |
| ui/desktop/src/components/recipes/*.tsx | Multiple recipe-related files updated with syntax errors |
| ui/desktop/src/components/apps/*.tsx | Updated app-related error handling |
| ui/desktop/src/components/alerts/AlertBox.tsx | Simplified error handling in alert operations |
| ui/desktop/src/components/OllamaSetup.tsx | Added import (incorrect path) and incorrect usage |
| ui/desktop/src/components/ModelAndProviderContext.tsx | Added import (incorrect path, malformed) and incorrect usage |
| ui/desktop/src/components/McpApps/McpAppRenderer.tsx | Added import (incorrect path) and usage |
| ui/desktop/src/components/MCPUIResourceRenderer.tsx | Added import (incorrect path) and questionable usage |
| ui/desktop/src/components/ExtensionInstallModal.tsx | Added import (incorrect path) and variable shadowing |
| ui/desktop/src/App.tsx | Updated error handling in app initialization |
| documentation/src/pages/recipes/index.tsx | Added cross-module import and variable shadowing |
| documentation/src/pages/prompt-library/index.tsx | Added cross-module import and variable shadowing |
| documentation/src/pages/extensions/index.tsx | Added cross-module import and variable shadowing |
| import { errorMessage } from '../../../ui/desktop/src/utils/conversionUtils'; | ||
|
|
There was a problem hiding this comment.
This import path crosses module boundaries, importing from the desktop app into the documentation site. This creates an unintended dependency and will likely fail at build time. Consider either duplicating the errorMessage utility in the documentation codebase or using a shared utilities package.
| import { errorMessage } from '../../../ui/desktop/src/utils/conversionUtils'; | |
| function errorMessage(error: unknown): string { | |
| if (typeof error === "string") { | |
| return error; | |
| } | |
| if (error instanceof Error) { | |
| return error.message || "An unknown error occurred."; | |
| } | |
| try { | |
| const serialized = JSON.stringify(error); | |
| return serialized === "{}" ? "An unknown error occurred." : serialized; | |
| } catch { | |
| return "An unknown error occurred."; | |
| } | |
| } |
| import Layout from "@theme/Layout"; | ||
| import Link from "@docusaurus/Link"; | ||
| import { Wand2 } from "lucide-react"; | ||
| import { errorMessage } from '../../../ui/desktop/src/utils/conversionUtils'; |
There was a problem hiding this comment.
This import path crosses module boundaries, importing from the desktop app into the documentation site. This creates an unintended dependency and will likely fail at build time. Consider either duplicating the errorMessage utility in the documentation codebase or using a shared utilities package.
| import { View, ViewOptions } from '../utils/navigationUtils'; | ||
| import { useConfig } from './ConfigContext'; | ||
| import { toastService } from '../toasts'; | ||
| import { errorMessage } from '../../utils/conversionUtils'; |
There was a problem hiding this comment.
The import path is incorrect. From ui/desktop/src/components/ExtensionInstallModal.tsx, the correct path to conversionUtils should be ../utils/conversionUtils, not ../../utils/conversionUtils.
| import { errorMessage } from '../../utils/conversionUtils'; | |
| import { errorMessage } from '../utils/conversionUtils'; |
| msg: `Failed to save recipe: ${error instanceof Error ? error.message : 'Unknown error'}`, | ||
| traceback: error instanceof Error ? error.message : String(error), | ||
| msg: `Failed to save recipe: ${errorMessage(error, 'Unknown error')}`, | ||
| traceback: errorMessage(error, String)(error), |
There was a problem hiding this comment.
This syntax is incorrect. The errorMessage function doesn't return a function. This should be: errorMessage(error) or errorMessage(error, String(error)).
| msg: `Failed to save and run recipe: ${error instanceof Error ? error.message : 'Unknown error'}`, | ||
| traceback: error instanceof Error ? error.message : String(error), | ||
| msg: `Failed to save and run recipe: ${errorMessage(error, 'Unknown error')}`, | ||
| traceback: errorMessage(error, String)(error), |
There was a problem hiding this comment.
This syntax is incorrect. The errorMessage function doesn't return a function. This should be: errorMessage(error) or errorMessage(error, String(error)).
| import { SidebarFilter, type SidebarFilterGroup } from "@site/src/components/ui/sidebar-filter"; | ||
| import { Menu, X } from "lucide-react"; | ||
| import Link from '@docusaurus/Link'; | ||
| import { errorMessage } from '../../../ui/desktop/src/utils/conversionUtils'; |
There was a problem hiding this comment.
This import path crosses module boundaries, importing from the desktop app into the documentation site. This creates an unintended dependency and will likely fail at build time. Consider either duplicating the errorMessage utility in the documentation codebase or using a shared utilities package.
| const errorMessage = errorMessage(err, "Unknown error"); | ||
| setError(`Failed to load recipes: ${errorMessage}`); |
There was a problem hiding this comment.
Variable shadowing: the constant errorMessage shadows the imported function errorMessage. This creates a recursive call where the variable is assigned to itself, resulting in a runtime error. Change the variable name to something like errorMsg or errMsg.
| const errorMessage = errorMessage(err, "Unknown error"); | |
| setError(`Failed to load recipes: ${errorMessage}`); | |
| const errorMsg = errorMessage(err, "Unknown error"); | |
| setError(`Failed to load recipes: ${errorMsg}`); |
| import Layout from "@theme/Layout"; | ||
| import Link from "@docusaurus/Link"; | ||
| import { Wand2 } from "lucide-react"; | ||
| import { errorMessage } from '../../../ui/desktop/src/utils/conversionUtils'; |
There was a problem hiding this comment.
Unused import errorMessage.
| import { PillFilter, type PillFilterOption } from "@site/src/components/ui/pill-filter"; | ||
| import { SidebarFilter, type SidebarFilterGroup } from "@site/src/components/ui/sidebar-filter"; | ||
| import { Menu, X } from 'lucide-react'; | ||
| import { errorMessage } from '../../../ui/desktop/src/utils/conversionUtils'; |
There was a problem hiding this comment.
Unused import errorMessage.
| import { errorMessage } from '../../../ui/desktop/src/utils/conversionUtils'; |
| import { SidebarFilter, type SidebarFilterGroup } from "@site/src/components/ui/sidebar-filter"; | ||
| import { Menu, X } from "lucide-react"; | ||
| import Link from '@docusaurus/Link'; | ||
| import { errorMessage } from '../../../ui/desktop/src/utils/conversionUtils'; |
There was a problem hiding this comment.
Unused import errorMessage.
| import { | ||
| import { errorMessage } from '../../utils/conversionUtils'; |
There was a problem hiding this comment.
The import statement is malformed - there's a duplicate import { keyword. Line 11 should continue with the Dialog imports, not start a new import statement. This will cause a syntax error.
| import { | |
| import { errorMessage } from '../../utils/conversionUtils'; | |
| import { errorMessage } from '../../utils/conversionUtils'; | |
| import { |
| dismissModal(); | ||
| } catch (error) { | ||
| const errorMessage = error instanceof Error ? error.message : 'Installation failed'; | ||
| const errorMessage = errorMessage(error, 'Installation failed'); |
There was a problem hiding this comment.
Variable name shadowing: the local variable errorMessage on line 256 shadows the imported errorMessage function. The code attempts to call errorMessage(error, 'Installation failed') but assigns the result to a variable also named errorMessage, which will cause a runtime error because it's trying to call the string result as a function.
| code: UIActionErrorCode.PROMPT_FAILED, | ||
| message: 'Failed to send prompt to chat', | ||
| details: error instanceof Error ? error.message : error, | ||
| details: errorMessage(error, error), |
There was a problem hiding this comment.
The second argument to errorMessage() should be a string fallback message, but here error (the caught error object) is being passed. This should use a string like 'Unknown error' instead.
| details: errorMessage(error, error), | |
| details: errorMessage(error, 'Unknown error'), |
| code: UIActionErrorCode.NAVIGATION_FAILED, | ||
| message: `Unexpected error opening URL: ${url}`, | ||
| details: error instanceof Error ? error.message : error, | ||
| details: errorMessage(error, error), |
There was a problem hiding this comment.
The second argument to errorMessage() should be a string fallback message, but here error (the caught error object) is being passed. This should use a string like 'Unknown error' instead.
| details: errorMessage(error, error), | |
| details: errorMessage(error, 'Unexpected error opening URL'), |
Co-authored-by: Douwe Osinga <douwe@squareup.com> Signed-off-by: Elias Posen <elias@posen.ch>
Summary
We have a handy errorMessage uitlity that we weren't using all over