-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Add system notifications #6971
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add system notifications #6971
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution! I've reviewed the system notification implementation and found several issues that need attention before this can be merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling feedback to users. When notifications fail, users have no way to know. Consider implementing a fallback mechanism:
| console.error("Could not show system notification", error) | |
| } catch (error) { | |
| console.error("Could not show system notification", error); | |
| // Fallback to VSCode's built-in notification | |
| vscode.window.showInformationMessage(options.message); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Windows implementation hardcodes "Roo Code" which might not work correctly in all configurations. Could we make this configurable or detect the actual application name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 100ms debounce is hardcoded. Would it be beneficial to make this configurable for different use cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The NotificationOptions interface is quite basic. Based on the issue requirements, shouldn't it support priority levels and action buttons?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not supported yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good integration with the chat events! However, consider adding test coverage for these notification triggers to ensure they work correctly across different scenarios.
|
I like the idea of this a lot! I'm trying on my Mac though and the test system notification isn't working. Any ideas? |
|
@mrubens I need to confirm whether the click test on the settings page works. Unlike Kilo, my commits don't display notifications when the VSCode window is focused. This is because I'm using the secondary sidebar, and the system notifications obscure my send message button. |
|
I tested it on my Mac Mini, but I still couldn't get notifications, and Kilo's notification feature didn't work either. The reason is that the following script no longer works on macOS: osascript -e 'display notification "hello" with title "word" sound name "Frog" ' If I change it to: osascript -e 'display alert "hello" message "world"' |
|
@mrubens After more detailed testing, it is found that notifications can only be displayed when the script editor is allowed to notify and the user is not in do not disturb mode. This should be a limitation of macOS itself.
|
daniel-lxs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for implementing system notifications! I've reviewed the changes and found some issues that need attention before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the 100ms debounce intentional? This could cause distinct notifications to be dropped if they occur within the same window. For example, if both a "tool request" and "command" notification trigger within 100ms, only the last one will be shown.
Consider using throttling instead of debouncing, or implementing message-based deduplication to avoid losing important notifications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This design is inspired by Kilo. Switching to throttling is indeed more reasonable, as it helps send notifications faster. However, I need to introduce throttle-debounce as a dependency—is that acceptable?
Additionally, the two different types of notifications you described are unlikely to trigger within 100 milliseconds.
This is because notifications are limited to completion, failure, and waiting for user input—these three scenarios are unlikely to occur within 100 ms. Moreover, unlike Kilo, my notification requires VS Code not to be the currently focused window in order to trigger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case I think we can keep it like this unless we get reports of it causing issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Linux implementation assumes notify-send is available but doesn't handle the case where it's not installed. Consider checking for the binary first or providing a helpful error message:
| throw new Error(`Failed to show Linux notification: ${error}`) | |
| } catch (error: any) { | |
| if (error.code === 'ENOENT') { | |
| throw new Error('notify-send is not installed. Please install libnotify-bin (apt install libnotify-bin on Debian/Ubuntu)') | |
| } | |
| throw new Error(`Failed to show Linux notification: ${error}`) | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix
daniel-lxs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @NaccOll!
032c82b to
814040a
Compare
|
I still can't get this to work on Mac. I like the idea of system notifications but I think we have to make it more obvious to people how to set it up, otherwise it just seems broken. |
I have a Mac mini, but I'm not familiar with Mac OS, so writing a guide for Mac users is a challenge. Should I change "display notification" to "display alert"? This won't allow use the various Mac notification settings, but it will avoid the complicated setup process. |
|
@NaccOll Are the alerts visible if the window is in the background, if so then I think it makes sense. |
The advantage is that it no requires user configuration; simply toggle it on in RooCode and you'll receive alert when VSCode is not focused. However, even with Do Not Disturb enabled on mac, it still works. Is there a way to change notification on Mac to alert? |
47ba79e to
814040a
Compare
814040a to
2d3d43d
Compare
|
Without changing the existing implementation, I followed https://forum.latenightsw.com/t/trying-to-use-terminal-for-display-notification/5068/6 and enabled notification permissions in the script editor. System notifications are working on my mac mini. |
2d3d43d to
faf1d1e
Compare
1eebada to
faf1d1e
Compare
|
notification permissions in the script editor is not a sollution that works IMO. Closing. |



Related GitHub Issue
Closes: #5015 (comment)
Roo Code Task Context (Optional)
Description
Implementing system notifications similar to Kilo. But it is triggered only when vscode is not the focused window.
Currently, different system notifications are displayed to users for errors, pending user approval, and task completion.
However, user customization of issues, priority levels, and the ability to directly complete actions when prompted by system notifications are not yet implemented.
Test Procedure
You can try all scene with sound effects occur
Pre-Submission Checklist
Screenshots / Videos
Documentation Updates
Additional Notes
Get in Touch
Important
Adds system notifications for various events with platform-specific logic and UI settings, enabling users to receive alerts for important actions.
systemNotificationsEnabledinglobal-settings.tsandClineProvider.ts.showSystemNotification()function inshowSystemNotification.tshandles notification display with debounce.NotificationSettings.tsxto include system notification settings and test button.ChatView.tsxtriggers notifications for specific events.notifications/index.tsfor macOS, Windows, and Linux.settings.jsonfor various languages.This description was created by
for 5ec272163d0264a2250c14bab639e60b068baa56. You can customize this summary. It will automatically update as commits are pushed.