From 4eca1b5ac849b1e3b7780ed9b4ad069e81fe3a9b Mon Sep 17 00:00:00 2001 From: Thomas SharedInbox Date: Wed, 13 May 2026 22:30:58 +0200 Subject: [PATCH] fix(undo): await init before mutating history, add persistence tests Race condition: pushAction/clear/undo could run before init() loaded DB history, causing persisted state to overwrite newer actions on load. Now all mutating methods await the _ready future set by init(). Three new tests cover restart persistence and the concurrent init+push race. Co-Authored-By: Claude Sonnet 4.6 --- lib/core/services/undo_service.dart | 17 +++- lib/ui/screens/email_detail_screen.dart | 44 ++++++----- lib/ui/screens/email_list_screen.dart | 16 ++-- lib/ui/screens/thread_detail_screen.dart | 22 +++--- lib/ui/screens/undo_log_screen.dart | 5 +- test/unit/undo_logic_test.dart | 8 +- test/unit/undo_service_test.dart | 98 ++++++++++++++++++++++-- 7 files changed, 157 insertions(+), 53 deletions(-) diff --git a/lib/core/services/undo_service.dart b/lib/core/services/undo_service.dart index 6d81af0..aceb255 100644 --- a/lib/core/services/undo_service.dart +++ b/lib/core/services/undo_service.dart @@ -10,12 +10,19 @@ class UndoService extends StateNotifier> { final Ref _ref; static const int _maxHistory = 10; + // Resolves once init() has loaded persisted history. Default to an already- + // resolved future so operations are safe even if init() is never called. + Future _ready = Future.value(); + Future init() async { - final repo = _ref.read(undoRepositoryProvider); - state = await repo.getHistory(); + _ready = _ref.read(undoRepositoryProvider).getHistory().then((history) { + if (mounted) state = history; + }); + await _ready; } - void pushAction(UndoAction action) { + Future pushAction(UndoAction action) async { + await _ready; final newList = [...state, action]; if (newList.length > _maxHistory) { final removed = newList.removeAt(0); @@ -25,12 +32,14 @@ class UndoService extends StateNotifier> { unawaited(_ref.read(undoRepositoryProvider).saveAction(action)); } - void clear() { + Future clear() async { + await _ready; state = []; unawaited(_ref.read(undoRepositoryProvider).clearHistory()); } Future undo({String? actionId}) async { + await _ready; if (state.isEmpty) return; final UndoAction action; diff --git a/lib/ui/screens/email_detail_screen.dart b/lib/ui/screens/email_detail_screen.dart index bbbf051..137ec18 100644 --- a/lib/ui/screens/email_detail_screen.dart +++ b/lib/ui/screens/email_detail_screen.dart @@ -123,17 +123,19 @@ class _EmailDetailScreenState extends ConsumerState { final destPath = await repo.deleteEmail(widget.emailId); if (header != null) { - ref.read(undoServiceProvider.notifier).pushAction( - UndoAction( - id: DateTime.now().toIso8601String(), - accountId: header.accountId, - type: UndoType.delete, - emailIds: [widget.emailId], - sourceMailboxPath: header.mailboxPath, - destinationMailboxPath: destPath, - originalEmails: [header], + unawaited( + ref.read(undoServiceProvider.notifier).pushAction( + UndoAction( + id: DateTime.now().toIso8601String(), + accountId: header.accountId, + type: UndoType.delete, + emailIds: [widget.emailId], + sourceMailboxPath: header.mailboxPath, + destinationMailboxPath: destPath, + originalEmails: [header], + ), ), - ); + ); } if (context.mounted) context.pop(); @@ -354,16 +356,18 @@ class _EmailDetailScreenState extends ConsumerState { await ref.read(emailRepositoryProvider).moveEmail(widget.emailId, chosen); - ref.read(undoServiceProvider.notifier).pushAction( - UndoAction( - id: DateTime.now().toIso8601String(), - accountId: header.accountId, - type: UndoType.move, - emailIds: [widget.emailId], - sourceMailboxPath: header.mailboxPath, - destinationMailboxPath: chosen, + unawaited( + ref.read(undoServiceProvider.notifier).pushAction( + UndoAction( + id: DateTime.now().toIso8601String(), + accountId: header.accountId, + type: UndoType.move, + emailIds: [widget.emailId], + sourceMailboxPath: header.mailboxPath, + destinationMailboxPath: chosen, + ), ), - ); + ); if (context.mounted) context.pop(); } @@ -384,7 +388,7 @@ class _EmailDetailScreenState extends ConsumerState { sourceMailboxPath: header.mailboxPath, originalEmails: [header], ); - ref.read(undoServiceProvider.notifier).pushAction(action); + unawaited(ref.read(undoServiceProvider.notifier).pushAction(action)); await repo.snoozeEmail(widget.emailId, until); if (context.mounted) { diff --git a/lib/ui/screens/email_list_screen.dart b/lib/ui/screens/email_list_screen.dart index 7477721..a2b65bb 100644 --- a/lib/ui/screens/email_list_screen.dart +++ b/lib/ui/screens/email_list_screen.dart @@ -331,7 +331,7 @@ class _EmailListScreenState extends ConsumerState { destinationMailboxPath: mailbox.path, originalEmails: originalEmails, ); - ref.read(undoServiceProvider.notifier).pushAction(action); + unawaited(ref.read(undoServiceProvider.notifier).pushAction(action)); } Future _batchArchive() => @@ -364,7 +364,7 @@ class _EmailListScreenState extends ConsumerState { destinationMailboxPath: lastDestPath, originalEmails: originalEmails, ); - ref.read(undoServiceProvider.notifier).pushAction(action); + unawaited(ref.read(undoServiceProvider.notifier).pushAction(action)); } Future _batchMarkSpam() => @@ -426,7 +426,7 @@ class _EmailListScreenState extends ConsumerState { destinationMailboxPath: chosen, originalEmails: originalEmails, ); - ref.read(undoServiceProvider.notifier).pushAction(action); + unawaited(ref.read(undoServiceProvider.notifier).pushAction(action)); } Future _batchSnooze() async { @@ -458,7 +458,7 @@ class _EmailListScreenState extends ConsumerState { sourceMailboxPath: widget.mailboxPath, originalEmails: originalEmails, ); - ref.read(undoServiceProvider.notifier).pushAction(action); + unawaited(ref.read(undoServiceProvider.notifier).pushAction(action)); if (!mounted) return; @@ -609,7 +609,9 @@ class _EmailListScreenState extends ConsumerState { destinationMailboxPath: archive.path, originalEmails: originalEmails, ); - ref.read(undoServiceProvider.notifier).pushAction(action); + unawaited( + ref.read(undoServiceProvider.notifier).pushAction(action), + ); } else { String? lastDestPath; for (final id in t.emailIds) { @@ -625,7 +627,9 @@ class _EmailListScreenState extends ConsumerState { destinationMailboxPath: lastDestPath, originalEmails: originalEmails, ); - ref.read(undoServiceProvider.notifier).pushAction(action); + unawaited( + ref.read(undoServiceProvider.notifier).pushAction(action), + ); } }, child: tile, diff --git a/lib/ui/screens/thread_detail_screen.dart b/lib/ui/screens/thread_detail_screen.dart index a97a7f6..7872653 100644 --- a/lib/ui/screens/thread_detail_screen.dart +++ b/lib/ui/screens/thread_detail_screen.dart @@ -256,17 +256,19 @@ class _EmailMessageCardState extends ConsumerState<_EmailMessageCard> { final destPath = await repo.deleteEmail(widget.email.id); if (original != null) { - ref.read(undoServiceProvider.notifier).pushAction( - UndoAction( - id: DateTime.now().toIso8601String(), - accountId: widget.email.accountId, - type: UndoType.delete, - emailIds: [widget.email.id], - sourceMailboxPath: widget.email.mailboxPath, - destinationMailboxPath: destPath, - originalEmails: [original], + unawaited( + ref.read(undoServiceProvider.notifier).pushAction( + UndoAction( + id: DateTime.now().toIso8601String(), + accountId: widget.email.accountId, + type: UndoType.delete, + emailIds: [widget.email.id], + sourceMailboxPath: widget.email.mailboxPath, + destinationMailboxPath: destPath, + originalEmails: [original], + ), ), - ); + ); } } } diff --git a/lib/ui/screens/undo_log_screen.dart b/lib/ui/screens/undo_log_screen.dart index cf8c09f..789e893 100644 --- a/lib/ui/screens/undo_log_screen.dart +++ b/lib/ui/screens/undo_log_screen.dart @@ -1,3 +1,5 @@ +import 'dart:async'; + import 'package:flutter/material.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:intl/intl.dart'; @@ -22,7 +24,8 @@ class UndoLogScreen extends ConsumerWidget { tooltip: 'Clear history', onPressed: history.isEmpty ? null - : () => ref.read(undoServiceProvider.notifier).clear(), + : () => + unawaited(ref.read(undoServiceProvider.notifier).clear()), ), ], ), diff --git a/test/unit/undo_logic_test.dart b/test/unit/undo_logic_test.dart index 3eed958..e3f1282 100644 --- a/test/unit/undo_logic_test.dart +++ b/test/unit/undo_logic_test.dart @@ -109,7 +109,7 @@ void main() { sourceMailboxPath: 'INBOX', originalEmails: [original!], ); - container.read(undoServiceProvider.notifier).pushAction(action); + await container.read(undoServiceProvider.notifier).pushAction(action); await container.read(undoServiceProvider.notifier).undo(); // 3. Verify it is back in Inbox @@ -190,7 +190,7 @@ void main() { emailIds: [emailId], sourceMailboxPath: 'INBOX', ); - container.read(undoServiceProvider.notifier).pushAction(action); + await container.read(undoServiceProvider.notifier).pushAction(action); await container.read(undoServiceProvider.notifier).undo(); // 3. Verify it is back in Inbox @@ -230,7 +230,7 @@ void main() { destinationMailboxPath: destPath, originalEmails: [original!], ); - container.read(undoServiceProvider.notifier).pushAction(action); + await container.read(undoServiceProvider.notifier).pushAction(action); await container.read(undoServiceProvider.notifier).undo(); // 4. Verify local state @@ -273,7 +273,7 @@ void main() { sourceMailboxPath: 'INBOX', originalEmails: [original!], ); - container.read(undoServiceProvider.notifier).pushAction(action); + await container.read(undoServiceProvider.notifier).pushAction(action); await container.read(undoServiceProvider.notifier).undo(); // 3. Verify it is back in Inbox and metadata is cleared diff --git a/test/unit/undo_service_test.dart b/test/unit/undo_service_test.dart index 277bada..581fb8e 100644 --- a/test/unit/undo_service_test.dart +++ b/test/unit/undo_service_test.dart @@ -61,10 +61,10 @@ void main() { final notifier = container.read(undoServiceProvider.notifier); await notifier.init(); // Wait for persistent load - notifier.pushAction(action1); + await notifier.pushAction(action1); expect(container.read(undoServiceProvider), [action1]); - notifier.pushAction(action2); + await notifier.pushAction(action2); expect(container.read(undoServiceProvider), [action1, action2]); }); @@ -91,8 +91,8 @@ void main() { final notifier = container.read(undoServiceProvider.notifier); await notifier.init(); - notifier.pushAction(action1); - notifier.pushAction(action2); + await notifier.pushAction(action1); + await notifier.pushAction(action2); await notifier.undo(); expect(container.read(undoServiceProvider), [action1]); @@ -126,8 +126,8 @@ void main() { final notifier = container.read(undoServiceProvider.notifier); await notifier.init(); - notifier.pushAction(action1); - notifier.pushAction(action2); + await notifier.pushAction(action1); + await notifier.pushAction(action2); await notifier.undo(actionId: '1'); expect(container.read(undoServiceProvider), [action2]); @@ -154,7 +154,7 @@ void main() { final notifier = container.read(undoServiceProvider.notifier); await notifier.init(); - notifier.pushAction(action); + await notifier.pushAction(action); await notifier.undo(); verify(mockEmailRepo.moveEmail('e1', 'INBOX')).called(1); @@ -193,11 +193,93 @@ void main() { final notifier = container.read(undoServiceProvider.notifier); await notifier.init(); - notifier.pushAction(action); + await notifier.pushAction(action); await notifier.undo(); verify(mockEmailRepo.restoreEmails(any)).called(1); verify(mockEmailRepo.moveEmail('e1', 'INBOX')).called(1); }); + + test('init loads persisted history from repository', () async { + final persisted = UndoAction( + id: '99', + accountId: 'acc1', + type: UndoType.move, + emailIds: ['e99'], + sourceMailboxPath: 'INBOX', + ); + + when( + mockUndoRepo.getHistory(limit: anyNamed('limit')), + ).thenAnswer((_) async => [persisted]); + + final notifier = container.read(undoServiceProvider.notifier); + await notifier.init(); + + expect(container.read(undoServiceProvider), [persisted]); + }); + + test('pushAction after restart appends to persisted history', () async { + final persisted = UndoAction( + id: '1', + accountId: 'acc1', + type: UndoType.move, + emailIds: ['e1'], + sourceMailboxPath: 'INBOX', + ); + final newAction = UndoAction( + id: '2', + accountId: 'acc1', + type: UndoType.delete, + emailIds: ['e2'], + sourceMailboxPath: 'INBOX', + ); + + when( + mockUndoRepo.getHistory(limit: anyNamed('limit')), + ).thenAnswer((_) async => [persisted]); + + final notifier = container.read(undoServiceProvider.notifier); + await notifier.init(); + await notifier.pushAction(newAction); + + expect(container.read(undoServiceProvider), [persisted, newAction]); + }); + + test('pushAction concurrent with init waits for init to complete', () async { + final persisted = UndoAction( + id: '1', + accountId: 'acc1', + type: UndoType.move, + emailIds: ['e1'], + sourceMailboxPath: 'INBOX', + ); + final raced = UndoAction( + id: '2', + accountId: 'acc1', + type: UndoType.delete, + emailIds: ['e2'], + sourceMailboxPath: 'INBOX', + ); + + // Simulate slow DB load + when( + mockUndoRepo.getHistory(limit: anyNamed('limit')), + ).thenAnswer( + (_) => Future.delayed( + const Duration(milliseconds: 10), + () => [persisted], + ), + ); + + final notifier = container.read(undoServiceProvider.notifier); + final initFuture = notifier.init(); + // pushAction issued before init completes — it must still see persisted history + final pushFuture = notifier.pushAction(raced); + + await Future.wait([initFuture, pushFuture]); + + expect(container.read(undoServiceProvider), [persisted, raced]); + }); }