Fix: Improve Google Calendar OAuth reliability with retry and wake refresh#904
Conversation
dosubot
Bot
added
the
size:L
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can disable the changed files summary in the walkthrough.Disable the |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #904 +/- ##
==========================================
+ Coverage 26.30% 33.95% +7.64%
==========================================
Files 36 49 +13
Lines 5645 5307 -338
Branches 2210 1748 -462
==========================================
+ Hits 1485 1802 +317
+ Misses 4103 3462 -641
+ Partials 57 43 -14 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
MeetingBarTests/Helpers/FakeEventStore.swift (1)
58-60: Let the fake throw fromsignIn(forcePrompt:)too.The retry path now depends on re-auth failures as much as
fetchAllCalendars()failures, but this helper can only simulate the latter. Adding injectable sign-in failures here would let the new suite cover the auth-refresh branch that motivated the PR.Possible test-helper extension
+ var signInErrorToThrow: Error? + func signIn(forcePrompt: Bool = false) async throws { signInCallCount += 1 + if let error = signInErrorToThrow { + throw error + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MeetingBarTests/Helpers/FakeEventStore.swift` around lines 58 - 60, The FakeEventStore.signIn(forcePrompt:) currently only increments signInCallCount and cannot simulate auth failures; add an injectable failure control (e.g. a signInError: Error? or signInShouldThrow + signInError) to the FakeEventStore and update signIn(forcePrompt:) to throw that error when set while still incrementing signInCallCount; ensure tests can set/reset this injectable to exercise the auth-refresh retry branch.MeetingBarTests/RetryAndStalenessTests.swift (1)
28-45: Make this exhaustion test deterministic.The fixed 3s
asyncAftermakes this case slower and can flap if another refresh cycle starts before the timer fires. Waiting on a published terminal state—or injecting a sleeper/scheduler—would make the retry-count assertions precise instead of timing-based.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MeetingBarTests/RetryAndStalenessTests.swift` around lines 28 - 45, Replace the time-based DispatchQueue.main.asyncAfter in RetryAndStalenessTests with a deterministic wait for the store's terminal "exhausted" state (or use an injected test scheduler to advance timers); observe the store's published state (e.g., store.$state or equivalent property) and fulfill the expectation when it transitions to the exhaustion/terminal enum value, then assert store.fetchCalendarsCallCount and store.signInCallCount (still using the cycles = fetchCalendarsCallCount / 5 logic) so the test no longer relies on a fixed 3s delay and becomes deterministic and reliable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MeetingBar/Core/Managers/EventManager.swift`:
- Around line 119-124: The triggerRefresh() method currently only checks
lastSuccessfulRefresh and can enqueue duplicate refresh work when multiple
wake/unlock events arrive before that timestamp updates; add an "in-flight"
guard to coalesce concurrent refreshes by tracking whether a refresh is
currently running (e.g., an atomic Bool or DispatchQueue-protected flag like
isRefreshInFlight) and return early if set; set the flag true immediately when
starting the refresh pipeline (or inside triggerRefresh before calling
refreshSubject.send()) and clear it when the pipeline completes or fails so
subsequent calls will trigger again; reference triggerRefresh, refreshSubject,
lastSuccessfulRefresh, and Self.staleThreshold when adding the flag and its
set/clear points.
- Around line 170-174: EventManager currently calls provider.signIn(forcePrompt:
false) on any retry (inside the attempt > 0 block), which causes unwanted
permission/OAuth dialogs (e.g., EKEventStore ignores forcePrompt) and triggers
re-auth flows for non-auth errors; change this to only attempt a silent refresh
for providers that support it and only for authentication-related errors: add
and use a provider capability check (e.g., provider.supportsSilentRefresh or a
new Provider.refreshTokenSilently()) and guard the retry call by inspecting the
error (auth/token expired error kinds) before invoking the refresh; ensure
EKEventStore and other providers that always prompt are excluded from the
retry-path so signIn() is not called unconditionally during EventManager retries
(preserve existing maxRetryAttempts/attempt logic).
---
Nitpick comments:
In `@MeetingBarTests/Helpers/FakeEventStore.swift`:
- Around line 58-60: The FakeEventStore.signIn(forcePrompt:) currently only
increments signInCallCount and cannot simulate auth failures; add an injectable
failure control (e.g. a signInError: Error? or signInShouldThrow + signInError)
to the FakeEventStore and update signIn(forcePrompt:) to throw that error when
set while still incrementing signInCallCount; ensure tests can set/reset this
injectable to exercise the auth-refresh retry branch.
In `@MeetingBarTests/RetryAndStalenessTests.swift`:
- Around line 28-45: Replace the time-based DispatchQueue.main.asyncAfter in
RetryAndStalenessTests with a deterministic wait for the store's terminal
"exhausted" state (or use an injected test scheduler to advance timers); observe
the store's published state (e.g., store.$state or equivalent property) and
fulfill the expectation when it transitions to the exhaustion/terminal enum
value, then assert store.fetchCalendarsCallCount and store.signInCallCount
(still using the cycles = fetchCalendarsCallCount / 5 logic) so the test no
longer relies on a fixed 3s delay and becomes deterministic and reliable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b996686d-baf9-4614-8638-b3356bea8419
📒 Files selected for processing (4)
MeetingBar/App/AppDelegate.swiftMeetingBar/Core/Managers/EventManager.swiftMeetingBarTests/Helpers/FakeEventStore.swiftMeetingBarTests/RetryAndStalenessTests.swift
| public func triggerRefresh() { | ||
| guard Date().timeIntervalSince(lastSuccessfulRefresh) > Self.staleThreshold else { | ||
| NSLog("EventManager skipping refresh — last successful refresh was \(Int(Date().timeIntervalSince(lastSuccessfulRefresh)))s ago (threshold: \(Int(Self.staleThreshold))s)") | ||
| return | ||
| } | ||
| refreshSubject.send() |
There was a problem hiding this comment.
Coalesce wake/unlock refreshes while one is already running.
Both wake handlers in MeetingBar/App/AppDelegate.swift, Lines 333-345 call this, and this guard only checks the last successful refresh. After a long sleep, wake and unlock can both arrive before that timestamp is updated, so you still enqueue duplicate retry loops and duplicate Google calls. Add an in-flight guard here (or in the pipeline) to coalesce those notifications.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MeetingBar/Core/Managers/EventManager.swift` around lines 119 - 124, The
triggerRefresh() method currently only checks lastSuccessfulRefresh and can
enqueue duplicate refresh work when multiple wake/unlock events arrive before
that timestamp updates; add an "in-flight" guard to coalesce concurrent
refreshes by tracking whether a refresh is currently running (e.g., an atomic
Bool or DispatchQueue-protected flag like isRefreshInFlight) and return early if
set; set the flag true immediately when starting the refresh pipeline (or inside
triggerRefresh before calling refreshSubject.send()) and clear it when the
pipeline completes or fails so subsequent calls will trigger again; reference
triggerRefresh, refreshSubject, lastSuccessfulRefresh, and Self.staleThreshold
when adding the flag and its set/clear points.
| // On retries, attempt to re-authenticate in case the token expired | ||
| if attempt > 0 { | ||
| NSLog("EventManager retry attempt \(attempt)/\(Self.maxRetryAttempts) — re-authenticating…") | ||
| try await provider.signIn(forcePrompt: false) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "(EventManager|EventStore)" | head -20Repository: leits/MeetingBar
Length of output: 325
🏁 Script executed:
cat -n MeetingBar/Core/Managers/EventManager.swift | sed -n '160,180p'Repository: leits/MeetingBar
Length of output: 1176
🏁 Script executed:
cat -n MeetingBar/Core/EventStores/EKEventStore.swift | sed -n '1,50p'Repository: leits/MeetingBar
Length of output: 2055
🏁 Script executed:
cat -n MeetingBar/Core/EventStores/Protocol.swiftRepository: leits/MeetingBar
Length of output: 929
🏁 Script executed:
cat -n MeetingBar/Core/EventStores/GCEventStore.swift | head -100Repository: leits/MeetingBar
Length of output: 4432
🏁 Script executed:
cat -n MeetingBar/Core/Managers/EventManager.swift | sed -n '160,210p'Repository: leits/MeetingBar
Length of output: 2606
🏁 Script executed:
cat -n MeetingBar/Core/EventStores/GCEventStore.swift | sed -n '95,180p'Repository: leits/MeetingBar
Length of output: 4130
🏁 Script executed:
rg "forcePrompt" MeetingBar/Core/EventStores/EKEventStore.swiftRepository: leits/MeetingBar
Length of output: 124
Extract silent token refresh from universal retry hook.
The code calls provider.signIn(forcePrompt: false) on every retry attempt (EventManager.swift:173), but this is problematic:
-
EKEventStore (lines 22-38) ignores the
forcePromptparameter entirely and always triggers EventKit's permission request APIs (requestFullAccessToEventsorrequestAccess), which display system dialogs. -
The retry is unconditional—it calls
signIn()regardless of the error type. Network failures, API errors, and rate-limiting will incorrectly trigger re-authentication flows instead of being treated as transient failures.
This forces users through permission dialogs or OAuth flows even when the original failure had nothing to do with authorization. Either implement a provider-specific silent token refresh mechanism (distinct from signIn()), or gate this call to only auth-related errors and providers that support silent refresh.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MeetingBar/Core/Managers/EventManager.swift` around lines 170 - 174,
EventManager currently calls provider.signIn(forcePrompt: false) on any retry
(inside the attempt > 0 block), which causes unwanted permission/OAuth dialogs
(e.g., EKEventStore ignores forcePrompt) and triggers re-auth flows for non-auth
errors; change this to only attempt a silent refresh for providers that support
it and only for authentication-related errors: add and use a provider capability
check (e.g., provider.supportsSilentRefresh or a new
Provider.refreshTokenSilently()) and guard the retry call by inspecting the
error (auth/token expired error kinds) before invoking the refresh; ensure
EKEventStore and other providers that always prompt are excluded from the
retry-path so signIn() is not called unconditionally during EventManager retries
(preserve existing maxRetryAttempts/attempt logic).
1 participant
Problem
When MeetingBar uses Google Calendar as the data source, the connection often "drops" overnight. After waking the Mac in the morning, no meetings are displayed. Users have to manually go to Preferences → Calendars, switch to macOS Calendar, switch back to Google Calendar, and re-select their calendars to restore functionality.
Root Cause
Two issues were identified:
No refresh on wake/unlock: When the Mac sleeps overnight and the user unlocks it in the morning, nothing triggers an event refresh. The 180-second Combine timer gets suspended during sleep and may not fire reliably or immediately on wake.
Silent failure on auth errors: When a refresh fails (e.g., expired OAuth token after overnight sleep), the error is logged but the result is silently set to empty arrays — showing zero calendars and zero events with no user feedback.
Solution
Wake/Unlock Refresh
unlockListenernow triggerseventManager.triggerRefresh()on screen unlockNSWorkspace.didWakeNotificationobserver as a belt-and-suspenders for system wake (covers Touch ID auto-unlock and other edge cases)triggerRefresh()includes a 15-minute staleness check to avoid redundant API calls on quick lock/unlock cyclesExponential Backoff Retry
fetchWithRetry()provider.signIn(forcePrompt: false)to re-authenticate (triggering AppAuth's token refresh)Test Infrastructure
XCTestguard inapplicationDidFinishLaunchingto prevent the app UI from launching during testsFakeEventStorewith error injection and call counting for retry testingFiles Changed
MeetingBar/App/AppDelegate.swiftMeetingBar/Core/Managers/EventManager.swiftMeetingBarTests/Helpers/FakeEventStore.swiftMeetingBarTests/RetryAndStalenessTests.swiftSummary by CodeRabbit