fix code review issues: deferral date format, storeFlags SELECT, event loop leaks, GTD selection tracking
- fix deferral resurfacing using VTODOParser.formatDateOnly instead of ISO8601 (date format mismatch) - add SELECT mailbox before UID STORE in storeFlags (IMAP protocol requirement) - pass credentials to SyncCoordinator so IDLE monitoring activates - add selectedItem tracking to MailViewModel, wire List selection in GTD views - fix startPeriodicSync to sleep-first, preventing duplicate sync on launch - add deinit cleanup for EventLoopGroup in IMAPConnection, SMTPConnection - use separate IMAP client for attachment downloads, avoid shared connection interference - remove [weak self] from IMAPIdleClient actor Task to prevent orphaned connections - fix isGTDPerspective to check selectedMailbox instead of items.isEmpty - fix fetchBody to use complete RFC822 fetch instead of BODY[TEXT] - reuse single IMAP connection per ActionQueue.flush() batch - add requiresIMAP to ActionPayload for connection batching - load task categories from label store instead of hardcoded empty array - suppress NIOSSLHandler Sendable warnings via Package.swift unsafeFlags - fix unused variable warnings across codebase Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -42,18 +42,34 @@ public actor ActionQueue {
|
||||
// MARK: - Flush
|
||||
|
||||
/// Dispatch all pending actions in creation order. Called before IMAP fetch.
|
||||
/// Reuses a single IMAP connection for all IMAP actions in the batch.
|
||||
public func flush() async {
|
||||
guard let records = try? store.pendingActions(accountId: accountId) else { return }
|
||||
guard let records = try? store.pendingActions(accountId: accountId),
|
||||
!records.isEmpty else { return }
|
||||
|
||||
// Create one shared IMAP client for all IMAP actions in this flush
|
||||
let sharedImapClient = imapClientProvider()
|
||||
var imapConnected = false
|
||||
|
||||
defer {
|
||||
if imapConnected {
|
||||
Task { try? await sharedImapClient.disconnect() }
|
||||
}
|
||||
}
|
||||
|
||||
for record in records {
|
||||
guard let action = decodeAction(record) else {
|
||||
// Corrupt record — remove it
|
||||
try? store.deletePendingAction(id: record.id)
|
||||
continue
|
||||
}
|
||||
|
||||
do {
|
||||
try await dispatch(action)
|
||||
// Connect shared IMAP client on first IMAP action
|
||||
if !imapConnected, action.payload.requiresIMAP {
|
||||
try await sharedImapClient.connect()
|
||||
imapConnected = true
|
||||
}
|
||||
try await dispatch(action, sharedImapClient: sharedImapClient)
|
||||
try store.deletePendingAction(id: record.id)
|
||||
} catch {
|
||||
var updated = record
|
||||
@@ -88,26 +104,25 @@ public actor ActionQueue {
|
||||
}
|
||||
}
|
||||
|
||||
private func dispatch(_ action: PendingAction) async throws {
|
||||
private func dispatch(_ action: PendingAction, sharedImapClient: (any IMAPClientProtocol)? = nil) async throws {
|
||||
switch action.payload {
|
||||
case .setFlags(let uid, let mailbox, let add, let remove):
|
||||
let client = imapClientProvider()
|
||||
try await client.connect()
|
||||
defer { Task { try? await client.disconnect() } }
|
||||
let client = sharedImapClient ?? imapClientProvider()
|
||||
if sharedImapClient == nil { try await client.connect() }
|
||||
defer { if sharedImapClient == nil { Task { try? await client.disconnect() } } }
|
||||
try await client.storeFlags(uid: uid, mailbox: mailbox, add: add, remove: remove)
|
||||
|
||||
case .move(let uid, let from, let to):
|
||||
let client = imapClientProvider()
|
||||
try await client.connect()
|
||||
defer { Task { try? await client.disconnect() } }
|
||||
let client = sharedImapClient ?? imapClientProvider()
|
||||
if sharedImapClient == nil { try await client.connect() }
|
||||
defer { if sharedImapClient == nil { Task { try? await client.disconnect() } } }
|
||||
try await client.moveMessage(uid: uid, from: from, to: to)
|
||||
|
||||
case .delete(let uid, let mailbox, let trashMailbox):
|
||||
let client = imapClientProvider()
|
||||
try await client.connect()
|
||||
defer { Task { try? await client.disconnect() } }
|
||||
let client = sharedImapClient ?? imapClientProvider()
|
||||
if sharedImapClient == nil { try await client.connect() }
|
||||
defer { if sharedImapClient == nil { Task { try? await client.disconnect() } } }
|
||||
if mailbox == trashMailbox {
|
||||
// Already in trash — permanent delete
|
||||
try await client.storeFlags(uid: uid, mailbox: mailbox, add: ["\\Deleted"], remove: [])
|
||||
try await client.expunge(mailbox: mailbox)
|
||||
} else {
|
||||
@@ -122,9 +137,9 @@ public actor ActionQueue {
|
||||
try await client.send(message: message)
|
||||
|
||||
case .append(let mailbox, let messageData, let flags):
|
||||
let client = imapClientProvider()
|
||||
try await client.connect()
|
||||
defer { Task { try? await client.disconnect() } }
|
||||
let client = sharedImapClient ?? imapClientProvider()
|
||||
if sharedImapClient == nil { try await client.connect() }
|
||||
defer { if sharedImapClient == nil { Task { try? await client.disconnect() } } }
|
||||
guard let data = messageData.data(using: .utf8) else {
|
||||
throw ActionQueueError.invalidMessageData
|
||||
}
|
||||
|
||||
@@ -47,4 +47,11 @@ public enum ActionPayload: Sendable, Codable {
|
||||
case .append: return .append
|
||||
}
|
||||
}
|
||||
|
||||
public var requiresIMAP: Bool {
|
||||
switch self {
|
||||
case .setFlags, .move, .delete, .append: true
|
||||
case .send: false
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -10,6 +10,7 @@ import TaskStore
|
||||
public final class SyncCoordinator {
|
||||
private let accountConfig: AccountConfig
|
||||
private let imapClient: any IMAPClientProtocol
|
||||
private let imapClientProvider: (@Sendable () -> any IMAPClientProtocol)?
|
||||
private let store: MailStore
|
||||
private let actionQueue: ActionQueue?
|
||||
private let taskStore: TaskStore?
|
||||
@@ -27,7 +28,8 @@ public final class SyncCoordinator {
|
||||
store: MailStore,
|
||||
actionQueue: ActionQueue? = nil,
|
||||
taskStore: TaskStore? = nil,
|
||||
credentials: Credentials? = nil
|
||||
credentials: Credentials? = nil,
|
||||
imapClientProvider: (@Sendable () -> any IMAPClientProtocol)? = nil
|
||||
) {
|
||||
self.accountConfig = accountConfig
|
||||
self.imapClient = imapClient
|
||||
@@ -35,6 +37,7 @@ public final class SyncCoordinator {
|
||||
self.actionQueue = actionQueue
|
||||
self.taskStore = taskStore
|
||||
self.credentials = credentials
|
||||
self.imapClientProvider = imapClientProvider
|
||||
}
|
||||
|
||||
public func onEvent(_ handler: @escaping (SyncEvent) -> Void) {
|
||||
@@ -65,7 +68,7 @@ public final class SyncCoordinator {
|
||||
}
|
||||
|
||||
private func resurfaceDeferrals() {
|
||||
let now = ISO8601DateFormatter().string(from: Date())
|
||||
let now = VTODOParser.formatDateOnly(Date())
|
||||
do {
|
||||
// Resurface deferred emails: delete deferral records so messages reappear in Inbox
|
||||
let expiredDeferrals = try store.expiredDeferrals(beforeDate: now)
|
||||
@@ -359,10 +362,20 @@ public final class SyncCoordinator {
|
||||
throw AttachmentError.noSectionPath
|
||||
}
|
||||
|
||||
// Fetch the section data from IMAP
|
||||
try await imapClient.connect()
|
||||
let data = try await imapClient.fetchSection(uid: messageUid, mailbox: mailboxName, section: sectionPath)
|
||||
try? await imapClient.disconnect()
|
||||
// Use a separate IMAP connection to avoid interfering with the sync client
|
||||
guard let provider = imapClientProvider else {
|
||||
throw AttachmentError.noSectionPath
|
||||
}
|
||||
let client = provider()
|
||||
try await client.connect()
|
||||
let data: Data
|
||||
do {
|
||||
data = try await client.fetchSection(uid: messageUid, mailbox: mailboxName, section: sectionPath)
|
||||
try? await client.disconnect()
|
||||
} catch {
|
||||
try? await client.disconnect()
|
||||
throw error
|
||||
}
|
||||
|
||||
// Build cache directory
|
||||
let cacheDir = FileManager.default.urls(for: .applicationSupportDirectory, in: .userDomainMask).first!
|
||||
@@ -396,12 +409,12 @@ public final class SyncCoordinator {
|
||||
stopSync()
|
||||
syncTask = Task { [weak self] in
|
||||
while !Task.isCancelled {
|
||||
try? await self?.syncNow()
|
||||
do {
|
||||
try await Task.sleep(for: interval)
|
||||
} catch {
|
||||
break
|
||||
}
|
||||
try? await self?.syncNow()
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user