-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[$250] Notification – Have no sound for notification received #53700
Comments
Triggered auto assignment to @RachCHopkins ( |
Can repro! |
Job added to Upwork: https://www.upwork.com/jobs/~021866271667566111224 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @jayeshmangwani ( |
Edited by proposal-police: This proposal was edited at 2024-12-24 09:19:58 UTC. ProposalPlease re-state the problem that we are trying to solve in this issueBrowser notifications for new messages in New Expensify are currently playing without sound, when they should play a notification sound to alert users of new messages. What is the root cause of that problem?The root cause is twofold:
What changes do you think we should make in order to solve the problem?We need to make two key changes:
showCommentNotification(report: Report, reportAction: ReportAction, onClick: LocalNotificationClickHandler) {
// ... existing code ...
push(title, body, icon, data, onClick, false);
},
function push(
title: string,
body: string,
icon: string,
data: LocalNotificationData,
onClick: LocalNotificationClickHandler,
silent: boolean,
) {
// ... existing code ...
// Play notification sound if not silent
if (!silent) {
playSound(SOUNDS.RECEIVE);
}
// Continue with notification creation
const notificationID = Str.guid();
// ... rest of existing code ...
} What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?N/A What alternative solutions did you explore? (Optional)adding silent: true to the new Notification. notificationCache[notificationID] = new Notification(title, {
body,
icon: String(icon),
data,
silent: true,
tag,
}); If this is a bug from the Chrome web browser and it's fixed, we could expect double notifications but with this, only our custom sound via playSound(SOUNDS.RECEIVE) will play and the default browser notification sound will be suppressed. Result (Sound on)Recording.at.2024-12-12.00.22.41.mp4 |
ProposalPlease re-state the problem that we are trying to solve in this issue.
What is the root cause of that problem?
The 6th param is What changes do you think we should make in order to solve the problem?
Based on that, the logic: App/src/libs/Notification/LocalNotification/BrowserNotifications.ts Lines 64 to 70 in 082e14b
from What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?
What alternative solutions did you explore? (Optional)
|
@RachCHopkins, @jayeshmangwani Whoops! This issue is 2 days overdue. Let's get this updated quick! |
Not overdue, testing the above 2 proposals |
@JoshIri360 and @truph01 , did you receive the notification sound on desktop in the current version? |
No I didn't, tested in chrome and safari. |
Thanks! So we don’t get sound on the web (Chrome + Safari) or desktop either, right? Just confirming so I can test solutions on those platforms. |
Yes, exactly. |
@jayeshmangwani In Macos Safari, it works well. The issue only appears in Macos Chrome. But you need to hard-code the 6th param to
before testing the sound notification if you want the sound is triggered by sending a message. |
Thanks for the explanation @truph01. I feel that your proposal is quite similar to @JoshIri360, I’ve already tested their proposal, and it works fine on Safari, Chrome, and desktop. |
In safari, that solution can introduce the bug when the sound is triggered 2 times. Did you encounter it? @jayeshmangwani |
No @truph01 , I have tested it a few times on Safari and only received one notification sound. |
That means this bug appears in both Chrome and Safari from your side, right? |
However, I’m still unable to hear two sounds from the desktop when this line is uncommented 🤷. |
Here’s how I am testing it—please let me know if I’m doing something wrong. notif-test-vid.mov |
@truph01 @jayeshmangwani will adding
If this is a bug from the Chrome web browser and it's fixed, we could expect double notifications but with this, only our custom sound via playSound(SOUNDS.RECEIVE) will play and the default browser notification sound will be suppressed. |
|
@JoshIri360 Did you hear two sounds when applying the change in PR in Desktop platform? |
@truph01 No, I don't but I think this may be because I can't seem to enable notification banners and sounds for the Desktop platform. |
@JoshIri360 Please help try it and see what happened from your side: |
@truph01 Yes, 2 notifications here |
This solves it, no? |
@JoshIri360 I think we need to wait until C+ can reproduce the "two sounds" issue. |
Yes, I’ve made sure to reload the desktop, but strangely, you and @JoshIri360 consistently get two sounds. I’m probably doing something wrong in my testing 😄 |
You could try turning on DND, it would silence the system sound so you can listen for the custom one better. |
Does enabling DND also not stop notifications? |
No, only stops the banners and system sounds |
Finally, I'm able to reproduce the issue! Now, I can hear two notification sounds simultaneously—one custom and one default macOS notification sound. Thanks, @truph01 and @JoshIri360! |
@MarioExpensify ,Previously selected proposal resulted in two sounds. We need to revisit and rethink possible solutions for this scenario to ensure the expected behavior. Below are the key points outlined: When constructing App/src/libs/Notification/LocalNotification/BrowserNotifications.ts Lines 64 to 68 in 8b7096f
Possible Solutions:
|
@jayeshmangwani What do you think about my main and alternative solutions here? Does it match your two possible solutions? |
@truph01 your above solutions looks good and will def solve the problem at hand, but I think we should first reach a consensus on the expected result. Personally, I’m more inclined towards using only silent: false, since the Chrome issue might eventually be resolved on their end. Alternatively, if we want a more immediate and consistent solution, we could consider using a custom sound across all platforms. |
@jayeshmangwani If we're using a custom sound across all platforms, this would be a viable solution. This would silence the system notifications entirely, the major drawback would be the sound coming through when DND (same for web apps that use custom sounds for Notification like Discord and Whatsapp on web) is turned on as there isn't any support to check for this on Web yet. |
Let’s wait for @MarioExpensify to reach a conclusion on whether we want to use custom sounds for all platforms. |
Hello @jayeshmangwani, thanks for bringing this up to discussion. I don't like the idea that we would either accept Chrome's fault or have a custom code path only to address their issue. I'm inclined towards the custom notification sound (and make everything consistent), but I'll take this discussion to product/design so we have their perspective on this as well. Please hold a little bit more on this before moving forward. |
@arosiclair The notification sounds only play when you're in that chat and the browser app is active. Recording.at.2024-12-26.23.19.56.mp4 |
This bug is related to the web notification sound (handled here), where the notification sound is triggered in all web platform but does not work in the MacOS Chrome. And here are the possible solutions for it. |
@JoshIri360 It is an In-app sound, not the notification sound. |
Thanks, that makes sense. So when we added |
The issue was that chrome notifications weren’t playing the default notification sound, adding the custom notification fixed that on Chrome, but for platforms where it already worked before, it added another sound. This caused the desktop platform to play both the MacOS notification sounds and the custom sound added. |
There was no sound because we set |
You're right, the silent: false change aimed to fix Chrome but caused duplicate sounds on platforms where sound already worked. On desktop, it seems the notification sound doesn't play with silent enabled. The thought process was to give us more granular control over notification behavior across platforms. Keeping silent: true and adding playSound(SOUNDS.RECEIVE) during push should resolve this cleanly. |
@arosiclair Bypass the specific business logic determining when to trigger notification sounds to address the bug where the notification sound is not triggered in Chrome. There are two potential solutions:
Which approach do you prefer? |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 9.0.72-0
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team
Action Performed:
Expected Result:
Notification is with sound
Actual Result:
Notification is without sound
Workaround:
Unknown
Platforms:
Screenshots/Videos
View all open jobs on GitHub
Bug6686403_1733490528944.bandicam_2024-12-06_15-16-13-138.mp4
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @jayeshmangwaniThe text was updated successfully, but these errors were encountered: