Conversation
When a user taps a notification on Mac Catalyst, macOS Launch Services may resolve a different .app bundle (e.g. build output vs staging directory), launching a second PolyPilot instance instead of activating the existing one. Three fixes: 1. Single-instance enforcement in Program.cs (Mac Catalyst): Acquires an exclusive file lock (~/.polypilot/instance.lock) at startup. If another instance already holds the lock, activates its window via AppleScript and exits immediately. 2. Wire NotificationTapped event in App.xaml.cs: Subscribes to the notification service's NotificationTapped event and calls SwitchToSessionById to navigate to the correct session. Previously the event was defined but never subscribed to. 3. Add SwitchToSessionById to CopilotService: Notifications carry the SDK session GUID but sessions are keyed by name. This method looks up the session by SessionId and delegates to SwitchSession. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- TryAcquireInstanceLock: catch{return false} instead of catch{return true}
to prevent duplicate instances on unexpected lock errors (UnauthorizedAccessException, etc.)
- ActivateExistingInstance: accepts args[], extracts --session-id=<id>, writes
~/.polypilot/pending-navigation.json so the running instance can navigate
- NotificationManagerService.SendNotificationAsync: writes pending-navigation.json
when sending a notification with a sessionId; OnNotificationTapped deletes it
when the delegate fires normally (avoiding double-navigation)
- App.xaml.cs: CheckPendingNavigation() reads+deletes the sidecar on window.Activated
and OnResume(), then calls SwitchToSessionById on the main thread
- Fix JsonDocument disposal (using var) in CheckPendingNavigation
- Add 3 sidecar roundtrip tests to NotificationNavigationTests
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ation WritePendingNavigation now includes a writtenAt timestamp in the JSON payload. CheckPendingNavigation discards sidecars older than 30 seconds — preventing spurious session switches when the user ignored/dismissed the notification and later brings the app to the foreground via Cmd+Tab or dock click. The 30s window covers the brief second-instance race (the OS typically launches a second instance within ~2s of notification tap), while reliably discarding sidecars from notifications the user never interacted with. Add 3 TTL-specific tests to NotificationNavigationTests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ActivateExistingInstance now includes writtenAt=DateTime.UtcNow in the sidecar payload, matching the schema written by WritePendingNavigation. This ensures the 30s TTL in CheckPendingNavigation is applied even if the AppleScript activation fails and the sidecar is left on disk. The 30s window is orders of magnitude larger than the normal second-instance latency (~100ms), so it will never expire during normal operation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Multi-Model Review -- Round 3 Final ReportCI: Findings Status (3 rounds of fixes applied)
Verdict: ✅ ApproveAll consensus findings resolved across 3 fix rounds. Only remaining item (F3) is a diagnostic clarity edge case with no functional impact. 16 unit tests cover all key paths including TTL expiry. |
Owner
Multi-Model Review — Round 3 Final ReportCI: Findings Status (3 rounds of fixes applied)
Verdict: ✅ ApproveAll consensus findings resolved across 3 fix rounds. Only remaining item (F3) is a diagnostic clarity edge case with no functional impact. 16 unit tests cover all key paths including TTL expiry. |
PureWeen
deleted the
fix/when-i-get-a-notification-from-a-session-20260313-0958
branch
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
2 participants
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.
Bug
When a user taps a notification from a session on Mac Catalyst, macOS launches a new PolyPilot instance (with a different version) instead of activating the existing one and navigating to the session.
Root Cause
Two issues:
.appbundle (build output vs staging dir), launching a second process.NotificationTappedevent never subscribed — the event was defined onINotificationManagerServicebut nobody listened for it, so tapping a notification did nothing even in the correct instance.Fix (3 changes)
1. Single-instance lock in
Program.cs(Mac Catalyst)Acquires an exclusive file lock (
~/.polypilot/instance.lock) at startup. If another instance already holds the lock, activates the existing window via AppleScript and exits immediately. The OS releases the lock automatically when the process terminates (including crashes).2. Wire
NotificationTapped→ session navigation inApp.xaml.csSubscribes to the notification service's
NotificationTappedevent and callsSwitchToSessionById()on the main thread.3. Add
SwitchToSessionByIdtoCopilotServiceNotifications carry the SDK session GUID, but sessions are keyed by name. This method looks up the session by
SessionId(case-insensitive) and delegates toSwitchSession().Tests
NotificationNavigationTests.cscovering:SwitchToSessionById— correct lookup, non-existent ID, empty dict, case-insensitive, multiple sessions, remote mode bridge integrationNotificationTappedEventArgs— round-trip and null default