Skip to content

fix: prevent notification sound on attempt_completion with queued messages #8540

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.

Sign up for GitHub

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

Jump to bottom
Merged
mrubens merged 1 commit into from
Nov 6, 2025
Merged

fix: prevent notification sound on attempt_completion with queued messages #8540

mrubens merged 1 commit into from
Nov 6, 2025

Conversation

@roomote
Copy link
Contributor

@roomote roomote bot commented Oct 6, 2025 •
edited by ellipsis-dev bot
Loading

Summary

This PR fixes the issue where the notification sound incorrectly plays when attempt_completion is called despite having messages already queued. This was misleading users into thinking input was required when the task would actually continue automatically with the queued message.

Problem

When a user types a message during task execution, it gets queued. When the task reaches attempt_completion, the celebration sound was playing even though the queued message would be automatically processed, creating confusion about whether the task had truly paused or would continue on its own.

Solution

Added a check for messageQueue.length before playing the celebration sound. The sound now only plays when:

  • The message is not partial (!isPartial)
  • AND the message queue is empty (messageQueue.length === 0)

Changes

  • ChatView.tsx: Added messageQueue.length === 0 condition to line 398
  • Tests: Added comprehensive test suite with 4 test cases covering:
    • No sound when queue has messages
    • Sound plays when queue is empty
    • No sound with multiple queued messages
    • No sound for partial completion results

Testing

  • ✅ All new tests pass
  • ✅ Existing tests continue to pass
  • ✅ Linting and type checking pass

Fixes #8537

Important

Fixes notification sound issue in ChatView.tsx by adding a condition to prevent sound when messages are queued.

  • Behavior:
    • In ChatView.tsx, added messageQueue.length === 0 condition to play celebration sound only when there are no queued messages and the message is not partial.
  • Tests:
    • Added ChatView.notification-sound.spec.tsx with tests for:
      • No sound when queue has messages.
      • Sound plays when queue is empty.
      • No sound with multiple queued messages.
      • No sound for partial completion results.

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

@roomote
fix: prevent notification sound on attempt_completion with queued mes…
bc91daa 
…sages

- Added check for messageQueue.length before playing celebration sound
- Only plays sound when both !isPartial and messageQueue is empty
- Added comprehensive tests to verify the fix
- Fixes #8537
@roomote roomote bot requested review from cte, jr and mrubens as code owners October 6, 2025 18:47
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. bug Something isn't working labels Oct 6, 2025
@roomote roomote bot mentioned this pull request Oct 6, 2025
Closed
@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Oct 6, 2025
roomote[bot]
roomote bot commented Oct 6, 2025
View reviewed changes
Copy link
Contributor Author

@roomote roomote bot left a comment

Choose a reason for hiding this comment

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

Self-review complete: reviewing my own code like a robot auditing its reflection—optimizations detected, ego not found.

@hannesrudolph
Copy link
Collaborator

hannesrudolph commented Oct 27, 2025

Tested, works.

@hannesrudolph hannesrudolph moved this from Triage to PR [Needs Prelim Review] in Roo Code Roadmap Oct 27, 2025
@hannesrudolph hannesrudolph added PR - Needs Preliminary Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Oct 27, 2025
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 5, 2025
@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Needs Review] in Roo Code Roadmap Nov 5, 2025
@mrubens mrubens merged commit 4eebcf0 into main Nov 6, 2025
29 checks passed
@mrubens mrubens deleted the fix/notification-sound-queued-messages branch November 6, 2025 21:44
@github-project-automation github-project-automation bot moved this from PR [Needs Review] to Done in Roo Code Roadmap Nov 6, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Nov 6, 2025
mini2s added a commit to zgsm-ai/costrict that referenced this pull request Nov 7, 2025
@mini2s @mrubens
@brunobergher @roomote @github-actions @roomote @SannidhyaSah @jr @cte @ColbySerpa @PeterDaveHello @daniel-lxs @hassoncs @hannesrudolph @chrarnoldus @laz-001 @ellipsis-dev @mohammad154 @NaccOll @ThomsenDrake @elianiva @heyseth @Maosghoul @kevint-cerebras @Thibault00 @requesty-JohnCosta27 @dmarkey @sebastiand-cerebras @dleffel @ajjuaire @renovate
Roo to main (#628