feat: Implement desktop notifications for user permission prompts#14927
feat: Implement desktop notifications for user permission prompts#14927StoyanD wants to merge 48 commits intogoogle-gemini:mainfrom
Conversation
…ompts This commit introduces desktop notifications to alert users when the Gemini CLI requires their permission to execute a command, especially when the CLI application is not in focus. Key changes include: - Added setting to control notification behavior. - Integrated for cross-platform desktop notifications. - Implemented React hook to manage notification logic, considering application focus, operating system, and terminal program for proper activation on macOS. - Included an icon asset for notifications.
Ensures that the desktop notification for user permission prompts is displayed only once per confirmation event. Toggling application focus while waiting for confirmation will no longer trigger repeated notifications. - Added `useRef` to track notification state. - Updated `useNotification` hook to reset state only when confirmation is no longer pending. - Added regression tests for duplicate notification scenarios.
Summary of ChangesHello @StoyanD, 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 user experience of the Gemini CLI by integrating desktop notifications. When the CLI requires user confirmation or input, and the application is not in focus, a system-level notification will now be triggered. This feature aims to improve multitasking efficiency by providing immediate alerts, with the added benefit on macOS of bringing the terminal back into focus upon clicking the notification. The functionality is controlled by a new "ui.enableNotifications" setting, allowing users to opt-in. 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces desktop notifications for user permission prompts, which is a great enhancement for user experience, especially when multitasking. The implementation is solid, leveraging node-notifier and a new React hook useNotification. The logic correctly handles different states, platform specifics for macOS, and terminals with poor focus detection support. The addition of comprehensive unit tests for the new hook is commendable and ensures the feature is robust. The configuration options are well-documented and integrated into the settings schema. Overall, this is a high-quality contribution, and I have no high or critical concerns.
|
@Alexmarco-gif can i get a review? |
|
why anyone not reviewing i would love to havethis feature in windows |
|
@nahid6970 do u have a windows machine? if so can you test this branch out to see if it works( im pretty sure it should) |
|
@StoyanD how to test it in windows? i dont see anything related to notification in windows latest version of gemini |
|
@nahid6970 you'd have to install this branch locally on a windows machine and test. The notification library I'm using is cross platform, so it should work. The way I've tested is using a command to create a file, then opening another app before the cli asks for permission. When the cli app asks for permission it's already backgrounded so it shows a notification |
|
This will be a big improvement to the user experience. I'm waiting for this. |
|
Thanks |
There was a problem hiding this comment.
Code Review
This pull request introduces desktop notifications for user permission prompts, which is a great enhancement for user experience. The implementation is solid, adding a new NotificationService and integrating it with the existing configuration and event systems. I've found one critical issue related to string quoting in a platform-specific command which would cause a crash on Windows, and another high-severity issue for using a synchronous, blocking call where an asynchronous one would be safer and more performant, aligning with our guidelines on avoiding blocking the event loop. Once these are addressed, the feature should be in good shape.
Replace blocking execSync with exec in focusUtils to improve event loop performance. Update NotificationService to handle async focus checks.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a well-implemented desktop notification system for user permission prompts, which is a significant improvement to the user experience, especially for those who multitask. The implementation is robust, featuring a new NotificationService, integration with the policy engine through a notify flag, and a sophisticated focus detection mechanism with OS-level checks and fallbacks. The inclusion of terminal-specific notifications for iTerm, Kitty, and Warp demonstrates great attention to detail. The code is well-structured, thoroughly tested, and all relevant documentation and configuration schemas have been updated. Overall, this is a high-quality contribution with no major issues identified.
| default: | ||
| return `Tool requires confirmation`; | ||
| } | ||
| const name = confirmationDetails.title.startsWith('Confirm: ') |
Pull Request ReviewI have reviewed the changes in PR #14927. Overall, the implementation of desktop notifications is a great addition to the user experience. However, I've identified a few areas for improvement to ensure consistency with project standards and to prevent potential bugs. Core Architecture & Memory Management
Test Quality & Best Practices
General Observations
Please let me know if you have any questions about these suggestions! |
|
Hi, thanks for addressing my comments! Sorry for getting back to you now, I was a bit busy this week 😅 |
- Propagate notification settings dynamically without restart - Prevent memory leaks by disposing NotificationService in Config - Improve tests with MockMessageBus and idiomatic platform spying - Fix TypeScript errors in NotificationService callbacks
|
@Adib234 thanks for the reviews. i updated the pr |
|
hi the build is failing |
…policy-notifications
…-cli into feat/policy-notifications
jackwotherspoon
left a comment
There was a problem hiding this comment.
@StoyanD I did a quick pass.
We definitely want this to be disabled by default and be opt-in for the first little while.
Once we have more users testing it and any bugs or issues (Windows/Linux support) that may arise have been fixed we can then make it enabled by default.
Also I think we want to also notify when agent is finished, not just on tool/prompt confirmations.
Let me know what you think.
| | Show Spinner | `ui.showSpinner` | Show the spinner during operations. | `true` | | ||
| | Enable Loading Phrases | `ui.accessibility.enableLoadingPhrases` | Enable loading phrases during operations. | `true` | | ||
| | Screen Reader Mode | `ui.accessibility.screenReader` | Render output in plain-text to be more screen reader accessible | `false` | | ||
| | Enable Notifications | `ui.enableNotifications` | Enable notifications for user permission prompts when the CLI is not in focus. Tested on mac, experimental on win/linux | `true` | |
There was a problem hiding this comment.
ui components should be Gemini CLI UI related flags.
Notifications are not a Gemini CLI UI specific setting, would rather see this move to general I think.
| - **Description:** Enable notifications for user permission prompts when the | ||
| CLI is not in focus. Tested on mac, experimental on win/linux | ||
| - **Default:** `true` |
There was a problem hiding this comment.
Should not default to true to begin with. Especially when we are saying this is potentially a macOS only feature. This should be opt-in to begin with and default to false.
Also does this only notify when use confirmation is needed? Or does it also notify when agent is finished?
I think we would want notifications on agent being done as well.
There was a problem hiding this comment.
is there a reason we can't leverage existing useFocus for some of this?
|
Inspired by this PR we pulled out the MacOS stuff (which we felt is more stable) and will be merging our own version in #19056. The linux/win stuff feels shakier to us so we're leaving that out for now. Closing this out as support for non-macos should build on top of #19056. Thanks for the contribution! |
|
As a windows user, my disappointment is immeasurable and my day is ruined. |
Summary
Implements desktop notifications for permissions, should ease multitasking when
using gemini-cli
Details
Fixes #14696
This PR implements OS-level desktop notifications when the Gemini CLI pauses and
requires user confirmation or input. This significantly improves the user
experience by alerting users immediately when their input is needed, even if
they are focused on another application. Clicking the notification will bring
the terminal into focus, allowing for quick interaction.
Notification demo video:
policy-notification-promt.mp4
Settings toggle:

Related Issues
Related to #14696
How to Validate
Request a policy ask required permission in Xterm/VScode terminal, and move to
another screen causing focus to switch. Should see a notification that when
clicked takes you back to the terminal
Pre-Merge Checklist