Skip to content

Play sound only when action needed from the user#3065

Merged
mrubens merged 2 commits intoRooCodeInc:mainfrom
brendan-demo:bug/2190-play-sound-only-when-action-needed
Apr 30, 2025
Merged

Play sound only when action needed from the user#3065
mrubens merged 2 commits intoRooCodeInc:mainfrom
brendan-demo:bug/2190-play-sound-only-when-action-needed

Conversation

@olearycrew
Copy link
Copy Markdown
Contributor

@olearycrew olearycrew commented Apr 30, 2025

Context

Looking at #2190 I wanted to make sure that sounds only play when action is needed from the user.

There already was some logic around !isAutoApproved but in my testing, I found that to be lacking. Sometimes I would get the same sound played back to back, and there over all was a lot of sounds happening - some of which shouldn't be there.

Implementation

In looking at ChatView.tsx I found that there were two sections where we called playSound:

In the large case statement where we handle the incoming last messages based on type

useDeepCompareEffect(() => {
		// if last message is an ask, show user ask UI
		// if user finished a task, then start a new task with a new conversation history since in this moment that the extension is waiting for user response, the user could close the extension and the conversation history would be lost.
		// basically as long as a task is active, the conversation history will be persisted
		if (lastMessage) {
			switch (lastMessage.type) {

And then again further down in the streaming section which duplicated some of that case statement. I think this might have been a previous attempt to only play sounds when coming back and needing user input, but this resulted in often getting TWO sounds played

Additionally, most of the statements in the "main" case statement already were properly checking if we needed user input using !isAutoApproved.

Thus, I decided to remove that logic, which in my testing got rid of a lot of the double sounds. However, I was still sometimes getting double sounds on tool use or at completion of a task.

I found this to be when we get only a "partial" message back and are waiting for the final message to enable user interaction. Thus, I also added a check for !isPartial around these sounds - which is also what handles things like setTextAreaDisabled and setEnableButtons. Thus, this new sound logic matches how we enable/disable buttons and other user interactions and should more closely mimic that working logic

Closes #2190

Screenshots

before after

How to Test

  • Enable sounds in settings
  • Disable auto-approve, or approve it only for some actions (say read files but not write)
  • Ask Roo to make some modifications to the code
  • Observe when sounds are played (should be only when there is a button for the user to press or on API errors and completion)

Get in Touch

My discord handle is olearycrew


Important

Refines sound notification logic in ChatView.tsx to play sounds only when user action is required, addressing issue #2190.

This description was created by Ellipsis for 7dd9bdc. You can customize this summary. It will automatically update as commits are pushed.

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 30, 2025

⚠️ No Changeset found

Latest commit: 7dd9bdc

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Apr 30, 2025
Copy link
Copy Markdown
Collaborator

@mrubens mrubens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you for the cleanup! I'll just try it out locally and then merge.

@dosubot dosubot bot added lgtm This PR has been approved by a maintainer Enhancement New feature or request labels Apr 30, 2025
Copy link
Copy Markdown
Collaborator

@mrubens mrubens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I noticed is that it's no longer playing a sound when asking a question. Seems like we should probably handle that?

@mrubens
Copy link
Copy Markdown
Collaborator

mrubens commented Apr 30, 2025

I would also check non-retried API errors and non-auto-approved MCP

@olearycrew
Copy link
Copy Markdown
Contributor Author

One thing I noticed is that it's no longer playing a sound when asking a question. Seems like we should probably handle that?

Nice catch! I found this and will update

@olearycrew
Copy link
Copy Markdown
Contributor Author

I would also check non-retried API errors and non-auto-approved MCP

Good call!

Just checked the logic on the code I deleted again. I'll have a update shortly to address all this and the logic will (imho) but much cleaner 👨‍🍳 💋

@olearycrew
Copy link
Copy Markdown
Contributor Author

@mrubens sorry about those cases I missed - if you could retest again in the same way I think I've got them all now - at least all the ones that used to be handled by that deleted "sound only" section

@mrubens
Copy link
Copy Markdown
Collaborator

mrubens commented Apr 30, 2025

Nice, thank you!

@mrubens mrubens merged commit 5067190 into RooCodeInc:main Apr 30, 2025
12 checks passed
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Apr 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement New feature or request lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Change sound notification to only trigger only when it requires users attention

2 participants