feat(notifications): notify when focused on a different task#2210
Open
richardsolomou wants to merge 2 commits into
Open
feat(notifications): notify when focused on a different task#2210richardsolomou wants to merge 2 commits into
richardsolomou wants to merge 2 commits into
Conversation
Previously, permission requests and prompt completions only fired a sound, desktop notification, and dock badge if the window was unfocused. If the user was viewing task A in PostHog Code while task B asked for input, nothing surfaced — they had to manually check the other task. Now also notify when the window is focused but the user is viewing a different task (or no task at all). The window-focus path is unchanged. Generated-By: PostHog Code Task-Id: b14a2144-3143-4d34-a0e3-2139aabdaed9
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/code/src/renderer/utils/notifications.ts:16-23
**Missing automated tests for `shouldNotifyForTask`**
The new helper has three distinct, clearly-testable branches (unfocused → always notify; focused + no taskId → never notify; focused + taskId → compare with viewed task), yet no test file exists for `notifications.ts`. Given the vitest infrastructure already present (see `navigationStore.test.ts` for the mocking pattern), and simplicity rule #1 ("Passes all the tests"), these branches should have parameterised unit tests covering at minimum: window unfocused, window focused with matching taskId, window focused with different taskId, and window focused with no taskId.
Reviews (1): Last reviewed commit: "feat(notifications): notify when focused..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+16
to
+23
| function shouldNotifyForTask(taskId?: string): boolean { | ||
| if (!document.hasFocus()) return true; | ||
| if (!taskId) return false; | ||
| const view = useNavigationStore.getState().view; | ||
| const viewedTaskId = | ||
| view.type === "task-detail" ? (view.data?.id ?? view.taskId) : undefined; | ||
| return viewedTaskId !== taskId; | ||
| } |
Contributor
There was a problem hiding this comment.
shouldNotifyForTask
The new helper has three distinct, clearly-testable branches (unfocused → always notify; focused + no taskId → never notify; focused + taskId → compare with viewed task), yet no test file exists for notifications.ts. Given the vitest infrastructure already present (see navigationStore.test.ts for the mocking pattern), and simplicity rule #1 ("Passes all the tests"), these branches should have parameterised unit tests covering at minimum: window unfocused, window focused with matching taskId, window focused with different taskId, and window focused with no taskId.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/utils/notifications.ts
Line: 16-23
Comment:
**Missing automated tests for `shouldNotifyForTask`**
The new helper has three distinct, clearly-testable branches (unfocused → always notify; focused + no taskId → never notify; focused + taskId → compare with viewed task), yet no test file exists for `notifications.ts`. Given the vitest infrastructure already present (see `navigationStore.test.ts` for the mocking pattern), and simplicity rule #1 ("Passes all the tests"), these branches should have parameterised unit tests covering at minimum: window unfocused, window focused with matching taskId, window focused with different taskId, and window focused with no taskId.
How can I resolve this? If you propose a fix, please make it concise.Adds unit tests for the new shouldNotifyForTask gate via the notifyPermissionRequest / notifyPromptComplete public surface: - unfocused → notifies regardless of task - focused on same task → suppressed - focused on different task → notifies - focused, non-task-detail view → notifies - focused, no taskId supplied → suppressed - end_turn filter still gates notifyPromptComplete Generated-By: PostHog Code Task-Id: b14a2144-3143-4d34-a0e3-2139aabdaed9
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When you're focused on task A inside PostHog Code and task B asks for a permission or finishes its turn, nothing surfaces — the existing notification logic in
notifications.tsonly fires when the window is unfocused, so a same-window-but-different-task interrupt is silent. Reported as: "no meep when task need input while focus other task, sad".Changes
Added a
shouldNotifyForTask(taskId)helper inapps/code/src/renderer/utils/notifications.tsthat consultsuseNavigationStorefor the currently viewed task. BothnotifyPromptCompleteandnotifyPermissionRequestnow fire (sound, desktop notification, dock badge, dock bounce) when:All existing settings (
desktopNotifications,dockBadgeNotifications,dockBounceNotifications,completionSound,completionVolume) still gate the channels — no new preference. Falls back to "don't notify when focused" iftaskIdis unknown, so callers without ataskIdkeep their previous behaviour.Clicking the desktop notification already routes to the right task via
TaskLinkService→deepLink.onOpenTask→useTaskDeepLink→navigateToTask, so no extra wiring needed there.How did you test this?
pnpm --filter code typecheck— cleanpnpm exec biome check apps/code/src/renderer/utils/notifications.ts— cleanelectron-notifier.ts:25→notification/service.ts:38→deep-link.ts:27→useTaskDeepLink.ts→navigationStore.navigateToTask) to confirm clicking the notification still lands on the originating task.Publish to changelog?
no