Conversation
AntipodTX
force-pushed
the
feature/browser-chat-notifications
branch
from
May 18, 2026 10:36
c216d35 to
8ca1d19
Compare
Owner
Author
|
@greptileai Updated after review:
|
|
Both changes look good. The The guard removal is the right call — if |
AntipodTX
pushed a commit
that referenced
this pull request
Jul 17, 2026
Fixes from PR review (jakemor#79): - #2 Composer mutations dropped Cursor: setChatComposerModel/ModelOptions and the provider-default setters rewrote any non-Claude state to Codex, so picking the Composer 2.5 model in a new Cursor chat silently switched it to Codex. Replace the two-way `claude ? … : codex` branching with an exhaustive provider dispatcher (switch + assertNever), shared by the setters and composerFromProviderDefaults. Same exhaustiveness applied to getEffectiveComposerState (resolves the #5 nit). - #3 Tool-result false-positive: a `completed` event whose `result` is absent/ non-object was flagged as an error. Only flag when an explicit error/failure is present. - #1 Disable forking for Cursor (no fork primitive) in canForkChat; simplify the agent cursor branch accordingly. Adds `src/shared/assert.ts` (assertNever) and tests for each fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
AntipodTX
force-pushed
the
feature/browser-chat-notifications
branch
from
July 19, 2026 19:17
8ca1d19 to
2886889
Compare
Owner
Author
|
@greptileai Please re-review the latest update. Changes since your last review:
|
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
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.
Summary
Adds an opt-in browser notification setting for chat activity so users can receive system notifications alongside existing chat sound events.
What changed
Notes
The setting defaults to Never and only requests browser notification permission when the user enables notification delivery.
Greptile Summary
This PR adds an opt-in browser notification system for chat activity, pairing it with the existing chat sound setting. A new
ChatBrowserNotificationPreferencetype andchatBrowserNotificationPreferencesetting are wired from the shared type layer through server persistence, the WebSocket router, and the client store, and ultimately consumed in auseEffectinKannaLayoutthat firesshowChatBrowserNotificationfor each newly-unread or newly-waiting chat.getChatNotificationEventsto compute per-chat notification events from sidebar state diffs, usingpendingUserInputPreview(the active tool question/plan) as the body forwaiting_for_usertransitions andlastAgentMessagePreviewas the body for unread transitions.getPendingToolPreviews()onAgentCoordinatorand fixes a subtle timing bug:pendingToolis now assigned andstatusset towaiting_for_usersynchronously inside the promise constructor, ensuring the preview is available whenemitStateChangefires.pendingUserInputPreviewsfrom the agent intoderiveSidebarDataso the read model includes the preview only while a chat is actively inwaiting_for_userstatus.Confidence Score: 5/5
Safe to merge; the new notification path is purely additive and the only shared-path change (the
pendingTool/emitStateChangeordering inagent.ts) corrects a pre-existing race rather than introducing one.Every changed layer — type definitions, server persistence, read-model projection, agent coordinator, client store, settings UI, and the notification dispatch loop — is independently unit-tested and follows the conventions already established for chat sounds. The timing fix in
agent.tsis a strict improvement:pendingToolis now atomically set with the status change inside the promise constructor before the state-change event fires, ensuring the preview is always present when the ws-router snapshot is computed. No existing behavior is altered when the preference is "never" (the default).No files require special attention.
Important Files Changed
showChatBrowserNotificationwith click handler; well-guarded withNotification.permission !== "granted"check at the call site.getChatNotificationEventsthat diffs previous/next sidebar state to produce per-chat notification events; correctly emits a single event per chat even when bothbecameUnreadandbecameWaitingare true simultaneously.getPendingToolPreviews()and fixes a race:pendingToolis now set beforestatus = "waiting_for_user"andemitStateChangeinside the Promise constructor, so the preview is always available when the ws-router snapshot is computed.pendingUserInputPreviewsoption toderiveSidebarData; conditionally includespendingUserInputPreviewonSidebarChatRowonly when the derived status iswaiting_for_user.agent.getPendingToolPreviews()aspendingUserInputPreviewstoderiveSidebarData; one-line change with no logic risk.useEffect, sharing the same previous-state ref as the sound burst count; restructured so both sound and notification paths use the same capturedpreviousSidebarData.ChatBrowserNotificationPreferenceas an alias forChatSoundPreference,pendingUserInputPreviewonSidebarChatRow, andchatBrowserNotificationPreferenceonAppSettingsSnapshot/AppSettingsPatch.chatBrowserNotificationPreference; consistent with the existingchatSoundPreferencepattern.chatBrowserNotificationPreferencestate and setter with normalization; defaults to "never" as specified.resolveChatBrowserNotificationPreferenceAfterPermissionhelper that maps requested preference + permission result to the effective preference.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Agent as AgentCoordinator participant WsRouter as ws-router participant ReadModel as deriveSidebarData participant Client as KannaLayout (React) participant BrowserAPI as Browser Notification API Agent->>Agent: "pendingTool = { tool, resolve }" Agent->>Agent: "status = waiting_for_user" Agent->>WsRouter: emitStateChange(chatId) WsRouter->>Agent: getPendingToolPreviews() Agent-->>WsRouter: "Map<chatId, previewText>" WsRouter->>ReadModel: "deriveSidebarData({ pendingUserInputPreviews })" ReadModel-->>WsRouter: SidebarData (with pendingUserInputPreview) WsRouter-->>Client: sidebar update (WebSocket) Client->>Client: getChatNotificationEvents(previous, next) Client->>Client: shouldShowChatNotificationPopup(appSettings, preference) alt notifications enabled and conditions met Client->>BrowserAPI: new Notification(title, body) BrowserAPI-->>Client: onclick handler navigates to chat end%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Agent as AgentCoordinator participant WsRouter as ws-router participant ReadModel as deriveSidebarData participant Client as KannaLayout (React) participant BrowserAPI as Browser Notification API Agent->>Agent: "pendingTool = { tool, resolve }" Agent->>Agent: "status = waiting_for_user" Agent->>WsRouter: emitStateChange(chatId) WsRouter->>Agent: getPendingToolPreviews() Agent-->>WsRouter: "Map<chatId, previewText>" WsRouter->>ReadModel: "deriveSidebarData({ pendingUserInputPreviews })" ReadModel-->>WsRouter: SidebarData (with pendingUserInputPreview) WsRouter-->>Client: sidebar update (WebSocket) Client->>Client: getChatNotificationEvents(previous, next) Client->>Client: shouldShowChatNotificationPopup(appSettings, preference) alt notifications enabled and conditions met Client->>BrowserAPI: new Notification(title, body) BrowserAPI-->>Client: onclick handler navigates to chat endReviews (18): Last reviewed commit: "Add browser chat notifications" | Re-trigger Greptile