fix: guard Notification.isSupported for standalone/Docker (#42)#67
fix: guard Notification.isSupported for standalone/Docker (#42)#67matt1398 merged 1 commit intomatt1398:mainfrom
Conversation
Add typeof checks before calling Notification.isSupported() to prevent crashes in environments where the Notification API is unavailable.
Summary of ChangesHello @Psypeal, 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 enhances the application's robustness by implementing defensive programming around the Highlights
Changelog
Activity
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
|
There was a problem hiding this comment.
Code Review
The pull request correctly addresses potential crashes in environments where the Electron Notification API is unavailable, such as Docker or standalone mode. The added guards are effective and ensure the application remains stable in headless environments. The implementation can be made more concise by leveraging modern JavaScript features like optional chaining, which is already supported in the project.
| if ( | ||
| typeof Notification === 'undefined' || | ||
| typeof Notification.isSupported !== 'function' || | ||
| !Notification.isSupported() | ||
| ) { |
There was a problem hiding this comment.
This guard can be simplified using optional chaining for better readability and conciseness. This approach safely handles cases where Notification is undefined or isSupported is not a function, which is the primary goal of this fix. Since the project already uses modern syntax (like the nullish coalescing assignment operator), this is consistent with the existing codebase.
if (!Notification?.isSupported?.()) {
📝 WalkthroughWalkthroughAdds defensive null/type checks to the NotificationManager's Electron Notification API usage to handle environments where the Notification API is unavailable, preventing runtime errors when attempting to call potentially undefined methods. Changes
Suggested labels
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/services/infrastructure/NotificationManager.ts`:
- Around line 375-384: The inline runtime guard in showNativeNotification that
checks Notification support should be extracted into a clearly named type-guard
function (e.g., isNotificationSupported) to follow the isXxx convention; create
a new function isNotificationSupported(): boolean that performs the existing
typeof Notification and Notification.isSupported checks, replace the inline
condition in showNativeNotification with a call to isNotificationSupported(),
and update any imports/exports or tests that reference this check to use the new
function.
| private showNativeNotification(error: DetectedError): void { | ||
| // Check if Notification is supported | ||
| if (!Notification.isSupported()) { | ||
| // Guard against standalone/Docker mode where Electron's Notification API is unavailable | ||
| if ( | ||
| typeof Notification === 'undefined' || | ||
| typeof Notification.isSupported !== 'function' || | ||
| !Notification.isSupported() | ||
| ) { | ||
| logger.warn('Native notifications not supported'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and verify the NotificationManager.ts file
fd -t f "NotificationManager.ts" --exec wc -l {} \;Repository: matt1398/claude-devtools
Length of output: 129
🏁 Script executed:
# Check the actual code at lines 375-384
fd -t f "NotificationManager.ts" --exec sed -n '370,390p' {} +Repository: matt1398/claude-devtools
Length of output: 835
🏁 Script executed:
# Search for existing type guards following isXxx pattern
rg -n --type=ts "^\s*(?:private|public|static)?\s+\w*is[A-Z][a-zA-Z0-9]*\s*\(" --max-count=20Repository: matt1398/claude-devtools
Length of output: 2741
🏁 Script executed:
# Check if there's a types/notifications.ts file as mentioned in learnings
fd -t f "types/notifications.ts" --exec cat {} +Repository: matt1398/claude-devtools
Length of output: 50
🏁 Script executed:
# Search for existing Notification support checks in codebase
rg -n --type=ts "Notification\.isSupported|typeof Notification" -C 2Repository: matt1398/claude-devtools
Length of output: 789
Extract an isXxx type guard for Notification support checks.
Runtime guards should follow the isXxx naming convention per coding guidelines. The inline typeof checks should be extracted into a type guard method.
Suggested refactor
+ private static isNotificationSupported(): boolean {
+ return (
+ typeof Notification !== 'undefined' &&
+ typeof Notification.isSupported === 'function' &&
+ Notification.isSupported()
+ );
+ }
+
private showNativeNotification(error: DetectedError): void {
// Guard against standalone/Docker mode where Electron's Notification API is unavailable
- if (
- typeof Notification === 'undefined' ||
- typeof Notification.isSupported !== 'function' ||
- !Notification.isSupported()
- ) {
+ if (!NotificationManager.isNotificationSupported()) {
logger.warn('Native notifications not supported');
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private showNativeNotification(error: DetectedError): void { | |
| // Check if Notification is supported | |
| if (!Notification.isSupported()) { | |
| // Guard against standalone/Docker mode where Electron's Notification API is unavailable | |
| if ( | |
| typeof Notification === 'undefined' || | |
| typeof Notification.isSupported !== 'function' || | |
| !Notification.isSupported() | |
| ) { | |
| logger.warn('Native notifications not supported'); | |
| return; | |
| } | |
| private static isNotificationSupported(): boolean { | |
| return ( | |
| typeof Notification !== 'undefined' && | |
| typeof Notification.isSupported === 'function' && | |
| Notification.isSupported() | |
| ); | |
| } | |
| private showNativeNotification(error: DetectedError): void { | |
| // Guard against standalone/Docker mode where Electron's Notification API is unavailable | |
| if (!NotificationManager.isNotificationSupported()) { | |
| logger.warn('Native notifications not supported'); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/services/infrastructure/NotificationManager.ts` around lines 375 -
384, The inline runtime guard in showNativeNotification that checks Notification
support should be extracted into a clearly named type-guard function (e.g.,
isNotificationSupported) to follow the isXxx convention; create a new function
isNotificationSupported(): boolean that performs the existing typeof
Notification and Notification.isSupported checks, replace the inline condition
in showNativeNotification with a call to isNotificationSupported(), and update
any imports/exports or tests that reference this check to use the new function.
Summary
Fixes #42
Changes
Test plan
Summary by CodeRabbit