fix remaining code review issues, add HTML email rendering, charset-aware MIME decoding

- add charset-aware string decoding in MIMEParser (supports UTF-8, Latin-1, Windows-1252, etc.) - fix prefetchBodies: remove broken ISO8601 date filter that prevented body fetching - fix ensureBodyLoaded to use fetchFullMessage + MIMEParser instead of broken fetchBody - add N+1 query fix: inboxMessagesExcludingDeferred uses SQL LEFT JOIN instead of per-message deferral check - add inboxMessageCountExcludingDeferred for efficient perspective counts - add unreadMessageCount, totalMessageCount queries to MailStore - wire mailbox unread/total counts in loadMailboxes (were hardcoded to 0) - add flag sync: reconcileFlags fetches flags for existing UIDs, updates local read/flagged state - move account config from UserDefaults to Application Support file, auto-migrate existing config - render HTML emails by default (toggle to plain text), render plain text as HTML for proper Unicode/emoji - replace print() with os_log Logger in SyncCoordinator Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-03-15 10:05:19 +01:00
parent 10b7cb2fd2
commit 31ab18cb2a
6 changed files with 188 additions and 53 deletions
--- a/Apps/MagnumOpus/ContentView.swift
+++ b/Apps/MagnumOpus/ContentView.swift
@@ -163,9 +163,7 @@ struct ContentView: View {
 		do {
 			try viewModel.setup(config: config, credentials: credentials)
 			try KeychainService.saveCredentials(credentials, for: config.id)
-			if let data = try? JSONEncoder().encode(config) {
-				UserDefaults.standard.set(data, forKey: "accountConfig")
-			}
+			Self.saveAccountConfig(config)
 			Task {
 				await viewModel.syncNow()
 				await viewModel.loadMailboxes(accountId: config.id)
@@ -178,8 +176,7 @@ struct ContentView: View {
 	}

 	private func loadExistingAccount() {
-		guard let data = UserDefaults.standard.data(forKey: "accountConfig"),
-			  let config = try? JSONDecoder().decode(AccountConfig.self, from: data),
+		guard let config = Self.loadAccountConfig(),
 			  let credentials = try? KeychainService.loadCredentials(for: config.id)
 		else { return }
 		do {
@@ -196,6 +193,40 @@ struct ContentView: View {
 	}
 }

+extension ContentView {
+	/// Store account config in Application Support (not UserDefaults) for reliability.
+	static func saveAccountConfig(_ config: AccountConfig) {
+		let url = accountConfigURL
+		let dir = url.deletingLastPathComponent()
+		try? FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true)
+		if let data = try? JSONEncoder().encode(config) {
+			try? data.write(to: url, options: .atomic)
+		}
+	}
+
+	static func loadAccountConfig() -> AccountConfig? {
+		// Try Application Support first, then migrate from UserDefaults
+		if let data = try? Data(contentsOf: accountConfigURL),
+		   let config = try? JSONDecoder().decode(AccountConfig.self, from: data) {
+			return config
+		}
+		// Migrate from UserDefaults if present
+		if let data = UserDefaults.standard.data(forKey: "accountConfig"),
+		   let config = try? JSONDecoder().decode(AccountConfig.self, from: data) {
+			saveAccountConfig(config)
+			UserDefaults.standard.removeObject(forKey: "accountConfig")
+			return config
+		}
+		return nil
+	}
+
+	private static var accountConfigURL: URL {
+		FileManager.default.urls(for: .applicationSupportDirectory, in: .userDomainMask)[0]
+			.appendingPathComponent("MagnumOpus", isDirectory: true)
+			.appendingPathComponent("accountConfig.json")
+	}
+}
+
 // Wrapper to make ComposeMode Identifiable for .sheet(item:)
 struct ComposeModeWrapper: Identifiable {
 	let id = UUID()
--- a/Apps/MagnumOpus/ViewModels/MailViewModel.swift
+++ b/Apps/MagnumOpus/ViewModels/MailViewModel.swift
@@ -2,6 +2,7 @@ import SwiftUI
 import GRDB
 import Models
 import MailStore
+import MIMEParser
 import SyncEngine
 import IMAPClient
 import SMTPClient
@@ -152,9 +153,11 @@ final class MailViewModel {
 		do {
 			let records = try store.mailboxes(accountId: accountId)
 			mailboxes = records.map { record in
-				MailboxInfo(
+				let unread = (try? store.unreadMessageCount(mailboxId: record.id)) ?? 0
+				let total = (try? store.totalMessageCount(mailboxId: record.id)) ?? 0
+				return MailboxInfo(
 					id: record.id, accountId: record.accountId,
-					name: record.name, unreadCount: 0, totalCount: 0
+					name: record.name, unreadCount: unread, totalCount: total
 				)
 			}
 			if selectedMailbox == nil, let inbox = mailboxes.first(where: { $0.name.lowercased() == "inbox" }) {
@@ -231,14 +234,10 @@ final class MailViewModel {

 			switch perspective {
 			case .inbox:
-				// Emails in INBOX with no deferral
+				// Emails in INBOX with no deferral (single SQL query)
 				if let inboxMailbox = mailboxes.first(where: { $0.name.lowercased() == "inbox" }) {
-					let msgs = try store.messages(mailboxId: inboxMailbox.id)
+					let msgs = try store.inboxMessagesExcludingDeferred(mailboxId: inboxMailbox.id)
 					for msg in msgs {
-						// Skip deferred emails
-						if let _ = try? store.deferralForMessage(messageId: msg.id) {
-							continue
-						}
 						result.append(.email(MailStore.toMessageSummary(msg)))
 					}
 				}
@@ -312,15 +311,10 @@ final class MailViewModel {
 		var counts: [Perspective: Int] = [:]

 		do {
-			// Inbox count
+			// Inbox count (single SQL query instead of N+1)
 			var inboxCount = 0
 			if let inboxMailbox = mailboxes.first(where: { $0.name.lowercased() == "inbox" }) {
-				let msgs = try store.messages(mailboxId: inboxMailbox.id)
-				for msg in msgs {
-					if (try? store.deferralForMessage(messageId: msg.id)) == nil {
-						inboxCount += 1
-					}
-				}
+				inboxCount = try store.inboxMessageCountExcludingDeferred(mailboxId: inboxMailbox.id)
 			}
 			inboxCount += try store.inboxTasks(accountId: accountId).count
 			counts[.inbox] = inboxCount
@@ -649,18 +643,20 @@ final class MailViewModel {
 		let client = provider()
 		do {
 			try await client.connect()
-			let (text, html) = try await client.fetchBody(uid: record.uid)
+			let rawMessage = try await client.fetchFullMessage(uid: record.uid)
 			try? await client.disconnect()
-			if text != nil || html != nil {
-				try store.storeBody(messageId: message.id, text: text, html: html)
-				var updated = message
-				updated.bodyText = text
-				updated.bodyHtml = html
-				return updated
+			if !rawMessage.isEmpty {
+				let parsed = MIMEParser.parse(rawMessage)
+				if parsed.textBody != nil || parsed.htmlBody != nil {
+					try store.storeBody(messageId: message.id, text: parsed.textBody, html: parsed.htmlBody)
+					var updated = message
+					updated.bodyText = parsed.textBody
+					updated.bodyHtml = parsed.htmlBody
+					return updated
+				}
 			}
 		} catch {
 			try? await client.disconnect()
-			// Offline or fetch failed — compose without quoted text
 		}
 		return message
 	}
--- a/Apps/MagnumOpus/Views/ThreadDetailView.swift
+++ b/Apps/MagnumOpus/Views/ThreadDetailView.swift
@@ -187,7 +187,7 @@ struct StatusBadge: View {

 struct MessageView: View {
 	let message: MessageSummary
-	@State private var showHTML = false
+	@State private var showSource = false

 	var body: some View {
 		VStack(alignment: .leading, spacing: 8) {
@@ -195,9 +195,9 @@ struct MessageView: View {
 				Text(message.from?.displayName ?? "Unknown")
 					.fontWeight(.semibold)
 				Spacer()
-				if message.bodyHtml != nil {
-					Toggle(isOn: $showHTML) {
-						Text("HTML")
+				if message.bodyHtml != nil && message.bodyText != nil {
+					Toggle(isOn: $showSource) {
+						Text("Plain")
 							.font(.caption)
 					}
 					.toggleStyle(.button)
@@ -214,13 +214,17 @@ struct MessageView: View {
 					.foregroundStyle(.secondary)
 			}

-			if showHTML, let html = message.bodyHtml {
-				MessageWebView(html: html)
-					.frame(minHeight: 200)
-			} else if let bodyText = message.bodyText {
+			if showSource, let bodyText = message.bodyText {
 				Text(bodyText)
 					.font(.body)
 					.textSelection(.enabled)
+			} else if let html = message.bodyHtml {
+				MessageWebView(html: html)
+					.frame(minHeight: 200)
+			} else if let bodyText = message.bodyText {
+				// Render plain text as HTML for proper Unicode/emoji rendering
+				MessageWebView(html: plainTextToHTML(bodyText))
+					.frame(minHeight: 100)
 			} else if let snippet = message.snippet {
 				Text(snippet)
 					.font(.body)
@@ -234,4 +238,13 @@ struct MessageView: View {
 		}
 		.padding()
 	}
+
+	private func plainTextToHTML(_ text: String) -> String {
+		let escaped = text
+			.replacingOccurrences(of: "&", with: "&amp;")
+			.replacingOccurrences(of: "<", with: "&lt;")
+			.replacingOccurrences(of: ">", with: "&gt;")
+			.replacingOccurrences(of: "\n", with: "<br>")
+		return "<pre style=\"white-space: pre-wrap; word-wrap: break-word; font-family: -apple-system, system-ui; font-size: 14px;\">\(escaped)</pre>"
+	}
 }
--- a/Packages/MagnumOpusCore/Sources/MIMEParser/MIMEParser.swift
+++ b/Packages/MagnumOpusCore/Sources/MIMEParser/MIMEParser.swift
@@ -29,13 +29,14 @@ public enum MIMEParser {
 			// Single-part message
 			let transferEncoding = parseTransferEncoding(headers["content-transfer-encoding"])
 			let decoded = decodeContent(body, encoding: transferEncoding)
+			let charset = extractParameter(contentType, name: "charset")

 			if contentType.lowercased().contains("text/html") {
-				return MIMEMessage(headers: headers, htmlBody: String(data: decoded, encoding: .utf8))
+				return MIMEMessage(headers: headers, htmlBody: decodeString(decoded, charset: charset))
 			} else {
 				return MIMEMessage(
 					headers: headers,
-					textBody: String(data: decoded, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines)
+					textBody: decodeString(decoded, charset: charset)?.trimmingCharacters(in: .whitespacesAndNewlines)
 				)
 			}
 		}
@@ -238,9 +239,9 @@ public enum MIMEParser {
 				if !part.subparts.isEmpty {
 					extractBodiesAndAttachments(from: part.subparts, contentType: part.contentType, into: &message, sectionPrefix: "")
 				} else if part.contentType == "text/plain" && message.textBody == nil {
-					message.textBody = String(data: part.body, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines)
+					message.textBody = decodeString(part.body, charset: part.charset)?.trimmingCharacters(in: .whitespacesAndNewlines)
 				} else if part.contentType == "text/html" && message.htmlBody == nil {
-					message.htmlBody = String(data: part.body, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines)
+					message.htmlBody = decodeString(part.body, charset: part.charset)?.trimmingCharacters(in: .whitespacesAndNewlines)
 				}
 			}
 		} else if lowerType.contains("multipart/related") {
@@ -250,9 +251,9 @@ public enum MIMEParser {
 					if !part.subparts.isEmpty {
 						extractBodiesAndAttachments(from: part.subparts, contentType: part.contentType, into: &message, sectionPrefix: "")
 					} else if part.contentType == "text/html" {
-						message.htmlBody = String(data: part.body, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines)
+						message.htmlBody = decodeString(part.body, charset: part.charset)?.trimmingCharacters(in: .whitespacesAndNewlines)
 					} else if part.contentType == "text/plain" {
-						message.textBody = String(data: part.body, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines)
+						message.textBody = decodeString(part.body, charset: part.charset)?.trimmingCharacters(in: .whitespacesAndNewlines)
 					}
 				} else {
 					let sectionIndex = sectionPrefix.isEmpty ? "\(index + 1)" : "\(sectionPrefix).\(index + 1)"
@@ -279,9 +280,9 @@ public enum MIMEParser {
 					bodyFound = true
 				} else if !bodyFound && part.disposition != .attachment && part.contentType.hasPrefix("text/") {
 					if part.contentType == "text/html" {
-						message.htmlBody = String(data: part.body, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines)
+						message.htmlBody = decodeString(part.body, charset: part.charset)?.trimmingCharacters(in: .whitespacesAndNewlines)
 					} else {
-						message.textBody = String(data: part.body, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines)
+						message.textBody = decodeString(part.body, charset: part.charset)?.trimmingCharacters(in: .whitespacesAndNewlines)
 					}
 					bodyFound = true
 				} else if part.disposition == .attachment || part.filename != nil || !part.contentType.hasPrefix("text/") {
@@ -303,6 +304,35 @@ public enum MIMEParser {
 		}
 	}

+	// MARK: - String Decoding
+
+	/// Decode Data to String using the specified charset, falling back to UTF-8.
+	public static func decodeString(_ data: Data, charset: String?) -> String? {
+		let encoding = charsetToEncoding(charset)
+		if let result = String(data: data, encoding: encoding) {
+			return result
+		}
+		// Fallback: try UTF-8, then Latin-1 (which never fails)
+		return String(data: data, encoding: .utf8) ?? String(data: data, encoding: .isoLatin1)
+	}
+
+	private static func charsetToEncoding(_ charset: String?) -> String.Encoding {
+		guard let charset = charset?.lowercased().trimmingCharacters(in: .whitespaces) else { return .utf8 }
+		switch charset {
+		case "utf-8", "utf8": return .utf8
+		case "iso-8859-1", "latin1", "iso_8859-1": return .isoLatin1
+		case "iso-8859-2", "latin2": return .isoLatin2
+		case "iso-8859-15": return .isoLatin1 // close enough
+		case "windows-1252", "cp1252": return .windowsCP1252
+		case "windows-1251", "cp1251": return .windowsCP1251
+		case "us-ascii", "ascii": return .ascii
+		case "utf-16", "utf16": return .utf16
+		case "utf-16be": return .utf16BigEndian
+		case "utf-16le": return .utf16LittleEndian
+		default: return .utf8
+		}
+	}
+
 	// MARK: - Helper Functions

 	private static func parseTransferEncoding(_ value: String?) -> TransferEncoding {
--- a/Packages/MagnumOpusCore/Sources/MailStore/MailStore.swift
+++ b/Packages/MagnumOpusCore/Sources/MailStore/MailStore.swift
@@ -431,6 +431,47 @@ public final class MailStore: Sendable {
 		}
 	}

+	/// Fetch inbox messages excluding deferred ones in a single SQL query (avoids N+1).
+	public func inboxMessagesExcludingDeferred(mailboxId: String) throws -> [MessageRecord] {
+		try dbWriter.read { db in
+			try MessageRecord.fetchAll(db, sql: """
+				SELECT m.* FROM message m
+				LEFT JOIN deferral d ON d.messageId = m.id
+				WHERE m.mailboxId = ? AND d.id IS NULL
+				ORDER BY m.date DESC
+				""", arguments: [mailboxId])
+		}
+	}
+
+	/// Count inbox messages excluding deferred ones.
+	public func inboxMessageCountExcludingDeferred(mailboxId: String) throws -> Int {
+		try dbWriter.read { db in
+			try Int.fetchOne(db, sql: """
+				SELECT COUNT(*) FROM message m
+				LEFT JOIN deferral d ON d.messageId = m.id
+				WHERE m.mailboxId = ? AND d.id IS NULL
+				""", arguments: [mailboxId]) ?? 0
+		}
+	}
+
+	/// Count unread messages in a mailbox.
+	public func unreadMessageCount(mailboxId: String) throws -> Int {
+		try dbWriter.read { db in
+			try Int.fetchOne(db, sql: """
+				SELECT COUNT(*) FROM message WHERE mailboxId = ? AND isRead = 0
+				""", arguments: [mailboxId]) ?? 0
+		}
+	}
+
+	/// Count total messages in a mailbox.
+	public func totalMessageCount(mailboxId: String) throws -> Int {
+		try dbWriter.read { db in
+			try Int.fetchOne(db, sql: """
+				SELECT COUNT(*) FROM message WHERE mailboxId = ?
+				""", arguments: [mailboxId]) ?? 0
+		}
+	}
+
 	public func expiredDeferrals(beforeDate: String) throws -> [DeferralRecord] {
 		try dbWriter.read { db in
 			try DeferralRecord
--- a/Packages/MagnumOpusCore/Sources/SyncEngine/SyncCoordinator.swift
+++ b/Packages/MagnumOpusCore/Sources/SyncEngine/SyncCoordinator.swift
@@ -1,10 +1,13 @@
 import Foundation
+import os
 import Models
 import IMAPClient
 import MailStore
 import MIMEParser
 import TaskStore

+private let logger = Logger(subsystem: "de.felixfoertsch.MagnumOpus", category: "SyncCoordinator")
+
@Observable
@MainActor
 public final class SyncCoordinator {
@@ -84,7 +87,7 @@ public final class SyncCoordinator {
 			}
 		} catch {
 			// Resurfacing is non-fatal; log and continue
-			print("[SyncCoordinator] resurfaceDeferrals error: \(error)")
+			logger.warning("[SyncCoordinator] resurfaceDeferrals error: \(error)")
 		}
 	}

@@ -168,6 +171,11 @@ public final class SyncCoordinator {
 			emit(.newMessages(count: envelopes.count, mailbox: remoteMailbox.name))
 		}

+		// Reconcile flags for existing messages (read/unread state from other devices)
+		if lastUid > 0 {
+			await reconcileFlags(mailboxId: mailboxId, uidRange: 1...lastUid)
+		}
+
 		await prefetchBodies(mailboxId: mailboxId)

 		// Detect and store mailbox role from LIST attributes
@@ -183,14 +191,30 @@ public final class SyncCoordinator {
 		)
 	}

+	/// Reconcile read/flagged state for existing messages with the server.
+	private func reconcileFlags(mailboxId: String, uidRange: ClosedRange<Int>) async {
+		do {
+			let remoteFlagPairs = try await imapClient.fetchFlags(uids: uidRange)
+			let localMessages = try store.messages(mailboxId: mailboxId)
+			let localByUid = Dictionary(localMessages.map { ($0.uid, $0) }, uniquingKeysWith: { first, _ in first })
+
+			for pair in remoteFlagPairs {
+				guard let local = localByUid[pair.uid] else { continue }
+				if local.isRead != pair.isRead || local.isFlagged != pair.isFlagged {
+					try store.updateFlags(messageId: local.id, isRead: pair.isRead, isFlagged: pair.isFlagged)
+				}
+			}
+		} catch {
+			// Flag reconciliation is non-fatal
+			logger.warning("[SyncCoordinator] reconcileFlags error: \(error)")
+		}
+	}
+
 	/// Fetch full RFC822 messages and parse MIME for body + attachments
 	private func prefetchBodies(mailboxId: String) async {
-		let thirtyDaysAgo = ISO8601DateFormatter().string(
-			from: Calendar.current.date(byAdding: .day, value: -30, to: Date())!
-		)
 		do {
 			let messages = try store.messages(mailboxId: mailboxId)
-			let recent = messages.filter { $0.bodyText == nil && $0.bodyHtml == nil && $0.date >= thirtyDaysAgo }
+			let recent = messages.filter { $0.bodyText == nil && $0.bodyHtml == nil }
 			for message in recent.prefix(50) {
 				guard !Task.isCancelled else { break }
 				let rawMessage = try await imapClient.fetchFullMessage(uid: message.uid)
@@ -221,7 +245,7 @@ public final class SyncCoordinator {
 			}
 		} catch {
 			// Background prefetch failure is non-fatal
-			print("[SyncCoordinator] prefetchBodies error: \(error)")
+			logger.warning("[SyncCoordinator] prefetchBodies error: \(error)")
 		}
 	}

@@ -300,14 +324,14 @@ public final class SyncCoordinator {
 	/// Must be called after at least one successful sync (so capabilities are cached).
 	public func startIdleMonitoring() async {
 		guard let credentials else {
-			print("[SyncCoordinator] No credentials provided, cannot start IDLE")
+			logger.warning("[SyncCoordinator] No credentials provided, cannot start IDLE")
 			return
 		}

 		do {
 			let caps = try await imapClient.capabilities()
 			guard caps.contains("IDLE") else {
-				print("[SyncCoordinator] Server does not support IDLE, using periodic sync only")
+				logger.warning("[SyncCoordinator] Server does not support IDLE, using periodic sync only")
 				return
 			}

@@ -326,7 +350,7 @@ public final class SyncCoordinator {
 				}
 			}
 		} catch {
-			print("[SyncCoordinator] Failed to start IDLE monitoring: \(error)")
+			logger.warning("[SyncCoordinator] Failed to start IDLE monitoring: \(error)")
 		}
 	}