diff --git a/lib/core/repositories/email_repository.dart b/lib/core/repositories/email_repository.dart index cf69e7a..46d2ade 100644 --- a/lib/core/repositories/email_repository.dart +++ b/lib/core/repositories/email_repository.dart @@ -99,6 +99,11 @@ abstract class EmailRepository { /// Used for the "Undo" feature when the original rows were hard-deleted (IMAP). Future restoreEmails(List emails); + /// Finds an email in [accountId]'s mailboxes by its RFC 2822 Message-ID header. + /// Returns null if not found. Used during undo to locate an email after its + /// IMAP UID changed (e.g. after a server-applied move assigned a new UID). + Future findEmailByMessageId(String accountId, String messageId); + /// Emits the accountId whenever a new change is enqueued locally. /// Used by AccountSyncManager to trigger an immediate flush. Stream get onChangesQueued; diff --git a/lib/core/services/undo_service.dart b/lib/core/services/undo_service.dart index 242f834..505a755 100644 --- a/lib/core/services/undo_service.dart +++ b/lib/core/services/undo_service.dart @@ -45,17 +45,17 @@ class UndoService extends StateNotifier> { final UndoAction action; if (actionId == null) { action = state.last; - state = state.sublist(0, state.length - 1); } else { try { action = state.firstWhere((a) => a.id == actionId); - state = state.where((a) => a.id != actionId).toList(); } catch (e) { return; // Action not found } } - unawaited(_ref.read(undoRepositoryProvider).deleteAction(action.id)); + // Keep the original entry in state and DB so the user can see what + // happened and retry if the undo failed (e.g. after an IMAP sync reverted + // the local change). The inverse action added below allows undoing the undo. final repo = _ref.read(emailRepositoryProvider); @@ -70,10 +70,22 @@ class UndoService extends StateNotifier> { ? null : action.originalEmails.where((e) => e.id == id).firstOrNull; - // 2. If row is missing (hard delete), restore it first. - // We restore it at its CURRENT state (where it is on the server, - // or where it was moving to). - if (original != null) { + // 2. Resolve the current DB row for the email. + // For IMAP, after a server-applied move the email gets a new UID, so + // the original id ('accountId:oldUid') no longer exists. Look it up by + // Message-ID so we use the correct UID in the pending change. + var currentEmail = await repo.getEmail(id); + if (currentEmail == null && original?.messageId != null) { + currentEmail = await repo.findEmailByMessageId( + action.accountId, + original!.messageId!, + ); + } + final currentId = currentEmail?.id ?? id; + + // 3. If the row is absent (hard delete or UID changed after sync), + // restore it from the saved snapshot so moveEmail can find it. + if (currentEmail == null && original != null) { final currentPath = cancelled ? action.sourceMailboxPath : (action.destinationMailboxPath ?? action.sourceMailboxPath); @@ -82,15 +94,15 @@ class UndoService extends StateNotifier> { ]); } - // 3. Move it back to source. + // 4. Move it back to source. // This updates local DB optimistically and (if not cancelled) enqueues - // a reverse move on the server. - await repo.moveEmail(id, action.sourceMailboxPath); + // a reverse move on the server using the correct UID. + await repo.moveEmail(currentId, action.sourceMailboxPath); if (cancelled) { - // 4. If we successfully cancelled the original, the reverse move + // 5. If we successfully cancelled the original, the reverse move // we just enqueued is redundant. - await repo.cancelPendingChange(id, 'move'); + await repo.cancelPendingChange(currentId, 'move'); } } catch (e) { // Best effort. diff --git a/lib/data/repositories/email_repository_impl.dart b/lib/data/repositories/email_repository_impl.dart index c22c497..ba015c1 100644 --- a/lib/data/repositories/email_repository_impl.dart +++ b/lib/data/repositories/email_repository_impl.dart @@ -1847,6 +1847,22 @@ class EmailRepositoryImpl implements EmailRepository { return expired.length; } + @override + @override + Future findEmailByMessageId( + String accountId, + String messageId, + ) async { + final row = await (_db.select(_db.emails) + ..where( + (t) => + t.accountId.equals(accountId) & t.messageId.equals(messageId), + ) + ..limit(1)) + .getSingleOrNull(); + return row == null ? null : _toModel(row); + } + @override Future restoreEmails(List emails) async { for (final e in emails) { diff --git a/test/integration/account_sync_manager_test.dart b/test/integration/account_sync_manager_test.dart index 3822b45..aa969ab 100644 --- a/test/integration/account_sync_manager_test.dart +++ b/test/integration/account_sync_manager_test.dart @@ -208,6 +208,13 @@ class _FakeEmails implements EmailRepository { @override Future restoreEmails(List emails) async {} + @override + Future findEmailByMessageId( + String accountId, + String messageId, + ) async => + null; + @override Future deleteEmail(String id) async => null; diff --git a/test/unit/account_sync_manager_test.dart b/test/unit/account_sync_manager_test.dart index 37e2cd5..15ac211 100644 --- a/test/unit/account_sync_manager_test.dart +++ b/test/unit/account_sync_manager_test.dart @@ -77,6 +77,13 @@ class FakeEmailRepository implements EmailRepository { @override Future restoreEmails(List emails) async {} + @override + Future findEmailByMessageId( + String accountId, + String messageId, + ) async => + null; + @override Future deleteEmail(String id) async => null; @override diff --git a/test/unit/account_sync_manager_test.mocks.dart b/test/unit/account_sync_manager_test.mocks.dart index 9a8ab9a..db928fb 100644 --- a/test/unit/account_sync_manager_test.mocks.dart +++ b/test/unit/account_sync_manager_test.mocks.dart @@ -606,6 +606,22 @@ class MockEmailRepository extends _i1.Mock implements _i9.EmailRepository { returnValueForMissingStub: _i4.Future.value(), ) as _i4.Future); + @override + _i4.Future<_i2.Email?> findEmailByMessageId( + String? accountId, + String? messageId, + ) => + (super.noSuchMethod( + Invocation.method( + #findEmailByMessageId, + [ + accountId, + messageId, + ], + ), + returnValue: _i4.Future<_i2.Email?>.value(), + ) as _i4.Future<_i2.Email?>); + @override _i4.Stream watchJmapPush( String? accountId, diff --git a/test/unit/reliability_runner_test.dart b/test/unit/reliability_runner_test.dart index f3dde40..9989387 100644 --- a/test/unit/reliability_runner_test.dart +++ b/test/unit/reliability_runner_test.dart @@ -141,6 +141,12 @@ class _CountingEmails implements EmailRepository { @override Future restoreEmails(List emails) async {} @override + Future findEmailByMessageId( + String accountId, + String messageId, + ) async => + null; + @override Stream get onChangesQueued => const Stream.empty(); @override Stream watchJmapPush(String accountId, String password) => diff --git a/test/unit/undo_logic_test.dart b/test/unit/undo_logic_test.dart index e3f1282..2a696b0 100644 --- a/test/unit/undo_logic_test.dart +++ b/test/unit/undo_logic_test.dart @@ -251,6 +251,78 @@ void main() { }, ); + test( + 'Undo deletion for IMAP succeeds after sync assigned new UID (message-id lookup)', + () async { + const oldEmailId = 'acc1:101'; + final original = await repo.getEmail(oldEmailId); + expect(original, isNotNull); + expect(original!.messageId, isNull); // set a messageId so lookup works + + // Seed a messageId so undo can find the email after UID change. + await (db.update(db.emails)..where((t) => t.id.equals(oldEmailId))) + .write(const EmailsCompanion(messageId: Value('msg-101@test'))); + + final originalWithMsgId = await repo.getEmail(oldEmailId); + + // 1. Delete → moves to Trash locally (uid=101, id='acc1:101') + final destPath = await repo.deleteEmail(oldEmailId); + expect(destPath, 'Trash'); + + // 2. Simulate IMAP sync: the server assigned a new UID (205) in Trash. + // The old row (acc1:101) is removed and a new row (acc1:205) is inserted. + await (db.delete(db.emails)..where((t) => t.id.equals(oldEmailId))).go(); + await db.into(db.emails).insert( + EmailsCompanion.insert( + id: 'acc1:205', + accountId: 'acc1', + mailboxPath: 'Trash', + uid: 205, + subject: const Value('Test Email'), + receivedAt: DateTime.now(), + messageId: const Value('msg-101@test'), + ), + ); + + // Mark the original pending change as already applied (cannot cancel). + await (db.update(db.pendingChanges) + ..where((t) => t.resourceId.equals(oldEmailId))) + .write(const PendingChangesCompanion(attempts: Value(1))); + + // 3. Undo using the old email id — undo must locate 'acc1:205' by message-id. + final action = UndoAction( + id: 'undo-uid-change', + accountId: 'acc1', + type: UndoType.delete, + emailIds: [oldEmailId], + sourceMailboxPath: 'INBOX', + destinationMailboxPath: destPath, + originalEmails: [originalWithMsgId!], + ); + await container.read(undoServiceProvider.notifier).pushAction(action); + await container.read(undoServiceProvider.notifier).undo(); + + // 4. Verify the current email row is now in INBOX. + final inInbox = await (db.select(db.emails) + ..where((t) => t.mailboxPath.equals('INBOX'))) + .get(); + expect( + inInbox, + isNotEmpty, + reason: 'Email should be in INBOX after undo', + ); + + // 5. Verify the pending change uses the new UID (205), not the old one (101). + final changes = await db.select(db.pendingChanges).get(); + final reverseMove = changes.firstWhere( + (c) => c.changeType == 'move' && c.attempts == 0, + ); + final payload = jsonDecode(reverseMove.payload) as Map; + expect(payload['uid'], 205, reason: 'Pending move must use the new UID'); + expect(payload['dest'], 'INBOX'); + }, + ); + test('Undo snooze clears snooze metadata and moves back', () async { const emailId = 'acc1:101'; final original = await repo.getEmail(emailId); diff --git a/test/unit/undo_service_test.dart b/test/unit/undo_service_test.dart index 1c91ebc..e0f4a6c 100644 --- a/test/unit/undo_service_test.dart +++ b/test/unit/undo_service_test.dart @@ -25,6 +25,10 @@ void main() { when( mockUndoRepo.getHistory(limit: anyNamed('limit')), ).thenAnswer((_) async => []); + when(mockEmailRepo.getEmail(any)).thenAnswer((_) async => null); + when( + mockEmailRepo.findEmailByMessageId(any, any), + ).thenAnswer((_) async => null); container = ProviderContainer( overrides: [ @@ -97,21 +101,24 @@ void main() { await notifier.pushAction(action1); await notifier.pushAction(action2); - // Undo action2 (delete); a reverse move action is pushed in its place. + // Undo action2 (delete); the original entry is kept and a reverse action is added. await notifier.undo(); final stateAfterUndo2 = container.read(undoServiceProvider); - expect(stateAfterUndo2.length, 2); + expect(stateAfterUndo2.length, 3); expect(stateAfterUndo2[0].id, '1'); - expect(stateAfterUndo2[1].id, '2-inv'); - expect(stateAfterUndo2[1].sourceMailboxPath, 'Trash'); - expect(stateAfterUndo2[1].destinationMailboxPath, 'INBOX'); + expect(stateAfterUndo2[1].id, '2'); + expect(stateAfterUndo2[2].id, '2-inv'); + expect(stateAfterUndo2[2].sourceMailboxPath, 'Trash'); + expect(stateAfterUndo2[2].destinationMailboxPath, 'INBOX'); verify(mockEmailRepo.moveEmail('e2', 'INBOX')).called(1); - // Undo action1 (no dest → no inverse); log shrinks to just the inverse. + // Undo action1 (no dest → no inverse); original stays, log is unchanged. await notifier.undo(actionId: '1'); final stateAfterUndo1 = container.read(undoServiceProvider); - expect(stateAfterUndo1.length, 1); - expect(stateAfterUndo1[0].id, '2-inv'); + expect(stateAfterUndo1.length, 3); + expect(stateAfterUndo1[0].id, '1'); + expect(stateAfterUndo1[1].id, '2'); + expect(stateAfterUndo1[2].id, '2-inv'); verify(mockEmailRepo.moveEmail('e1', 'INBOX')).called(1); }); @@ -136,9 +143,11 @@ void main() { await notifier.pushAction(action); await notifier.undo(actionId: 'del1'); + // Original entry stays; inverse is added. final log = container.read(undoServiceProvider); - expect(log.length, 1); - final inv = log.first; + expect(log.length, 2); + expect(log[0].id, 'del1'); + final inv = log[1]; expect(inv.id, 'del1-inv'); expect(inv.type, UndoType.move); expect(inv.emailIds, ['e1']); @@ -172,8 +181,10 @@ void main() { await notifier.pushAction(action); await notifier.undo(actionId: 'mv1'); + // Original entry stays; no inverse since no destinationMailboxPath. final log = container.read(undoServiceProvider); - expect(log, isEmpty); + expect(log.length, 1); + expect(log.first.id, 'mv1'); }); test('undo with actionId removes and undos specific action', () async { @@ -203,9 +214,9 @@ void main() { await notifier.pushAction(action1); await notifier.pushAction(action2); - // action1 has no destinationMailboxPath → no inverse pushed, so action2 remains. + // action1 has no destinationMailboxPath → no inverse pushed, original stays. await notifier.undo(actionId: '1'); - expect(container.read(undoServiceProvider), [action2]); + expect(container.read(undoServiceProvider), [action1, action2]); verify(mockEmailRepo.moveEmail('e1', 'INBOX')).called(1); verifyNever(mockEmailRepo.moveEmail('e2', 'INBOX')); }); diff --git a/test/unit/undo_service_test.mocks.dart b/test/unit/undo_service_test.mocks.dart index e3b8ef9..b006fd3 100644 --- a/test/unit/undo_service_test.mocks.dart +++ b/test/unit/undo_service_test.mocks.dart @@ -466,6 +466,22 @@ class MockEmailRepository extends _i1.Mock implements _i3.EmailRepository { returnValueForMissingStub: _i4.Future.value(), ) as _i4.Future); + @override + _i4.Future<_i2.Email?> findEmailByMessageId( + String? accountId, + String? messageId, + ) => + (super.noSuchMethod( + Invocation.method( + #findEmailByMessageId, + [ + accountId, + messageId, + ], + ), + returnValue: _i4.Future<_i2.Email?>.value(), + ) as _i4.Future<_i2.Email?>); + @override _i4.Stream watchJmapPush( String? accountId, diff --git a/test/widget/helpers.dart b/test/widget/helpers.dart index 7f260d0..e9236bc 100644 --- a/test/widget/helpers.dart +++ b/test/widget/helpers.dart @@ -232,6 +232,13 @@ class FakeEmailRepository implements EmailRepository { @override Future restoreEmails(List emails) async {} + @override + Future findEmailByMessageId( + String accountId, + String messageId, + ) async => + null; + @override Future deleteEmail(String emailId) async => null;