diff --git a/app/changelog/unreleased/task-indent-persistence.json b/app/changelog/unreleased/task-indent-persistence.json new file mode 100644 index 000000000000..91fa012341f5 --- /dev/null +++ b/app/changelog/unreleased/task-indent-persistence.json @@ -0,0 +1 @@ +{"change": "Indented sub-tasks and reordered tasks keep their position when the list refreshes, and a failed save is retried instead of being lost."} diff --git a/app/lib/providers/action_items_provider.dart b/app/lib/providers/action_items_provider.dart index a5d65bfa872a..fac37212235a 100644 --- a/app/lib/providers/action_items_provider.dart +++ b/app/lib/providers/action_items_provider.dart @@ -22,6 +22,7 @@ import 'package:omi/utils/logger.dart'; import 'package:omi/utils/platform/platform_service.dart'; import 'package:omi/utils/analytics/product_telemetry.dart'; import 'package:omi/ui/feedback/omi_feedback.dart'; +import 'package:omi/providers/task_layout_writes.dart'; import 'package:omi/providers/task_suggestions_controller.dart'; typedef ActionItemsFetcher = Future Function({ @@ -57,6 +58,7 @@ class ActionItemsProvider extends ChangeNotifier { BulkDeleteActionItemsRequest? bulkDeleteActionItemsRequest, CreateActionItemRequest? createActionItemRequest, UpdateDueDateRequest? updateDueDateRequest, + BatchUpdateActionItemsRequest? batchUpdateActionItemsRequest, api.ActionItemsApi? actionItemsApi, TaskSuggestionsApi? taskSuggestionsApi, AutomaticAppleRemindersExport? automaticAppleRemindersExport, @@ -67,6 +69,7 @@ class ActionItemsProvider extends ChangeNotifier { _bulkDeleteActionItemsRequest = bulkDeleteActionItemsRequest ?? api.bulkDeleteActionItems, _createActionItemRequest = createActionItemRequest ?? api.createActionItem, _updateDueDateRequest = updateDueDateRequest ?? api.updateActionItem, + _layoutWrites = TaskLayoutWrites(batchUpdateActionItemsRequest ?? api.batchUpdateActionItems), _actionItemsApi = actionItemsApi, _automaticAppleRemindersExport = automaticAppleRemindersExport ?? AutomaticAppleRemindersExport(), _exportActionItem = exportActionItem ?? ActionItemExportService.export { @@ -83,6 +86,7 @@ class ActionItemsProvider extends ChangeNotifier { final DeleteActionItemRequest _deleteActionItemRequest; final UpdateActionItemRequest _updateActionItemRequest; final BulkDeleteActionItemsRequest _bulkDeleteActionItemsRequest; + final TaskLayoutWrites _layoutWrites; final CreateActionItemRequest _createActionItemRequest; final UpdateDueDateRequest _updateDueDateRequest; final api.ActionItemsApi? _actionItemsApi; @@ -123,12 +127,6 @@ class ActionItemsProvider extends ChangeNotifier { DateTime? _lastRefreshTime; static const Duration _refreshCooldown = Duration(seconds: 30); - // Debounced persistence for sort order and indent level - final Map _pendingSortUpdates = {}; - final Map _pendingIndentUpdates = {}; - Timer? _sortDebounce; - Timer? _indentDebounce; - // Multi-selection state bool _isSelectionMode = false; Set _selectedItems = {}; @@ -307,6 +305,7 @@ class ActionItemsProvider extends ChangeNotifier { final writeRevision = _taskWriteRevision; final readOrdinal = ++_listReadOrdinal; _activeListReads.add(readOrdinal); + final layoutMark = _layoutWrites.readMark; final startOfTomorrow = DateTime(now.year, now.month, now.day + 1); final sevenDaysAgo = now.subtract(const Duration(days: 7)); try { @@ -318,11 +317,13 @@ class ActionItemsProvider extends ChangeNotifier { dueEndDate: startOfTomorrow.subtract(const Duration(microseconds: 1)), ); if (response != null && generation == _sessionGeneration && writeRevision == _taskWriteRevision) { - _homeDayItems = _pendingDeletionIds.isEmpty - ? List.of(response.actionItems) - : response.actionItems.where((item) => !_pendingDeletionIds.contains(item.id)).toList(); + _homeDayItems = response.actionItems + .where((item) => !_pendingDeletionIds.contains(item.id)) + .map(_layoutWrites.overlayFor(layoutMark)) + .toList(); SiriIntegration.current.queueUpsertTasks(_homeDayItems); _homeDayLoaded = true; + _layoutWrites.retryPending(); } } catch (e) { Logger.debug('Error fetching home today tasks: $e'); @@ -414,6 +415,7 @@ class ActionItemsProvider extends ChangeNotifier { final readOrdinal = ++_listReadOrdinal; _latestTaskListRead = readOrdinal; _activeListReads.add(readOrdinal); + final layoutMark = _layoutWrites.readMark; final query = (includeCompleted: _includeCompleted, startDate: _startDate, endDate: _endDate); bool ownsQuery() => !_disposed && @@ -448,6 +450,7 @@ class ActionItemsProvider extends ChangeNotifier { response, decodedPageIsComplete: decodedPageIsComplete, readOrdinal: readOrdinal, + layoutMark: layoutMark, ); loaded = true; } @@ -491,6 +494,7 @@ class ActionItemsProvider extends ChangeNotifier { ActionItemsResponse response, { required bool decodedPageIsComplete, required int readOrdinal, + required int layoutMark, }) async { // Snapshot server IDs before filtering so tombstone retirement is // based on the full server response, not the filtered subset. @@ -499,7 +503,9 @@ class ActionItemsProvider extends ChangeNotifier { // in-place: the response list may be unmodifiable, and aliasing it // would cause removeWhere to throw in deleteActionItem/deleteSelectedItems // and retire tombstones prematurely. - _actionItems = TaskCategorizationList(response.actionItems.where((item) => !_pendingDeletionIds.contains(item.id))); + _actionItems = TaskCategorizationList(response.actionItems + .where((item) => !_pendingDeletionIds.contains(item.id)) + .map(_layoutWrites.overlayFor(layoutMark))); _loadedTaskPageIds ..clear() ..addAll(serverIds); @@ -539,6 +545,7 @@ class ActionItemsProvider extends ChangeNotifier { _showCompletedView = true; _completedViewWasAutoRevealed = true; } + _layoutWrites.retryPending(); } void _revealNewIncompleteTasks() { @@ -555,6 +562,7 @@ class ActionItemsProvider extends ChangeNotifier { final writeRevision = _taskWriteRevision; final readOrdinal = ++_listReadOrdinal; _activeListReads.add(readOrdinal); + final layoutMark = _layoutWrites.readMark; setFetching(true); @@ -585,9 +593,11 @@ class ActionItemsProvider extends ChangeNotifier { final seenIds = _actionItems.map((item) => item.id).toSet(); final filtered = response.actionItems .where((item) => !_pendingDeletionIds.contains(item.id) && seenIds.add(item.id)) + .map(_layoutWrites.overlayFor(layoutMark)) .toList(); _actionItems.addAll(filtered); _revealNewIncompleteTasks(); + _layoutWrites.retryPending(); _hasMore = response.hasMore; if (!_hasMore && !hasActiveFilter && @@ -1003,7 +1013,7 @@ class ActionItemsProvider extends ChangeNotifier { if (newItem != null) { _actionItems.removeWhere((item) => item.id == optimisticItem.id); - _receiveCanonicalTask(newItem); + _receiveCanonicalTask(newItem, _layoutWrites.readMark); unawaited(() async { final exported = await _automaticAppleRemindersExport.export(newItem, canContinue: () => !_disposed && generation == _sessionGeneration); @@ -1033,17 +1043,20 @@ class ActionItemsProvider extends ChangeNotifier { final generation = _sessionGeneration; final typedApi = _actionItemsApi; if (typedApi == null || _disposed) return false; + final layoutMark = _layoutWrites.readMark; final result = await typedApi.getById(id); if (_disposed || generation != _sessionGeneration) return false; if (result case ApiSuccess(:final data)) { if (data.id != id) return false; - _receiveCanonicalTask(data); + _receiveCanonicalTask(data, layoutMark); + _layoutWrites.retryPending(); return true; } return false; } - void _receiveCanonicalTask(ActionItemWithMetadata item) { + void _receiveCanonicalTask(ActionItemWithMetadata item, int layoutMark) { + item = _layoutWrites.overlayFor(layoutMark)(item); _taskWriteRevision++; if (_pendingDeletionIds.contains(item.id)) return; // An explicit Add Task gesture should reveal its new incomplete task, @@ -1098,46 +1111,22 @@ class ActionItemsProvider extends ChangeNotifier { void updateItemSortOrder(String id, int sortOrder) { _updateItemInPlace(id, sortOrder: sortOrder); - _pendingSortUpdates[id] = sortOrder; + _layoutWrites.setSortOrder(id, sortOrder); notifyListeners(); - _sortDebounce?.cancel(); - _sortDebounce = Timer(const Duration(milliseconds: 500), _flushSortUpdates); } void updateItemIndentLevel(String id, int indentLevel) { _updateItemInPlace(id, indentLevel: indentLevel); - _pendingIndentUpdates[id] = indentLevel; + _layoutWrites.setIndentLevel(id, indentLevel); notifyListeners(); - _indentDebounce?.cancel(); - _indentDebounce = Timer(const Duration(milliseconds: 500), _flushIndentUpdates); } void batchUpdateSortOrders(Map updates) { for (final entry in updates.entries) { _updateItemInPlace(entry.key, sortOrder: entry.value); - _pendingSortUpdates[entry.key] = entry.value; + _layoutWrites.setSortOrder(entry.key, entry.value); } notifyListeners(); - _sortDebounce?.cancel(); - _sortDebounce = Timer(const Duration(milliseconds: 500), _flushSortUpdates); - } - - Future _flushSortUpdates() async { - if (_pendingSortUpdates.isEmpty) return; - final updates = Map.from(_pendingSortUpdates); - _pendingSortUpdates.clear(); - - final items = updates.entries.map((e) => {'id': e.key, 'sort_order': e.value}).toList(); - await api.batchUpdateActionItems(items); - } - - Future _flushIndentUpdates() async { - if (_pendingIndentUpdates.isEmpty) return; - final updates = Map.from(_pendingIndentUpdates); - _pendingIndentUpdates.clear(); - - final items = updates.entries.map((e) => {'id': e.key, 'indent_level': e.value}).toList(); - await api.batchUpdateActionItems(items); } ActionItemWithMetadata? _findAndUpdateItemState(String itemId, bool newState) { @@ -1324,8 +1313,7 @@ class ActionItemsProvider extends ChangeNotifier { _pendingDeletionIds.clear(); _deletionAcknowledgedAtRead.clear(); _activeListReads.clear(); - _pendingSortUpdates.clear(); - _pendingIndentUpdates.clear(); + _layoutWrites.clear(); suggestions?.reset(); notifyListeners(); } @@ -1524,10 +1512,7 @@ class ActionItemsProvider extends ChangeNotifier { suggestions?.removeListener(notifyListeners); suggestions?.dispose(); _refreshDebounceTimer?.cancel(); - _sortDebounce?.cancel(); - _indentDebounce?.cancel(); - _flushSortUpdates(); - _flushIndentUpdates(); + _layoutWrites.flush(); super.dispose(); } } diff --git a/app/lib/providers/task_layout_writes.dart b/app/lib/providers/task_layout_writes.dart new file mode 100644 index 000000000000..ee3b5e113edb --- /dev/null +++ b/app/lib/providers/task_layout_writes.dart @@ -0,0 +1,135 @@ +import 'dart:async'; + +import 'package:omi/backend/schema/schema.dart'; +import 'package:omi/utils/logger.dart'; + +// Owns debounced persistence of task sort order and indent level. +// +// An edit stays pending until the server acknowledges that exact value, so a +// list read cannot revert it on screen and a failed write is retried. One +// write is in flight at a time, so an older layout cannot land after a newer +// one. Acknowledged values are still overlaid on reads that started before the +// acknowledgement, because their server snapshot may predate the write. + +typedef BatchUpdateActionItemsRequest = Future Function(List> items); + +class TaskLayoutWrites { + TaskLayoutWrites(this._send); + + final BatchUpdateActionItemsRequest _send; + final Map _sortOrders = {}; + final Map _indentLevels = {}; + // ponytail: kept for the session (one entry per edited task); prune by in-flight read marks if this ever grows. + final Map _ackedSortOrders = {}; + final Map _ackedIndentLevels = {}; + int _acks = 0; + Timer? _debounce; + Future? _flush; + bool _flushRequestedDuringSend = false; + int _generation = 0; + + /// Capture before a task read starts; pass to [overlayFor] for its rows. + int get readMark => _acks; + + void setSortOrder(String id, int sortOrder) { + _sortOrders[id] = sortOrder; + _schedule(); + } + + void setIndentLevel(String id, int indentLevel) { + _indentLevels[id] = indentLevel; + _schedule(); + } + + /// Re-applies unacknowledged edits, and edits acknowledged after [readMark], + /// to rows from a read that started at [readMark]. + ActionItemWithMetadata Function(ActionItemWithMetadata) overlayFor(int readMark) => (item) { + final ackedSort = _ackedSortOrders[item.id]; + final ackedIndent = _ackedIndentLevels[item.id]; + final sortOrder = + _sortOrders[item.id] ?? (ackedSort != null && ackedSort.ack > readMark ? ackedSort.value : null); + final indentLevel = + _indentLevels[item.id] ?? (ackedIndent != null && ackedIndent.ack > readMark ? ackedIndent.value : null); + if (sortOrder == null && indentLevel == null) return item; + return item.copyWith(sortOrder: sortOrder, indentLevel: indentLevel); + }; + + /// Called after any successful task read: the server is reachable again. + void retryPending() { + if (_sortOrders.isEmpty && _indentLevels.isEmpty) return; + // A send in flight may still fail; remember that the server answered meanwhile. + if (_flush != null) { + _flushRequestedDuringSend = true; + } else { + _schedule(); + } + } + + /// Drops pending edits, e.g. on sign-out; an in-flight write cannot restore them. + void clear() { + _generation++; + _debounce?.cancel(); + _sortOrders.clear(); + _indentLevels.clear(); + _ackedSortOrders.clear(); + _ackedIndentLevels.clear(); + } + + Future flush() { + _debounce?.cancel(); + if (_flush != null) { + _flushRequestedDuringSend = true; + return _flush!; + } + return _flush = _sendPending().whenComplete(() { + _flush = null; + // An edit made while the send was in flight lost its timer to this send. + // If the send failed, give that edit its own attempt. + final retry = _flushRequestedDuringSend && (_sortOrders.isNotEmpty || _indentLevels.isNotEmpty); + _flushRequestedDuringSend = false; + if (retry) _schedule(); + }); + } + + void _schedule() { + _debounce?.cancel(); + _debounce = Timer(const Duration(milliseconds: 500), flush); + } + + Future _sendPending() async { + final generation = _generation; + while (generation == _generation) { + final sortOrders = Map.of(_sortOrders); + final indentLevels = Map.of(_indentLevels); + if (sortOrders.isEmpty && indentLevels.isEmpty) return; + + final items = [ + for (final id in {...sortOrders.keys, ...indentLevels.keys}) + { + 'id': id, + if (sortOrders.containsKey(id)) 'sort_order': sortOrders[id], + if (indentLevels.containsKey(id)) 'indent_level': indentLevels[id], + }, + ]; + bool saved; + try { + saved = await _send(items); + } catch (e) { + Logger.debug('Error saving task layout: $e'); + saved = false; + } + // Failed edits stay pending; the next edit or list read retries them. + if (!saved || generation != _generation) return; + _acks++; + sortOrders.forEach((id, value) { + if (_sortOrders[id] == value) _sortOrders.remove(id); + _ackedSortOrders[id] = (value: value, ack: _acks); + }); + indentLevels.forEach((id, value) { + if (_indentLevels[id] == value) _indentLevels.remove(id); + _ackedIndentLevels[id] = (value: value, ack: _acks); + }); + _flushRequestedDuringSend = false; + } + } +} diff --git a/app/test/providers/action_items_layout_persistence_test.dart b/app/test/providers/action_items_layout_persistence_test.dart new file mode 100644 index 000000000000..29ea8eb7d935 --- /dev/null +++ b/app/test/providers/action_items_layout_persistence_test.dart @@ -0,0 +1,195 @@ +import 'dart:async'; + +import 'package:flutter_test/flutter_test.dart'; +import 'package:shared_preferences/shared_preferences.dart'; + +import 'package:omi/backend/schema/schema.dart'; +import 'package:omi/providers/action_items_provider.dart'; + +ActionItemWithMetadata _item(String id, {int indent = 0}) => + ActionItemWithMetadata(id: id, description: id, completed: false, indentLevel: indent); + +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + setUp(() => SharedPreferences.setMockInitialValues({})); + + late List serverRows; + late List>> writes; + late List> receipts; + // When set, the next read takes its server snapshot now but resolves only when completed. + Completer? holdRead; + + Future provider() async { + serverRows = [_item('parent'), _item('child')]; + writes = []; + receipts = []; + holdRead = null; + final p = ActionItemsProvider( + getActionItems: ({ + int limit = 100, + int offset = 0, + bool? completed, + String? conversationId, + DateTime? startDate, + DateTime? endDate, + DateTime? dueStartDate, + DateTime? dueEndDate, + }) async { + final snapshot = List.of(serverRows); + final hold = holdRead; + holdRead = null; + if (hold != null) await hold.future; + return ActionItemsResponse(actionItems: snapshot); + }, + batchUpdateActionItemsRequest: (items) { + writes.add(items); + final receipt = Completer(); + receipts.add(receipt); + return receipt.future; + }, + ); + await p.ensureLoaded(); + addTearDown(p.dispose); + return p; + } + + int indentOf(ActionItemsProvider p, String id) => p.actionItems.firstWhere((item) => item.id == id).indentLevel; + + Future debounce() => Future.delayed(const Duration(milliseconds: 600)); + + test('a list read during an unsaved indent does not revert the sub-task', () async { + final p = await provider(); + + p.updateItemIndentLevel('child', 1); + await p.fetchActionItems(); // server still has indent 0 + expect(indentOf(p, 'child'), 1); + + await debounce(); + expect(writes, [ + [ + {'id': 'child', 'indent_level': 1} + ] + ]); + await p.fetchActionItems(); // write still in flight + expect(indentOf(p, 'child'), 1); + + serverRows = [_item('parent'), _item('child', indent: 1)]; + receipts.single.complete(true); + await Future.delayed(Duration.zero); + await p.fetchActionItems(); + expect(indentOf(p, 'child'), 1); + expect(writes, hasLength(1)); + }); + + test('a failed indent write stays visible and is retried after the next list read', () async { + final p = await provider(); + + p.updateItemIndentLevel('child', 1); + await debounce(); + receipts.single.complete(false); + await Future.delayed(Duration.zero); + + await p.fetchActionItems(); + expect(indentOf(p, 'child'), 1); + + await debounce(); + expect(writes, hasLength(2)); + expect(writes.last, [ + {'id': 'child', 'indent_level': 1} + ]); + serverRows = [_item('parent'), _item('child', indent: 1)]; + receipts.last.complete(true); + await Future.delayed(Duration.zero); + + await p.fetchActionItems(); + await debounce(); + expect(writes, hasLength(2), reason: 'an acknowledged indent is not written again'); + }); + + test('an older acknowledgement cannot clear a newer indent or race it to the server', () async { + final p = await provider(); + + p.updateItemIndentLevel('child', 1); + await debounce(); + p.updateItemIndentLevel('child', 2); + await debounce(); + expect(writes, hasLength(1), reason: 'one layout write at a time'); + + receipts.first.complete(true); + await Future.delayed(Duration.zero); + expect(writes, hasLength(2)); + expect(writes.last, [ + {'id': 'child', 'indent_level': 2} + ]); + + await p.fetchActionItems(); // server still has the first value + expect(indentOf(p, 'child'), 2); + receipts.last.complete(true); + }); + + test('an edit made while a failing write is in flight is still sent', () async { + final p = await provider(); + + p.updateItemIndentLevel('child', 1); + await debounce(); + p.updateItemIndentLevel('child', 2); + await debounce(); // its timer fires while the first write is in flight + expect(writes, hasLength(1)); + + receipts.first.complete(false); + await debounce(); + expect(writes, hasLength(2), reason: 'no further edit or list read is needed'); + expect(writes.last, [ + {'id': 'child', 'indent_level': 2} + ]); + receipts.last.complete(true); + }); + + test('a read whose snapshot predates the acknowledgement cannot revert the indent', () async { + final p = await provider(); + + p.updateItemIndentLevel('child', 1); + await debounce(); + final released = Completer(); + holdRead = released; + final read = p.fetchActionItems(); // snapshot taken now: indent 0 + await Future.delayed(Duration.zero); + + serverRows = [_item('parent'), _item('child', indent: 1)]; + receipts.single.complete(true); + await Future.delayed(Duration.zero); + released.complete(); + await read; + expect(indentOf(p, 'child'), 1); + + await p.fetchActionItems(); // a read after the acknowledgement uses the server value + expect(indentOf(p, 'child'), 1); + }); + + test('a successful Home read retries a failed indent write', () async { + final p = await provider(); + + p.updateItemIndentLevel('child', 1); + await debounce(); + receipts.single.complete(false); + await Future.delayed(Duration.zero); + + await p.ensureHomeTodayTasksLoaded(); + await debounce(); + expect(writes, hasLength(2)); + receipts.last.complete(true); + }); + + test('a successful read during a write that then fails still retries it', () async { + final p = await provider(); + + p.updateItemIndentLevel('child', 1); + await debounce(); + await p.fetchActionItems(); // succeeds while the write is in flight + receipts.single.complete(false); + await debounce(); + expect(writes, hasLength(2), reason: 'the read proved the server reachable'); + receipts.last.complete(true); + }); +}