feat: add support for multiple Google Calendar accounts#911
Open
vlordier wants to merge 14 commits intoleits:masterfrom
Open
feat: add support for multiple Google Calendar accounts#911vlordier wants to merge 14 commits intoleits:masterfrom
vlordier wants to merge 14 commits intoleits:masterfrom
Conversation
Allow users to connect multiple Google accounts (e.g., personal and work) simultaneously. Each account's calendars are fetched and displayed grouped by email address. Changes: - Add GoogleAccount model for tracking multiple accounts - Refactor GCEventStore to manage per-account OAuth states - Prefix calendar IDs with account ID to prevent collisions - Add Google Accounts section in Preferences with add/remove UI - Add confirmation dialog before removing an account - Persist accounts in Defaults and auth states in Keychain
dosubot
Bot
added
the
size:L
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughGCEventStore is refactored from a single-account model to a multi-account model: adds Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as CalendarsTab (UI)
participant Store as GCEventStore
participant Defaults
participant Keychain
participant Google as Google API
User->>UI: Tap "Add Google Account"
UI->>Store: addAccount()
Store->>Google: Start OAuth flow
Google->>Store: Return OIDAuthState
Store->>Defaults: Append GoogleAccount to googleAccounts
Store->>Keychain: Store auth state for accountId
Store->>UI: Return new GoogleAccount
User->>UI: Request calendars
UI->>Store: fetchAllCalendars()
Store->>Defaults: Read googleAccounts
loop per account
Store->>Keychain: Load auth state for accountId
Store->>Google: Fetch calendar list (using account token)
Google->>Store: Return calendars
Store->>Store: Prefix calendar IDs as "accountId:calendarId"
end
Store->>UI: Return aggregated prefixed calendars
UI->>User: Display calendars grouped by account
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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)
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 |
There was a problem hiding this comment.
Pull request overview
This PR adds multi-account Google Calendar support, allowing users to connect multiple Google accounts and fetch/display calendars grouped by account email, with add/remove account management in Preferences.
Changes:
- Added a
GoogleAccountmodel and persisted connected accounts inDefaults(googleAccounts). - Refactored
GCEventStoreto manage per-account OAuth state, store each auth state under a unique Keychain entry, and prefix calendar IDs asaccountId:calendarIdto avoid collisions. - Updated Preferences → Calendars UI to show a new “Google Accounts” section with add/remove flows and a remove confirmation dialog.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| MeetingBar/UI/Views/Preferences/CalendarsTab.swift | Adds Google Accounts management UI and a Google empty state in the calendars list. |
| MeetingBar/Extensions/DefaultsKeys.swift | Persists connected Google accounts in Defaults via a new key. |
| MeetingBar/Core/Models/GoogleAccount.swift | Introduces the model representing a connected Google account (id + email). |
| MeetingBar/Core/Managers/EventManager.swift | Adds a weak shared instance reference (currently unused). |
| MeetingBar/Core/EventStores/GCEventStore.swift | Implements per-account OAuth storage and multi-account calendar/event fetching with ID prefixing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+24
to
+29
| if Defaults[.eventStoreProvider] == .googleCalendar { | ||
| GroupBox(label: Label("Google Accounts", systemImage: "person.3")) { | ||
| GoogleAccountsSection(eventManager: eventManager) | ||
| } | ||
| .padding(.bottom, 5) | ||
| } |
There was a problem hiding this comment.
New UI strings in this file are hard-coded (e.g., "Google Accounts", "No accounts connected…", "Add Google Account", "Connected", "Remove Account", etc.) while the surrounding Preferences UI uses localized strings via .loco(). Please switch these new user-facing strings to the localization system so they participate in translations.
Comment on lines
+36
to
+39
| } else { | ||
| Text("No accounts connected. Add a Google account to get started.") | ||
| .foregroundColor(.secondary) | ||
| .padding() |
There was a problem hiding this comment.
The empty-state message shown when eventManager.calendars.isEmpty currently assumes no Google accounts are connected. calendars can also be empty due to fetch errors, permissions issues, or accounts that exist but have no calendars, so this text can be misleading. Consider basing the message on Defaults[.googleAccounts].isEmpty (and/or surfacing an error/loading state) rather than calendars.isEmpty alone.
Suggested change
| } else { | |
| Text("No accounts connected. Add a Google account to get started.") | |
| .foregroundColor(.secondary) | |
| .padding() | |
| } else if Defaults[.googleAccounts].isEmpty { | |
| Text("No accounts connected. Add a Google account to get started.") | |
| .foregroundColor(.secondary) | |
| .padding() | |
| } else { | |
| Text("No calendars available. Try refreshing or check account access.") | |
| .foregroundColor(.secondary) | |
| .padding() |
Comment on lines
+143
to
+156
| func removeAccount(_ account: GoogleAccount) async { | ||
| guard let state = authStates[account.id] else { return } | ||
|
|
||
| // Revoke tokens in parallel | ||
| let access = state.lastTokenResponse?.accessToken | ||
| let refresh = state.lastTokenResponse?.refreshToken | ||
| await withTaskGroup(of: Void.self) { grp in | ||
| if let acc = access { grp.addTask { try? await self.revoke(token: acc) } } | ||
| if let ref = refresh { grp.addTask { try? await self.revoke(token: ref) } } | ||
| } | ||
|
|
||
| clearAuthState() | ||
| authStates.removeValue(forKey: account.id) | ||
| accounts.removeAll { $0.id == account.id } | ||
| Keychain.delete(for: accountKeychainKey(accountId: account.id)) | ||
| } |
There was a problem hiding this comment.
removeAccount returns early when there’s no in-memory authStates[account.id]. This prevents users from removing an account if Keychain restore failed or state was already evicted, leaving a stuck account entry in Defaults[.googleAccounts]. removeAccount should still remove the account from Defaults, delete the Keychain entry, and ideally also purge any selectedCalendarIDs prefixed with this account id.
Comment on lines
79
to
84
| // Singleton | ||
| static let shared = GCEventStore() | ||
| private override init() { | ||
| super.init() | ||
| self.authState = restoreAuthState() | ||
| // delegates were set in didSet, but set them again just in case restore returned nil | ||
| self.authState?.stateChangeDelegate = self | ||
| self.authState?.errorDelegate = self | ||
| restoreAllAuthStates() | ||
| } |
There was a problem hiding this comment.
This change switches Keychain storage from a single legacy key (googleAuthKeychainName) to per-account keys (googleAuthKeychainName_<uuid>), but restoreAllAuthStates() only loads states for accounts already present in Defaults[.googleAccounts]. On upgrade, existing users will likely have a valid legacy auth state in Keychain but no googleAccounts entry, effectively forcing a sign-out and leaving selectedCalendarIDs unmigrated. Consider adding a one-time migration path: if googleAccounts is empty, try loading the legacy Keychain entry, derive email from the ID token, create an initial GoogleAccount, persist it under the new key, and migrate existing selected calendar IDs by prefixing them.
Comment on lines
+225
to
+227
| let calendarsByAccount = Dictionary(grouping: calendars) { calendar in | ||
| calendar.id.components(separatedBy: ":").first ?? "" | ||
| } |
There was a problem hiding this comment.
The account id extraction uses calendar.id.components(separatedBy: ":").first, which will treat any unprefixed calendar id as the account id and silently skip fetching events (no matching authStates entry). It also splits on every :, which is more work than needed and can be error-prone if ids ever contain :. Consider parsing with a single split (maxSplits: 1) and explicitly handling/migrating unprefixed ids.
Comment on lines
+366
to
+375
| nonisolated func didChange(_ state: OIDAuthState) { | ||
| let accessToken = state.lastTokenResponse?.accessToken | ||
| Task { @MainActor in | ||
| if let account = self.accounts.first(where: { account in | ||
| guard let storedState = self.authStates[account.id] else { return false } | ||
| return storedState.lastTokenResponse?.accessToken == accessToken | ||
| }) { | ||
| self.persistAuthState(for: account) | ||
| } | ||
| } |
There was a problem hiding this comment.
didChange(_:) / didEncounterAuthorizationError attempt to map the incoming OIDAuthState back to an account by comparing access token strings. This can fail when the access token is nil, has just rotated, or (in edge cases) if two states temporarily share the same token value, leading to missed persistence or removing the wrong account. Since OIDAuthState is a class, prefer mapping by object identity (e.g., storedState === state) or keep a dedicated [ObjectIdentifier: accountId] index.
- GoogleAccount model: creation, hashable, codable, defaults persistence - Multi-account calendar: ID prefixing, uniqueness, grouping by source
dosubot
Bot
added
size:XL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/EventStores/GCEventStore.swift`:
- Around line 305-317: In restoreAllAuthStates(), handle failures instead of
silently skipping: when Keychain.load(for: accountKeychainKey(accountId:))
returns nil or NSKeyedUnarchiver throws, remove or mark that account from the
accounts collection (and delete any stale Keychain entry) so it won't be treated
as connected by signIn()/fetchAllCalendars(); additionally, if replacing a
legacy single-account storage, attempt to detect and migrate that legacy
OIDAuthState into the per-account list here before pruning. Ensure
state.stateChangeDelegate and state.errorDelegate are still set and
authStates[account.id] is populated only for successfully restored or migrated
states.
- Around line 125-133: The code is creating a new GoogleAccount with a fresh
accountId even when the email already exists, causing duplicate accounts; change
the logic in GCEventStore (where GoogleAccount(id: accountId, email: email) is
created and appended) to first check for an existing account by email in
self.accounts, and if found reuse that account's id and update that account (do
not append), otherwise create and append a new GoogleAccount; also update
self.authStates[accountId] (using the reused id), set state.stateChangeDelegate
and state.errorDelegate, call persistAuthState(for:) with the reused/existing
GoogleAccount, and only call sendNotification/cont.resume once with the correct
account instance. Ensure no duplicate account objects are appended to
self.accounts.
- Around line 366-389: The code currently matches accounts by comparing access
tokens in didChange(_:) and authState(_:didEncounterAuthorizationError:);
instead, find the matching account by comparing the stored OIDAuthState instance
identity with the provided state using ===. In both nonisolated methods
(didChange and authState(_:didEncounterAuthorizationError:)) iterate your
authStates to locate the account whose storedState === state (or check
authStates[account.id] === state) and then call persistAuthState(for: account)
or await removeAccount(account) as before; remove reliance on
lastTokenResponse?.accessToken which may be nil or rotated.
- Around line 143-156: The removal currently waits for revoke tasks before
clearing local state; instead make removeAccount(_ account: GoogleAccount) clear
local records first (remove from authStates, drop from accounts, call
Keychain.delete(accountKeychainKey(accountId:))) and then fire-and-forget
revocations by calling revoke(token:) asynchronously (e.g., spawn tasks without
awaiting or use Task { try? await self.revoke(token: ...) }) so revocation is
best-effort and does not block local deletion; apply the same pattern for
signOut() to ensure local state is cleared immediately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 033c97e6-5882-429e-ae8b-8b93929d8a64
📒 Files selected for processing (5)
MeetingBar/Core/EventStores/GCEventStore.swiftMeetingBar/Core/Managers/EventManager.swiftMeetingBar/Core/Models/GoogleAccount.swiftMeetingBar/Extensions/DefaultsKeys.swiftMeetingBar/UI/Views/Preferences/CalendarsTab.swift
Comment on lines
+185
to
+187
| let prefixedCalendarId = "\(account.id):\(calendarID)" | ||
| return MBCalendar(title: title, | ||
| id: prefixedCalendarId, |
There was a problem hiding this comment.
Migrate stored Google calendar IDs before namespacing them.
MeetingBar/Core/Managers/EventManager.swift, Line 117 still matches Defaults[.selectedCalendarIDs] by exact string equality. After introducing accountId:calendarId, every previously saved Google selection becomes unmatched and users have to reselect calendars on upgrade.
✅ Addressed in commit 59b9cc4
Comment on lines
+366
to
+389
| nonisolated func didChange(_ state: OIDAuthState) { | ||
| let accessToken = state.lastTokenResponse?.accessToken | ||
| Task { @MainActor in | ||
| if let account = self.accounts.first(where: { account in | ||
| guard let storedState = self.authStates[account.id] else { return false } | ||
| return storedState.lastTokenResponse?.accessToken == accessToken | ||
| }) { | ||
| self.persistAuthState(for: account) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func authState(_ state: OIDAuthState, didEncounterAuthorizationError error: Error) { | ||
| nonisolated func authState(_ state: OIDAuthState, didEncounterAuthorizationError error: Error) { | ||
| let nsErr = error as NSError | ||
| if nsErr.domain == OIDOAuthTokenErrorDomain { | ||
| // refresh token invalid → clean state & notify | ||
| clearAuthState() | ||
| let accessToken = state.lastTokenResponse?.accessToken | ||
| Task { @MainActor in | ||
| if let account = self.accounts.first(where: { account in | ||
| guard let storedState = self.authStates[account.id] else { return false } | ||
| return storedState.lastTokenResponse?.accessToken == accessToken | ||
| }) { | ||
| await self.removeAccount(account) | ||
| } | ||
| } |
There was a problem hiding this comment.
Look up delegate callbacks by OIDAuthState identity, not access token.
lastTokenResponse?.accessToken is unstable: it rotates on refresh and can be nil on authorization-error paths. Matching on that value can persist/remove the wrong account or none at all. Compare the stored OIDAuthState instances with === instead.
Suggested change
nonisolated func didChange(_ state: OIDAuthState) {
- let accessToken = state.lastTokenResponse?.accessToken
Task { `@MainActor` in
if let account = self.accounts.first(where: { account in
- guard let storedState = self.authStates[account.id] else { return false }
- return storedState.lastTokenResponse?.accessToken == accessToken
+ self.authStates[account.id] === state
}) {
self.persistAuthState(for: account)
}
@@
nonisolated func authState(_ state: OIDAuthState, didEncounterAuthorizationError error: Error) {
let nsErr = error as NSError
if nsErr.domain == OIDOAuthTokenErrorDomain {
- let accessToken = state.lastTokenResponse?.accessToken
Task { `@MainActor` in
if let account = self.accounts.first(where: { account in
- guard let storedState = self.authStates[account.id] else { return false }
- return storedState.lastTokenResponse?.accessToken == accessToken
+ self.authStates[account.id] === state
}) {
await self.removeAccount(account)
}
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MeetingBar/Core/EventStores/GCEventStore.swift` around lines 366 - 389, The
code currently matches accounts by comparing access tokens in didChange(_:) and
authState(_:didEncounterAuthorizationError:); instead, find the matching account
by comparing the stored OIDAuthState instance identity with the provided state
using ===. In both nonisolated methods (didChange and
authState(_:didEncounterAuthorizationError:)) iterate your authStates to locate
the account whose storedState === state (or check authStates[account.id] ===
state) and then call persistAuthState(for: account) or await
removeAccount(account) as before; remove reliance on
lastTokenResponse?.accessToken which may be nil or rotated.
- Always clean up Defaults/Keychain in removeAccount even if auth state missing - Clean up selectedCalendarIDs when removing an account - Add legacy auth state migration for existing single-account users - Restore forcePrompt/consent behavior for refresh_token - Fix didChange delegate to use ObjectIdentifier instead of token matching - Remove unused ensureAccountSignedIn and signInTask - Fix force-cast of items in fetchJSON, throw descriptive error - Add accessibility labels to remove button - Remove unused EventManager.shared - Use forcePrompt parameter in signIn
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #911 +/- ##
==========================================
+ Coverage 26.30% 34.51% +8.21%
==========================================
Files 36 50 +14
Lines 5645 5536 -109
Branches 2210 1797 -413
==========================================
+ Hits 1485 1911 +426
+ Misses 4103 3568 -535
Partials 57 57 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MeetingBar/Core/EventStores/GCEventStore.swift (1)
32-38:⚠️ Potential issue | 🟠 MajorDecode JWT payload segments as Base64URL before extracting
JWT ID token payload segments are Base64URL-encoded and commonly unpadded. The current code uses plain
Data(base64Encoded:), which will fail to decode valid Google ID tokens. This causesstate.userEmailto returnnil, breaking account creation and legacy migration flows.Before decoding with
Data(base64Encoded:), normalize the segment by replacing-with+,_with/, and adding padding as needed to reach a multiple of 4 characters.🩹 Suggested fix
var userEmail: String? { guard let idToken = lastTokenResponse?.idToken else { return nil } let parts = idToken.split(separator: ".") - guard parts.count > 1, - let payloadData = Data(base64Encoded: String(parts[1])), + let payload = String(parts[1]) + .replacingOccurrences(of: "-", with: "+") + .replacingOccurrences(of: "_", with: "/") + let paddedPayload = payload + String(repeating: "=", count: (4 - payload.count % 4) % 4) + guard parts.count > 1, + let payloadData = Data(base64Encoded: paddedPayload), let json = try? JSONSerialization.jsonObject(with: payloadData) as? [String: Any] else { return nil } return json["email"] as? String }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MeetingBar/Core/EventStores/GCEventStore.swift` around lines 32 - 38, userEmail currently decodes idToken payload using Data(base64Encoded:) which fails for Base64URL (unpadded) JWT segments; update the userEmail getter (referencing lastTokenResponse and idToken) to normalize the middle JWT segment by replacing '-'→'+', '_'→'/', and appending '=' padding until length % 4 == 0 before calling Data(base64Encoded:), then proceed with JSONSerialization to extract the "email" field.
♻️ Duplicate comments (5)
MeetingBar/Core/EventStores/GCEventStore.swift (5)
144-160:⚠️ Potential issue | 🟠 MajorClear local state before awaiting revoke calls.
removeAccount(_:)waits on best-effort token revocations before deleting Defaults/Keychain state. Because the sharedURLSessionis configured withwaitsForConnectivity, an offline remove/sign-out can sit for a long time while the UI still shows the account as connected. Drop local state first and revoke in background.⚡ Suggested ordering
func removeAccount(_ account: GoogleAccount) async { - if let state = authStates[account.id] { - let access = state.lastTokenResponse?.accessToken - let refresh = state.lastTokenResponse?.refreshToken - await withTaskGroup(of: Void.self) { grp in - if let acc = access { grp.addTask { try? await self.revoke(token: acc) } } - if let ref = refresh { grp.addTask { try? await self.revoke(token: ref) } } - } - authStates.removeValue(forKey: account.id) - } - + let state = authStates.removeValue(forKey: account.id) accounts.removeAll { $0.id == account.id } Keychain.delete(for: accountKeychainKey(accountId: account.id)) let prefix = "\(account.id):" Defaults[.selectedCalendarIDs] = Defaults[.selectedCalendarIDs].filter { !$0.hasPrefix(prefix) } + + if let access = state?.lastTokenResponse?.accessToken { + Task { try? await self.revoke(token: access) } + } + if let refresh = state?.lastTokenResponse?.refreshToken { + Task { try? await self.revoke(token: refresh) } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MeetingBar/Core/EventStores/GCEventStore.swift` around lines 144 - 160, The code currently awaits token revocations before clearing local state in removeAccount(_:); instead capture access/refresh tokens from authStates, then immediately remove authStates/account entry, Keychain.delete and update Defaults[.selectedCalendarIDs], and only after local cleanup spawn a background Task (or Task.detached) that performs the revoke(token:) calls via a withTaskGroup without awaiting it; reference removeAccount(_:), authStates, accounts, Keychain.delete, Defaults[.selectedCalendarIDs], and revoke(token:) when making the change.
189-194:⚠️ Potential issue | 🟠 MajorMigrate legacy Google selections before prefixing calendar IDs.
fetchAllCalendars()now rewrites every Google ID toaccountId:calendarId, butMeetingBar/Core/Managers/EventManager.swiftstill filters calendars by exact membership inDefaults[.selectedCalendarIDs]. Existing Google selections will all drop on upgrade unless those stored IDs are migrated, at least for the single-account legacy path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MeetingBar/Core/EventStores/GCEventStore.swift` around lines 189 - 194, fetchAllCalendars() currently prefixes Google calendar IDs to "accountId:calendarId" which will break existing stored selections in Defaults[.selectedCalendarIDs]; before you construct MBCalendar with the prefixed id, detect and migrate legacy Google IDs (entries that match raw calendarIDs or lack the "accountId:" prefix) to the new "accountId:calendarId" form and update Defaults[.selectedCalendarIDs] accordingly (handle the single-account legacy path specifically), so EventManager.swift's exact-membership check against Defaults[.selectedCalendarIDs] continues to match; modify the logic around where you build prefixedCalendarId in fetchAllCalendars() to perform this migration step and persist the updated selected IDs.
126-133:⚠️ Potential issue | 🟠 MajorDon't create a new logical account for an email that's already connected.
Both auth paths always mint a fresh
accountIdand append it. Re-authing the same mailbox duplicates calendars/events under the same source section, and the new prefix invalidates previously savedselectedCalendarIDsfor that mailbox. Upsert by email and reuse the existing account ID instead.Also applies to: 289-296
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MeetingBar/Core/EventStores/GCEventStore.swift` around lines 126 - 133, When creating a GoogleAccount in GCEventStore (where code currently does GoogleAccount(id: accountId, email: email), sets self.authStates[accountId] = state, state.stateChangeDelegate = self, state.errorDelegate = self, self.persistAuthState(for: account), and self.accounts.append(account)), first look up an existing account by the email and if found reuse its existing account.id instead of minting a new accountId; update authStates[existingId] = state, set the delegates on state, call persistAuthState(for: existingAccount) and do not append a duplicate to self.accounts. Apply the same upsert-by-email logic to the other auth path that also constructs GoogleAccount to avoid duplicated accounts and invalidated selectedCalendarIDs.
380-391:⚠️ Potential issue | 🟠 MajorPrune accounts whose auth state can't be restored.
Missing or unreadable Keychain entries are silently skipped, but the corresponding
GoogleAccountstays inDefaults. The Preferences UI then still shows it as connected,fetchAllCalendars()ignores it, andsignIn()early-returns becauseaccountsis non-empty. Remove the stale account here, delete its bad keychain entry, and clean its prefixed selections.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MeetingBar/Core/EventStores/GCEventStore.swift` around lines 380 - 391, In restoreAllAuthStates(), when Keychain.load(for: accountKeychainKey(accountId:)) returns nil or unarchiving OIDAuthState fails, treat the account as stale: remove it from the accounts/defaults collection (so signIn()/fetchAllCalendars() won't early-return), delete its keychain entry (e.g. Keychain.delete(for: key)), and clear any per-account prefixed selections stored in Defaults (keys that include the account.id, e.g. selected-calendar or similar prefixes) before continuing; also ensure authStates[account.id] is not left set.
59-60:⚠️ Potential issue | 🟠 MajorSerialize
addAccountwhile an OAuth flow is already in flight.
currentAuthorizationFlowandpendingAuthAccountIdare singleton-wide, but neither add path rejects a second call. A later add can overwrite the first session, and the first callback then clears the second flow's state. The sheet inMeetingBar/UI/Views/Preferences/CalendarsTab.swiftcan still be dismissed mid-auth, so this race is user-reachable today.🔒 Minimal guard
enum AuthError: Error { case notSignedIn case refreshFailed case invalidResponse + case authorizationInProgress } @@ func addAccount() async throws -> GoogleAccount { + guard currentAuthorizationFlow == nil else { throw AuthError.authorizationInProgress } let accountId = UUID().uuidString @@ private func addAccountWithForcePrompt(_ forcePrompt: Bool) async throws -> GoogleAccount { + guard currentAuthorizationFlow == nil else { throw AuthError.authorizationInProgress } let accountId = UUID().uuidStringAlso applies to: 93-95, 118-124, 251-253, 281-287
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MeetingBar/Core/EventStores/GCEventStore.swift` around lines 59 - 60, Concurrent calls to addAccount can overwrite singleton state (currentAuthorizationFlow and pendingAuthAccountId) causing one auth flow to cancel another; fix by serializing and validating flows: in addAccount (and any other methods that set currentAuthorizationFlow/pendingAuthAccountId) guard at the start under MainActor that no authorization is already in flight (e.g., if currentAuthorizationFlow != nil or pendingAuthAccountId != nil then reject/return an error or queue the request), set both variables only once, and in the OIDExternalUserAgentSession completion handlers verify that the session and pendingAuthAccountId still match the values you set before clearing them (do not unconditionally nil out currentAuthorizationFlow/pendingAuthAccountId); apply this pattern to the other affected sites that reference currentAuthorizationFlow and pendingAuthAccountId to prevent races.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@MeetingBar/Core/EventStores/GCEventStore.swift`:
- Around line 32-38: userEmail currently decodes idToken payload using
Data(base64Encoded:) which fails for Base64URL (unpadded) JWT segments; update
the userEmail getter (referencing lastTokenResponse and idToken) to normalize
the middle JWT segment by replacing '-'→'+', '_'→'/', and appending '=' padding
until length % 4 == 0 before calling Data(base64Encoded:), then proceed with
JSONSerialization to extract the "email" field.
---
Duplicate comments:
In `@MeetingBar/Core/EventStores/GCEventStore.swift`:
- Around line 144-160: The code currently awaits token revocations before
clearing local state in removeAccount(_:); instead capture access/refresh tokens
from authStates, then immediately remove authStates/account entry,
Keychain.delete and update Defaults[.selectedCalendarIDs], and only after local
cleanup spawn a background Task (or Task.detached) that performs the
revoke(token:) calls via a withTaskGroup without awaiting it; reference
removeAccount(_:), authStates, accounts, Keychain.delete,
Defaults[.selectedCalendarIDs], and revoke(token:) when making the change.
- Around line 189-194: fetchAllCalendars() currently prefixes Google calendar
IDs to "accountId:calendarId" which will break existing stored selections in
Defaults[.selectedCalendarIDs]; before you construct MBCalendar with the
prefixed id, detect and migrate legacy Google IDs (entries that match raw
calendarIDs or lack the "accountId:" prefix) to the new "accountId:calendarId"
form and update Defaults[.selectedCalendarIDs] accordingly (handle the
single-account legacy path specifically), so EventManager.swift's
exact-membership check against Defaults[.selectedCalendarIDs] continues to
match; modify the logic around where you build prefixedCalendarId in
fetchAllCalendars() to perform this migration step and persist the updated
selected IDs.
- Around line 126-133: When creating a GoogleAccount in GCEventStore (where code
currently does GoogleAccount(id: accountId, email: email), sets
self.authStates[accountId] = state, state.stateChangeDelegate = self,
state.errorDelegate = self, self.persistAuthState(for: account), and
self.accounts.append(account)), first look up an existing account by the email
and if found reuse its existing account.id instead of minting a new accountId;
update authStates[existingId] = state, set the delegates on state, call
persistAuthState(for: existingAccount) and do not append a duplicate to
self.accounts. Apply the same upsert-by-email logic to the other auth path that
also constructs GoogleAccount to avoid duplicated accounts and invalidated
selectedCalendarIDs.
- Around line 380-391: In restoreAllAuthStates(), when Keychain.load(for:
accountKeychainKey(accountId:)) returns nil or unarchiving OIDAuthState fails,
treat the account as stale: remove it from the accounts/defaults collection (so
signIn()/fetchAllCalendars() won't early-return), delete its keychain entry
(e.g. Keychain.delete(for: key)), and clear any per-account prefixed selections
stored in Defaults (keys that include the account.id, e.g. selected-calendar or
similar prefixes) before continuing; also ensure authStates[account.id] is not
left set.
- Around line 59-60: Concurrent calls to addAccount can overwrite singleton
state (currentAuthorizationFlow and pendingAuthAccountId) causing one auth flow
to cancel another; fix by serializing and validating flows: in addAccount (and
any other methods that set currentAuthorizationFlow/pendingAuthAccountId) guard
at the start under MainActor that no authorization is already in flight (e.g.,
if currentAuthorizationFlow != nil or pendingAuthAccountId != nil then
reject/return an error or queue the request), set both variables only once, and
in the OIDExternalUserAgentSession completion handlers verify that the session
and pendingAuthAccountId still match the values you set before clearing them (do
not unconditionally nil out currentAuthorizationFlow/pendingAuthAccountId);
apply this pattern to the other affected sites that reference
currentAuthorizationFlow and pendingAuthAccountId to prevent races.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fa33a629-18ee-41a2-a054-41e175f2b401
📒 Files selected for processing (4)
MeetingBar/Core/EventStores/GCEventStore.swiftMeetingBar/UI/Views/Preferences/CalendarsTab.swiftMeetingBarTests/GoogleAccountTests.swiftMeetingBarTests/TimelineLogicTests.swift
✅ Files skipped from review due to trivial changes (1)
- MeetingBarTests/TimelineLogicTests.swift
- Add localization strings for all new UI text (preferences_calendars_*) - Use @default(.googleAccounts) instead of @State + manual refresh - Distinguish between no accounts vs no calendars in empty state - Safe calendar ID parsing with hasPrefix check instead of blind dropFirst - Use split on first ':' instead of components(separatedBy:) - Remove duplicate addAccountWithForcePrompt method - Fix async closure in AddAccountSheet
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
MeetingBar/Core/EventStores/GCEventStore.swift (1)
160-163: UnusedforcePromptparameter.The
forcePromptparameter is ignored (underscore prefix). If the intention is thatsignInonly adds an account when none exist, consider removing this parameter from the protocol/signature to avoid confusion. Otherwise, if force-prompting should be supported, the logic needs adjustment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MeetingBar/Core/EventStores/GCEventStore.swift` around lines 160 - 163, The signIn(forcePrompt _: Bool) method currently ignores its forcePrompt parameter; either remove the unused parameter from the signIn signature in GCEventStore (and the protocol it implements) or honor it by changing the logic in signIn to call addAccount() when forcePrompt is true even if accounts is not empty; update the method on GCEventStore (symbol: signIn) and any protocol/interface declarations, or adjust the implementation to check the forcePrompt boolean and always call addAccount() when true (symbol: addAccount, accounts) so the parameter is no longer unused.MeetingBar/UI/Views/Preferences/CalendarsTab.swift (1)
160-164: Consider disabling cancel while authentication is in progress.The cancel button remains active while
isAddingis true, allowing users to dismiss the sheet while the OAuth flow is pending in the browser. While the account will still be added successfully, users may be confused seeing no feedback after completing browser authentication.Suggested improvement
Button("preferences_calendars_cancel") { presentationMode.wrappedValue.dismiss() } .keyboardShortcut(.escape, modifiers: []) +.disabled(isAdding)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MeetingBar/UI/Views/Preferences/CalendarsTab.swift` around lines 160 - 164, The Cancel button in CalendarsTab.swift should be disabled while the OAuth flow is in progress to prevent dismissing the sheet mid-auth; update the Button (the one calling presentationMode.wrappedValue.dismiss()) to honor the isAdding state (e.g., add a .disabled(isAdding) or .disabled(viewModel.isAdding) modifier) so it becomes inactive during authentication and optionally also prevent the Escape keyboard shortcut from firing while isAdding is true.
🤖 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/UI/Views/Preferences/CalendarsTab.swift`:
- Around line 35-43: When Defaults[.googleAccounts] is non-empty but the
calendars list is empty, change the UI branch to distinguish between "no
calendars" and load/auth failures: detect per-account fetch/auth state (e.g. an
account.calendarFetchError or account.authRestored flag provided by your
calendar loading logic) and, for accounts with auth or network failures, show an
account-specific message like "Couldn't load calendars for {account.email} —
re-authenticate or remove account" plus Buttons that call existing or new
handlers such as reauthenticateAccount(account) and
removeGoogleAccount(account); only fall back to the generic "no calendars
available" message when the account is healthy and truly has no calendars.
---
Nitpick comments:
In `@MeetingBar/Core/EventStores/GCEventStore.swift`:
- Around line 160-163: The signIn(forcePrompt _: Bool) method currently ignores
its forcePrompt parameter; either remove the unused parameter from the signIn
signature in GCEventStore (and the protocol it implements) or honor it by
changing the logic in signIn to call addAccount() when forcePrompt is true even
if accounts is not empty; update the method on GCEventStore (symbol: signIn) and
any protocol/interface declarations, or adjust the implementation to check the
forcePrompt boolean and always call addAccount() when true (symbol: addAccount,
accounts) so the parameter is no longer unused.
In `@MeetingBar/UI/Views/Preferences/CalendarsTab.swift`:
- Around line 160-164: The Cancel button in CalendarsTab.swift should be
disabled while the OAuth flow is in progress to prevent dismissing the sheet
mid-auth; update the Button (the one calling
presentationMode.wrappedValue.dismiss()) to honor the isAdding state (e.g., add
a .disabled(isAdding) or .disabled(viewModel.isAdding) modifier) so it becomes
inactive during authentication and optionally also prevent the Escape keyboard
shortcut from firing while isAdding is true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0a06c60d-0096-4e8b-9bee-e34c71b08adc
📒 Files selected for processing (3)
MeetingBar/Core/EventStores/GCEventStore.swiftMeetingBar/Resources /Localization /en.lproj/Localizable.stringsMeetingBar/UI/Views/Preferences/CalendarsTab.swift
✅ Files skipped from review due to trivial changes (1)
- MeetingBar/Resources /Localization /en.lproj/Localizable.strings
Comment on lines
+35
to
43
| } else if Defaults[.googleAccounts].isEmpty { | ||
| Text("preferences_calendars_no_accounts_connected") | ||
| .foregroundColor(.secondary) | ||
| .padding() | ||
| } else { | ||
| Text("preferences_calendars_no_calendars_available") | ||
| .foregroundColor(.secondary) | ||
| .padding() | ||
| } |
There was a problem hiding this comment.
"No calendars available" may be misleading for failed auth states.
When Defaults[.googleAccounts] is non-empty but calendars is empty, this could mean:
- The account genuinely has no calendars
- Auth state restoration failed (phantom account)
- Network error during fetch
The message "no calendars available" doesn't guide users to re-authenticate if their session is invalid. Consider showing account-specific status or prompting to remove and re-add accounts that fail to load.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MeetingBar/UI/Views/Preferences/CalendarsTab.swift` around lines 35 - 43,
When Defaults[.googleAccounts] is non-empty but the calendars list is empty,
change the UI branch to distinguish between "no calendars" and load/auth
failures: detect per-account fetch/auth state (e.g. an
account.calendarFetchError or account.authRestored flag provided by your
calendar loading logic) and, for accounts with auth or network failures, show an
account-specific message like "Couldn't load calendars for {account.email} —
re-authenticate or remove account" plus Buttons that call existing or new
handlers such as reauthenticateAccount(account) and
removeGoogleAccount(account); only fall back to the generic "no calendars
available" message when the account is healthy and truly has no calendars.
…parsing - Add GCParserTests (9 tests): minimal event, full event, all-day, cancelled, unknown status, attendees, conference data, multiple entry points, all attendee statuses - Add calendar ID prefix check and safe split-on-first-colon tests - Add selectedCalendarIDs cleanup verification on account removal - Add test for Defaults[.googleAccounts] empty default
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MeetingBarTests/GoogleAccountTests.swift`:
- Around line 100-114: The test test_selectedCalendarIDsCleanedOnAccountRemoval
currently mutates Defaults[.selectedCalendarIDs] directly to emulate cleanup;
instead invoke the real account-removal flow so the production cleanup logic is
exercised. Replace the inline filter with a call to the app's removal API/path
(e.g., the method that actually removes GoogleAccount instances such as the
AccountManager/AccountsController remove method) passing the GoogleAccount(id:
accountId, ...), then assert Defaults[.selectedCalendarIDs] no longer contains
entries with prefix "\(accountId):"; keep references to GoogleAccount and
Defaults[.selectedCalendarIDs] in the test to locate where to change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f38847ac-10c8-462a-8065-aff338e0e9f1
📒 Files selected for processing (1)
MeetingBarTests/GoogleAccountTests.swift
- Replace as! with guard let for event id, start, and end fields - Safely parse dates with ISO8601 and yyyyMMdd formatters, return nil on failure - Add NSLog diagnostics for malformed events - Simplify redundant ternary for recurrent field - Replace empty Text spacer with HStack spacing in CalendarRow - Add DateFormatter.yyyyMMdd static extension with POSIX locale
- Replace hardcoded notification strings with localization keys - Log errors when token revocation fails instead of silently ignoring - Add notification localization keys to English strings file
- Remove local state before revoking tokens (atomic removal) - Upsert existing account by email instead of creating duplicates - Prune accounts with missing/unreadable auth states after restore - Use ObjectIdentifier for delegate state matching (already done) - Use firstIndex for calendar ID splitting (already done) - Test uses real GCEventStore.removeAccount API instead of duplicating logic - Add refreshed notification variant for re-authenticated accounts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MeetingBar/Core/EventStores/GCEventStore.swift (1)
212-221:⚠️ Potential issue | 🟠 MajorProperly encode the calendar ID in the path component.
Google Calendar IDs are often email addresses (e.g.,
user@example.com), which contain the@character that requires URL encoding. Direct interpolation into the URL string bypasses this encoding, breaking the request for calendars with such IDs. UseappendPathComponent()to ensure proper percent-encoding.Suggested change
- var comps = URLComponents(string: "https://www.googleapis.com/calendar/v3/calendars/\(originalCalendarId)/events")! + var url = URL(string: "https://www.googleapis.com/calendar/v3/calendars")! + url.appendPathComponent(originalCalendarId) + url.appendPathComponent("events") + var comps = URLComponents(url: url, resolvingAgainstBaseURL: false)!🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MeetingBar/Core/EventStores/GCEventStore.swift` around lines 212 - 221, The calendar ID is interpolated into the URL string which breaks for IDs containing characters like '@'; instead build the URL by appending the calendar id as a path component so it gets percent-encoded: create a base URL for "https://www.googleapis.com/calendar/v3/calendars", call appendingPathComponent(originalCalendarId) to get the calendar-specific URL, then initialize URLComponents(url:calendarURL, resolvingAgainstBaseURL:false), set the queryItems, and pass components.url to fetchJSON(accountId:). Update the code around URLComponents/string creation (the variables comps, originalCalendarId, and the fetchJSON call) accordingly.
♻️ Duplicate comments (5)
MeetingBar/Core/EventStores/GCEventStore.swift (4)
330-341:⚠️ Potential issue | 🟠 MajorDrop accounts whose auth state cannot be restored.
This loop silently skips missing or unreadable Keychain entries, but
accountsstill makes those mailboxes look connected andsignIn()returns early because the array is non-empty.Suggested change
private func restoreAllAuthStates() { + authStates.removeAll() + var restoredAccounts: [GoogleAccount] = [] for account in accounts { let key = accountKeychainKey(accountId: account.id) - guard let data = Keychain.load(for: key) else { continue } + guard let data = Keychain.load(for: key) else { + Keychain.delete(for: key) + continue + } do { - guard let state = try NSKeyedUnarchiver.unarchivedObject(ofClass: OIDAuthState.self, from: data) else { continue } + guard let state = try NSKeyedUnarchiver.unarchivedObject(ofClass: OIDAuthState.self, from: data) else { + Keychain.delete(for: key) + continue + } state.stateChangeDelegate = self state.errorDelegate = self authStates[account.id] = state + restoredAccounts.append(account) } catch { NSLog("Error unarchiving OIDAuthState: \(error)") + Keychain.delete(for: key) } } + accounts = restoredAccounts }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MeetingBar/Core/EventStores/GCEventStore.swift` around lines 330 - 341, In restoreAllAuthStates, if Keychain.load(for: accountKeychainKey(accountId:)) returns nil or unarchiving OIDAuthState fails, remove that account from the in-memory accounts collection (e.g. accounts.removeAll { $0.id == account.id }) so it no longer appears connected and so signIn() won't short-circuit; also ensure you do not insert a nil authStates entry and keep the existing NSLog for errors (and set delegates only on successfully restored OIDAuthState instances).
126-133:⚠️ Potential issue | 🟠 MajorUpsert by email instead of appending a fresh account every time.
Re-authing the same mailbox still creates a new
accountId, which duplicates calendars/events and changes the{accountId}:calendarIdprefix that saved selections are matched against.Suggested change
- let account = GoogleAccount(id: accountId, email: email) - self.authStates[accountId] = state + let account = self.accounts.first(where: { + $0.email.caseInsensitiveCompare(email) == .orderedSame + }) ?? GoogleAccount(id: accountId, email: email) + self.authStates[account.id] = state state.stateChangeDelegate = self state.errorDelegate = self self.persistAuthState(for: account) - self.accounts.append(account) + if !self.accounts.contains(where: { $0.id == account.id }) { + self.accounts.append(account) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MeetingBar/Core/EventStores/GCEventStore.swift` around lines 126 - 133, When handling a new auth state in the block that creates GoogleAccount(id: accountId, email: email), do an upsert by email instead of blindly appending: look for an existing GoogleAccount in self.accounts whose email matches state.userEmail; if found reuse that account's id (replace accountId with the existing account.id), update/replace the stored auth state in self.authStates for that id, set delegates and persistAuthState(for: existingAccount), and do not append a new GoogleAccount or change the id/prefix; only append a new GoogleAccount when no account with that email exists, then persist and notify as currently done.
93-124:⚠️ Potential issue | 🟠 MajorSerialize
addAccount()before the discovery request suspends.
currentAuthorizationFlowandpendingAuthAccountIdare still singleton-wide, but neither is claimed until after configuration discovery returns. A second caller can start another OAuth flow in that window and overwrite the first one. The add-account sheet is still dismissible while browser auth is pending, so this is reachable today.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MeetingBar/Core/EventStores/GCEventStore.swift` around lines 93 - 124, The addAccount() method can race because pendingAuthAccountId and currentAuthorizationFlow are only set after the asynchronous OIDAuthorizationService.discoverConfiguration call returns; set/claim pendingAuthAccountId (and if possible reserve currentAuthorizationFlow) before starting discovery so a second caller can't overwrite them while awaiting discovery, then proceed with discovery and clear the claim on error or when the flow completes; touch the symbols addAccount(), pendingAuthAccountId, currentAuthorizationFlow and the discovery call OIDAuthorizationService.discoverConfiguration to implement this reservation logic (or use a serializing lock/actor around addAccount()) and ensure claims are cleared in all failure/completion paths.
142-157:⚠️ Potential issue | 🟠 MajorClear local state first, then revoke tokens best-effort.
This still waits on network revocation before removing the account from
Defaults, Keychain, andselectedCalendarIDs. When revoke is slow or offline, the app keeps showing a connected account that should already be gone, andsignOut()pays that latency for every account.Suggested change
func removeAccount(_ account: GoogleAccount) async { - if let state = authStates[account.id] { - let access = state.lastTokenResponse?.accessToken - let refresh = state.lastTokenResponse?.refreshToken - await withTaskGroup(of: Void.self) { grp in - if let acc = access { grp.addTask { try? await self.revoke(token: acc) } } - if let ref = refresh { grp.addTask { try? await self.revoke(token: ref) } } - } - authStates.removeValue(forKey: account.id) - } - + let state = authStates.removeValue(forKey: account.id) accounts.removeAll { $0.id == account.id } Keychain.delete(for: accountKeychainKey(accountId: account.id)) let prefix = "\(account.id):" Defaults[.selectedCalendarIDs] = Defaults[.selectedCalendarIDs].filter { !$0.hasPrefix(prefix) } + + if let access = state?.lastTokenResponse?.accessToken { + Task { try? await self.revoke(token: access) } + } + if let refresh = state?.lastTokenResponse?.refreshToken { + Task { try? await self.revoke(token: refresh) } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MeetingBar/Core/EventStores/GCEventStore.swift` around lines 142 - 157, The removeAccount(_:) implementation currently revokes tokens before clearing local state, causing UI and signOut latency; change it to first remove local state (authStates, accounts, Keychain.delete, Defaults[.selectedCalendarIDs]) immediately, then perform token revocation as a best-effort background task (fire-and-forget) so revoke(token:) is awaited only if needed but does not block removal; specifically, in removeAccount(_:) clear authStates[account.id], remove the account from accounts, delete the keychain entry (accountKeychainKey), and filter Defaults[.selectedCalendarIDs] first, then dispatch revoke(token:) calls for lastTokenResponse?.accessToken and refreshToken via an unawaited Task or a Task.detached/Task { } so failures are ignored (try? await) and do not delay UI.MeetingBar/UI/Views/Preferences/CalendarsTab.swift (1)
160-186:⚠️ Potential issue | 🟡 MinorDon’t let the sheet look cancelable while OAuth is still running.
isAddingdisables only the sign-in button. Cancel and Escape can still dismiss the sheet whileGCEventStore.shared.addAccount()continues in the background, so the flow appears canceled and can be reopened into another auth attempt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MeetingBar/UI/Views/Preferences/CalendarsTab.swift` around lines 160 - 186, The cancel button and ESC shortcut must be disabled while the async OAuth is running so the sheet cannot be dismissed mid-flow; update the Button labeled "preferences_calendars_cancel" and its .keyboardShortcut to be inactive when isAdding is true (or guard the presentationMode.wrappedValue.dismiss() so it only runs when !isAdding) and also apply interactiveDismissDisabled(isAdding) on the sheet hosting CalendarsTab so system/drag dismiss is prevented while GCEventStore.shared.addAccount() is in progress; ensure presentationMode.wrappedValue.dismiss() is only called after await onAccountAdded() completes.
🧹 Nitpick comments (1)
MeetingBar/Utilities/Helpers.swift (1)
272-279: Well-implemented static formatter with thread-safe initialization and POSIX locale.The static lazy-initialized
DateFormatterfollows best practices: Swift'sstatic letguarantees thread-safe initialization, and the POSIX locale ensures consistent parsing regardless of user locale settings.Optional follow-up: Consider refactoring
openInFantastical(lines 182-183) to use this static formatter instead of creating an inline instance—it would eliminate duplication and add the missing POSIX locale to that function.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MeetingBar/Utilities/Helpers.swift` around lines 272 - 279, The openInFantastical function currently creates an inline DateFormatter without the POSIX locale; replace that inline instance with the shared static formatter DateFormatter.yyyyMMdd to remove duplication and ensure POSIX locale usage—locate the openInFantastical method and swap its local DateFormatter creation/usage to reference DateFormatter.yyyyMMdd wherever the date is formatted for the Fantastical URL.
🤖 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/UI/Views/Preferences/CalendarsTab.swift`:
- Around line 114-129: The confirmation dialog is receiving the selected
GoogleAccount (accountToRemove) but the title/message do not display which
account will be removed; update the .confirmationDialog block to interpolate the
passed-in account (the closure parameter named account) into the localized title
and/or message so the destructive prompt identifies the specific GoogleAccount
(e.g., include account.name or account.email in the displayed Text). Keep the
same destructive Button that calls GCEventStore.shared.removeAccount(account)
and eventManager.refreshSources(), only change the Text(...) for the
title/message inside the presenting closure to include the account identifier.
---
Outside diff comments:
In `@MeetingBar/Core/EventStores/GCEventStore.swift`:
- Around line 212-221: The calendar ID is interpolated into the URL string which
breaks for IDs containing characters like '@'; instead build the URL by
appending the calendar id as a path component so it gets percent-encoded: create
a base URL for "https://www.googleapis.com/calendar/v3/calendars", call
appendingPathComponent(originalCalendarId) to get the calendar-specific URL,
then initialize URLComponents(url:calendarURL, resolvingAgainstBaseURL:false),
set the queryItems, and pass components.url to fetchJSON(accountId:). Update the
code around URLComponents/string creation (the variables comps,
originalCalendarId, and the fetchJSON call) accordingly.
---
Duplicate comments:
In `@MeetingBar/Core/EventStores/GCEventStore.swift`:
- Around line 330-341: In restoreAllAuthStates, if Keychain.load(for:
accountKeychainKey(accountId:)) returns nil or unarchiving OIDAuthState fails,
remove that account from the in-memory accounts collection (e.g.
accounts.removeAll { $0.id == account.id }) so it no longer appears connected
and so signIn() won't short-circuit; also ensure you do not insert a nil
authStates entry and keep the existing NSLog for errors (and set delegates only
on successfully restored OIDAuthState instances).
- Around line 126-133: When handling a new auth state in the block that creates
GoogleAccount(id: accountId, email: email), do an upsert by email instead of
blindly appending: look for an existing GoogleAccount in self.accounts whose
email matches state.userEmail; if found reuse that account's id (replace
accountId with the existing account.id), update/replace the stored auth state in
self.authStates for that id, set delegates and persistAuthState(for:
existingAccount), and do not append a new GoogleAccount or change the id/prefix;
only append a new GoogleAccount when no account with that email exists, then
persist and notify as currently done.
- Around line 93-124: The addAccount() method can race because
pendingAuthAccountId and currentAuthorizationFlow are only set after the
asynchronous OIDAuthorizationService.discoverConfiguration call returns;
set/claim pendingAuthAccountId (and if possible reserve
currentAuthorizationFlow) before starting discovery so a second caller can't
overwrite them while awaiting discovery, then proceed with discovery and clear
the claim on error or when the flow completes; touch the symbols addAccount(),
pendingAuthAccountId, currentAuthorizationFlow and the discovery call
OIDAuthorizationService.discoverConfiguration to implement this reservation
logic (or use a serializing lock/actor around addAccount()) and ensure claims
are cleared in all failure/completion paths.
- Around line 142-157: The removeAccount(_:) implementation currently revokes
tokens before clearing local state, causing UI and signOut latency; change it to
first remove local state (authStates, accounts, Keychain.delete,
Defaults[.selectedCalendarIDs]) immediately, then perform token revocation as a
best-effort background task (fire-and-forget) so revoke(token:) is awaited only
if needed but does not block removal; specifically, in removeAccount(_:) clear
authStates[account.id], remove the account from accounts, delete the keychain
entry (accountKeychainKey), and filter Defaults[.selectedCalendarIDs] first,
then dispatch revoke(token:) calls for lastTokenResponse?.accessToken and
refreshToken via an unawaited Task or a Task.detached/Task { } so failures are
ignored (try? await) and do not delay UI.
In `@MeetingBar/UI/Views/Preferences/CalendarsTab.swift`:
- Around line 160-186: The cancel button and ESC shortcut must be disabled while
the async OAuth is running so the sheet cannot be dismissed mid-flow; update the
Button labeled "preferences_calendars_cancel" and its .keyboardShortcut to be
inactive when isAdding is true (or guard the
presentationMode.wrappedValue.dismiss() so it only runs when !isAdding) and also
apply interactiveDismissDisabled(isAdding) on the sheet hosting CalendarsTab so
system/drag dismiss is prevented while GCEventStore.shared.addAccount() is in
progress; ensure presentationMode.wrappedValue.dismiss() is only called after
await onAccountAdded() completes.
---
Nitpick comments:
In `@MeetingBar/Utilities/Helpers.swift`:
- Around line 272-279: The openInFantastical function currently creates an
inline DateFormatter without the POSIX locale; replace that inline instance with
the shared static formatter DateFormatter.yyyyMMdd to remove duplication and
ensure POSIX locale usage—locate the openInFantastical method and swap its local
DateFormatter creation/usage to reference DateFormatter.yyyyMMdd wherever the
date is formatted for the Fantastical URL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c9db9d43-4a3c-4f0f-ae2c-a7f5b065b7eb
📒 Files selected for processing (3)
MeetingBar/Core/EventStores/GCEventStore.swiftMeetingBar/UI/Views/Preferences/CalendarsTab.swiftMeetingBar/Utilities/Helpers.swift
vlordier
commented
Apr 3, 2026
Author
vlordier
left a comment
vlordier
left a comment
There was a problem hiding this comment.
All Copilot review comments reference the initial commit and have been addressed in subsequent commits:
- Hardcoded UI strings → ✅ All user-facing strings now use
.loco()localization keys - Empty state misleading → ✅ Distinguishes no accounts vs no calendars via
Defaults[.googleAccounts].isEmpty - removeAccount early return → ✅ Always cleans up Defaults/Keychain/selectedCalendarIDs regardless of auth state
- Legacy migration → ✅
migrateLegacyAuthStateIfNeeded()runs in init - signIn forcePrompt → ✅ Parameter kept for protocol conformance
- Calendar ID parsing → ✅ Uses
firstIndex(of: ":")for safe single split - didChange token matching → ✅ Uses
ObjectIdentifierfor identity comparison - Duplicate accounts → ✅ Upserts by email instead of creating duplicates
- Atomic removal → ✅ Clears local state first, then revokes tokens
- Prune missing auth states → ✅
pruneAccountsMissingAuthState()runs after restore
All changes are verified by 20 passing tests.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
- Button text now reads 'Remove user@example.com' - Message text now includes the specific account email
- Replace force-unwraps with descriptive fatalError messages - Tells users exactly which env var is missing and how to configure it
- Replace force unwrap on redirectURL with guard that validates URL creation - Show descriptive error message in UI when credentials are missing - Mock mode bypasses all OAuth requirements entirely
dosubot
Bot
added
size:XXL
- Extract status, URL, organizer, attendee, and date parsing into private top-level functions - Replace 3-tuple return with named ParsedDates struct - GCParser.event complexity reduced from 17 to 5 (threshold: 15)
- Detect unresolved $(VARIABLE) placeholders in Info.plist - Fall back to mock mode automatically when credentials are missing - No manual env var setup needed for local testing
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
Changes
GoogleAccountfor tracking multiple accountsaccountId:calendarIdprevents collisions across accountsUX Improvements
Testing
Summary by CodeRabbit
New Features
Localization