fix(frontend): fix memory leaks, unhandled promises, and add error bo… #257
Conversation
Summary of ChangesHello @jmanhype, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the frontend's robustness and stability by addressing several common issues. It introduces a global error boundary for graceful error handling and Sentry reporting, prevents unhandled promise rejections in asynchronous operations, mitigates memory leaks associated with audio playback, and safeguards against state updates on unmounted components. These changes collectively contribute to a more resilient and reliable user experience. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several valuable fixes for memory leaks, unhandled promises, and adds a crucial ErrorBoundary component to enhance application stability. The implementations are solid and follow best practices. The memory leak fix in useTaskCompletionNotification is particularly well-handled. I have one minor suggestion in useDraftAutosave.ts to remove a redundant line of code for better clarity and maintainability.
| useEffect(() => { | ||
| isMountedRef.current = true; | ||
| return () => { | ||
| isMountedRef.current = false; | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
The line isMountedRef.current = true; inside this useEffect is redundant. The ref is already initialized with useRef(true), and this effect runs after the component has mounted. The effect only needs to contain the cleanup logic to set the ref to false on unmount. This change simplifies the effect.
useEffect(() => {
return () => {
isMountedRef.current = false;
};
}, []);
|
@codex review this pr |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "@radix-ui/react-slider": "^1.3.6", | ||
| "@radix-ui/react-toggle-group": "^1.1.11", | ||
| "framer-motion": "^12.23.22", |
There was a problem hiding this comment.
The lodash dependencies were dropped from the manifests here (and @types/lodash was removed from frontend/package.json), but pnpm-lock.yaml still lists both packages under the workspace importers. Any install that enforces the lock (e.g., pnpm install --frozen-lockfile) will now fail with an out‑of‑date lockfile error because the lock still expects lodash. Please regenerate pnpm-lock.yaml to keep it aligned with these removals.
Useful? React with 👍 / 👎.
…undary - useConversationHistory: Add .catch() to fire-and-forget async call to prevent unhandled promise rejections - useTaskCompletionNotification: Track and cleanup audio elements on unmount to prevent memory leaks - useDraftAutosave: Add isMountedRef guard to prevent setState calls on unmounted components - App: Add ErrorBoundary wrapper with Sentry integration for graceful error handling instead of white screen crashes Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
- Replace lodash cloneDeep with native structuredClone() - Replace lodash merge with custom deepMerge utility - Replace lodash isEqual with custom isEqual utility - Remove lodash from root package.json dependencies - Remove @types/lodash from frontend devDependencies The custom utilities in lib/utils.ts handle the same use cases with zero external dependencies. Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
jmanhype
force-pushed
the
claude/pull-latest-changes-01AkXi3KKw2iBTPazmTf9VGT
branch
from
6ea6f8e to
6eb67f8
Compare
|
@codex review this pr |
1 similar comment
|
@codex review this pr |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| playNotificationSound( | ||
| config.notifications.sound_file, | ||
| config.notifications.sound_volume ?? 0.3, |
There was a problem hiding this comment.
The notification hook now passes config.notifications.sound_volume into playNotificationSound, but NotificationConfig (shared/types.ts around the Config definition) only defines sound_enabled, push_enabled, and sound_file. This property does not exist, so the frontend no longer type-checks/builds, and even if it did, the value is always undefined meaning the default volume is always used. Either add a volume field to the config schema or remove this access.
Useful? React with 👍 / 👎.
|
@codex review this pr |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| playNotificationSound( | ||
| config.notifications.sound_file, | ||
| config.notifications.sound_volume ?? 0.3, |
There was a problem hiding this comment.
useTaskCompletionNotification now passes config.notifications.sound_volume into playNotificationSound, but NotificationConfig in shared/types.ts (lines 101‑104) only exposes sound_enabled, push_enabled, and sound_file. The new property does not exist in the typed config, so TypeScript checks (npm run frontend:check) will fail and the code always falls back to 0.3 volume. Either add a real volume field to the shared type or remove this access.
Useful? React with 👍 / 👎.
3 participants
…undary