-
Notifications
You must be signed in to change notification settings - Fork 10.5k
feat(cli): add terminal bell notification on response completion #14809
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @afarber, 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 user experience of the CLI by adding an optional audible terminal bell notification. This feature provides immediate feedback to users when a long-running operation, such as receiving a response or requiring tool approval, has completed, ensuring they are promptly alerted without needing to constantly monitor the terminal. The implementation includes a new configurable setting and safeguards to only activate in interactive sessions. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a useful terminal bell notification feature, which is a nice addition for user experience. The implementation looks solid, adding a new setting and integrating the bell logic at the two key points: response completion and tool approval waiting. However, the test suite for this new feature is incomplete. While there are good tests for the tool approval scenario, tests for the response completion scenario are missing. I've added a comment to address this.
bd4633e to
8b49d07
Compare
|
@gemini-code-assist review |
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.
Code Review
This pull request adds a terminal bell notification for response completion and tool approval, controlled by a new setting. The implementation is well-structured and includes comprehensive tests. I've identified one potential robustness issue where writing to stdout could cause an unhandled error during application shutdown. I've suggested a fix to handle this gracefully.
48683ca to
7715df3
Compare
|
@afarber how is it going i was also trying to do this, but you are already working on it. Just wanted to know how it is going |
|
Hi, my implementation is complete, tested on my Ubuntu and Macbook Air computers and now just waiting for the merge by @jackwotherspoon or his colleagues |
7715df3 to
2a10f79
Compare
|
Our hooks implementation is in preview (behind a feature flag), I always envisioned us implementing a robust notifications system (bell noise, visual notification, customizing when they happen, etc) that users can fully customize through the hooks system. So I am a little torn if this PR makes sense with that being said. Maybe you would like to start on the notification system? We probably need to look at an open issue and work on a design approach together. |
|
Thanks for the feedback @jackwotherspoon! For audio notifications via hooks, would users configure something like this in ~/.gemini/settings.json? {
"tools": {
"enableHooks": true,
"enableMessageBusIntegration": true
},
"hooks": {
"Notification": [{
"matcher": "ToolPermission",
"hooks": [{
"type": "command",
"command": "node my-audio-notification-script.js"
}]
}]
}
}The hooks approach is very flexible, but I see a few trade-offs compared to this PR's simple terminal bell:
Would it make sense to have both?
|
|
@jackwotherspoon i likewhat you guy are doing, am i want to be part of it, so dont know if there is anything i can add, if you would permit |
2a10f79 to
db19fe9
Compare
db19fe9 to
ebf2dfc
Compare


Summary
Adds an optional terminal bell notification (
\x07) that plays when:The
general.terminalBellsetting is enabled by default and can be disabled in settings. Only triggers in TTY environments.Details
Writes \x07 (BEL character) to stderr when:
Uses Ink's
useStdout()hook rather thanprocess.stdoutto bypasspatchStdio()which redirects stdout writes to event emitters.Users who find it annoying can disable with general.terminalBell: false in settings.
Related Issues
Closes #14643
Partially fixes #14696
How to Validate
Pre-Merge Checklist