Add error boundary to isolate extension crashes from main app#1964
Add error boundary to isolate extension crashes from main app#1964
Conversation
Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
✅ Deploy Preview for hyprnote ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for hyprnote-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThe changes enhance the extensions tab UI with error resilience by introducing an ExtensionErrorBoundary component that captures rendering errors from extension components. A retry mechanism allows users to remount extensions after failures via state-driven remounting logic. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/desktop/src/components/main/body/extensions/index.tsx (2)
1-90: Error boundary implementation is solid; retry state reset is slightly redundant
- The
ExtensionErrorBoundaryclass follows the standard React error-boundary pattern (getDerivedStateFromError, localhasErrorflag, and a clear fallback UI with extension id and message). Imports forComponent/ReactNodeandAlertTriangleIconare correct and used appropriately.- Because the boundary is keyed at the call site (
key={retryKey}), changingretryKeyalready forces a full unmount/remount of the boundary and its children. ThehandleRetrycall tothis.setState({ hasError: false, error: null })is therefore redundant in practice—you could simplifyhandleRetryto just callthis.props.onRetry()if you like, keeping behavior the same.This is a nit; current code is correct and safe as-is.
161-245: Retry key wiring correctly remounts extensions; behavior separation from loader looks good
- Using
retryKeystate pluskey={retryKey}onExtensionErrorBoundaryis an effective way to remount the extension subtree on “Try again” without re-invokingloadExtensionUI. That cleanly separates initial load failures (handled by the loader + “failed to load” UI) from runtime render errors (handled by the boundary).- Excluding
retryKeyfrom theuseEffectdependency array is appropriate here, since a retry should not cause the loader to re-run—only to re-mount the already-registeredLoadedComponent.- If you ever decide that retrying should also re-run the loader (e.g., for transient dynamic-import failures), you could introduce a separate
loadVersionkey that’s included in the effect deps and also used as the boundary key.Also nice that this continues to use
Reorderfrommotion/reactandcn([...])as per the repo’s guidelines. Based on learnings, this aligns with the preferred motion and className patterns.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/components/main/body/extensions/index.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/desktop/src/components/main/body/extensions/index.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-24T16:32:19.706Z
Learnt from: CR
Repo: fastrepl/hyprnote PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T16:32:19.706Z
Learning: Applies to **/*.{ts,tsx} : Use `motion/react` instead of `framer-motion`.
Applied to files:
apps/desktop/src/components/main/body/extensions/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: fmt
- GitHub Check: ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: ci (macos, depot-macos-14)
- GitHub Check: ci (linux, depot-ubuntu-22.04-8)
…e-based extension system Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
Add error boundary to isolate extension crashes from main app
Summary
Adds a local
ExtensionErrorBoundarycomponent that wraps extension content, preventing extension errors from crashing the entire app view. When an extension throws an error, users now see a contained error message with a "Try again" button instead of the global "Something went wrong" screen.The original issue reported was a
useStore is undefinederror when opening the hello-world extension. Investigation revealed this is likely caused by a stale extension bundle on the user's machine (the current source code doesn't useuseStore). This PR addresses the error isolation requirement - the user may still need to reinstall the extension to fix the root cause.Updates since last revision
Review & Testing Checklist for Human
Test Plan
pnpm run build:hello-world && pnpm run install:devin/extensions)Notes
useStoreerror appears to be a stale extension bundle. The user should runpnpm run install:devin the extensions directory to reinstall with the latest build.Link to Devin run: https://app.devin.ai/sessions/02ea0bd1a5d74223b3babd724f7ace9c
Requested by: yujonglee (@yujonglee)