Synchronize Mailbox Connection definitions across devices - #91

apps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swift (1)

805-978: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift

Every provider-access path now performs a fresh remote/cache round trip before the actual Gmail call.

loadInbox, syncInbox, syncRecentInbox, searchProvider, registerOrRenewPush, perform, and send all funnel through gmailConnectionForProviderAccess → ensureConnectionIsActive, which calls definitionSyncService.loadSnapshotForProviderAccess (a Product Sync network fetch, falling back to Keychain cache) on every single invocation. For high-frequency paths like loadMessageBody/message-list refreshes, this adds a serialized network round trip before each mail action, which can noticeably increase latency and load on the Product Sync backend.

Consider a short-lived in-memory TTL cache (or event-driven invalidation on known removal) for the "is this connection still active" check, rather than re-fetching before every call.

🤖 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 805 - 978, The gmailConnectionForProviderAccess path performs a
Product Sync/cache lookup on every provider call through
ensureConnectionIsActive, adding unnecessary latency. Add a short-lived
in-memory TTL cache for successful active-connection checks, keyed by connection
ID, and reuse it in gmailConnectionForProviderAccess while preserving validation
after expiry. Invalidate the cached status when a connection is removed or
otherwise known to become inactive.

apps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swift (1)

1007-1035: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift

Every provider-access call now performs a full remote sync fetch.

gmailConnectionForProviderAccess → ensureConnectionIsActive calls definitionSyncService.loadSnapshotForProviderAccess, which starts with a network round-trip (loadRemotePayload) plus decrypt, on every invocation. This helper is now invoked before loadInbox, syncInbox, syncRecentInbox, searchProvider, registerOrRenewPush, perform, send, and via ensureConnectionIsActive directly for overrideCategory and loadMessageBody — i.e. before essentially every message read/action. This adds a network+decrypt round-trip to the hot path for reading/sending mail, increasing latency and backend load proportional to mail usage, not to connection-management actions.

Consider caching the provider-access snapshot for a short TTL (or per-session) rather than fetching remotely on every call, while still refreshing periodically enough to honor the removal-fencing guarantee.

🤖 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 1007 - 1035, Update ensureConnectionIsActive and the
provider-access flow to reuse a cached snapshot for a short TTL or session scope
instead of calling definitionSyncService.loadSnapshotForProviderAccess on every
invocation. Preserve periodic refreshes so removed connections are still
detected and connectionRemoved fencing remains enforced, including for direct
ensureConnectionIsActive callers.

apps/unwired-mail/unwired-mail/Services/Mailbox/GenericMailSetupService.swift (1)

96-122: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

username isn't case-normalized when deriving connectionId, risking cross-device identity mismatches.

hostname is explicitly .lowercased() before hashing, but username is hashed as-is. If the same mailbox is set up on two devices with the username entered in different casing (e.g. copy/paste vs. manual typing), the derived MailboxConnectionId differs between devices, breaking the dedup/sync-convergence contract this PR relies on (connections created on one device should appear as the same connection on others). Most IMAP/POP3/SMTP providers treat mailbox usernames case-insensitively in practice, and validatedDefinition (line ~446) only trims whitespace, not case.

🐛 Proposed fix

     let identityInput = [
-      username,
+      username.lowercased(),
       incomingEndpoint.mailProtocol.rawValue,

🤖 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/GenericMailSetupService.swift`
around lines 96 - 122, Normalize username casing before hashing in the
connectionId property, matching the existing hostname normalization, so
equivalent mailbox configurations produce the same MailboxConnectionId across
devices. Update the username component of identityInput while preserving the
remaining identity fields and hashing flow.

apps/unwired-mail/unwired-mail/Features/Account/GenericMailSetupPanel.swift (1)

269-308: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift

Same double snapshot-fetch pattern as AccountView.refreshConnections().

service.loadSyncedDefinitions(session:) internally calls definitionSyncService.loadSnapshot, and the very next line calls service.loadDefaultSendingConnectionId(session:), which performs a second, independent loadSnapshot (network fetch + decrypt) for a value the first call's snapshot already contained. This doubles the network/decrypt cost of every loadSyncedDefinitions() call (initial load, scene-active, after every connect/remove).

🤖 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/Features/Account/GenericMailSetupPanel.swift`
around lines 269 - 308, Update GenericMailSetupViewModel.loadSyncedDefinitions
to load the synced snapshot once and derive both syncedDefinitions and
defaultSendingConnectionId from that result, rather than calling
service.loadDefaultSendingConnectionId separately after
service.loadSyncedDefinitions. Reuse the existing snapshot-backed service/API
symbols and preserve the current sorting, selection, authorization, and
error-handling behavior.

apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift (1)

883-891: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift

refreshConnections() performs two independent full snapshot network fetches.

service.loadConnections(session:) internally reconciles/fetches a MailboxConnectionSyncSnapshot (which already carries defaultSendingConnectionId), then service.loadDefaultSendingConnectionId(session:) performs a second, entirely separate loadSnapshot call (network fetch + decrypt) purely to read a field the first call already resolved internally. This doubles latency/backend load every time refreshConnections() runs — on initial load, after connect(), removeLocalAuthorization(), and removeEverywhere(). The same pattern recurs in GenericMailSetupPanel.loadSyncedDefinitions().

Consider exposing the already-computed snapshot (or a combined connections+default-id result) from loadConnections/loadSyncedDefinitions instead of a second independent fetch.

🤖 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/Features/Account/AccountView.swift` around
lines 883 - 891, The refreshConnections flow performs two independent snapshot
fetches; update the service API so loadConnections returns the already-resolved
connections together with defaultSendingConnectionId, then assign both values
from that single result and remove the separate loadDefaultSendingConnectionId
call. Apply the same combined-result or snapshot-reuse approach in
GenericMailSetupPanel.loadSyncedDefinitions.

apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift (1)

868-878: 🗄️ Data Integrity & Integration | 🟠 Major

Preserve the synchronized default after global removal.

If the removed connection is selected while another synchronized default remains, this unconditionally selects connections.first, silently replacing the default-aware selection. Reconcile with defaultSendingConnectionId: select its matching connection, select the first only when the default is nil, otherwise leave selection unset.

🤖 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/Features/Account/AccountView.swift` around
lines 868 - 878, Update removeEverywhere so that when the removed connection was
selected, it preserves the synchronized default selection: choose the connection
matching defaultSendingConnectionId when present, choose connections.first only
when the default ID is nil, and otherwise leave selectedConnectionId unset.

apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift (2)

898-905: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Reorder refreshConnections() to assign only after both reads succeed.

connections is assigned immediately after the first await, then defaultSendingConnectionId is fetched with a second await that can throw. If it throws, connections is left updated while defaultSendingConnectionId remains stale — the exact partial-update hazard this contract was previously fixed for (see past review on try? conversion). Compute both locally before assigning either property.

🐛 Proposed fix

 private func refreshConnections() async throws {
   let loadedConnections = try await service.loadConnections(session: session)
     .sorted {
       $0.displayName.localizedCaseInsensitiveCompare($1.displayName) == .orderedAscending
     }
-    connections = loadedConnections
-    defaultSendingConnectionId = try await service.loadDefaultSendingConnectionId(session: session)
+  let loadedDefaultSendingConnectionId = try await service.loadDefaultSendingConnectionId(
+    session: session
+  )
+  connections = loadedConnections
+  defaultSendingConnectionId = loadedDefaultSendingConnectionId
 }

🤖 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/Features/Account/AccountView.swift` around
lines 898 - 905, Update refreshConnections() to load and sort the connections
and fetch defaultSendingConnectionId into local values first; assign both
properties only after both awaited service calls succeed, preserving the
existing sort and error propagation.

---

852-864: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Reuse refreshConnections() in the error path instead of duplicating it.

This catch block re-implements a connections-only reload (sort + push-status filter) instead of calling try? await refreshConnections() as removeEverywhere does. It duplicates logic and never refreshes defaultSendingConnectionId, so a failed local-authorization removal can leave the default stale until the next full load().

♻️ Proposed fix

     } catch {
-      if let refreshedConnections = try? await service.loadConnections(session: session) {
-        connections = refreshedConnections.sorted {
-          $0.displayName.localizedCaseInsensitiveCompare($1.displayName) == .orderedAscending
-        }
-        pushStatusMessages = pushStatusMessages.filter { connectionId, _ in
-          connections.contains { $0.id == connectionId }
-        }
-        if selectedConnectionId == connection.id {
-          selectedConnectionId = connections.first?.id
-        }
-      }
+      try? await refreshConnections()
+      pushStatusMessages = pushStatusMessages.filter { connectionId, _ in
+        connections.contains { $0.id == connectionId }
+      }
+      if selectedConnectionId == connection.id {
+        selectedConnectionId = connections.first?.id
+      }
       errorMessage = error.localizedDescription
     }

🤖 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/Features/Account/AccountView.swift` around
lines 852 - 864, In the catch block handling the removal failure, replace the
duplicated loadConnections sorting, push-status filtering, and selection update
with a best-effort call to refreshConnections(), matching the removeEverywhere
error path. Preserve assigning error.localizedDescription to errorMessage after
the refresh attempt so defaultSendingConnectionId and all connection-related
state are refreshed consistently.

apps/unwired-mail/unwired-mail/Features/Account/GenericMailSetupPanel.swift (1)

281-318: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Compute authorizedSyncedConnectionIds before mutating other state.

syncedDefinitions and defaultSendingConnectionId are assigned before the throwing try Set(...) computation over hasLocalAuthorization. If that throws for any definition, this catch aborts, but syncedDefinitions/defaultSendingConnectionId are already updated while authorizedSyncedConnectionIds stays stale relative to the new definitions list — the same partial-state-update pattern flagged (and fixed) in AccountView's refreshConnections(). It fails safe (unmatched IDs read as unauthorized) but is still an avoidable inconsistency.

🐛 Proposed fix

     do {
       let definitions = try await service.loadSyncedDefinitions(session: syncSession)
         .sorted { $0.emailAddress < $1.emailAddress }
-      defaultSendingConnectionId = try await service.loadDefaultSendingConnectionId(
+      let loadedDefaultSendingConnectionId = try await service.loadDefaultSendingConnectionId(
         session: syncSession
       )
-      syncedDefinitions = definitions
-      authorizedSyncedConnectionIds = try Set(
+      let loadedAuthorizedIds = try Set(
         definitions.compactMap { definition in
           try service.hasLocalAuthorization(
             definition,
             productAccountId: productAccountId
           ) ? definition.connectionId : nil
         }
       )
+      defaultSendingConnectionId = loadedDefaultSendingConnectionId
+      syncedDefinitions = definitions
+      authorizedSyncedConnectionIds = loadedAuthorizedIds

🤖 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/Features/Account/GenericMailSetupPanel.swift`
around lines 281 - 318, In loadSyncedDefinitions, compute the throwing
authorizedSyncedConnectionIds Set from definitions before assigning
syncedDefinitions or defaultSendingConnectionId. Only mutate those state
properties after all service calls and authorization checks succeed, preserving
the existing catch behavior without leaving partially updated state.