fix: prevent random doorbell notification sound#3260
fix: prevent random doorbell notification sound#3260Ram-sah19 wants to merge 7 commits intoRocketChat:developfrom
Conversation
WalkthroughThe PR addresses a notification alert issue and adds two platform-specific enhancements. It fixes silent flag evaluation in notification creation by changing the logic to explicitly check for false, adds macOS Kerberos/SPNEGO authentication switches, and enables WebView plugins in two contexts without altering control flow or public APIs. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/app/main/app.ts (1)
69-73: Kerberos/SPNEGO flags are correctly placed but unrelated to PR objectives.The macOS-specific Kerberos authentication flags are correctly applied before
app.whenReady(). However, this change addresses SAML authentication, not the notification sound issue (#2957) stated in the PR objectives.Consider making the whitelist patterns configurable for enterprise deployments that may use different domain patterns.
♻️ Optional: Make whitelist configurable
// Enable Kerberos/SPNEGO authentication for SAML on macOS if (process.platform === 'darwin') { - app.commandLine.appendSwitch('auth-server-whitelist', '*.local,*.domain.local'); - app.commandLine.appendSwitch('auth-negotiate-delegate-whitelist', '*.local,*.domain.local'); + const kerberosWhitelist = process.env.KERBEROS_AUTH_WHITELIST || '*.local,*.domain.local'; + app.commandLine.appendSwitch('auth-server-whitelist', kerberosWhitelist); + app.commandLine.appendSwitch('auth-negotiate-delegate-whitelist', kerberosWhitelist); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/main/app.ts` around lines 69 - 73, The macOS-only Kerberos/SPNEGO flags (the process.platform === 'darwin' block using app.commandLine.appendSwitch for 'auth-server-whitelist' and 'auth-negotiate-delegate-whitelist') are unrelated to the PR goal and should either be removed from this change set or made configurable; update the code so these whitelist patterns are not hard-coded: extract the patterns into a configuration source (env var or config API) and read them when setting app.commandLine.appendSwitch, or remove the block from this PR and add a follow-up task to implement a config-driven approach for app.commandLine.appendSwitch usage.KERBEROS_SAML_FIX.md (1)
1-92: Documentation is comprehensive but unrelated to PR objectives.This documentation thoroughly covers the Kerberos/SAML authentication fix for macOS. However, it's unrelated to the PR's stated objective of fixing notification sounds (issue
#2957).Minor markdown improvements: Some code blocks are missing language specifiers (lines 29, 41, 46). Consider adding language hints for proper syntax highlighting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@KERBEROS_SAML_FIX.md` around lines 1 - 92, The KERBEROS_SAML_FIX.md documentation is unrelated to the PR goal (notification sounds) so remove it from this PR (or move it into a separate docs/ or feature branch commit) to keep the PR focused; if you keep the file, add language specifiers to the code fences (mark the TypeScript snippet showing the app.commandLine.appendSwitch calls as ```typescript and the shell/build snippets as ```bash) and ensure the note references the performElectronStartup() addition in src/app/main/app.ts only as part of that separate docs/commit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@NOTIFICATION_SOUND_FIX.md`:
- Around line 54-68: The documentation table and summary are inverted: update
the table rows for silent:true / silent:false / silent:undefined / Not specified
to reflect Electron semantics (silent:true => Silent (no sound); silent:false =>
Plays sound; silent:undefined / Not specified => Plays sound by default), and
change the explanatory text around the "Key Change" to state that replacing
`silent ?? undefined` with `silent === true` makes the notification silent only
when `silent` is explicitly true (otherwise it will play sound), and add a note
to verify the code path using `silent === true` (the symbol `silent === true`)
actually yields the described behavior. Ensure the wording no longer claims
"default behavior is silent" and that the sentence "Sound only plays when
`silent` is explicitly `false`" is corrected to "Sound plays unless `silent` is
explicitly `true`."
In `@src/app/main/app.ts`:
- Around line 71-72: Prettier flagged the two appendSwitch calls for formatting;
update the two app.commandLine.appendSwitch invocations (the lines calling
app.commandLine.appendSwitch('auth-server-whitelist', '*.local,*.domain.local')
and app.commandLine.appendSwitch('auth-negotiate-delegate-whitelist',
'*.local,*.domain.local')) to match the project's Prettier/ESLint style (e.g.,
adjust quotes/spacing/commas or line breaks as required) or simply run the
project's Prettier formatter on this file so the calls conform to the configured
formatting rules.
In `@src/notifications/main.ts`:
- Line 66: The current assignment "silent: silent === true" in the notification
options in src/notifications/main.ts inverts intent and causes undefined to
become false (allowing sound); change the logic so undefined defaults to true
(silence by default) while preserving explicit false to play sound — e.g. set
the notification option to compute silent as (silent === false ? false : true)
or equivalent so that silent:true remains true, silent:false remains false, and
undefined becomes true; also update NOTIFICATION_SOUND_FIX.md to correct the
table entry that incorrectly states "silent:false" yields silence and document
that false explicitly enables sound per Electron's API.
---
Nitpick comments:
In `@KERBEROS_SAML_FIX.md`:
- Around line 1-92: The KERBEROS_SAML_FIX.md documentation is unrelated to the
PR goal (notification sounds) so remove it from this PR (or move it into a
separate docs/ or feature branch commit) to keep the PR focused; if you keep the
file, add language specifiers to the code fences (mark the TypeScript snippet
showing the app.commandLine.appendSwitch calls as ```typescript and the
shell/build snippets as ```bash) and ensure the note references the
performElectronStartup() addition in src/app/main/app.ts only as part of that
separate docs/commit.
In `@src/app/main/app.ts`:
- Around line 69-73: The macOS-only Kerberos/SPNEGO flags (the process.platform
=== 'darwin' block using app.commandLine.appendSwitch for
'auth-server-whitelist' and 'auth-negotiate-delegate-whitelist') are unrelated
to the PR goal and should either be removed from this change set or made
configurable; update the code so these whitelist patterns are not hard-coded:
extract the patterns into a configuration source (env var or config API) and
read them when setting app.commandLine.appendSwitch, or remove the block from
this PR and add a follow-up task to implement a config-driven approach for
app.commandLine.appendSwitch usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ce0a398-2034-430c-9e41-e5bfa60b1faf
📒 Files selected for processing (8)
FIXES_SUMMARY.mdKERBEROS_QUICK_REF.mdKERBEROS_SAML_FIX.mdMERGE_CONFLICT_RESOLUTION.mdNOTIFICATION_SOUND_FIX.mdsrc/app/main/app.tssrc/notifications/main.tssrc/ui/main/serverView/index.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript strict mode enabled in TypeScript configuration
Use React functional components with hooks instead of class components
Follow FSA (Flux Standard Action) pattern for Redux actions
Use camelCase for file names and PascalCase for component file names
All code must pass ESLint and TypeScript checks
Write self-documenting code with clear naming; avoid unnecessary comments except for complex business logic or non-obvious decisions
Use Fuselage components from@rocket.chat/fuselagefor all UI work and only create custom components when Fuselage doesn't provide what's needed
CheckTheme.d.tsfor valid color tokens when using Fuselage components
Use defensive coding with optional chaining and fallbacks for Linux-only APIs (process.getuid(), process.getgid(), process.geteuid(), process.getegid()) to ensure cross-platform compatibility across Windows, macOS, and Linux
Files:
src/app/main/app.tssrc/ui/main/serverView/index.tssrc/notifications/main.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: Ram-sah19
Repo: RocketChat/Rocket.Chat.Electron PR: 3254
File: .github/workflows/build-release.yml:80-94
Timestamp: 2026-03-11T06:38:40.426Z
Learning: In the RocketChat/Rocket.Chat.Electron repository, the issues flagged in `.github/workflows/build-release.yml` (e.g., `node12` runtime in the release action and missing `snapcraft_token` input), i18n files, and `electron-builder.json` are pre-existing in the `develop` branch and are pulled in during merge conflict resolution. Do not flag these as new issues introduced by PRs that only modify `src/injected.ts` and `src/ui/main/rootWindow.ts`.
📚 Learning: 2026-03-11T06:38:40.426Z
Learnt from: Ram-sah19
Repo: RocketChat/Rocket.Chat.Electron PR: 3254
File: .github/workflows/build-release.yml:80-94
Timestamp: 2026-03-11T06:38:40.426Z
Learning: In the RocketChat/Rocket.Chat.Electron repository, the issues flagged in `.github/workflows/build-release.yml` (e.g., `node12` runtime in the release action and missing `snapcraft_token` input), i18n files, and `electron-builder.json` are pre-existing in the `develop` branch and are pulled in during merge conflict resolution. Do not flag these as new issues introduced by PRs that only modify `src/injected.ts` and `src/ui/main/rootWindow.ts`.
Applied to files:
FIXES_SUMMARY.mdKERBEROS_SAML_FIX.mdNOTIFICATION_SOUND_FIX.mdMERGE_CONFLICT_RESOLUTION.md
🪛 ESLint
src/app/main/app.ts
[error] 71-71: Replace 'auth-server-whitelist',·'*.local,*.domain.local' with ⏎······'auth-server-whitelist',⏎······'*.local,*.domain.local'⏎····
(prettier/prettier)
[error] 72-72: Replace 'auth-negotiate-delegate-whitelist',·'*.local,*.domain.local' with ⏎······'auth-negotiate-delegate-whitelist',⏎······'*.local,*.domain.local'⏎····
(prettier/prettier)
🪛 LanguageTool
FIXES_SUMMARY.md
[style] ~114-~114: This phrase is redundant (‘OS’ stands for ‘operating system’). Use simply “macOS”.
Context: ...Kerberos/SAML Fix Prerequisites: - macOS system - Valid Kerberos ticket (klist) - SAM...
(ACRONYM_TAUTOLOGY)
KERBEROS_SAML_FIX.md
[style] ~69-~69: This phrase is redundant (‘OS’ stands for ‘operating system’). Use simply “macOS”.
Context: ...sting Instructions ### Prerequisites - macOS system - Rocket.Chat server with SAML configur...
(ACRONYM_TAUTOLOGY)
KERBEROS_QUICK_REF.md
[style] ~29-~29: This phrase is redundant (‘OS’ stands for ‘operating system’). Use simply “macOS”.
Context: ... ## 🧪 Quick Test ### Prerequisites - macOS system - Valid Kerberos ticket: klist - SAML...
(ACRONYM_TAUTOLOGY)
🪛 markdownlint-cli2 (0.21.0)
KERBEROS_SAML_FIX.md
[warning] 29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 41-41: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 46-46: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
KERBEROS_QUICK_REF.md
[warning] 29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 41-41: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 46-46: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
NOTIFICATION_SOUND_FIX.md
[warning] 82-82: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (5)
src/ui/main/serverView/index.ts (2)
213-213: LGTM - PDF plugin enablement is appropriate.Enabling
plugins: truein webPreferences correctly enables Chromium's built-in PDFium viewer for PDF rendering. Security settings (nodeIntegration, contextIsolation, webSecurity) remain properly configured.Note: This change is unrelated to the PR's stated objective of fixing notification sounds (issue
#2957).
254-254: LGTM - Consistent plugin setting for video call windows.This ensures PDF rendering capability is also available in video call popup windows, maintaining consistency with the main webview configuration.
FIXES_SUMMARY.md (1)
1-48: Documentation does not match PR objectives.This summary documents PDF rendering and Kerberos/SAML fixes, but the PR (
#3260) is titled "fix: prevent random doorbell notification sound" and references issue#2957about notification sounds. The notification sound fix insrc/notifications/main.tsis not documented here.Consider either:
- Adding documentation for the notification sound fix, or
- Splitting unrelated changes (PDF, Kerberos) into separate PRs for cleaner history
KERBEROS_QUICK_REF.md (1)
1-92: Quick reference documentation is helpful but unrelated to PR objectives.The quick reference provides a concise summary of the Kerberos/SAML fix. This documentation is useful for the Kerberos changes but is not related to the notification sound fix that is the stated PR objective.
MERGE_CONFLICT_RESOLUTION.md (1)
1-50: Documentation focuses on PDF fix, not the PR's stated notification fix.This merge conflict resolution document describes the PDF rendering fix workflow, but the PR is titled "fix: prevent random doorbell notification sound." The notification sound changes in
src/notifications/main.tsare not mentioned.Consider updating the documentation to reflect all changes in the PR or splitting the unrelated changes into separate PRs.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/notifications/main.ts (1)
136-138: Inconsistentsilenthandling inupdateNotification.The truthy check
if (silent)meanssilent: falsewon't update the notification's silent property (sincefalseis falsy), whilecreateNotificationcorrectly handlessilent: falseto enable sound. Consider aligning the logic:♻️ Suggested fix for consistency
- if (silent) { - notification.silent = silent; + if (silent !== undefined) { + notification.silent = silent; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notifications/main.ts` around lines 136 - 138, The updateNotification function currently uses a truthy check (if (silent)) so passing silent: false will not update notification.silent; change the logic in updateNotification to explicitly check for undefined (e.g., if (silent !== undefined)) and assign notification.silent = silent so false values are applied; update references to notification.silent in updateNotification to match how createNotification handles silent so behavior is consistent across createNotification and updateNotification.NOTIFICATION_SOUND_FIX.md (1)
82-90: Add language specifier to fenced code block.The code block should specify a language for proper syntax highlighting and to satisfy linting rules.
📝 Suggested fix
-``` +```text Rocket.Chat Web → injected.ts (RocketChatDesktopNotification)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NOTIFICATION_SOUND_FIX.md` around lines 82 - 90, The fenced code block showing the notification call chain ("Rocket.Chat Web → injected.ts (RocketChatDesktopNotification) ↓ preload.ts (createNotification) ↓ main.ts (createNotification) ← FIX APPLIED HERE ↓ Electron Notification API") is missing a language specifier; update the opening fence to include a language (for example use "text") so the block becomes ```text ... ``` to satisfy syntax highlighting and lint rules, locating the block by the string "Rocket.Chat Web → injected.ts (RocketChatDesktopNotification)" or the "preload.ts (createNotification)" / "main.ts (createNotification)" lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@NOTIFICATION_SOUND_FIX.md`:
- Around line 138-142: Update Test 3 so its title, steps, and expected result
match the intended behavior for explicit sound enabling: change the test title
"Non-Silent Notification (Explicitly with Sound)" and keep the configuration
`silent: false`, but correct the expected outcome to state that the notification
appears WITH sound (i.e., sound playback is enabled when `silent: false`),
ensuring the test description, step list, and expected assertion all
consistently reflect that `silent: false` enables sound playback.
- Around line 148-156: The verification checklist item "No sound plays when
silent is false (new behavior)" is incorrect; update the checklist so it expects
sound to play when silent is false (e.g., change the line to "Sound plays when
silent is false (new behavior)" or similar) to match the implementation; ensure
the other related items ("No sound plays for default notifications", "No sound
plays when volume is 0", "No sound plays when silent is true") remain unchanged
and consistent with the intended behavior.
- Around line 220-225: The debug logging in main.ts is wrong: it logs "Computed
silent value: silent === true" which doesn't match the runtime check used
elsewhere ("silent !== false"); update the debug message to compute the same
expression used by the code by replacing the comparison with silent !== false so
the logged computed value reflects actual behavior of the notification creation
(look for the console.log lines that mention 'Creating notification with
silent:' and the subsequent 'Computed silent value:' and change the latter to
use silent !== false).
---
Nitpick comments:
In `@NOTIFICATION_SOUND_FIX.md`:
- Around line 82-90: The fenced code block showing the notification call chain
("Rocket.Chat Web → injected.ts (RocketChatDesktopNotification) ↓ preload.ts
(createNotification) ↓ main.ts (createNotification) ← FIX APPLIED HERE ↓
Electron Notification API") is missing a language specifier; update the opening
fence to include a language (for example use "text") so the block becomes
```text ... ``` to satisfy syntax highlighting and lint rules, locating the
block by the string "Rocket.Chat Web → injected.ts
(RocketChatDesktopNotification)" or the "preload.ts (createNotification)" /
"main.ts (createNotification)" lines.
In `@src/notifications/main.ts`:
- Around line 136-138: The updateNotification function currently uses a truthy
check (if (silent)) so passing silent: false will not update
notification.silent; change the logic in updateNotification to explicitly check
for undefined (e.g., if (silent !== undefined)) and assign notification.silent =
silent so false values are applied; update references to notification.silent in
updateNotification to match how createNotification handles silent so behavior is
consistent across createNotification and updateNotification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f47f4a4f-cc69-4ac1-a43b-6d6c057934c6
📒 Files selected for processing (3)
NOTIFICATION_SOUND_FIX.mdsrc/app/main/app.tssrc/notifications/main.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/main/app.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript strict mode enabled in TypeScript configuration
Use React functional components with hooks instead of class components
Follow FSA (Flux Standard Action) pattern for Redux actions
Use camelCase for file names and PascalCase for component file names
All code must pass ESLint and TypeScript checks
Write self-documenting code with clear naming; avoid unnecessary comments except for complex business logic or non-obvious decisions
Use Fuselage components from@rocket.chat/fuselagefor all UI work and only create custom components when Fuselage doesn't provide what's needed
CheckTheme.d.tsfor valid color tokens when using Fuselage components
Use defensive coding with optional chaining and fallbacks for Linux-only APIs (process.getuid(), process.getgid(), process.geteuid(), process.getegid()) to ensure cross-platform compatibility across Windows, macOS, and Linux
Files:
src/notifications/main.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: Ram-sah19
Repo: RocketChat/Rocket.Chat.Electron PR: 3254
File: .github/workflows/build-release.yml:80-94
Timestamp: 2026-03-11T06:38:40.426Z
Learning: In the RocketChat/Rocket.Chat.Electron repository, the issues flagged in `.github/workflows/build-release.yml` (e.g., `node12` runtime in the release action and missing `snapcraft_token` input), i18n files, and `electron-builder.json` are pre-existing in the `develop` branch and are pulled in during merge conflict resolution. Do not flag these as new issues introduced by PRs that only modify `src/injected.ts` and `src/ui/main/rootWindow.ts`.
🪛 markdownlint-cli2 (0.21.0)
NOTIFICATION_SOUND_FIX.md
[warning] 82-82: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (1)
src/notifications/main.ts (1)
66-66: LGTM! Silent flag logic correctly defaults to silent.The change from the previous logic to
silent !== falseproperly implements the intended behavior:
silent: true→true(suppresses sound)silent: false→false(plays sound when explicitly requested)silent: undefined→true(silent by default)This correctly addresses issue
#2957by preventing unwanted doorbell sounds.
| #### Test 3: Non-Silent Notification (Explicitly with Sound) | ||
| 1. Configure notification with `silent: false` | ||
| 2. Trigger notification | ||
| 3. **Expected**: Notification appears WITHOUT sound (default behavior) | ||
|
|
There was a problem hiding this comment.
Test case contradicts the fix behavior.
Test 3 is titled "Non-Silent Notification (Explicitly with Sound)" with silent: false, but the expected behavior states "Notification appears WITHOUT sound." Per the fix, silent: false should explicitly enable sound playback. The expected result should be:
📝 Suggested fix
#### Test 3: Non-Silent Notification (Explicitly with Sound)
1. Configure notification with `silent: false`
2. Trigger notification
-3. **Expected**: Notification appears WITHOUT sound (default behavior)
+3. **Expected**: Notification appears WITH sound📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #### Test 3: Non-Silent Notification (Explicitly with Sound) | |
| 1. Configure notification with `silent: false` | |
| 2. Trigger notification | |
| 3. **Expected**: Notification appears WITHOUT sound (default behavior) | |
| #### Test 3: Non-Silent Notification (Explicitly with Sound) | |
| 1. Configure notification with `silent: false` | |
| 2. Trigger notification | |
| 3. **Expected**: Notification appears WITH sound |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@NOTIFICATION_SOUND_FIX.md` around lines 138 - 142, Update Test 3 so its
title, steps, and expected result match the intended behavior for explicit sound
enabling: change the test title "Non-Silent Notification (Explicitly with
Sound)" and keep the configuration `silent: false`, but correct the expected
outcome to state that the notification appears WITH sound (i.e., sound playback
is enabled when `silent: false`), ensuring the test description, step list, and
expected assertion all consistently reflect that `silent: false` enables sound
playback.
| ### Verification Checklist | ||
|
|
||
| - [ ] No sound plays for default notifications | ||
| - [ ] No sound plays when volume is 0 | ||
| - [ ] No sound plays when silent is true | ||
| - [ ] No sound plays when silent is false (new behavior) | ||
| - [ ] Notifications still appear visually | ||
| - [ ] Click/reply functionality still works | ||
| - [ ] No console errors |
There was a problem hiding this comment.
Verification checklist contains incorrect expectation.
Line 153 states "No sound plays when silent is false (new behavior)" but the actual fix behavior is that sound should play when silent: false. This contradicts the implementation and would cause incorrect test validation.
📝 Suggested fix
- [ ] No sound plays for default notifications
- [ ] No sound plays when volume is 0
- [ ] No sound plays when silent is true
-- [ ] No sound plays when silent is false (new behavior)
+- [ ] Sound plays when silent is explicitly false
- [ ] Notifications still appear visually🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@NOTIFICATION_SOUND_FIX.md` around lines 148 - 156, The verification checklist
item "No sound plays when silent is false (new behavior)" is incorrect; update
the checklist so it expects sound to play when silent is false (e.g., change the
line to "Sound plays when silent is false (new behavior)" or similar) to match
the implementation; ensure the other related items ("No sound plays for default
notifications", "No sound plays when volume is 0", "No sound plays when silent
is true") remain unchanged and consistent with the intended behavior.
| **Debug**: | ||
| ```typescript | ||
| // Add logging in main.ts | ||
| console.log('Creating notification with silent:', silent); | ||
| console.log('Computed silent value:', silent === true); | ||
| ``` |
There was a problem hiding this comment.
Debug logging example doesn't match actual implementation.
The debug example shows silent === true but the actual code uses silent !== false. This would output incorrect values during debugging.
📝 Suggested fix
**Debug**:
```typescript
// Add logging in main.ts
console.log('Creating notification with silent:', silent);
-console.log('Computed silent value:', silent === true);
+console.log('Computed silent value:', silent !== false);</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @NOTIFICATION_SOUND_FIX.md around lines 220 - 225, The debug logging in
main.ts is wrong: it logs "Computed silent value: silent === true" which doesn't
match the runtime check used elsewhere ("silent !== false"); update the debug
message to compute the same expression used by the code by replacing the
comparison with silent !== false so the logged computed value reflects actual
behavior of the notification creation (look for the console.log lines that
mention 'Creating notification with silent:' and the subsequent 'Computed silent
value:' and change the latter to use silent !== false).
</details>
<!-- fingerprinting:phantom:poseidon:ocelot -->
<!-- This is an auto-generated comment by CodeRabbit -->
1 participant
Fixes issue where the doorbell notification sound plays randomly
even when notification sound volume is set to 0 or a silent
notification sound is configured.
Changes:
Fixes #2957
Summary by CodeRabbit
Bug Fixes
New Features