fix(undo): keep undo log entry and fix IMAP UID mismatch after sync (#81)

Two fixes for the UndoLog:

1. Don't delete the original undo log entry when undo is performed.
   The entry stays in the log alongside the new inverse action, so
   the user can retry the undo if it was silently reverted by an
   IMAP sync.

2. Fix IMAP UID mismatch: after an IMAP move is applied on the server
   the email gets a new UID in the destination folder. The undo service
   now looks up the email by its RFC 2822 Message-ID when the original
   row is gone, so the reverse-move pending change carries the correct
   UID and actually succeeds on the server.

Add findEmailByMessageId to EmailRepository interface and impl.
Add a regression test that simulates the UID change scenario.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

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<void> restoreEmails(List<Email> 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<Email?> findEmailByMessageId(String accountId, String messageId);
+
   /// Emits the accountId whenever a new change is enqueued locally.
   /// Used by AccountSyncManager to trigger an immediate flush.
   Stream<String> get onChangesQueued;

lib/core/services/undo_service.dart

@@ -45,17 +45,17 @@ class UndoService extends StateNotifier<List<UndoAction>> {
     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<List<UndoAction>> {
             ? 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<List<UndoAction>> {
           ]);
         }
 
-        // 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.

lib/data/repositories/email_repository_impl.dart

@@ -1847,6 +1847,22 @@ class EmailRepositoryImpl implements EmailRepository {
     return expired.length;
   }
 
+  @override
+  @override
+  Future<model.Email?> 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<void> restoreEmails(List<model.Email> emails) async {
     for (final e in emails) {

test/integration/account_sync_manager_test.dart

@@ -208,6 +208,13 @@ class _FakeEmails implements EmailRepository {
   @override
   Future<void> restoreEmails(List<Email> emails) async {}
 
+  @override
+  Future<Email?> findEmailByMessageId(
+    String accountId,
+    String messageId,
+  ) async =>
+      null;
+
   @override
   Future<String?> deleteEmail(String id) async => null;
 

test/unit/account_sync_manager_test.dart

@@ -77,6 +77,13 @@ class FakeEmailRepository implements EmailRepository {
   @override
   Future<void> restoreEmails(List<Email> emails) async {}
 
+  @override
+  Future<Email?> findEmailByMessageId(
+    String accountId,
+    String messageId,
+  ) async =>
+      null;
+
   @override
   Future<String?> deleteEmail(String id) async => null;
   @override

test/unit/account_sync_manager_test.mocks.dart

@@ -606,6 +606,22 @@ class MockEmailRepository extends _i1.Mock implements _i9.EmailRepository {
         returnValueForMissingStub: _i4.Future<void>.value(),
       ) as _i4.Future<void>);
 
+  @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<void> watchJmapPush(
     String? accountId,

test/unit/reliability_runner_test.dart

@@ -141,6 +141,12 @@ class _CountingEmails implements EmailRepository {
   @override
   Future<void> restoreEmails(List<Email> emails) async {}
   @override
+  Future<Email?> findEmailByMessageId(
+    String accountId,
+    String messageId,
+  ) async =>
+      null;
+  @override
   Stream<String> get onChangesQueued => const Stream.empty();
   @override
   Stream<void> watchJmapPush(String accountId, String password) =>

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<String, dynamic>;
+      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);

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'));
   });

test/unit/undo_service_test.mocks.dart

@@ -466,6 +466,22 @@ class MockEmailRepository extends _i1.Mock implements _i3.EmailRepository {
         returnValueForMissingStub: _i4.Future<void>.value(),
       ) as _i4.Future<void>);
 
+  @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<void> watchJmapPush(
     String? accountId,

test/widget/helpers.dart

@@ -232,6 +232,13 @@ class FakeEmailRepository implements EmailRepository {
   @override
   Future<void> restoreEmails(List<Email> emails) async {}
 
+  @override
+  Future<Email?> findEmailByMessageId(
+    String accountId,
+    String messageId,
+  ) async =>
+      null;
+
   @override
   Future<String?> deleteEmail(String emailId) async => null;