From 6a457a9f7a0ea45682bce8376fcf3e2236841801 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=BCttler?= Date: Tue, 21 Apr 2026 07:43:30 +0200 Subject: [PATCH] fix: IMAP full sync via UID SEARCH+FETCH; add sync log UI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace full-sync fetchMessages(1:*) with UID SEARCH ALL + UID FETCH so every message gets a reliable UID on all servers - Guard CONDSTORE select on server capability to avoid BAD from servers that do not advertise CONDSTORE/QRESYNC - Add SyncLogEntry model + observeSyncLogs stream to SyncLogRepository - Add SyncLogScreen with per-entry duration/error display - Wire history icon in SettingsScreen → /accounts/:id/sync-log route - Fix FakeImapClient to expose initialized serverInfo via field override Co-Authored-By: Claude Sonnet 4.6 --- LATER.md | 18 +++++ .../repositories/sync_log_repository.dart | 25 +++++++ .../repositories/email_repository_impl.dart | 46 ++++++------ .../sync_log_repository_impl.dart | 22 ++++++ lib/ui/router.dart | 7 ++ lib/ui/screens/settings_screen.dart | 61 ++++++++------- lib/ui/screens/sync_log_screen.dart | 75 +++++++++++++++++++ scripts/check_coverage.dart | 1 + test/unit/email_repository_impl_test.dart | 12 ++- test/unit/fake_imap.dart | 10 +++ 10 files changed, 226 insertions(+), 51 deletions(-) create mode 100644 lib/ui/screens/sync_log_screen.dart diff --git a/LATER.md b/LATER.md index 5f311cc..7fef1ff 100644 --- a/LATER.md +++ b/LATER.md @@ -1,11 +1,29 @@ # Later +done? + think about that: Maybe we should not mock jmap/imap/smtp. We have a temproary Stalwart. Just like mocking DB in Django makes no sense. --- +Try Qwen, vscode plugin + +--- + +test/unit/fake_imap.dart + +Why is that still needed? We have Stalwart. + +--- + +du -a | sort -rn | head + +Why is the directory so big? 4 GByte? + +--- + After Try Connection, show some matching icon next to the text. --- diff --git a/lib/core/repositories/sync_log_repository.dart b/lib/core/repositories/sync_log_repository.dart index d2bca48..c416abf 100644 --- a/lib/core/repositories/sync_log_repository.dart +++ b/lib/core/repositories/sync_log_repository.dart @@ -1,3 +1,22 @@ +class SyncLogEntry { + const SyncLogEntry({ + required this.id, + required this.result, + this.errorMessage, + required this.startedAt, + required this.finishedAt, + }); + + final int id; + final String result; // 'ok' or 'error' + final String? errorMessage; + final DateTime startedAt; + final DateTime finishedAt; + + Duration get duration => finishedAt.difference(startedAt); + bool get isOk => result == 'ok'; +} + abstract class SyncLogRepository { Future log({ required String accountId, @@ -6,6 +25,8 @@ abstract class SyncLogRepository { required DateTime startedAt, required DateTime finishedAt, }); + + Stream> observeSyncLogs(String accountId); } class NoOpSyncLogRepository implements SyncLogRepository { @@ -19,4 +40,8 @@ class NoOpSyncLogRepository implements SyncLogRepository { required DateTime startedAt, required DateTime finishedAt, }) async {} + + @override + Stream> observeSyncLogs(String accountId) => + Stream.value([]); } diff --git a/lib/data/repositories/email_repository_impl.dart b/lib/data/repositories/email_repository_impl.dart index 3f7e95f..29ab431 100644 --- a/lib/data/repositories/email_repository_impl.dart +++ b/lib/data/repositories/email_repository_impl.dart @@ -227,10 +227,14 @@ class EmailRepositoryImpl implements EmailRepository { final client = await _imapConnect(account, _effectiveUsername(account), password); try { - // Enable CONDSTORE so the server returns HIGHESTMODSEQ in SELECT and - // honours CHANGEDSINCE modifiers on FETCH (RFC 7162). - final selectedMailbox = - await client.selectMailboxByPath(mailboxPath, enableCondStore: true); + // Only request CONDSTORE if the server advertises it. Servers that don't + // support the extension may reject SELECT with (CONDSTORE) with BAD. + final supportsCondStore = client.serverInfo.supports('CONDSTORE') || + client.serverInfo.supports('QRESYNC'); + final selectedMailbox = await client.selectMailboxByPath( + mailboxPath, + enableCondStore: supportsCondStore, + ); final uidValidity = selectedMailbox.uidValidity ?? 0; final serverModSeq = selectedMailbox.highestModSequence; final resourceType = 'IMAP:$mailboxPath'; @@ -248,13 +252,21 @@ class EmailRepositoryImpl implements EmailRepository { )) .go(); } - await _fetchAndUpsertImap( - client, - account, - mailboxPath, - imap.MessageSequence.fromAll(), - ); - final maxUid = await _maxLocalUid(account.id, mailboxPath); + // Use UID SEARCH ALL + UID FETCH so every message gets a reliable UID. + // Regular FETCH 1:* may not populate msg.uid on all servers. + final allUids = (await client.uidSearchMessages(searchCriteria: 'ALL')) + .matchingSequence + ?.toList() ?? + []; + if (allUids.isNotEmpty) { + await _fetchAndUpsertImap( + client, + account, + mailboxPath, + imap.MessageSequence.fromIds(allUids, isUid: true), + ); + } + final maxUid = allUids.isEmpty ? 0 : allUids.reduce(math.max); await _saveImapCheckpoint( account.id, resourceType, @@ -391,18 +403,6 @@ class EmailRepositoryImpl implements EmailRepository { } } - Future _maxLocalUid(String accountId, String mailboxPath) async { - final rows = await (_db.select(_db.emails) - ..where( - (t) => - t.accountId.equals(accountId) & - t.mailboxPath.equals(mailboxPath), - )) - .get(); - if (rows.isEmpty) return 0; - return rows.map((r) => r.uid).reduce(math.max); - } - Future?> _loadImapCheckpoint( String accountId, String resourceType, diff --git a/lib/data/repositories/sync_log_repository_impl.dart b/lib/data/repositories/sync_log_repository_impl.dart index 8fd25e3..f50ac63 100644 --- a/lib/data/repositories/sync_log_repository_impl.dart +++ b/lib/data/repositories/sync_log_repository_impl.dart @@ -26,4 +26,26 @@ class SyncLogRepositoryImpl implements SyncLogRepository { ), ); } + + @override + Stream> observeSyncLogs(String accountId) { + return (_db.select(_db.syncLogs) + ..where((t) => t.accountId.equals(accountId)) + ..orderBy([(t) => OrderingTerm.desc(t.startedAt)]) + ..limit(100)) + .watch() + .map( + (rows) => rows + .map( + (r) => SyncLogEntry( + id: r.id, + result: r.result, + errorMessage: r.errorMessage, + startedAt: r.startedAt, + finishedAt: r.finishedAt, + ), + ) + .toList(), + ); + } } diff --git a/lib/ui/router.dart b/lib/ui/router.dart index 750d7e7..7737510 100644 --- a/lib/ui/router.dart +++ b/lib/ui/router.dart @@ -8,6 +8,7 @@ import 'screens/email_detail_screen.dart'; import 'screens/email_list_screen.dart'; import 'screens/mailbox_list_screen.dart'; import 'screens/settings_screen.dart'; +import 'screens/sync_log_screen.dart'; final router = GoRouter( initialLocation: '/accounts', @@ -26,6 +27,12 @@ final router = GoRouter( accountId: state.pathParameters['accountId']!, ), ), + GoRoute( + path: ':accountId/sync-log', + builder: (ctx, state) => SyncLogScreen( + accountId: state.pathParameters['accountId']!, + ), + ), GoRoute( path: ':accountId/mailboxes', builder: (ctx, state) => diff --git a/lib/ui/screens/settings_screen.dart b/lib/ui/screens/settings_screen.dart index 024b457..000a88c 100644 --- a/lib/ui/screens/settings_screen.dart +++ b/lib/ui/screens/settings_screen.dart @@ -1,5 +1,6 @@ import 'package:flutter/material.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:go_router/go_router.dart'; import '../../di.dart'; @@ -28,32 +29,42 @@ class SettingsScreen extends ConsumerWidget { leading: const Icon(Icons.account_circle), title: Text(a.displayName), subtitle: Text(a.email), - trailing: IconButton( - icon: const Icon(Icons.delete), - onPressed: () async { - final confirm = await showDialog( - context: ctx, - builder: (ctx) => AlertDialog( - title: const Text('Remove account?'), - content: Text( - 'Remove ${a.displayName}? Local data will be deleted.', - ), - actions: [ - TextButton( - onPressed: () => Navigator.pop(ctx, false), - child: const Text('Cancel'), + trailing: Row( + mainAxisSize: MainAxisSize.min, + children: [ + IconButton( + icon: const Icon(Icons.history), + tooltip: 'Sync log', + onPressed: () => ctx.push('/accounts/${a.id}/sync-log'), + ), + IconButton( + icon: const Icon(Icons.delete), + onPressed: () async { + final confirm = await showDialog( + context: ctx, + builder: (ctx) => AlertDialog( + title: const Text('Remove account?'), + content: Text( + 'Remove ${a.displayName}? Local data will be deleted.', + ), + actions: [ + TextButton( + onPressed: () => Navigator.pop(ctx, false), + child: const Text('Cancel'), + ), + FilledButton( + onPressed: () => Navigator.pop(ctx, true), + child: const Text('Remove'), + ), + ], ), - FilledButton( - onPressed: () => Navigator.pop(ctx, true), - child: const Text('Remove'), - ), - ], - ), - ); - if (confirm == true) { - await repo.removeAccount(a.id); - } - }, + ); + if (confirm == true) { + await repo.removeAccount(a.id); + } + }, + ), + ], ), ), ], diff --git a/lib/ui/screens/sync_log_screen.dart b/lib/ui/screens/sync_log_screen.dart new file mode 100644 index 0000000..7c329f4 --- /dev/null +++ b/lib/ui/screens/sync_log_screen.dart @@ -0,0 +1,75 @@ +import 'package:flutter/material.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:intl/intl.dart'; + +import '../../core/repositories/sync_log_repository.dart'; +import '../../di.dart'; + +final _timeFmt = DateFormat('MMM d, HH:mm:ss'); + +class SyncLogScreen extends ConsumerWidget { + const SyncLogScreen({super.key, required this.accountId}); + + final String accountId; + + @override + Widget build(BuildContext context, WidgetRef ref) { + final repo = ref.watch(syncLogRepositoryProvider); + return Scaffold( + appBar: AppBar(title: const Text('Sync log')), + body: StreamBuilder>( + stream: repo.observeSyncLogs(accountId), + builder: (ctx, snap) { + if (!snap.hasData) { + return const Center(child: CircularProgressIndicator()); + } + final entries = snap.data!; + if (entries.isEmpty) { + return const Center(child: Text('No sync entries yet')); + } + return ListView.builder( + itemCount: entries.length, + itemBuilder: (ctx, i) { + final e = entries[i]; + final ms = e.duration.inMilliseconds; + final durationLabel = + ms < 1000 ? '${ms}ms' : '${(ms / 1000).toStringAsFixed(1)}s'; + return ListTile( + leading: Icon( + e.isOk ? Icons.check_circle : Icons.error_outline, + color: + e.isOk ? Colors.green : Theme.of(ctx).colorScheme.error, + ), + title: Text( + e.isOk ? 'OK' : 'Error', + style: e.isOk + ? null + : TextStyle(color: Theme.of(ctx).colorScheme.error), + ), + subtitle: Column( + crossAxisAlignment: CrossAxisAlignment.start, + mainAxisSize: MainAxisSize.min, + children: [ + Text(_timeFmt.format(e.startedAt)), + Text('took $durationLabel'), + if (e.errorMessage != null) + Text( + e.errorMessage!, + style: TextStyle( + color: Theme.of(ctx).colorScheme.error, + fontSize: 12, + ), + maxLines: 3, + overflow: TextOverflow.ellipsis, + ), + ], + ), + isThreeLine: e.errorMessage != null, + ); + }, + ); + }, + ), + ); + } +} diff --git a/scripts/check_coverage.dart b/scripts/check_coverage.dart index 0e2d23c..d44781d 100644 --- a/scripts/check_coverage.dart +++ b/scripts/check_coverage.dart @@ -41,6 +41,7 @@ const _excluded = { 'lib/ui/screens/add_account_screen.dart', 'lib/ui/screens/compose_screen.dart', 'lib/ui/screens/email_detail_screen.dart', + 'lib/ui/screens/sync_log_screen.dart', }; void main() { diff --git a/test/unit/email_repository_impl_test.dart b/test/unit/email_repository_impl_test.dart index c35b38e..79e0ae8 100644 --- a/test/unit/email_repository_impl_test.dart +++ b/test/unit/email_repository_impl_test.dart @@ -485,7 +485,9 @@ void main() { final r = _makeReposWithFakes(); await r.accounts.addAccount(_account, 'pw'); r.fakeImap.uidValidityResult = 1000; - r.fakeImap.fetchResults = [ + // Full sync now uses UID SEARCH ALL then UID FETCH. + r.fakeImap.searchUids = [10, 20]; + r.fakeImap.uidFetchResults = [ buildEnvelopeMessage(uid: 10, subject: 'First'), buildEnvelopeMessage(uid: 20, subject: 'Second'), ]; @@ -599,7 +601,9 @@ void main() { receivedAt: DateTime(2024), ), ); - r.fakeImap.fetchResults = [ + // Full sync (UID validity reset) uses UID SEARCH ALL then UID FETCH. + r.fakeImap.searchUids = [1]; + r.fakeImap.uidFetchResults = [ buildEnvelopeMessage(uid: 1, subject: 'Fresh start'), ]; @@ -618,7 +622,9 @@ void main() { test('syncEmails skips messages with no envelope or no uid', () async { final r = _makeReposWithFakes(); await r.accounts.addAccount(_account, 'pw'); - r.fakeImap.fetchResults = [ + // Full sync uses UID SEARCH ALL then UID FETCH. + r.fakeImap.searchUids = [99, 42]; // 99 = buildMessageWithoutEnvelope uid + r.fakeImap.uidFetchResults = [ buildMessageWithoutEnvelope(), // no envelope → skip buildEnvelopeMessage(uid: 42, subject: 'Valid'), ]; diff --git a/test/unit/fake_imap.dart b/test/unit/fake_imap.dart index f6812d7..99e28b5 100644 --- a/test/unit/fake_imap.dart +++ b/test/unit/fake_imap.dart @@ -1,10 +1,20 @@ import 'package:enough_mail/enough_mail.dart' as imap; +// ignore: implementation_imports +import 'package:enough_mail/src/private/util/client_base.dart' + show ConnectionInfo; /// Configurable fake IMAP client that extends the real ImapClient but /// overrides every network method to return pre-set data. class FakeImapClient extends imap.ImapClient { FakeImapClient() : super(); + /// Override serverInfo with a pre-built instance so tests can access + /// capabilities without going through the real (uninitialized) late field. + @override + final imap.ImapServerInfo serverInfo = imap.ImapServerInfo( + const ConnectionInfo('fake.host', 993, isSecure: true), + )..capabilities = [const imap.Capability('CONDSTORE')]; + List fetchResults = []; List uidFetchResults = []; List listMailboxesResult = [];