Repository navigation
Expose tokenless Gmail connections for safe removal - #211
Conversation
|
@codex review |
📝 WalkthroughSummary
WalkthroughGmail connection loading now preserves tokenless backend connections, marks them as requiring authorization, supports safe removal, and blocks push synchronization without local tokens. Tests and fixtures adopt explicit authorization states and cover these flows. ChangesGmail tokenless connection handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant GmailPushWakeupHandler
participant GmailMailboxConnectionAdapter
participant GmailProviderConnectionService
participant tokenStore
GmailPushWakeupHandler->>GmailMailboxConnectionAdapter: Check local authorization
GmailMailboxConnectionAdapter->>GmailProviderConnectionService: hasLocalAuthorization
GmailProviderConnectionService->>tokenStore: Load Gmail tokens
tokenStore-->>GmailProviderConnectionService: Tokens or nil
GmailProviderConnectionService-->>GmailMailboxConnectionAdapter: Authorization result
GmailMailboxConnectionAdapter-->>GmailPushWakeupHandler: Allow or reject wakeup
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
apps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swift (1)
1379-1389: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winA single connection's Keychain read failure hides the entire connection list.
hasLocalAuthorizationis invoked per status insidetry localStatuses.map { ... }. If any one connection's token lookup throws (e.g., a transient Keychain read failure), the wholeloadConnections()call throws and no connections are returned — even ones that would otherwise resolve fine. This directly undercuts the PR's goal of keeping connections visible instead of hiding them; a per-connection auth-check failure should degrade to "authorization required" for that one connection rather than failing the whole list.♻️ Suggested resilience fix
let localConnections = try localStatuses.map { status in status.mailboxConnection( productAccountId: session.productAccountId, - authorizationState: try connectionService.hasLocalAuthorization(status, session: session) + authorizationState: (try? connectionService.hasLocalAuthorization(status, session: session)) == true ? .authorized : .required ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swift` around lines 1379 - 1389, Update loadConnections so a failure from hasLocalAuthorization for one status is treated as .required for that connection instead of propagating through the map and aborting the entire list. Preserve successful authorization results and continue returning all connections that can be mapped.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swift`:
- Around line 1379-1389: Update loadConnections so a failure from
hasLocalAuthorization for one status is treated as .required for that connection
instead of propagating through the map and aborting the entire list. Preserve
successful authorization results and continue returning all connections that can
be mapped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 89dbd702-6228-4c5f-b94e-c8abed32f32c
📒 Files selected for processing (12)
.changeset/expose-tokenless-gmail-removal.mdREADME.mdapps/unwired-mail/unwired-mail/Services/Gmail/GmailPushRelayService.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/GmailProviderConnectionService.swiftapps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailProviderConnectionServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailPushRelayServiceTests.swiftapps/unwired-mail/unwired-mailTests/MailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mailTests/MailboxConnectionSyncServiceTests.swiftapps/unwired-mail/unwired-mailTests/PendingProviderActionServiceTests.swiftapps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swift
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
.changeset/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Create a Changeset for runtime behavior changes or exported type/API changes; tests-only, internal refactors, docs-only changes, and JSDoc-only maintenance may skip it.
Files:
.changeset/expose-tokenless-gmail-removal.md
apps/unwired-mail/**/*.{swift,m,mm}
📄 CodeRabbit inference engine (AGENTS.md)
apps/unwired-mail/**/*.{swift,m,mm}: The Apple app must provide formatter, linter, and test commands; usezsh scripts/check-apple-lint.zshfor formatting and linting.
Run Apple tests withxcodebuild test -project apps/unwired-mail/unwired-mail.xcodeproj -scheme unwired-mail -destination 'platform=iOS Simulator,name=iPhone 17'.
Every Apple code change must receive formatter, linter, and test validation unless Apple tooling or a platform dependency is unavailable.
Files:
apps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swiftapps/unwired-mail/unwired-mailTests/MailboxConnectionSyncServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailProviderConnectionServiceTests.swiftapps/unwired-mail/unwired-mailTests/PendingProviderActionServiceTests.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/GmailProviderConnectionService.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailPushRelayService.swiftapps/unwired-mail/unwired-mailTests/GmailPushRelayServiceTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swiftapps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swiftapps/unwired-mail/unwired-mailTests/MailboxConnectionAdapterTests.swift
apps/unwired-mail/**/*.swift
📄 CodeRabbit inference engine (AGENTS.md)
SwiftLint runs in strict mode, so warnings fail validation.
Files:
apps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swiftapps/unwired-mail/unwired-mailTests/MailboxConnectionSyncServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailProviderConnectionServiceTests.swiftapps/unwired-mail/unwired-mailTests/PendingProviderActionServiceTests.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/GmailProviderConnectionService.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailPushRelayService.swiftapps/unwired-mail/unwired-mailTests/GmailPushRelayServiceTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swiftapps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swiftapps/unwired-mail/unwired-mailTests/MailboxConnectionAdapterTests.swift
⚙️ CodeRabbit configuration file
apps/unwired-mail/**/*.swift: Prioritize functional correctness, data protection, and Apple platform
behavior over style already enforced by swift-format and SwiftLint.
Check actor isolation, cancellation, lifecycle transitions, persistence,
retry and offline behavior, and Keychain accessibility. Flag credentials,
tokens, mail content, or decrypted Product Sync data that can be logged,
persisted outside approved device-local storage, or cross a documented
privacy boundary. Require focused tests for changed behavior and failure
paths.
Files:
apps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swiftapps/unwired-mail/unwired-mailTests/MailboxConnectionSyncServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailProviderConnectionServiceTests.swiftapps/unwired-mail/unwired-mailTests/PendingProviderActionServiceTests.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/GmailProviderConnectionService.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailPushRelayService.swiftapps/unwired-mail/unwired-mailTests/GmailPushRelayServiceTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swiftapps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swiftapps/unwired-mail/unwired-mailTests/MailboxConnectionAdapterTests.swift
**/*.{swift,m,mm}
📄 CodeRabbit inference engine (AGENTS.md)
Run the smallest meaningful checks for the touched Apple area first, then broaden validation for shared tooling or cross-project behavior.
Files:
apps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swiftapps/unwired-mail/unwired-mailTests/MailboxConnectionSyncServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailProviderConnectionServiceTests.swiftapps/unwired-mail/unwired-mailTests/PendingProviderActionServiceTests.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/GmailProviderConnectionService.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailPushRelayService.swiftapps/unwired-mail/unwired-mailTests/GmailPushRelayServiceTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swiftapps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swiftapps/unwired-mail/unwired-mailTests/MailboxConnectionAdapterTests.swift
**/*
📄 CodeRabbit inference engine (AGENTS.md)
Touch only files required by the request; do not refactor, reformat, or improve adjacent code, and remove only unused items created by your changes.
Files:
apps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swiftapps/unwired-mail/unwired-mailTests/MailboxConnectionSyncServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailProviderConnectionServiceTests.swiftREADME.mdapps/unwired-mail/unwired-mailTests/PendingProviderActionServiceTests.swiftapps/unwired-mail/unwired-mail/Services/ProductAccount/GmailProviderConnectionService.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailPushRelayService.swiftapps/unwired-mail/unwired-mailTests/GmailPushRelayServiceTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swiftapps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swiftapps/unwired-mail/unwired-mailTests/MailboxConnectionAdapterTests.swift
**/*Tests/**/*.swift
⚙️ CodeRabbit configuration file
**/*Tests/**/*.swift: Verify externally observable behavior, privacy invariants, error paths,
cancellation or restart behavior where relevant, and deterministic
isolation. Flag fragile sleeps, ordering dependencies, and tests that
duplicate the implementation instead of exercising it.
Files:
apps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swiftapps/unwired-mail/unwired-mailTests/MailboxConnectionSyncServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailProviderConnectionServiceTests.swiftapps/unwired-mail/unwired-mailTests/PendingProviderActionServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailPushRelayServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swiftapps/unwired-mail/unwired-mailTests/MailboxConnectionAdapterTests.swift
README.md
📄 CodeRabbit inference engine (AGENTS.md)
Document developer setup, common commands, and high-level project navigation in
README.md.
Files:
README.md
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Treat docs as product truth, not marketing. Prefer precise domain language
from CONTEXT.md; flag undefined terms, synonym drift, and conflicts with
existing ADRs under docs/adr/. For ADR changes, require a clear decision,
consequences, and privacy or security boundary impact; flag silent
reversals of prior ADRs without an explicit supersession note. For
docs/agents/ and bootstrap docs, verify agent workflow, validation, and
triage guidance stay aligned with AGENTS.md. Flag broken cross-links,
speculative future claims presented as shipped behavior, and docs that
omit the Apple vs TypeScript ownership of a boundary they describe.
Files:
README.md
🧠 Learnings (1)
📚 Learning: 2026-07-28T09:17:20.141Z
Learnt from: rajzik
Repo: unwired-dev/product PR: 210
File: apps/unwired-mail/unwired-mailTests/GmailProviderConnectionServiceTests.swift:930-982
Timestamp: 2026-07-28T09:17:20.141Z
Learning: In unwired-mail’s Gmail connection service tests (e.g., GmailProviderConnectionServiceTests.swift), treat `ConvexClient.removeGmailConnection` as an idempotent “remove” operation: the underlying `pushRelay:removeGmailConnection` mutation returns success even when the operational Gmail connection is already absent (e.g., it should not produce a duplicate-removal error path). The client intentionally exposes only `hasRemainingGmailConnections`, so do not add/require tests that expect an error/throw for duplicate-removal; instead assert behavior via `hasRemainingGmailConnections`/success results.
Applied to files:
apps/unwired-mail/unwired-mailTests/GmailProviderConnectionServiceTests.swift
🧬 Code graph analysis (4)
apps/unwired-mail/unwired-mailTests/MailboxConnectionSyncServiceTests.swift (1)
apps/unwired-mail/unwired-mail/Services/ProductSync/MailboxConnectionDefinition.swift (1)
mailboxConnection(108-123)
apps/unwired-mail/unwired-mailTests/GmailProviderConnectionServiceTests.swift (1)
apps/unwired-mail/unwired-mail/Services/Mailbox/GenericMailSetupService.swift (1)
hasLocalAuthorization(264-272)
apps/unwired-mail/unwired-mailTests/PendingProviderActionServiceTests.swift (1)
apps/unwired-mail/unwired-mail/Services/ProductSync/MailboxConnectionDefinition.swift (1)
mailboxConnection(108-123)
apps/unwired-mail/unwired-mailTests/MailboxConnectionAdapterTests.swift (2)
apps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swift (1)
removeMailboxConnectionEverywhere(1519-1528)apps/unwired-mail/unwired-mail/Services/ProductSync/MailboxConnectionDefinition.swift (1)
mailboxConnection(108-123)
🪛 markdownlint-cli2 (0.23.0)
.changeset/expose-tokenless-gmail-removal.md
[warning] 4-4: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (22)
apps/unwired-mail/unwired-mail/Services/ProductAccount/GmailProviderConnectionService.swift (2)
178-182:hasLocalAuthorizationcorrectly derives authorization from token presence.The new protocol method and its implementation directly check
tokenStore.load(...) != nil, matching the "tokenless" contract described in CONTEXT.md.Also applies to: 704-712
701-702: Filter correctly keeps tokenless statuses visible.Removing the token-presence check from the final
filter(now only filtering bytrustedDeviceId) is exactly what's needed to keep tokenless backend connections visible for removal, per Issue#88.apps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swift (4)
346-374: Factory correctly gates capabilities on authorization state and provider.
mailboxConnection(productAccountId:authorizationState:)only grants.gmailcapabilities whenauthorizationState == .authorized, matching "Unauthorized ... Mailbox Connections remain visible but cannot be selected for sending" from CONTEXT.md.
1343-1366: Reconnect cleanup semantics correctly account for tokenless reconnects.
hadExistingConnectionnow requires both a matching connection id andhasLocalAuthorization, so re-authorizing a previously tokenless connection is treated like a fresh connect for save-failure cleanup purposes — consistent with the PR's reconnection/cleanup-retry behavior.
1415-1415: Cleanup lookup and forced.requiredstate for removed connections look correct.
localStatusForCleanupnow sources fromlocalStatusesByIdbuilt over all local statuses (not just authorized ones), andremovedMailboxConnectionforces.requiredwhenever a local status exists — appropriate since a synchronized-removed connection should never expose authorized capabilities regardless of local token state.Also applies to: 1449-1485
2272-2280: LGTM!apps/unwired-mail/unwired-mail/Services/Gmail/GmailPushRelayService.swift (3)
1072-1078: Authorization-checker wiring is sound.Constraining
syncServicetoMailboxMetadataSyncing & GmailConnectionAuthorizationCheckingand derivingauthorizationCheckerfrom it avoids introducing a separate dependency while keeping the gate testable via the existingsyncServiceinjection point.Also applies to: 1088-1128
1185-1192: Push wakeup correctly closes before sync when local authorization is missing.Adding
try authorizationChecker.hasLocalAuthorization(connection, session: productSession)to the eligibility guard prevents provider mail fetch/push work for tokenless connections, matching CONTEXT.md's "Background wakeups/push should not fetch provider mail or use provider credentials when authorization is unavailable." This mirrors the existingtry sessionStore.load()pattern elsewhere in the same guard (real failures propagate; transient/expected-missing lookups usetry?), so the style is consistent with the rest of the function.
1269-1272: LGTM!apps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swift (1)
1019-1019: LGTM!These are mechanical fixture updates to pass the new explicit
authorizationState: .authorizedargument tomailboxConnection(productAccountId:authorizationState:); they don't change test intent or coverage.Also applies to: 1386-1418, 1493-1508, 1543-1546, 1596-1599, 1628-1631, 1689-1696, 1743-1746, 1780-1783, 1829-1830, 1866-1869, 1899-1902, 1935-1938, 1951-1954, 3594-3598, 3634-3638
apps/unwired-mail/unwired-mailTests/MailboxConnectionSyncServiceTests.swift (1)
238-238: LGTM!Also applies to: 248-248
apps/unwired-mail/unwired-mailTests/PendingProviderActionServiceTests.swift (1)
9-17: LGTM!Also applies to: 1276-1284, 1311-1320, 1442-1450
apps/unwired-mail/unwired-mailTests/ProductAccountSessionTests.swift (1)
895-900: LGTM!Stub
hasLocalAuthorizationimplementations correctly satisfy the expandedGmailProviderConnectingprotocol for these unrelated sign-out/session tests.Also applies to: 983-988
apps/unwired-mail/unwired-mailTests/GmailPushRelayServiceTests.swift (2)
630-635: LGTM!Mechanical fixture updates for the new factory signature.
Also applies to: 742-786, 790-833, 1159-1167, 1371-1403
714-741: New tokenless-rejection test and recording-service authorization flag are well targeted.
testGmailWakeupRejectsTokenlessConnectionBeforeMailboxSyncexercises the new guard directly, andRecordingPushGmailMetadataSyncService.hasLocalAuthorizationdefaulting totruekeeps all other existing tests passing without needing individual updates.Also applies to: 2999-3023
apps/unwired-mail/unwired-mailTests/MailboxConnectionAdapterTests.swift (2)
154-155: LGTM!Fixture updates are mechanical and consistent with the new factory signature; no behavior change in these segments.
Also applies to: 214-250, 273-309, 345-346, 392-394, 469-471, 533-536, 548-550, 665-668, 700-703, 746-749, 772-774, 1050-1052, 1094-1106, 1154-1156, 1190-1193, 1254-1257, 1285-1288, 1394-1396, 1878-1880, 2654-2657, 2715-2718, 2764-2767, 2781-2793, 3088-3089
418-432: New tokenless-connection tests andRecordingAdapterConnectionServiceauthorization simulation are appropriate coverage.
testGmailAdapterRestrictsTokenlessDeviceConnectionUntilReauthorizedandtestGmailAdapterRemovesTokenlessDeviceConnectionEverywherecorrectly validate that tokenless connections surface with.required/.nonecapabilities and can still be removed everywhere viarequiresAuthorization: false. The mock'shasLocalAuthorizationimplementation matches the real service's contract.Also applies to: 492-523, 3561-3567, 3622-3633
apps/unwired-mail/unwired-mailTests/GmailProviderConnectionServiceTests.swift (3)
27-28: LGTM!
623-634: New tokenless-visibility test directly validates the core PR change.
testLoadConnectionsKeepsTokenlessDeviceStatusVisibleForRemovalexercisesloadConnectionswith no tokens stored and confirms the status is still returned — this is exactly the regression guard needed for Issue#88.
966-970: Updated assertions correctly reflect the new visibility + authorization-check contract.Asserting
visibleStatuses == [transport.status](still visible) alongsidehasLocalAuthorization == false(post-token-removal) precisely captures the tokenless-but-visible behavior this PR introduces, and is consistent with the retrieved learning thatremoveGmailConnectionis idempotent — no duplicate-removal error is expected here.Source: Learnings
.changeset/expose-tokenless-gmail-removal.md (1)
1-4: Appropriate changeset for this runtime-behavior change; markdownlint heading warning is a false positive.Per path instructions, "Create a Changeset for runtime behavior changes or exported type/API changes" — this qualifies since tokenless connections are now exposed. The markdownlint MD041 hint (missing top-level heading) is expected for
.changeset/*.mdfiles, which intentionally start with the changesets tool's front-matter block rather than a heading.Sources: Path instructions, Linters/SAST tools
README.md (1)
64-64: Wording aligns with CONTEXT.md domain language.The description of tokenless connections remaining "visible as authorization-required with no mail or push capabilities" and the distinction between "Remove Device Authorization" and "Remove Mailbox Connection Everywhere" match the documented domain terms.
Source: Path instructions
1 participant
Closes #88
Summary
Validation
mise exec -- zsh scripts/check-apple-lint.zshmise exec -- xcodebuild test -quiet -project apps/unwired-mail/unwired-mail.xcodeproj -scheme unwired-mail -destination 'platform=iOS Simulator,name=iPhone 17'git diff --check origin/main...HEADReview
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.