Conversation
Greptile SummaryThis PR replaces macOS system ( However, two behavioral regressions and one logic bug were found:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant PA as ProactiveAssistantsPlugin
participant NS as NotificationService
participant FCM as FloatingControlBarManager
participant FCW as FloatingControlBarWindow
participant View as FloatingControlBarView
PA->>NS: sendNotification(title, message)
NS->>FCM: showNotification(title, message, assistantId, sound)
FCM->>FCM: play custom sound (focusLost/focusRegained only)
alt window hidden && bar disabled
FCM->>FCW: orderFrontRegardless()
FCM->>FCM: notificationWasTemporarilyShown = true
end
alt currentNotification == nil && !showingAIConversation
FCM->>FCW: showNotification(notification)
FCW->>View: state.currentNotification = notification
FCW->>FCW: resizeAnchored(expandedWidth × notificationHeight)
FCM->>FCM: schedule auto-dismiss after 6s
else
FCM->>FCM: pendingNotifications.append(notification)
end
alt User taps ✕
View->>FCM: dismissCurrentNotification()
FCM->>FCM: cancel auto-dismiss timer
else Auto-dismiss fires
FCM->>FCM: dismissNotificationAndAdvanceQueue()
end
FCM->>FCW: dismissNotification()
FCW->>View: state.currentNotification = nil
FCW->>FCW: resizeAnchored(minBarSize or expandedBarSize)
alt pendingNotifications not empty
FCM->>FCM: presentNotification(next) [⚠ resets notificationWasTemporarilyShown]
else no more pending
alt notificationWasTemporarilyShown && !isEnabled
FCM->>FCW: orderOut(nil)
end
FCM->>FCM: notificationWasTemporarilyShown = false
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private func presentNotification(_ notification: FloatingBarNotification, in window: FloatingControlBarWindow) { | ||
| if !window.isVisible { | ||
| notificationWasTemporarilyShown = true | ||
| window.orderFrontRegardless() | ||
| } else { | ||
| notificationWasTemporarilyShown = false | ||
| } |
There was a problem hiding this comment.
notificationWasTemporarilyShown incorrectly reset when chaining queued notifications
When the bar is disabled (!isEnabled) and two notifications arrive in quick succession:
- First notification: window is hidden →
notificationWasTemporarilyShown = true, window shown temporarily. - Second notification: queued in
pendingNotifications. - First dismissed →
dismissNotificationAndAdvanceQueuecallspresentNotification(#2, window)and returns early, skipping theorderOut+ reset block. - In
presentNotification(#2), the window is now visible sonotificationWasTemporarilyShown = falseis set unconditionally. - Second notification dismissed →
notificationWasTemporarilyShown == false, sowindow.orderOut(nil)is never called.
Result: the floating bar window remains visible even when the user has the bar disabled, as long as at least two notifications fired.
The fix is to not reset notificationWasTemporarilyShown to false when window is already visible during a queued-notification chain:
private func presentNotification(_ notification: FloatingBarNotification, in window: FloatingControlBarWindow) {
if !window.isVisible {
notificationWasTemporarilyShown = true
window.orderFrontRegardless()
}
// Do NOT set notificationWasTemporarilyShown = false here;
// it was set true by the first notification and should persist through the chain.
// ...
}| func sendNotification(title: String, message: String, assistantId: String = "default", sound: NotificationSound = .default) { | ||
| // Check permission before attempting delivery to avoid UNErrorDomain code 1 errors | ||
| UNUserNotificationCenter.current().getNotificationSettings { [weak self] settings in | ||
| Task { @MainActor in | ||
| guard settings.authorizationStatus == .authorized else { | ||
| log("Notification skipped (auth=\(settings.authorizationStatus.rawValue)): \(title)") | ||
|
|
||
| // If auth reverted to notDetermined (not explicitly denied), trigger repair | ||
| // Debounce: at most once per 10 minutes to avoid hammering lsregister | ||
| if settings.authorizationStatus == .notDetermined { | ||
| let now = Date() | ||
| if self?.lastRepairAttempt == nil || now.timeIntervalSince(self?.lastRepairAttempt ?? .distantPast) > 600 { | ||
| self?.lastRepairAttempt = now | ||
| log("Notification auth is notDetermined at send time — triggering repair") | ||
| AnalyticsManager.shared.notificationRepairTriggered( | ||
| reason: "send_time_not_determined", | ||
| previousStatus: "unknown", | ||
| currentStatus: "notDetermined" | ||
| ) | ||
| ProactiveAssistantsPlugin.repairNotificationRegistration() | ||
| } | ||
| } | ||
|
|
||
| return | ||
| } | ||
|
|
||
| self?.deliverNotification(title: title, message: message, assistantId: assistantId, sound: sound) | ||
| } | ||
| } | ||
| FloatingControlBarManager.shared.showNotification( | ||
| title: title, | ||
| message: message, | ||
| assistantId: assistantId, | ||
| sound: sound | ||
| ) | ||
| } |
There was a problem hiding this comment.
Screen capture reset "Reset Now" action button silently dropped
sendNotification is now the only public entry point and it routes all notifications through FloatingControlBarManager.showNotification. However, the screen capture reset notification (NotificationService.screenCaptureResetTitle) is sent via NotificationService.shared.sendNotification(...) from ProactiveAssistantsPlugin (lines 1342–1345 and 1355–1358 in ProactiveAssistantsPlugin.swift).
Previously, deliverNotification detected this title and assigned screenCaptureResetCategoryId, which added a "Reset Now" UNNotificationAction. The UNUserNotificationCenter delegate then called handleScreenCaptureResetAction when the user tapped it, triggering ScreenCaptureService.resetScreenCapturePermissionAndRestart().
With this change:
- The notification appears only as a floating bar notification with no action button.
deliverNotification(and all itsscreenCaptureResetCategoryIdlogic) is now unreachable dead code.- Users who encounter a broken screen recording session will see a message saying "Click to open Settings" but no actionable button and no automatic reset mechanism.
The screen capture reset notification likely needs to keep using a system UNNotification with its action button, or a dedicated action button needs to be added to the floating bar notification view for this specific case.
| switch sound { | ||
| case .focusLost, .focusRegained: | ||
| sound.playCustomSound() | ||
| case .default, .none: | ||
| break | ||
| } |
There was a problem hiding this comment.
Default notification sound is silently dropped
In showNotification, the switch only plays sounds for .focusLost and .focusRegained. For NotificationSound.default (the case used by most proactive assistant notifications), no sound is played at all.
Previously, deliverNotification passed sound.unSound (which returns UNNotificationSound.default for the .default case) to UNMutableNotificationContent.sound, so the system played the default notification chime. That feedback is now absent for all standard notifications.
If silent in-app notifications are intentional, the NotificationSound.default case in NotificationSound.unSound and the comment on line 130 of OnboardingNotificationStepView should be updated to reflect this. If not, a sound should be played here for .default — for example:
case .default:
NSSound.beep()or load and play a bundled sound similar to the custom cases.
1 participant
Summary
Testing