fix: notification sound settings sync#2092
Conversation
Greptile SummaryThis PR replaces the old direct IPC read-and-assign pattern in
Confidence Score: 5/5Safe to merge; the settings sync logic is straightforward and correctly handles re-initialisation through the module-level subscription reference. The change is well-scoped to a single utility file. The default settings object prevents any regression in the startup sound window, and the subscription cleanup pattern is sound. The only notes are a defensive null-check in No files require special attention beyond the minor guard in
|
| Filename | Overview |
|---|---|
| src/renderer/utils/soundPlayer.ts | Refactors notification settings sync to use the React Query cache subscription pattern instead of direct IPC reads; adds proper cleanup tracking via a module-level unsubscribe reference and sensible default settings to avoid a silent-sound window at startup. |
Sequence Diagram
sequenceDiagram
participant Main as main.tsx
participant SP as soundPlayer.ts
participant IPC as rpc.appSettings
participant QC as QueryClient cache
Main->>SP: initSoundPlayer()
SP->>IPC: getWithMeta('notifications') [async]
SP->>QC: getQueryCache().subscribe(handler)
SP-->>Main: returns cleanup fn (discarded)
IPC-->>SP: .then(meta)
SP->>QC: setQueryData(['appSettings','notifications','meta'], meta)
QC->>SP: cache event fired
SP->>SP: applyMeta(event.query.state.data)
SP->>SP: "settings = meta.value"
Note over QC,SP: Any future settings change in the cache triggers applyMeta via subscription
SP->>SP: play(event, appFocused)
SP->>SP: guard: settings.enabled, settings.sound, soundFocusMode
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/renderer/utils/soundPlayer.ts:17-20
If `applyMeta` receives a meta object where `value` is `null` or `undefined` (e.g. the IPC returns `{ value: null }`), the `'value' in meta` guard passes but `settings` is assigned `null`. Every subsequent `play()` call then throws a `TypeError` on `settings.enabled`. Adding a nullish check on the value itself prevents this.
```suggestion
function applyMeta(meta: unknown): void {
if (!meta || typeof meta !== 'object' || !('value' in meta)) return;
const value = (meta as { value: NotificationSettings }).value;
if (!value) return;
settings = value;
}
```
### Issue 2 of 2
src/renderer/utils/soundPlayer.ts:30-31
`initSoundPlayer()` now returns a cleanup function, but the call site in `main.tsx` discards it (`initSoundPlayer();`). The module-level `unsubscribeSettingsCache` reference handles re-initialisation correctly, but the returned function is the intended external tear-down handle. Capturing it at the call site would make the contract explicit and keep the API usable if cleanup is ever needed (e.g., during hot-module replacement or testing).
```suggestion
// NOTE: callers should store and invoke the returned cleanup function.
unsubscribeSettingsCache?.();
const unsubscribe = queryClient.getQueryCache().subscribe((event) => {
```
Reviews (2): Last reviewed commit: "Fix sound player settings subscription" | Re-trigger Greptile
arnestrickmann
left a comment
There was a problem hiding this comment.
LGTM — thanks! Updating the user who reported this!
summary