From b286bbbdfa7211fb8256b9f5e84ac89d6e1a8b60 Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 13:14:32 -0400 Subject: [PATCH 01/11] Copy the page on screen when "+" is pressed on a cloud strategy On a local strategy "+" adds a copy of the page on screen; on a cloud strategy it added a blank page, because the server keeps item ids unique across every strategy. The new cloud page now gets a copy of every item and lineup as the canvas draws them, each under a copy id that keeps its root, so turning to the page glides rather than fades. Images have their pictures copied first (images:copyAsset); one that can't be copied yet is left out and the "+" caller says so. The page opens once the copies land, waiting at most three seconds. A copied stroke now counts as its original when deciding whether the drawing layer changes between pages. Co-Authored-By: Claude Opus 5.5 --- lib/page_transition/transition_planner.dart | 17 +- .../active_page_live_sync_provider.dart | 15 ++ lib/providers/strategy_provider.dart | 223 ++++++++++++++---- lib/widgets/global_shortcuts.dart | 5 +- lib/widgets/new_page_copy_toast.dart | 23 ++ lib/widgets/pages_bar.dart | 3 +- test/page_copy_id_test.dart | 39 +++ test/strategy_page_session_provider_test.dart | 123 ++++++++++ 8 files changed, 403 insertions(+), 45 deletions(-) create mode 100644 lib/widgets/new_page_copy_toast.dart diff --git a/lib/page_transition/transition_planner.dart b/lib/page_transition/transition_planner.dart index cc650e10..0384efa9 100644 --- a/lib/page_transition/transition_planner.dart +++ b/lib/page_transition/transition_planner.dart @@ -1,3 +1,5 @@ +import 'dart:convert'; + import 'package:icarus/const/drawing_element.dart'; import 'package:icarus/const/page_copy_id.dart'; import 'package:icarus/const/placed_classes.dart'; @@ -121,14 +123,23 @@ class TransitionPlanner { /// Whether the drawing layer changes between two pages, and therefore /// whether it should fade in early during the transition. Compares the /// serialized form so geometry/style edits count, not just added or - /// removed strokes. + /// removed strokes. A copied stroke counts as its original (see + /// page_copy_id.dart): a cloud page copy gives each its own id. static bool drawingsChanged( List prev, List next, ) { if (identical(prev, next)) return false; if (prev.length != next.length) return true; - return DrawingProvider.objectToJson(prev) != - DrawingProvider.objectToJson(next); + return _drawingsByRoot(prev) != _drawingsByRoot(next); } + + static String _drawingsByRoot(List drawings) => jsonEncode([ + for (final drawing + in jsonDecode(DrawingProvider.objectToJson(drawings)) as List) + { + ...drawing as Map, + if (drawing['id'] case final String id) 'id': pageCopyRoot(id), + }, + ]); } diff --git a/lib/providers/collab/active_page_live_sync_provider.dart b/lib/providers/collab/active_page_live_sync_provider.dart index d9fa0956..53a1fd46 100644 --- a/lib/providers/collab/active_page_live_sync_provider.dart +++ b/lib/providers/collab/active_page_live_sync_provider.dart @@ -1005,6 +1005,21 @@ class ActivePageLiveSyncNotifier extends Notifier { return entities; } + /// The items of page [pageId], the page on screen, as the rows live sync + /// keeps for them: what a copy of the page sends. Lineups are left out; + /// their groups are written from the lineup graph. + List<({String publicId, CloudPayload payload, int sortIndex})> + elementRowsAsDrawn(String pageId) => [ + for (final entity in _normalizedLocalEntities(pageId).values) + if (entity.key.kind == EntitySyncKeyKind.element && + entity.payload is CloudPayload) + ( + publicId: entity.key.entityId!, + payload: entity.payload as CloudPayload, + sortIndex: entity.sortIndex ?? 0, + ), + ]; + Map _normalizedLocalEntities( String pageId) { final entities = {}; diff --git a/lib/providers/strategy_provider.dart b/lib/providers/strategy_provider.dart index adb2638c..ca804032 100644 --- a/lib/providers/strategy_provider.dart +++ b/lib/providers/strategy_provider.dart @@ -46,6 +46,7 @@ import 'package:icarus/providers/collab/remote_library_provider.dart'; import 'package:icarus/providers/collab/cloud_media_upload_queue_provider.dart'; import 'package:icarus/providers/auth_provider.dart'; import 'package:icarus/providers/collab/remote_strategy_snapshot_provider.dart'; +import 'package:icarus/providers/collab/active_page_live_sync_provider.dart'; import 'package:icarus/providers/collab/strategy_op_queue_provider.dart'; import 'package:icarus/providers/strategy_page_session_provider.dart'; import 'package:icarus/providers/strategy_save_state_provider.dart'; @@ -84,6 +85,11 @@ enum PageCopyResult { imageUnavailable, } +/// What of a cloud page could not be copied onto a new page: images whose +/// pictures could not be copied yet, and whether this device failed to +/// store some of the copy to send. +typedef NewPageCopyGaps = ({int imagesLeft, bool notSaved}); + class StrategyProvider extends Notifier { @override StrategyState build() { @@ -1065,40 +1071,12 @@ class StrategyProvider extends Notifier { // the original's picture under its own id first. It shares the stored // bytes, so nothing is uploaded again. if (element.kind == 'image') { - final CloudImageCopyResult picture; - try { - picture = - await ref.read(convexStrategyRepositoryProvider).copyImageAsset( - strategyPublicId: strategyId, - sourceAssetPublicId: widgetId, - targetAssetPublicId: copyId, - ); - } catch (error) { - log('Could not copy the picture of image $widgetId: $error'); - return PageCopyResult.unreachable; - } - switch (picture) { - case CloudImageCopyResult.uploading: - return PageCopyResult.imageUploading; - case CloudImageCopyResult.unavailable: - // An image this device placed may not have reached the server yet. - // Its upload only goes once the image itself is saved to send - // (referenceDurable); one whose save failed never will. - final stillUploading = ref - .read(cloudMediaUploadQueueProvider) - .jobsForStrategy(strategyId) - .any( - (job) => - job.assetPublicId == widgetId && - job.referenceDurable && - job.state != CloudMediaJobState.failed, - ); - return stillUploading - ? PageCopyResult.imageUploading - : PageCopyResult.imageUnavailable; - case CloudImageCopyResult.copied: - break; - } + final picture = await _copyImagePicture( + strategyId: strategyId, + imageId: widgetId, + copyId: copyId, + ); + if (picture != null) return picture; if (state.strategyId != strategyId) return PageCopyResult.unavailable; } // The canvas never draws the copy: its page shows it from the server. @@ -1124,6 +1102,48 @@ class StrategyProvider extends Notifier { return PageCopyResult.copied; } + /// Has the server give image [copyId] the picture of image [imageId], + /// sharing its stored bytes. Null once it has; otherwise why not. + Future _copyImagePicture({ + required String strategyId, + required String imageId, + required String copyId, + }) async { + final CloudImageCopyResult picture; + try { + picture = await ref.read(convexStrategyRepositoryProvider).copyImageAsset( + strategyPublicId: strategyId, + sourceAssetPublicId: imageId, + targetAssetPublicId: copyId, + ); + } catch (error) { + log('Could not copy the picture of image $imageId: $error'); + return PageCopyResult.unreachable; + } + switch (picture) { + case CloudImageCopyResult.copied: + return null; + case CloudImageCopyResult.uploading: + return PageCopyResult.imageUploading; + case CloudImageCopyResult.unavailable: + // An image this device placed may not have reached the server yet. + // Its upload only goes once the image itself is saved to send + // (referenceDurable); one whose save failed never will. + final stillUploading = ref + .read(cloudMediaUploadQueueProvider) + .jobsForStrategy(strategyId) + .any( + (job) => + job.assetPublicId == imageId && + job.referenceDurable && + job.state != CloudMediaJobState.failed, + ); + return stillUploading + ? PageCopyResult.imageUploading + : PageCopyResult.imageUnavailable; + } + } + /// The items on [page] as the server has them with the work still queued /// for it laid over, refused work waiting for the user's choice included, /// by id, with their sortIndexes. @@ -1513,11 +1533,13 @@ class StrategyProvider extends Notifier { return null; } - Future addPage([String? name]) async { - if (!_currentStrategyCanEditPages()) return; + /// Adds a copy of the page on screen after it and turns to it. On a cloud + /// strategy, returns what of the page could not be copied, if anything. + Future addPage([String? name]) async { + if (!_currentStrategyCanEditPages()) return null; if (_currentStrategyIsCloud()) { final snapshot = ref.read(remoteEditorSnapshotProvider).valueOrNull; - if (snapshot == null) return; + if (snapshot == null) return null; final pages = [...snapshot.pages] ..sortBySortIndex((item) => item.sortIndex); final pageID = const Uuid().v4(); @@ -1528,6 +1550,18 @@ class StrategyProvider extends Notifier { final sourceIndex = activeIndex >= 0 ? activeIndex : pages.length - 1; final nextIndex = sourceIndex + 1; final isAutoNamed = name == null; + // The new page copies the page on screen as it is drawn now, as a + // local "+" does, each item and lineup under a copy id (see + // page_copy_id.dart) so the turn to it glides rather than fades. + final strategyId = state.strategyId; + final elements = activeIndex >= 0 + ? ref + .read(activePageLiveSyncProvider.notifier) + .elementRowsAsDrawn(activePageId!) + : const <({String publicId, CloudPayload payload, int sortIndex})>[]; + final lineUps = ref.read(lineUpProvider).graph; + final lineUpCopies = + lineUps.copyOfLinks({for (final link in lineUps.links) link.id}); final ack = await _enqueueCloudPageDescriptorOp(PageAddOp( opId: const Uuid().v4(), pagePublicId: pageID, @@ -1540,7 +1574,16 @@ class StrategyProvider extends Notifier { sortIndex: nextIndex, expectedStrategyRevision: snapshot.header.revision, )); + NewPageCopyGaps? gaps; if (ack?.isAck ?? false) { + if (strategyId != null && state.strategyId == strategyId) { + gaps = await _copyPageRowsToCloudPage( + strategyId: strategyId, + pageId: pageID, + elements: elements, + lineUps: lineUpCopies, + ); + } await ref.read(remoteEditorSnapshotProvider.notifier).refresh(); await ref .read(strategyPageSessionProvider.notifier) @@ -1549,7 +1592,7 @@ class StrategyProvider extends Notifier { direction: PageTransitionDirection.forward, ); } - return; + return gaps; } final box = Hive.box(HiveBoxNames.strategiesBox); @@ -1558,9 +1601,9 @@ class StrategyProvider extends Notifier { await _syncCurrentPageToHive(); final strategyId = state.strategyId; - if (strategyId == null) return; + if (strategyId == null) return null; final strat = box.get(strategyId); - if (strat == null || strat.pages.isEmpty) return; + if (strat == null || strat.pages.isEmpty) return null; final orderedPages = [...strat.pages] ..sortBySortIndex((item) => item.sortIndex); @@ -1590,6 +1633,106 @@ class StrategyProvider extends Notifier { await box.put(updated.id, updated); await setActivePageAnimated(newPage.id); + return null; + } + + /// How long a new cloud page waits for its copied items to land before it + /// is shown, so it opens with them rather than filling in as they arrive. + @visibleForTesting + static Duration cloudPageCopyLandingWait = const Duration(seconds: 3); + + /// Sends copies of the page rows [elements] (under copy ids made here) and + /// the lineups [lineUps] (already under copy ids) to new cloud page + /// [pageId], then waits a little for them to land. An image whose picture + /// cannot be copied is left out. Returns what did not make it. + Future _copyPageRowsToCloudPage({ + required String strategyId, + required String pageId, + required List<({String publicId, CloudPayload payload, int sortIndex})> + elements, + required LineUpGraph lineUps, + }) async { + final queue = ref.read(strategyOpQueueProvider.notifier); + final accountId = ref.read(strategyOpQueueProvider).accountId; + // A copy id too long for this device to store a change under (only an + // item imported with an unusually long id) is a plain one: that item + // then fades between the two pages rather than gliding. + String copyIdOf(String id) { + final copyId = newPageCopyId(id); + final fits = accountId != null && + DurableOutboxRecord.createStorageKey( + accountId: accountId, + strategyPublicId: strategyId, + entityKey: EntitySyncKey.element(pageId, copyId), + ).length <= + 255; + return fits ? copyId : const Uuid().v4(); + } + + final sent = {}; + var imagesLeft = 0; + var notSaved = false; + for (final element in elements) { + final copyId = copyIdOf(element.publicId); + if (element.payload['kind'] == 'image' && + await _copyImagePicture( + strategyId: strategyId, + imageId: element.publicId, + copyId: copyId, + ) != + null) { + imagesLeft++; + continue; + } + final queued = await queue.enqueueOffCanvas( + ElementAddOp( + opId: const Uuid().v4(), + elementPublicId: copyId, + pagePublicId: pageId, + payload: { + ...element.payload, + 'data': {...cloudPayloadData(element.payload), 'id': copyId}, + }, + sortIndex: element.sortIndex, + ), + ); + if (queued) { + sent.add(EntitySyncKey.element(pageId, copyId)); + } else { + notSaved = true; + } + } + for (final (index, row) in cloudLineupRows(lineUps).rows.indexed) { + final queued = await queue.enqueueOffCanvas( + LineupAddOp( + opId: const Uuid().v4(), + lineupPublicId: row.publicId, + pagePublicId: pageId, + payload: row.payload, + sortIndex: index, + ), + ); + if (queued) { + sent.add(EntitySyncKey.lineup(pageId, row.publicId)); + } else { + notSaved = true; + } + } + if (sent.isNotEmpty) { + ref.read(strategySaveStateProvider.notifier) + ..markDirty() + ..setPendingCloudSync(true) + ..setCloudSyncError(null); + await queue.flushNow(); + final deadline = DateTime.now().add(cloudPageCopyLandingWait); + bool waiting() => ref.read(strategyOpQueueProvider).pending.any( + (pending) => sent.contains(EntitySyncKey.forStrategyOp(pending.op)), + ); + while (waiting() && DateTime.now().isBefore(deadline)) { + await Future.delayed(const Duration(milliseconds: 50)); + } + } + return (imagesLeft: imagesLeft, notSaved: notSaved); } Future renamePage(String pageId, String newName) async { diff --git a/lib/widgets/global_shortcuts.dart b/lib/widgets/global_shortcuts.dart index 4f3807af..90aabd2a 100644 --- a/lib/widgets/global_shortcuts.dart +++ b/lib/widgets/global_shortcuts.dart @@ -22,6 +22,7 @@ import 'package:icarus/widgets/rotate_helpers.dart'; import 'package:icarus/config/platform_policy.dart'; import 'package:icarus/widgets/platform_feature_toast.dart'; import 'package:uuid/uuid.dart'; +import 'package:icarus/widgets/new_page_copy_toast.dart'; class GlobalShortcuts extends ConsumerStatefulWidget { const GlobalShortcuts({super.key, required this.child}); @@ -143,7 +144,9 @@ class _GlobalShortcutsState extends ConsumerState onInvoke: (intent) async { if (!capabilities.canAddPage) return null; _dismissDeleteMenu(); - await ref.read(strategyProvider.notifier).addPage(); + showNewPageCopyGaps( + await ref.read(strategyProvider.notifier).addPage(), + ); return null; }, ), diff --git a/lib/widgets/new_page_copy_toast.dart b/lib/widgets/new_page_copy_toast.dart new file mode 100644 index 00000000..e662086c --- /dev/null +++ b/lib/widgets/new_page_copy_toast.dart @@ -0,0 +1,23 @@ +import 'package:icarus/const/settings.dart'; +import 'package:icarus/providers/strategy_provider.dart'; + +/// Says what of the page on screen a new cloud page could not copy, if +/// anything (see StrategyProvider.addPage). +void showNewPageCopyGaps(NewPageCopyGaps? gaps) { + if (gaps == null) return; + if (gaps.notSaved) { + Settings.showToast( + message: "Couldn't save all of the page's copy on this device, so some " + 'of it is missing from the new page.', + backgroundColor: Settings.tacticalVioletTheme.destructive, + ); + } else if (gaps.imagesLeft > 0) { + Settings.showToast( + message: gaps.imagesLeft == 1 + ? "An image couldn't be copied yet, so it isn't on the new page." + : "${gaps.imagesLeft} images couldn't be copied yet, so they aren't " + 'on the new page.', + backgroundColor: Settings.tacticalVioletTheme.primary, + ); + } +} diff --git a/lib/widgets/pages_bar.dart b/lib/widgets/pages_bar.dart index 6c1fecdd..1f79de23 100644 --- a/lib/widgets/pages_bar.dart +++ b/lib/widgets/pages_bar.dart @@ -19,6 +19,7 @@ import 'package:icarus/widgets/dialogs/delete_page_dialog.dart'; import 'package:icarus/widgets/dialogs/recently_deleted_dialog.dart'; import 'package:shadcn_ui/shadcn_ui.dart'; import 'package:toastification/toastification.dart'; +import 'package:icarus/widgets/new_page_copy_toast.dart'; const double _pagesBarCornerRadius = 12; const double _pagesBarFooterHeight = 48; @@ -216,7 +217,7 @@ class _PagesBarState extends ConsumerState { Future _addPage() async { final caps = ref.read(currentStrategyCapabilitiesProvider); if (!caps.canAddPage) return; - await ref.read(strategyProvider.notifier).addPage(); + showNewPageCopyGaps(await ref.read(strategyProvider.notifier).addPage()); } Future _selectPage(String id) async { diff --git a/test/page_copy_id_test.dart b/test/page_copy_id_test.dart index 4d137201..040dea00 100644 --- a/test/page_copy_id_test.dart +++ b/test/page_copy_id_test.dart @@ -1,5 +1,6 @@ import 'package:flutter_test/flutter_test.dart'; import 'package:icarus/const/agents.dart'; +import 'package:icarus/const/drawing_element.dart'; import 'package:icarus/const/page_copy_id.dart'; import 'package:icarus/const/placed_classes.dart'; import 'package:icarus/const/transition_data.dart'; @@ -138,4 +139,42 @@ void main() { expect(move.to, onPage3); }); }); + + group('drawings between pages', () { + Line stroke(String id, {Offset end = const Offset(100, 100)}) => Line( + id: id, + lineStart: const Offset(10, 10), + lineEnd: end, + colorValue: 0xFFFFFFFF, + isDotted: false, + hasArrow: false, + ); + + test('a copied stroke counts as its original', () { + expect( + TransitionPlanner.drawingsChanged( + [stroke('stroke-1')], + [stroke('stroke-1~cp1~$_occurrence')], + ), + isFalse, + ); + }); + + test('a copied stroke that moved, or another stroke, is a change', () { + expect( + TransitionPlanner.drawingsChanged( + [stroke('stroke-1')], + [stroke('stroke-1~cp1~$_occurrence', end: const Offset(200, 50))], + ), + isTrue, + ); + expect( + TransitionPlanner.drawingsChanged( + [stroke('stroke-1')], + [stroke('stroke-2')], + ), + isTrue, + ); + }); + }); } diff --git a/test/strategy_page_session_provider_test.dart b/test/strategy_page_session_provider_test.dart index dfb79923..b76c9c57 100644 --- a/test/strategy_page_session_provider_test.dart +++ b/test/strategy_page_session_provider_test.dart @@ -11373,6 +11373,11 @@ void main() { final container = await _cloudContainer( remote: _FakeRemoteEditorNotifier( _editorSnapshot(pages: pages, activePage: onScreen), + pageCatalog: { + 'page-1': _pageSnapshot(pages[0]), + 'page-2': onScreen, + 'page-3': _pageSnapshot(pages[2], elements: nextPage), + }, ), queue: queue, repository: reader, @@ -11614,6 +11619,124 @@ void main() { expect(adds(container).single.pagePublicId, 'page-3'); }); + group('"+" on a cloud strategy', () { + /// Opens page 2 with its text, a lineup and an image on the canvas, + /// the server answering every op sent; returns every op sent. + Future<(ProviderContainer, List, _PageReader)> openToCopy({ + CloudImageCopyResult image = CloudImageCopyResult.copied, + }) async { + final (container, queue, reader) = await open(); + reader.imageCopy = image; + final sent = []; + final remote = container.read(remoteEditorSnapshotProvider.notifier) + as _FakeRemoteEditorNotifier; + queue.onFlush = () { + final ops = [ + for (final intent in queue.state.queuedByEntityKey.values) + intent.pending.op, + ]; + sent.addAll(ops); + // The server now has each page added, as yet empty. + for (final op in ops.whereType()) { + final page = _page(op.pagePublicId, op.sortIndex); + remote.pageCatalog[page.publicId] = _pageSnapshot(page); + remote.initialSnapshot = _editorSnapshot( + pages: [...pages, page], + activePage: remote.initialSnapshot.activePage!, + ); + } + queue.ackQueued(); + }; + container.read(placedImageProvider.notifier).fromHive([ + PlacedImage( + id: 'image', + position: const Offset(12, 34), + aspectRatio: 1.5, + scale: 100, + fileExtension: '.png', + ), + ]); + container.read(lineUpProvider.notifier).mergeRemote( + lineUpGraphFromCloudRows([ + CloudLineupRow.remote(_lineup('page-2', 'a')), + ]).graph, + ); + await _settle(); + sent.clear(); + return (container, sent, reader); + } + + setUp(() => StrategyProvider.cloudPageCopyLandingWait = Duration.zero); + + test('copies the page on screen under copy ids, in its order', () async { + final (container, sent, reader) = await openToCopy(); + + await container.read(strategyProvider.notifier).addPage(); + + final page = sent.whereType().single.pagePublicId; + final elements = sent + .whereType() + .where((op) => op.pagePublicId == page) + .toList(); + expect( + { + for (final op in elements) + pageCopyRoot(op.elementPublicId): op.payload['kind'], + }, + {'text-page-2': 'text', 'image': 'image'}, + ); + for (final op in elements) { + expect(op.elementPublicId, contains('~cp1~')); + expect(cloudPayloadData(op.payload)['id'], op.elementPublicId); + } + final text = elements.singleWhere( + (op) => pageCopyRoot(op.elementPublicId) == 'text-page-2'); + expect(cloudPayloadData(text.payload)['text'], 'two'); + final image = elements + .singleWhere((op) => pageCopyRoot(op.elementPublicId) == 'image'); + expect(reader.imageCopies, [('image', image.elementPublicId)]); + + final lineup = sent + .whereType() + .singleWhere((op) => op.pagePublicId == page); + final data = cloudPayloadData(lineup.payload); + expect( + _entries(data, 'links').map((link) => pageCopyRoot(link['id'])), + ['link-a'], + ); + expect(pageCopyRoot(_entries(data, 'origins').single['id']), 'a'); + // The page copied from is left as it was. + expect( + sent.where((op) => + op.pagePublicId == 'page-2' && + (op is ElementDeleteOp || op is LineupDeleteOp)), + isEmpty, + ); + await _settle(); + }); + + test('leaves out an image still uploading and copies the rest', () async { + final (container, sent, _) = + await openToCopy(image: CloudImageCopyResult.uploading); + + await container.read(strategyProvider.notifier).addPage(); + + final page = sent.whereType().single.pagePublicId; + expect( + sent + .whereType() + .where((op) => op.pagePublicId == page) + .map((op) => pageCopyRoot(op.elementPublicId)), + ['text-page-2'], + ); + expect( + sent.whereType().where((op) => op.pagePublicId == page), + hasLength(1), + ); + await _settle(); + }); + }); + group('an image', () { /// Opens page 2 with a placed image on it, whose picture the server /// copies with [result] (null: the call fails, as when offline). From 0cc36a727b87511cb64ab41911442b484529fadd Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 13:33:44 -0400 Subject: [PATCH 02/11] Finish a cloud page copy when the page lands late; stop it on a switch Astra's review: - "+" checked once for the new page's ack, so a busy queue or a dropped connection meant the page landed later, empty, with nothing said. It now waits for the page within the three seconds, keeping the queue sending; past them the copy follows once the page lands, and a toast says the page is on its way. - Copies queue through the queue of the strategy open now, so leaving the strategy while one was copied sent the rest to the wrong one. Each copy now checks the strategy first, and a stopped copy says so. - The three seconds didn't cover the send or the image picture copies. They now bound everything; an image that takes longer is left out. - Lineup copy ids skipped the storage-length check items get. Co-Authored-By: Claude Opus 5.5 --- lib/const/line_provider.dart | 10 +- lib/providers/strategy_provider.dart | 228 +++++++++++++----- lib/widgets/global_shortcuts.dart | 4 +- lib/widgets/new_page_copy_toast.dart | 8 +- lib/widgets/pages_bar.dart | 6 +- test/strategy_page_session_provider_test.dart | 111 ++++++++- 6 files changed, 293 insertions(+), 74 deletions(-) diff --git a/lib/const/line_provider.dart b/lib/const/line_provider.dart index c8f5af24..28e4821b 100644 --- a/lib/const/line_provider.dart +++ b/lib/const/line_provider.dart @@ -375,10 +375,14 @@ class LineUpGraph { /// The lineups [linkIds] and the spots they aim at, ready to be put on /// another page, each under a new id that carries its original's (see - /// page_copy_id.dart). Spots they share stay shared in the copy. - LineUpGraph copyOfLinks(Set linkIds) { + /// page_copy_id.dart), or that [newId] makes. Spots they share stay shared + /// in the copy. + LineUpGraph copyOfLinks( + Set linkIds, { + String Function(String id) newId = newPageCopyId, + }) { final newIds = {}; - String renamed(String id) => newIds[id] ??= newPageCopyId(id); + String renamed(String id) => newIds[id] ??= newId(id); final part = linksWithSpots(linkIds); return LineUpGraph( origins: [ diff --git a/lib/providers/strategy_provider.dart b/lib/providers/strategy_provider.dart index ca804032..90f92906 100644 --- a/lib/providers/strategy_provider.dart +++ b/lib/providers/strategy_provider.dart @@ -85,10 +85,12 @@ enum PageCopyResult { imageUnavailable, } -/// What of a cloud page could not be copied onto a new page: images whose -/// pictures could not be copied yet, and whether this device failed to -/// store some of the copy to send. -typedef NewPageCopyGaps = ({int imagesLeft, bool notSaved}); +/// What of a cloud page could not be copied onto a new page, or is still on +/// its way: images whose pictures could not be copied yet; whether some of +/// the copy could not be stored to send (or the strategy was left first); +/// and whether the new page itself is still waiting for the cloud, its copy +/// to follow once it is added. +typedef NewPageCopyGaps = ({int imagesLeft, bool notSaved, bool pageWaiting}); class StrategyProvider extends Notifier { @override @@ -1534,8 +1536,12 @@ class StrategyProvider extends Notifier { } /// Adds a copy of the page on screen after it and turns to it. On a cloud - /// strategy, returns what of the page could not be copied, if anything. - Future addPage([String? name]) async { + /// strategy, returns what of the page could not be copied, if anything; + /// when the page is added later, [onLaterGaps] hears what its copy missed. + Future addPage([ + String? name, + void Function(NewPageCopyGaps gaps)? onLaterGaps, + ]) async { if (!_currentStrategyCanEditPages()) return null; if (_currentStrategyIsCloud()) { final snapshot = ref.read(remoteEditorSnapshotProvider).valueOrNull; @@ -1554,15 +1560,23 @@ class StrategyProvider extends Notifier { // local "+" does, each item and lineup under a copy id (see // page_copy_id.dart) so the turn to it glides rather than fades. final strategyId = state.strategyId; + if (strategyId == null) return null; final elements = activeIndex >= 0 ? ref .read(activePageLiveSyncProvider.notifier) .elementRowsAsDrawn(activePageId!) : const <({String publicId, CloudPayload payload, int sortIndex})>[]; final lineUps = ref.read(lineUpProvider).graph; - final lineUpCopies = - lineUps.copyOfLinks({for (final link in lineUps.links) link.id}); - final ack = await _enqueueCloudPageDescriptorOp(PageAddOp( + final lineUpCopies = lineUps.copyOfLinks( + {for (final link in lineUps.links) link.id}, + newId: (id) => _storableCopyId( + strategyId, + id, + (copyId) => EntitySyncKey.lineup(pageID, copyId), + ), + ); + final deadline = DateTime.now().add(cloudPageCopyLandingWait); + final pageOp = PageAddOp( opId: const Uuid().v4(), pagePublicId: pageID, payload: { @@ -1573,17 +1587,31 @@ class StrategyProvider extends Notifier { }, sortIndex: nextIndex, expectedStrategyRevision: snapshot.header.revision, - )); - NewPageCopyGaps? gaps; - if (ack?.isAck ?? false) { - if (strategyId != null && state.strategyId == strategyId) { - gaps = await _copyPageRowsToCloudPage( + ); + await enqueueOps([pageOp]); + Future copy({DateTime? until}) => + _copyPageRowsToCloudPage( strategyId: strategyId, pageId: pageID, elements: elements, lineUps: lineUpCopies, + deadline: until, ); - } + final added = await _untilLanded(strategyId, {pageOp.opId}, deadline); + if (added == null) { + // The cloud is slow to answer (busy, or offline): the page is added + // when it does, and its copy follows then. + unawaited(() async { + if (await _untilLanded(strategyId, {pageOp.opId}, null) == true) { + final gaps = await copy(); + if (gaps.imagesLeft > 0 || gaps.notSaved) onLaterGaps?.call(gaps); + } + }()); + return (imagesLeft: 0, notSaved: false, pageWaiting: true); + } + if (!added) return null; + final gaps = await copy(until: deadline); + if (state.strategyId == strategyId) { await ref.read(remoteEditorSnapshotProvider.notifier).refresh(); await ref .read(strategyPageSessionProvider.notifier) @@ -1636,55 +1664,134 @@ class StrategyProvider extends Notifier { return null; } - /// How long a new cloud page waits for its copied items to land before it - /// is shown, so it opens with them rather than filling in as they arrive. + /// A copy id for [id] (see page_copy_id.dart), or a plain id when a change + /// to the copy, stored under [keyOf] its id, would pass Hive's + /// 255-character key limit. Only an item imported with an unusually long + /// id gets a plain one, and it then fades between pages rather than + /// gliding. + String _storableCopyId( + String strategyId, + String id, + EntitySyncKey Function(String copyId) keyOf, + ) { + final copyId = newPageCopyId(id); + final accountId = ref.read(strategyOpQueueProvider).accountId; + final fits = accountId != null && + DurableOutboxRecord.createStorageKey( + accountId: accountId, + strategyPublicId: strategyId, + entityKey: keyOf(copyId), + ).length <= + 255; + return fits ? copyId : const Uuid().v4(); + } + + /// Whether the queued ops [opIds] of strategy [strategyId] land: true once + /// all have, false once one is refused (it waits for the user's choice) + /// or the strategy is left, null if [deadline] comes first. Keeps the + /// queue sending meanwhile; it may be busy with an earlier batch. + Future _untilLanded( + String strategyId, + Set ids, + DateTime? deadline, + ) async { + final queue = ref.read(strategyOpQueueProvider.notifier); + var nextFlush = DateTime.now(); + while (true) { + final queueState = ref.read(strategyOpQueueProvider); + if (queueState.attentionByEntityKey.values + .any((intent) => ids.contains(intent.pending.op.opId))) { + return false; + } + if (!queueState.pending.any((pending) => ids.contains(pending.op.opId))) { + return true; + } + if (state.strategyId != strategyId || + queueState.strategyPublicId != strategyId) { + return false; + } + final now = DateTime.now(); + if (deadline != null && !now.isBefore(deadline)) return null; + if (!now.isBefore(nextFlush)) { + unawaited(queue.flushNow()); + nextFlush = now.add(const Duration(milliseconds: 500)); + } + await Future.delayed(const Duration(milliseconds: 50)); + } + } + + /// How long "+" on a cloud strategy waits for the new page, and then its + /// copied items, to land before showing it, so it opens full rather than + /// filling in. Past it, the page is shown (or, not yet added, arrives + /// later with its copy) and the queue keeps sending. @visibleForTesting static Duration cloudPageCopyLandingWait = const Duration(seconds: 3); /// Sends copies of the page rows [elements] (under copy ids made here) and /// the lineups [lineUps] (already under copy ids) to new cloud page - /// [pageId], then waits a little for them to land. An image whose picture - /// cannot be copied is left out. Returns what did not make it. + /// [pageId] of strategy [strategyId], then waits for them to land, until + /// [deadline] if given. An image whose picture cannot be copied (in time) + /// is left out; leaving the strategy stops the copy. Returns what did not + /// make it. Future _copyPageRowsToCloudPage({ required String strategyId, required String pageId, required List<({String publicId, CloudPayload payload, int sortIndex})> elements, required LineUpGraph lineUps, + DateTime? deadline, }) async { final queue = ref.read(strategyOpQueueProvider.notifier); - final accountId = ref.read(strategyOpQueueProvider).accountId; - // A copy id too long for this device to store a change under (only an - // item imported with an unusually long id) is a plain one: that item - // then fades between the two pages rather than gliding. - String copyIdOf(String id) { - final copyId = newPageCopyId(id); - final fits = accountId != null && - DurableOutboxRecord.createStorageKey( - accountId: accountId, - strategyPublicId: strategyId, - entityKey: EntitySyncKey.element(pageId, copyId), - ).length <= - 255; - return fits ? copyId : const Uuid().v4(); - } - - final sent = {}; + // Ops go through the queue of the strategy open now, so none is queued + // once another is. + bool stillOpen() => + state.strategyId == strategyId && + ref.read(strategyOpQueueProvider).strategyPublicId == strategyId; + final sent = {}; var imagesLeft = 0; var notSaved = false; + Future send(StrategyOp op) async { + if (!stillOpen()) { + notSaved = true; + return; + } + if (await queue.enqueueOffCanvas(op)) { + sent.add(op.opId); + } else { + notSaved = true; + } + } + for (final element in elements) { - final copyId = copyIdOf(element.publicId); - if (element.payload['kind'] == 'image' && - await _copyImagePicture( - strategyId: strategyId, - imageId: element.publicId, - copyId: copyId, - ) != - null) { - imagesLeft++; - continue; + final copyId = _storableCopyId( + strategyId, + element.publicId, + (copyId) => EntitySyncKey.element(pageId, copyId), + ); + if (element.payload['kind'] == 'image') { + final remaining = deadline?.difference(DateTime.now()); + final picture = _copyImagePicture( + strategyId: strategyId, + imageId: element.publicId, + copyId: copyId, + ); + final PageCopyResult? left; + try { + left = remaining == null + ? await picture + : await picture.timeout( + remaining.isNegative ? Duration.zero : remaining, + ); + } on TimeoutException { + imagesLeft++; + continue; + } + if (left != null) { + imagesLeft++; + continue; + } } - final queued = await queue.enqueueOffCanvas( + await send( ElementAddOp( opId: const Uuid().v4(), elementPublicId: copyId, @@ -1696,14 +1803,9 @@ class StrategyProvider extends Notifier { sortIndex: element.sortIndex, ), ); - if (queued) { - sent.add(EntitySyncKey.element(pageId, copyId)); - } else { - notSaved = true; - } } for (final (index, row) in cloudLineupRows(lineUps).rows.indexed) { - final queued = await queue.enqueueOffCanvas( + await send( LineupAddOp( opId: const Uuid().v4(), lineupPublicId: row.publicId, @@ -1712,27 +1814,19 @@ class StrategyProvider extends Notifier { sortIndex: index, ), ); - if (queued) { - sent.add(EntitySyncKey.lineup(pageId, row.publicId)); - } else { - notSaved = true; - } } - if (sent.isNotEmpty) { + if (sent.isNotEmpty && stillOpen()) { ref.read(strategySaveStateProvider.notifier) ..markDirty() ..setPendingCloudSync(true) ..setCloudSyncError(null); - await queue.flushNow(); - final deadline = DateTime.now().add(cloudPageCopyLandingWait); - bool waiting() => ref.read(strategyOpQueueProvider).pending.any( - (pending) => sent.contains(EntitySyncKey.forStrategyOp(pending.op)), - ); - while (waiting() && DateTime.now().isBefore(deadline)) { - await Future.delayed(const Duration(milliseconds: 50)); + if (deadline != null) { + await _untilLanded(strategyId, sent, deadline); + } else { + unawaited(queue.flushNow()); } } - return (imagesLeft: imagesLeft, notSaved: notSaved); + return (imagesLeft: imagesLeft, notSaved: notSaved, pageWaiting: false); } Future renamePage(String pageId, String newName) async { diff --git a/lib/widgets/global_shortcuts.dart b/lib/widgets/global_shortcuts.dart index 90aabd2a..73e818d8 100644 --- a/lib/widgets/global_shortcuts.dart +++ b/lib/widgets/global_shortcuts.dart @@ -145,7 +145,9 @@ class _GlobalShortcutsState extends ConsumerState if (!capabilities.canAddPage) return null; _dismissDeleteMenu(); showNewPageCopyGaps( - await ref.read(strategyProvider.notifier).addPage(), + await ref + .read(strategyProvider.notifier) + .addPage(null, showNewPageCopyGaps), ); return null; }, diff --git a/lib/widgets/new_page_copy_toast.dart b/lib/widgets/new_page_copy_toast.dart index e662086c..54ec961a 100644 --- a/lib/widgets/new_page_copy_toast.dart +++ b/lib/widgets/new_page_copy_toast.dart @@ -5,7 +5,13 @@ import 'package:icarus/providers/strategy_provider.dart'; /// anything (see StrategyProvider.addPage). void showNewPageCopyGaps(NewPageCopyGaps? gaps) { if (gaps == null) return; - if (gaps.notSaved) { + if (gaps.pageWaiting) { + Settings.showToast( + message: 'The cloud is slow to answer. The new page will appear, ' + 'copied, once it does.', + backgroundColor: Settings.tacticalVioletTheme.primary, + ); + } else if (gaps.notSaved) { Settings.showToast( message: "Couldn't save all of the page's copy on this device, so some " 'of it is missing from the new page.', diff --git a/lib/widgets/pages_bar.dart b/lib/widgets/pages_bar.dart index 1f79de23..19c89e49 100644 --- a/lib/widgets/pages_bar.dart +++ b/lib/widgets/pages_bar.dart @@ -217,7 +217,11 @@ class _PagesBarState extends ConsumerState { Future _addPage() async { final caps = ref.read(currentStrategyCapabilitiesProvider); if (!caps.canAddPage) return; - showNewPageCopyGaps(await ref.read(strategyProvider.notifier).addPage()); + showNewPageCopyGaps( + await ref + .read(strategyProvider.notifier) + .addPage(null, showNewPageCopyGaps), + ); } Future _selectPage(String id) async { diff --git a/test/strategy_page_session_provider_test.dart b/test/strategy_page_session_provider_test.dart index b76c9c57..f98a3549 100644 --- a/test/strategy_page_session_provider_test.dart +++ b/test/strategy_page_session_provider_test.dart @@ -11622,15 +11622,20 @@ void main() { group('"+" on a cloud strategy', () { /// Opens page 2 with its text, a lineup and an image on the canvas, /// the server answering every op sent; returns every op sent. + /// While [answering] is false, the server answers nothing. + var answering = true; + Future<(ProviderContainer, List, _PageReader)> openToCopy({ CloudImageCopyResult image = CloudImageCopyResult.copied, }) async { + answering = true; final (container, queue, reader) = await open(); reader.imageCopy = image; final sent = []; final remote = container.read(remoteEditorSnapshotProvider.notifier) as _FakeRemoteEditorNotifier; queue.onFlush = () { + if (!answering) return; final ops = [ for (final intent in queue.state.queuedByEntityKey.values) intent.pending.op, @@ -11666,7 +11671,15 @@ void main() { return (container, sent, reader); } - setUp(() => StrategyProvider.cloudPageCopyLandingWait = Duration.zero); + setUp(() => StrategyProvider.cloudPageCopyLandingWait = + const Duration(seconds: 2)); + + Iterable elementCopies(List sent) { + final pages = sent.whereType().map((op) => op.pagePublicId); + return sent + .whereType() + .where((op) => pages.contains(op.pagePublicId)); + } test('copies the page on screen under copy ids, in its order', () async { final (container, sent, reader) = await openToCopy(); @@ -11715,6 +11728,102 @@ void main() { await _settle(); }); + test('a page the cloud adds late gets its copy once it does', () async { + final (container, sent, _) = await openToCopy(); + StrategyProvider.cloudPageCopyLandingWait = + const Duration(milliseconds: 200); + answering = false; + + final gaps = await container.read(strategyProvider.notifier).addPage(); + + expect(gaps?.pageWaiting, isTrue); + expect(elementCopies(sent), isEmpty); + // The cloud answers: the page lands, then its copy is sent. + answering = true; + await _until(() => elementCopies(sent).length == 2); + expect( + elementCopies(sent).map((op) => pageCopyRoot(op.elementPublicId)), + containsAll(['text-page-2', 'image']), + ); + await _settle(); + }); + + test('leaving the strategy while copying stops the copy', () async { + final (container, sent, reader) = await openToCopy(); + final picture = reader.imageCopyGate = Completer(); + + final adding = container.read(strategyProvider.notifier).addPage(); + await _until(() => sent.whereType().isNotEmpty); + // While the image's picture is copied, another strategy opens. + container.read(strategyProvider.notifier).setFromState( + const StrategyState( + strategyId: 'other-strategy', + strategyName: 'Other', + source: StrategySource.cloud, + storageDirectory: null, + isOpen: true, + ), + ); + picture.complete(); + final gaps = await adding; + + expect(gaps?.notSaved, isTrue); + expect( + container + .read(strategyOpQueueProvider) + .pending + .where((pending) => pending.op.type == StrategyOpType.elementAdd), + isEmpty, + ); + await _settle(); + }); + + test('an image whose picture takes too long is left out, in time', + () async { + final (container, sent, reader) = await openToCopy(); + StrategyProvider.cloudPageCopyLandingWait = + const Duration(milliseconds: 300); + reader.imageCopyGate = Completer(); + + final started = DateTime.now(); + final gaps = await container.read(strategyProvider.notifier).addPage(); + + expect(DateTime.now().difference(started).inSeconds, lessThan(2)); + expect(gaps?.imagesLeft, 1); + expect( + elementCopies(sent).map((op) => pageCopyRoot(op.elementPublicId)), + ['text-page-2'], + ); + await _settle(); + }); + + test('a lineup whose copy id could not be stored gets a plain one', + () async { + final (container, sent, _) = await openToCopy(); + final long = 'x' * 150; + container.read(lineUpProvider.notifier).mergeRemote( + lineUpGraphFromCloudRows([ + CloudLineupRow.remote(_lineup('page-2', long)), + ]).graph, + ); + await _settle(); + sent.clear(); + + final gaps = await container.read(strategyProvider.notifier).addPage(); + + expect(gaps?.notSaved, isFalse); + final page = sent.whereType().single.pagePublicId; + final lineup = sent + .whereType() + .singleWhere((op) => op.pagePublicId == page); + expect(lineup.lineupPublicId, isNot(contains('~cp1~'))); + expect( + _entries(cloudPayloadData(lineup.payload), 'links').single['id'], + lineup.lineupPublicId, + ); + await _settle(); + }); + test('leaves out an image still uploading and copies the rest', () async { final (container, sent, _) = await openToCopy(image: CloudImageCopyResult.uploading); From 40226cec2320f92420f9b77bb44e09e3e538a5bf Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 13:39:31 -0400 Subject: [PATCH 03/11] Only watch, and stop on dispose, while a cloud page lands late Waiting for a late page in the background nudged the queue every half second, which resets its own retry backoff while offline, and kept polling after the provider was disposed. The background wait now only watches, less often, and stops on dispose. The existing page-add test now expects "+" to wait out its window when the server answers nothing. Co-Authored-By: Claude Opus 5.5 --- lib/providers/strategy_provider.dart | 21 ++++++++++++++----- test/strategy_page_session_provider_test.dart | 17 ++++++++++++--- 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/lib/providers/strategy_provider.dart b/lib/providers/strategy_provider.dart index 90f92906..f6e5efe2 100644 --- a/lib/providers/strategy_provider.dart +++ b/lib/providers/strategy_provider.dart @@ -95,6 +95,8 @@ typedef NewPageCopyGaps = ({int imagesLeft, bool notSaved, bool pageWaiting}); class StrategyProvider extends Notifier { @override StrategyState build() { + _disposed = false; + ref.onDispose(() => _disposed = true); _registerPersistenceTrackingListeners(); ref.listen(authProvider, (previous, next) { final strategyId = state.strategyId; @@ -1686,10 +1688,14 @@ class StrategyProvider extends Notifier { return fits ? copyId : const Uuid().v4(); } - /// Whether the queued ops [opIds] of strategy [strategyId] land: true once - /// all have, false once one is refused (it waits for the user's choice) - /// or the strategy is left, null if [deadline] comes first. Keeps the - /// queue sending meanwhile; it may be busy with an earlier batch. + bool _disposed = false; + + /// Whether the queued ops [ids] of strategy [strategyId] land: true once + /// all have, false once one is refused (it waits for the user's choice), + /// the strategy is left or this is disposed, null if [deadline] comes + /// first. With a deadline the user is waiting, so the queue is kept + /// sending (it may be busy with an earlier batch); without one this only + /// watches, leaving the queue's own retries alone. Future _untilLanded( String strategyId, Set ids, @@ -1698,6 +1704,7 @@ class StrategyProvider extends Notifier { final queue = ref.read(strategyOpQueueProvider.notifier); var nextFlush = DateTime.now(); while (true) { + if (_disposed) return false; final queueState = ref.read(strategyOpQueueProvider); if (queueState.attentionByEntityKey.values .any((intent) => ids.contains(intent.pending.op.opId))) { @@ -1710,8 +1717,12 @@ class StrategyProvider extends Notifier { queueState.strategyPublicId != strategyId) { return false; } + if (deadline == null) { + await Future.delayed(const Duration(milliseconds: 500)); + continue; + } final now = DateTime.now(); - if (deadline != null && !now.isBefore(deadline)) return null; + if (!now.isBefore(deadline)) return null; if (!now.isBefore(nextFlush)) { unawaited(queue.flushNow()); nextFlush = now.add(const Duration(milliseconds: 500)); diff --git a/test/strategy_page_session_provider_test.dart b/test/strategy_page_session_provider_test.dart index f98a3549..2aa45140 100644 --- a/test/strategy_page_session_provider_test.dart +++ b/test/strategy_page_session_provider_test.dart @@ -1504,7 +1504,14 @@ void main() { queue: queue, ); - await container.read(strategyProvider.notifier).addPage('Execute'); + // The server answers nothing here, so "+" waits out a short window. + StrategyProvider.cloudPageCopyLandingWait = + const Duration(milliseconds: 100); + addTearDown(() => + StrategyProvider.cloudPageCopyLandingWait = const Duration(seconds: 3)); + final gaps = + await container.read(strategyProvider.notifier).addPage('Execute'); + expect(gaps?.pageWaiting, isTrue); final intent = container .read(strategyOpQueueProvider) @@ -1527,7 +1534,7 @@ void main() { intent.key, EntitySyncKey.pageDescriptor(pending.op.entityPublicId!), ); - expect(queue.flushNowCount, 1); + expect(queue.flushNowCount, greaterThan(0)); }); test('cloud page rename is persisted with the page revision', () async { @@ -11673,6 +11680,8 @@ void main() { setUp(() => StrategyProvider.cloudPageCopyLandingWait = const Duration(seconds: 2)); + tearDown(() => StrategyProvider.cloudPageCopyLandingWait = + const Duration(seconds: 3)); Iterable elementCopies(List sent) { final pages = sent.whereType().map((op) => op.pagePublicId); @@ -11738,8 +11747,10 @@ void main() { expect(gaps?.pageWaiting, isTrue); expect(elementCopies(sent), isEmpty); - // The cloud answers: the page lands, then its copy is sent. + // The cloud answers the queue's next retry: the page lands, then + // its copy is sent. answering = true; + await container.read(strategyOpQueueProvider.notifier).flushNow(); await _until(() => elementCopies(sent).length == 2); expect( elementCopies(sent).map((op) => pageCopyRoot(op.elementPublicId)), From 279dae6ba00886d557f8935a0865e40bc2db539b Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 13:54:02 -0400 Subject: [PATCH 04/11] Add the cloud page directly before copying onto it Astra's second review found the copy inferring that the new page existed from the queue: an op leaves the queue when it lands, but also when it is discarded, replaced, or not yet written, so copies could go to a page that wasn't there. "+" on a cloud strategy now adds the page with a direct server call (pages:add), retrying once after a teammate's change, and copies onto it only once that call succeeds; offline, no page is added and a toast says so. enqueueOffCanvas checks, as it writes, that the strategy the copy belongs to is still the active one. The page read before turning to the new page is bound by the same three seconds, and a failed turn is logged rather than thrown. Co-Authored-By: Claude Opus 5.5 --- lib/collab/convex_strategy_repository.dart | 7 + .../collab/strategy_op_queue_provider.dart | 30 +- lib/providers/strategy_provider.dart | 236 ++++++++------- lib/widgets/global_shortcuts.dart | 4 +- lib/widgets/new_page_copy_toast.dart | 7 +- lib/widgets/pages_bar.dart | 6 +- test/strategy_op_queue_provider_test.dart | 48 ++++ test/strategy_page_session_provider_test.dart | 269 ++++++++++-------- 8 files changed, 370 insertions(+), 237 deletions(-) diff --git a/lib/collab/convex_strategy_repository.dart b/lib/collab/convex_strategy_repository.dart index 0cdd785b..4fc92c93 100644 --- a/lib/collab/convex_strategy_repository.dart +++ b/lib/collab/convex_strategy_repository.dart @@ -714,6 +714,13 @@ bool isTypedConvexUnauthenticatedError(Object error) { error.rawCode == ConvexErrorCode.unauthenticated.wireName); } +bool isTypedConvexConflictError(Object error) { + return (error is ConvexFunctionException && + error.code == ConvexErrorCode.conflict) || + (error is ConvexClientFunctionError && + error.rawCode == ConvexErrorCode.conflict.wireName); +} + bool isTypedConvexNotFoundError(Object error) { return (error is ConvexFunctionException && error.code == ConvexErrorCode.notFound) || diff --git a/lib/providers/collab/strategy_op_queue_provider.dart b/lib/providers/collab/strategy_op_queue_provider.dart index cb680a69..0b7ad1c1 100644 --- a/lib/providers/collab/strategy_op_queue_provider.dart +++ b/lib/providers/collab/strategy_op_queue_provider.dart @@ -572,22 +572,34 @@ class StrategyOpQueueNotifier extends Notifier { /// on screen by then, it shows the item from the server's copy, like a /// teammate's, instead of taking it as something the canvas removed. /// Returns whether the queue holds [op]: nothing on screen keeps it, so - /// when the outbox could not store it, the caller must say so. + /// when the outbox could not store it, the caller must say so. Given a + /// [strategyPublicId], [op] is queued only if that strategy is still the + /// active one when its turn to be written comes. Future enqueueOffCanvas( StrategyOp op, { bool flushImmediately = false, + String? strategyPublicId, }) async { final key = EntitySyncKey.forStrategyOp(op)!; final canvasSession = _canvasSession; final madeLive = liveStamp; - await _serializeWrite(() => _syncDesiredLocked( - keys: {key}, - desiredOps: {key: op}, - flushImmediately: flushImmediately, - canvasSession: canvasSession, - madeLive: madeLive, - onCanvas: false, - )); + var otherStrategy = false; + await _serializeWrite(() async { + if (strategyPublicId != null && + state.strategyPublicId != strategyPublicId) { + otherStrategy = true; + return; + } + await _syncDesiredLocked( + keys: {key}, + desiredOps: {key: op}, + flushImmediately: flushImmediately, + canvasSession: canvasSession, + madeLive: madeLive, + onCanvas: false, + ); + }); + if (otherStrategy) return false; return state.pending.any( (pending) => EntitySyncKey.forStrategyOp(pending.op) == key, ) || diff --git a/lib/providers/strategy_provider.dart b/lib/providers/strategy_provider.dart index f6e5efe2..742a14a8 100644 --- a/lib/providers/strategy_provider.dart +++ b/lib/providers/strategy_provider.dart @@ -85,12 +85,11 @@ enum PageCopyResult { imageUnavailable, } -/// What of a cloud page could not be copied onto a new page, or is still on -/// its way: images whose pictures could not be copied yet; whether some of -/// the copy could not be stored to send (or the strategy was left first); -/// and whether the new page itself is still waiting for the cloud, its copy -/// to follow once it is added. -typedef NewPageCopyGaps = ({int imagesLeft, bool notSaved, bool pageWaiting}); +/// What of a cloud page could not be copied onto a new page: images whose +/// pictures could not be copied yet; whether some of the copy could not be +/// stored to send (or the strategy was left first); and whether the new page +/// could not be added at all (the cloud could not be reached). +typedef NewPageCopyGaps = ({int imagesLeft, bool notSaved, bool pageNotAdded}); class StrategyProvider extends Notifier { @override @@ -1538,12 +1537,8 @@ class StrategyProvider extends Notifier { } /// Adds a copy of the page on screen after it and turns to it. On a cloud - /// strategy, returns what of the page could not be copied, if anything; - /// when the page is added later, [onLaterGaps] hears what its copy missed. - Future addPage([ - String? name, - void Function(NewPageCopyGaps gaps)? onLaterGaps, - ]) async { + /// strategy, returns what of the page could not be copied, if anything. + Future addPage([String? name]) async { if (!_currentStrategyCanEditPages()) return null; if (_currentStrategyIsCloud()) { final snapshot = ref.read(remoteEditorSnapshotProvider).valueOrNull; @@ -1578,49 +1573,50 @@ class StrategyProvider extends Notifier { ), ); final deadline = DateTime.now().add(cloudPageCopyLandingWait); - final pageOp = PageAddOp( - opId: const Uuid().v4(), - pagePublicId: pageID, - payload: { - 'name': name ?? 'Page ${nextIndex + 1}', - 'isAutoNamed': isAutoNamed, - 'isAttack': pages.isNotEmpty ? pages[sourceIndex].isAttack : true, - 'settings': ref.read(strategySettingsProvider).toJson(), - }, + // The page is added on the server directly, not queued: its copy can + // only go once the page exists, and the call says plainly whether it + // does. Offline, no page is added. + if (!await _addCloudPageNow( + strategyId: strategyId, + pageId: pageID, + name: name ?? 'Page ${nextIndex + 1}', + isAutoNamed: isAutoNamed, sortIndex: nextIndex, - expectedStrategyRevision: snapshot.header.revision, + isAttack: pages.isNotEmpty ? pages[sourceIndex].isAttack : true, + expectedRevision: snapshot.header.revision, + )) { + return (imagesLeft: 0, notSaved: false, pageNotAdded: true); + } + final gaps = await _copyPageRowsToCloudPage( + strategyId: strategyId, + pageId: pageID, + elements: elements, + lineUps: lineUpCopies, + deadline: deadline, ); - await enqueueOps([pageOp]); - Future copy({DateTime? until}) => - _copyPageRowsToCloudPage( - strategyId: strategyId, - pageId: pageID, - elements: elements, - lineUps: lineUpCopies, - deadline: until, + if (!_disposed && state.strategyId == strategyId) { + final remaining = deadline.difference(DateTime.now()); + try { + await ref + .read(remoteEditorSnapshotProvider.notifier) + .refresh() + .timeout(remaining.isNegative ? Duration.zero : remaining); + } on TimeoutException { + // The page is shown with whatever the read has by then. + } + if (!_disposed && state.strategyId == strategyId) { + unawaited( + ref + .read(strategyPageSessionProvider.notifier) + .setActivePageAnimated( + pageID, + direction: PageTransitionDirection.forward, + ) + .catchError((Object error) { + log('Could not turn to new cloud page $pageID: $error'); + }), ); - final added = await _untilLanded(strategyId, {pageOp.opId}, deadline); - if (added == null) { - // The cloud is slow to answer (busy, or offline): the page is added - // when it does, and its copy follows then. - unawaited(() async { - if (await _untilLanded(strategyId, {pageOp.opId}, null) == true) { - final gaps = await copy(); - if (gaps.imagesLeft > 0 || gaps.notSaved) onLaterGaps?.call(gaps); - } - }()); - return (imagesLeft: 0, notSaved: false, pageWaiting: true); - } - if (!added) return null; - final gaps = await copy(until: deadline); - if (state.strategyId == strategyId) { - await ref.read(remoteEditorSnapshotProvider.notifier).refresh(); - await ref - .read(strategyPageSessionProvider.notifier) - .setActivePageAnimated( - pageID, - direction: PageTransitionDirection.forward, - ); + } } return gaps; } @@ -1690,39 +1686,28 @@ class StrategyProvider extends Notifier { bool _disposed = false; - /// Whether the queued ops [ids] of strategy [strategyId] land: true once - /// all have, false once one is refused (it waits for the user's choice), - /// the strategy is left or this is disposed, null if [deadline] comes - /// first. With a deadline the user is waiting, so the queue is kept - /// sending (it may be busy with an earlier batch); without one this only - /// watches, leaving the queue's own retries alone. - Future _untilLanded( + /// Waits, until [deadline], for the queued ops [ids] of strategy + /// [strategyId] to leave the queue, keeping it sending meanwhile (it may + /// be busy with an earlier batch). Stops early once one is refused, the + /// strategy is left or this is disposed. Only paces the turn to a new + /// page: the ops stay queued either way. + Future _untilSent( String strategyId, Set ids, - DateTime? deadline, + DateTime deadline, ) async { final queue = ref.read(strategyOpQueueProvider.notifier); var nextFlush = DateTime.now(); - while (true) { - if (_disposed) return false; + while (!_disposed && state.strategyId == strategyId) { final queueState = ref.read(strategyOpQueueProvider); - if (queueState.attentionByEntityKey.values - .any((intent) => ids.contains(intent.pending.op.opId))) { - return false; - } - if (!queueState.pending.any((pending) => ids.contains(pending.op.opId))) { - return true; - } - if (state.strategyId != strategyId || - queueState.strategyPublicId != strategyId) { - return false; - } - if (deadline == null) { - await Future.delayed(const Duration(milliseconds: 500)); - continue; + if (queueState.strategyPublicId != strategyId || + queueState.attentionByEntityKey.values + .any((intent) => ids.contains(intent.pending.op.opId)) || + !queueState.pending.any((pending) => ids.contains(pending.op.opId))) { + return; } final now = DateTime.now(); - if (!now.isBefore(deadline)) return null; + if (!now.isBefore(deadline)) return; if (!now.isBefore(nextFlush)) { unawaited(queue.flushNow()); nextFlush = now.add(const Duration(milliseconds: 500)); @@ -1731,18 +1716,68 @@ class StrategyProvider extends Notifier { } } - /// How long "+" on a cloud strategy waits for the new page, and then its - /// copied items, to land before showing it, so it opens full rather than - /// filling in. Past it, the page is shown (or, not yet added, arrives - /// later with its copy) and the queue keeps sending. + /// Adds cloud page [pageId] on the server now, rather than through the + /// queue, so what follows knows it exists. A teammate's change to the + /// strategy since [expectedRevision] is read and the add tried once more. + /// False when the page could not be added. + Future _addCloudPageNow({ + required String strategyId, + required String pageId, + required String name, + required bool isAutoNamed, + required int sortIndex, + required bool isAttack, + required int expectedRevision, + }) async { + final repository = ref.read(convexStrategyRepositoryProvider); + final settings = ref.read(strategySettingsProvider).toJson(); + Future add(int revision) => repository.addPage( + strategyPublicId: strategyId, + pagePublicId: pageId, + name: name, + isAutoNamed: isAutoNamed, + sortIndex: sortIndex, + isAttack: isAttack, + expectedRevision: revision, + settings: settings, + ); + try { + await add(expectedRevision); + return true; + } catch (error) { + if (!isTypedConvexConflictError(error) || _disposed) { + log('Could not add cloud page $pageId: $error'); + return false; + } + } + try { + await ref.read(remoteEditorSnapshotProvider.notifier).refresh(); + final snapshot = ref.read(remoteEditorSnapshotProvider).valueOrNull; + if (_disposed || + snapshot == null || + snapshot.header.publicId != strategyId) { + return false; + } + await add(snapshot.header.revision); + return true; + } catch (error) { + log('Could not add cloud page $pageId: $error'); + return false; + } + } + + /// How long "+" on a cloud strategy waits, from the press, for the new + /// page's copies to be sent before showing it, so it opens full rather + /// than filling in. Past it, the page is shown and the queue keeps + /// sending. @visibleForTesting static Duration cloudPageCopyLandingWait = const Duration(seconds: 3); /// Sends copies of the page rows [elements] (under copy ids made here) and /// the lineups [lineUps] (already under copy ids) to new cloud page - /// [pageId] of strategy [strategyId], then waits for them to land, until - /// [deadline] if given. An image whose picture cannot be copied (in time) - /// is left out; leaving the strategy stops the copy. Returns what did not + /// [pageId] of strategy [strategyId], then waits until [deadline] for + /// them to be sent. An image whose picture cannot be copied in time is + /// left out; leaving the strategy stops the copy. Returns what did not /// make it. Future _copyPageRowsToCloudPage({ required String strategyId, @@ -1750,12 +1785,13 @@ class StrategyProvider extends Notifier { required List<({String publicId, CloudPayload payload, int sortIndex})> elements, required LineUpGraph lineUps, - DateTime? deadline, + required DateTime deadline, }) async { final queue = ref.read(strategyOpQueueProvider.notifier); // Ops go through the queue of the strategy open now, so none is queued - // once another is. + // once another is (the queue checks again as it writes each one). bool stillOpen() => + !_disposed && state.strategyId == strategyId && ref.read(strategyOpQueueProvider).strategyPublicId == strategyId; final sent = {}; @@ -1766,7 +1802,7 @@ class StrategyProvider extends Notifier { notSaved = true; return; } - if (await queue.enqueueOffCanvas(op)) { + if (await queue.enqueueOffCanvas(op, strategyPublicId: strategyId)) { sent.add(op.opId); } else { notSaved = true; @@ -1780,19 +1816,14 @@ class StrategyProvider extends Notifier { (copyId) => EntitySyncKey.element(pageId, copyId), ); if (element.payload['kind'] == 'image') { - final remaining = deadline?.difference(DateTime.now()); - final picture = _copyImagePicture( - strategyId: strategyId, - imageId: element.publicId, - copyId: copyId, - ); + final remaining = deadline.difference(DateTime.now()); final PageCopyResult? left; try { - left = remaining == null - ? await picture - : await picture.timeout( - remaining.isNegative ? Duration.zero : remaining, - ); + left = await _copyImagePicture( + strategyId: strategyId, + imageId: element.publicId, + copyId: copyId, + ).timeout(remaining.isNegative ? Duration.zero : remaining); } on TimeoutException { imagesLeft++; continue; @@ -1831,13 +1862,10 @@ class StrategyProvider extends Notifier { ..markDirty() ..setPendingCloudSync(true) ..setCloudSyncError(null); - if (deadline != null) { - await _untilLanded(strategyId, sent, deadline); - } else { - unawaited(queue.flushNow()); - } + unawaited(queue.flushNow()); + await _untilSent(strategyId, sent, deadline); } - return (imagesLeft: imagesLeft, notSaved: notSaved, pageWaiting: false); + return (imagesLeft: imagesLeft, notSaved: notSaved, pageNotAdded: false); } Future renamePage(String pageId, String newName) async { diff --git a/lib/widgets/global_shortcuts.dart b/lib/widgets/global_shortcuts.dart index 73e818d8..90aabd2a 100644 --- a/lib/widgets/global_shortcuts.dart +++ b/lib/widgets/global_shortcuts.dart @@ -145,9 +145,7 @@ class _GlobalShortcutsState extends ConsumerState if (!capabilities.canAddPage) return null; _dismissDeleteMenu(); showNewPageCopyGaps( - await ref - .read(strategyProvider.notifier) - .addPage(null, showNewPageCopyGaps), + await ref.read(strategyProvider.notifier).addPage(), ); return null; }, diff --git a/lib/widgets/new_page_copy_toast.dart b/lib/widgets/new_page_copy_toast.dart index 54ec961a..b46c263f 100644 --- a/lib/widgets/new_page_copy_toast.dart +++ b/lib/widgets/new_page_copy_toast.dart @@ -5,11 +5,10 @@ import 'package:icarus/providers/strategy_provider.dart'; /// anything (see StrategyProvider.addPage). void showNewPageCopyGaps(NewPageCopyGaps? gaps) { if (gaps == null) return; - if (gaps.pageWaiting) { + if (gaps.pageNotAdded) { Settings.showToast( - message: 'The cloud is slow to answer. The new page will appear, ' - 'copied, once it does.', - backgroundColor: Settings.tacticalVioletTheme.primary, + message: "Couldn't reach the cloud, so no page was added.", + backgroundColor: Settings.tacticalVioletTheme.destructive, ); } else if (gaps.notSaved) { Settings.showToast( diff --git a/lib/widgets/pages_bar.dart b/lib/widgets/pages_bar.dart index 19c89e49..1f79de23 100644 --- a/lib/widgets/pages_bar.dart +++ b/lib/widgets/pages_bar.dart @@ -217,11 +217,7 @@ class _PagesBarState extends ConsumerState { Future _addPage() async { final caps = ref.read(currentStrategyCapabilitiesProvider); if (!caps.canAddPage) return; - showNewPageCopyGaps( - await ref - .read(strategyProvider.notifier) - .addPage(null, showNewPageCopyGaps), - ); + showNewPageCopyGaps(await ref.read(strategyProvider.notifier).addPage()); } Future _selectPage(String id) async { diff --git a/test/strategy_op_queue_provider_test.dart b/test/strategy_op_queue_provider_test.dart index c0df6cc7..d53e39a4 100644 --- a/test/strategy_op_queue_provider_test.dart +++ b/test/strategy_op_queue_provider_test.dart @@ -780,6 +780,54 @@ void main() { expect(container.read(strategyOpQueueProvider).pending, hasLength(1)); }); + test( + 'work queued off the canvas for a strategy is dropped if another opens ' + 'before its write', () async { + final store = _BlockingStore(); + final container = ProviderContainer(overrides: [ + durableStrategyOutboxStoreProvider.overrideWithValue(store), + strategyOutboxSessionProvider.overrideWithValue( + const StrategyOutboxSession( + accountId: null, + isReady: false, + hasAuthIncident: false, + ), + ), + ]); + addTearDown(container.dispose); + final notifier = container.read(strategyOpQueueProvider.notifier) + ..setActiveStrategy('strategy-1', accountId: 'account-a'); + container + .read(cloudCollabModeProvider.notifier) + .setForceLocalFallback(true); + // An earlier write holds the queue while a copy for strategy 1 waits + // its turn; meanwhile strategy 2 opens. + final earlier = notifier.enqueue(_cloudElementOp()); + final copy = notifier.enqueueOffCanvas( + const ElementAddOp( + opId: 'copy-1', + elementPublicId: 'element-1~cp1~0f8fad5b-d9cb-469f-a165-70867728950e', + pagePublicId: 'page-2', + payload: {'value': 'copy'}, + sortIndex: 0, + ), + strategyPublicId: 'strategy-1', + ); + await Future.delayed(Duration.zero); + notifier.setActiveStrategy('strategy-2', accountId: 'account-a'); + store.allowWrite.complete(); + await earlier; + + expect(await copy, isFalse); + expect( + container + .read(strategyOpQueueProvider) + .pending + .where((pending) => pending.op.opId == 'copy-1'), + isEmpty, + ); + }); + test('replacement stays hidden until the durable record is written', () async { final store = _BlockingReplacementStore(); diff --git a/test/strategy_page_session_provider_test.dart b/test/strategy_page_session_provider_test.dart index 2aa45140..43a48c82 100644 --- a/test/strategy_page_session_provider_test.dart +++ b/test/strategy_page_session_provider_test.dart @@ -278,8 +278,13 @@ class _FakeStrategyOpQueueNotifier extends StrategyOpQueueNotifier { Future enqueueOffCanvas( StrategyOp op, { bool flushImmediately = false, + String? strategyPublicId, }) async { if (offCanvasStoreFails) return false; + if (strategyPublicId != null && + strategyPublicId != state.strategyPublicId) { + return false; + } final key = EntitySyncKey.forStrategyOp(op)!; await syncDesiredOpsForPage( pageId: key.pageId!, @@ -1492,49 +1497,50 @@ void main() { expect(queue.flushNowCount, 1); }); - test('cloud page add is persisted with its descriptor and content', () async { + test('cloud page add is added on the server with its descriptor and content', + () async { final page = _page('page-1', 0); final queue = _FakeStrategyOpQueueNotifier(); - final container = await _cloudContainer( - remote: _FakeRemoteEditorNotifier(_editorSnapshot( - pages: [page], + final reader = _PageReader({}); + final remote = _FakeRemoteEditorNotifier(_editorSnapshot( + pages: [page], + activePage: _pageSnapshot(page), + shellRevision: 8, + )); + reader.onAddPage = (pageId, sortIndex) { + final added = _page(pageId, sortIndex); + remote.pageCatalog[pageId] = _pageSnapshot(added); + remote.initialSnapshot = _editorSnapshot( + pages: [page, added], activePage: _pageSnapshot(page), - shellRevision: 8, - )), + shellRevision: 9, + ); + }; + final container = await _cloudContainer( + remote: remote, queue: queue, + repository: reader, ); - // The server answers nothing here, so "+" waits out a short window. - StrategyProvider.cloudPageCopyLandingWait = - const Duration(milliseconds: 100); - addTearDown(() => - StrategyProvider.cloudPageCopyLandingWait = const Duration(seconds: 3)); final gaps = await container.read(strategyProvider.notifier).addPage('Execute'); - expect(gaps?.pageWaiting, isTrue); - final intent = container - .read(strategyOpQueueProvider) - .queuedByEntityKey - .entries - .single; - final pending = intent.value.pending; - expect(pending.op.entityType, StrategyOpEntityType.page); - expect(pending.op.kind, StrategyOpKind.add); - expect(pending.op.entityPublicId, isNotEmpty); - expect(pending.op.sortIndex, 1); - expect(pending.op.expectedRevision, 8); - expect(pending.op.payload, { - 'name': 'Execute', - 'isAutoNamed': false, - 'isAttack': true, - 'settings': container.read(strategySettingsProvider).toJson(), - }); + expect(gaps?.pageNotAdded, isFalse); + expect(reader.addedPages, hasLength(1)); + final added = reader.lastAddedPage!; + expect(added.name, 'Execute'); + expect(added.isAutoNamed, isFalse); + expect(added.sortIndex, 1); + expect(added.isAttack, isTrue); + expect(added.expectedRevision, 8); + expect(added.settings, container.read(strategySettingsProvider).toJson()); + // Nothing is queued for the page itself. expect( - intent.key, - EntitySyncKey.pageDescriptor(pending.op.entityPublicId!), + queue.state.queuedByEntityKey.keys + .where((key) => key.kind == EntitySyncKeyKind.pageDescriptor), + isEmpty, ); - expect(queue.flushNowCount, greaterThan(0)); + await _settle(); }); test('cloud page rename is persisted with the page revision', () async { @@ -11627,36 +11633,30 @@ void main() { }); group('"+" on a cloud strategy', () { - /// Opens page 2 with its text, a lineup and an image on the canvas, - /// the server answering every op sent; returns every op sent. - /// While [answering] is false, the server answers nothing. - var answering = true; - + /// Opens page 2 with its text, a lineup and an image on the canvas. + /// The server adds pages asked for, and answers every op sent; returns + /// every op sent. Future<(ProviderContainer, List, _PageReader)> openToCopy({ CloudImageCopyResult image = CloudImageCopyResult.copied, }) async { - answering = true; final (container, queue, reader) = await open(); reader.imageCopy = image; final sent = []; final remote = container.read(remoteEditorSnapshotProvider.notifier) as _FakeRemoteEditorNotifier; + reader.onAddPage = (pageId, sortIndex) { + final page = _page(pageId, sortIndex); + remote.pageCatalog[page.publicId] = _pageSnapshot(page); + remote.initialSnapshot = _editorSnapshot( + pages: [...pages, page], + activePage: remote.initialSnapshot.activePage!, + ); + }; queue.onFlush = () { - if (!answering) return; - final ops = [ + sent.addAll([ for (final intent in queue.state.queuedByEntityKey.values) intent.pending.op, - ]; - sent.addAll(ops); - // The server now has each page added, as yet empty. - for (final op in ops.whereType()) { - final page = _page(op.pagePublicId, op.sortIndex); - remote.pageCatalog[page.publicId] = _pageSnapshot(page); - remote.initialSnapshot = _editorSnapshot( - pages: [...pages, page], - activePage: remote.initialSnapshot.activePage!, - ); - } + ]); queue.ackQueued(); }; container.read(placedImageProvider.notifier).fromHive([ @@ -11683,23 +11683,24 @@ void main() { tearDown(() => StrategyProvider.cloudPageCopyLandingWait = const Duration(seconds: 3)); - Iterable elementCopies(List sent) { - final pages = sent.whereType().map((op) => op.pagePublicId); - return sent - .whereType() - .where((op) => pages.contains(op.pagePublicId)); - } + /// The ops sent for pages the server was asked to add. + Iterable onNewPages( + List sent, + _PageReader reader, + ) => + sent + .whereType() + .where((op) => reader.addedPages.contains(op.pagePublicId)); - test('copies the page on screen under copy ids, in its order', () async { + test('adds the page, then copies the page on screen under copy ids', + () async { final (container, sent, reader) = await openToCopy(); await container.read(strategyProvider.notifier).addPage(); - final page = sent.whereType().single.pagePublicId; - final elements = sent - .whereType() - .where((op) => op.pagePublicId == page) - .toList(); + expect(reader.addedPages, hasLength(1)); + expect(sent.whereType(), isEmpty); + final elements = onNewPages(sent, reader).toList(); expect( { for (final op in elements) @@ -11718,9 +11719,7 @@ void main() { .singleWhere((op) => pageCopyRoot(op.elementPublicId) == 'image'); expect(reader.imageCopies, [('image', image.elementPublicId)]); - final lineup = sent - .whereType() - .singleWhere((op) => op.pagePublicId == page); + final lineup = onNewPages(sent, reader).single; final data = cloudPayloadData(lineup.payload); expect( _entries(data, 'links').map((link) => pageCopyRoot(link['id'])), @@ -11737,25 +11736,48 @@ void main() { await _settle(); }); - test('a page the cloud adds late gets its copy once it does', () async { - final (container, sent, _) = await openToCopy(); - StrategyProvider.cloudPageCopyLandingWait = - const Duration(milliseconds: 200); - answering = false; + test('a page the cloud cannot add gets no copy, and says so', () async { + final (container, sent, reader) = await openToCopy(); + reader.addPageFailures.add(const SocketException('offline')); final gaps = await container.read(strategyProvider.notifier).addPage(); - expect(gaps?.pageWaiting, isTrue); - expect(elementCopies(sent), isEmpty); - // The cloud answers the queue's next retry: the page lands, then - // its copy is sent. - answering = true; - await container.read(strategyOpQueueProvider.notifier).flushNow(); - await _until(() => elementCopies(sent).length == 2); + expect(gaps?.pageNotAdded, isTrue); + expect(sent.whereType(), isEmpty); + expect(sent.whereType(), isEmpty); + await _settle(); + }); + + test('a teammate\'s change to the strategy is read, then the page added', + () async { + final (container, sent, reader) = await openToCopy(); + reader.addPageFailures.add(const ConvexFunctionException( + code: ConvexErrorCode.conflict, + rawCode: 'CONFLICT', + message: 'stale revision', + )); + + final gaps = await container.read(strategyProvider.notifier).addPage(); + + expect(gaps?.pageNotAdded, isFalse); + expect(reader.addPageCalls, 2); + expect(onNewPages(sent, reader), hasLength(2)); + await _settle(); + }); + + test('leaves out an image still uploading and copies the rest', () async { + final (container, sent, reader) = + await openToCopy(image: CloudImageCopyResult.uploading); + + final gaps = await container.read(strategyProvider.notifier).addPage(); + + expect(gaps?.imagesLeft, 1); expect( - elementCopies(sent).map((op) => pageCopyRoot(op.elementPublicId)), - containsAll(['text-page-2', 'image']), + onNewPages(sent, reader) + .map((op) => pageCopyRoot(op.elementPublicId)), + ['text-page-2'], ); + expect(onNewPages(sent, reader), hasLength(1)); await _settle(); }); @@ -11764,7 +11786,7 @@ void main() { final picture = reader.imageCopyGate = Completer(); final adding = container.read(strategyProvider.notifier).addPage(); - await _until(() => sent.whereType().isNotEmpty); + await _until(() => reader.addedPages.isNotEmpty); // While the image's picture is copied, another strategy opens. container.read(strategyProvider.notifier).setFromState( const StrategyState( @@ -11779,13 +11801,16 @@ void main() { final gaps = await adding; expect(gaps?.notSaved, isTrue); + // The image's copy, due after the switch, is not queued. expect( - container - .read(strategyOpQueueProvider) - .pending - .where((pending) => pending.op.type == StrategyOpType.elementAdd), + container.read(strategyOpQueueProvider).pending.where((pending) => + pending.op is ElementAddOp && + reader.addedPages.contains(pending.op.pagePublicId) && + pageCopyRoot((pending.op as ElementAddOp).elementPublicId) == + 'image'), isEmpty, ); + expect(reader.imageCopies, hasLength(1)); await _settle(); }); @@ -11802,7 +11827,8 @@ void main() { expect(DateTime.now().difference(started).inSeconds, lessThan(2)); expect(gaps?.imagesLeft, 1); expect( - elementCopies(sent).map((op) => pageCopyRoot(op.elementPublicId)), + onNewPages(sent, reader) + .map((op) => pageCopyRoot(op.elementPublicId)), ['text-page-2'], ); await _settle(); @@ -11810,7 +11836,7 @@ void main() { test('a lineup whose copy id could not be stored gets a plain one', () async { - final (container, sent, _) = await openToCopy(); + final (container, sent, reader) = await openToCopy(); final long = 'x' * 150; container.read(lineUpProvider.notifier).mergeRemote( lineUpGraphFromCloudRows([ @@ -11823,10 +11849,7 @@ void main() { final gaps = await container.read(strategyProvider.notifier).addPage(); expect(gaps?.notSaved, isFalse); - final page = sent.whereType().single.pagePublicId; - final lineup = sent - .whereType() - .singleWhere((op) => op.pagePublicId == page); + final lineup = onNewPages(sent, reader).single; expect(lineup.lineupPublicId, isNot(contains('~cp1~'))); expect( _entries(cloudPayloadData(lineup.payload), 'links').single['id'], @@ -11834,27 +11857,6 @@ void main() { ); await _settle(); }); - - test('leaves out an image still uploading and copies the rest', () async { - final (container, sent, _) = - await openToCopy(image: CloudImageCopyResult.uploading); - - await container.read(strategyProvider.notifier).addPage(); - - final page = sent.whereType().single.pagePublicId; - expect( - sent - .whereType() - .where((op) => op.pagePublicId == page) - .map((op) => pageCopyRoot(op.elementPublicId)), - ['text-page-2'], - ); - expect( - sent.whereType().where((op) => op.pagePublicId == page), - hasLength(1), - ); - await _settle(); - }); }); group('an image', () { @@ -12231,6 +12233,49 @@ class _PageReader extends Fake implements ConvexStrategyRepository { /// While set, copying an image's picture waits for it. Completer? imageCopyGate; + /// The pages added, by id; what each next add throws, in turn; and how + /// many adds were asked for. + final List addedPages = []; + final List addPageFailures = []; + int addPageCalls = 0; + + /// Called for each page added, as the server's read would then show it. + void Function(String pageId, int sortIndex)? onAddPage; + + @override + Future addPage({ + required String strategyPublicId, + required String pagePublicId, + required String name, + bool? isAutoNamed, + required int sortIndex, + required bool isAttack, + required int expectedRevision, + Map? settings, + }) async { + addPageCalls++; + lastAddedPage = ( + name: name, + isAutoNamed: isAutoNamed, + sortIndex: sortIndex, + isAttack: isAttack, + expectedRevision: expectedRevision, + settings: settings, + ); + if (addPageFailures.isNotEmpty) throw addPageFailures.removeAt(0); + addedPages.add(pagePublicId); + onAddPage?.call(pagePublicId, sortIndex); + } + + ({ + String name, + bool? isAutoNamed, + int sortIndex, + bool isAttack, + int expectedRevision, + Map? settings, + })? lastAddedPage; + /// The pictures copied, as (source, target) image ids. final List<(String, String)> imageCopies = []; From 914cf144ebc9a928e32353a1a3e922fa1a633ffa Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 14:14:11 -0400 Subject: [PATCH 05/11] Take back the client-side page copy; the server will copy Co-Authored-By: Claude Opus 5.5 --- lib/collab/convex_strategy_repository.dart | 7 - lib/const/line_provider.dart | 10 +- .../active_page_live_sync_provider.dart | 15 - .../collab/strategy_op_queue_provider.dart | 30 +- lib/providers/strategy_provider.dart | 394 +++--------------- lib/widgets/global_shortcuts.dart | 5 +- lib/widgets/new_page_copy_toast.dart | 28 -- lib/widgets/pages_bar.dart | 3 +- test/strategy_op_queue_provider_test.dart | 48 --- test/strategy_page_session_provider_test.dart | 348 ++-------------- 10 files changed, 103 insertions(+), 785 deletions(-) delete mode 100644 lib/widgets/new_page_copy_toast.dart diff --git a/lib/collab/convex_strategy_repository.dart b/lib/collab/convex_strategy_repository.dart index 4fc92c93..0cdd785b 100644 --- a/lib/collab/convex_strategy_repository.dart +++ b/lib/collab/convex_strategy_repository.dart @@ -714,13 +714,6 @@ bool isTypedConvexUnauthenticatedError(Object error) { error.rawCode == ConvexErrorCode.unauthenticated.wireName); } -bool isTypedConvexConflictError(Object error) { - return (error is ConvexFunctionException && - error.code == ConvexErrorCode.conflict) || - (error is ConvexClientFunctionError && - error.rawCode == ConvexErrorCode.conflict.wireName); -} - bool isTypedConvexNotFoundError(Object error) { return (error is ConvexFunctionException && error.code == ConvexErrorCode.notFound) || diff --git a/lib/const/line_provider.dart b/lib/const/line_provider.dart index 28e4821b..c8f5af24 100644 --- a/lib/const/line_provider.dart +++ b/lib/const/line_provider.dart @@ -375,14 +375,10 @@ class LineUpGraph { /// The lineups [linkIds] and the spots they aim at, ready to be put on /// another page, each under a new id that carries its original's (see - /// page_copy_id.dart), or that [newId] makes. Spots they share stay shared - /// in the copy. - LineUpGraph copyOfLinks( - Set linkIds, { - String Function(String id) newId = newPageCopyId, - }) { + /// page_copy_id.dart). Spots they share stay shared in the copy. + LineUpGraph copyOfLinks(Set linkIds) { final newIds = {}; - String renamed(String id) => newIds[id] ??= newId(id); + String renamed(String id) => newIds[id] ??= newPageCopyId(id); final part = linksWithSpots(linkIds); return LineUpGraph( origins: [ diff --git a/lib/providers/collab/active_page_live_sync_provider.dart b/lib/providers/collab/active_page_live_sync_provider.dart index 53a1fd46..d9fa0956 100644 --- a/lib/providers/collab/active_page_live_sync_provider.dart +++ b/lib/providers/collab/active_page_live_sync_provider.dart @@ -1005,21 +1005,6 @@ class ActivePageLiveSyncNotifier extends Notifier { return entities; } - /// The items of page [pageId], the page on screen, as the rows live sync - /// keeps for them: what a copy of the page sends. Lineups are left out; - /// their groups are written from the lineup graph. - List<({String publicId, CloudPayload payload, int sortIndex})> - elementRowsAsDrawn(String pageId) => [ - for (final entity in _normalizedLocalEntities(pageId).values) - if (entity.key.kind == EntitySyncKeyKind.element && - entity.payload is CloudPayload) - ( - publicId: entity.key.entityId!, - payload: entity.payload as CloudPayload, - sortIndex: entity.sortIndex ?? 0, - ), - ]; - Map _normalizedLocalEntities( String pageId) { final entities = {}; diff --git a/lib/providers/collab/strategy_op_queue_provider.dart b/lib/providers/collab/strategy_op_queue_provider.dart index 0b7ad1c1..cb680a69 100644 --- a/lib/providers/collab/strategy_op_queue_provider.dart +++ b/lib/providers/collab/strategy_op_queue_provider.dart @@ -572,34 +572,22 @@ class StrategyOpQueueNotifier extends Notifier { /// on screen by then, it shows the item from the server's copy, like a /// teammate's, instead of taking it as something the canvas removed. /// Returns whether the queue holds [op]: nothing on screen keeps it, so - /// when the outbox could not store it, the caller must say so. Given a - /// [strategyPublicId], [op] is queued only if that strategy is still the - /// active one when its turn to be written comes. + /// when the outbox could not store it, the caller must say so. Future enqueueOffCanvas( StrategyOp op, { bool flushImmediately = false, - String? strategyPublicId, }) async { final key = EntitySyncKey.forStrategyOp(op)!; final canvasSession = _canvasSession; final madeLive = liveStamp; - var otherStrategy = false; - await _serializeWrite(() async { - if (strategyPublicId != null && - state.strategyPublicId != strategyPublicId) { - otherStrategy = true; - return; - } - await _syncDesiredLocked( - keys: {key}, - desiredOps: {key: op}, - flushImmediately: flushImmediately, - canvasSession: canvasSession, - madeLive: madeLive, - onCanvas: false, - ); - }); - if (otherStrategy) return false; + await _serializeWrite(() => _syncDesiredLocked( + keys: {key}, + desiredOps: {key: op}, + flushImmediately: flushImmediately, + canvasSession: canvasSession, + madeLive: madeLive, + onCanvas: false, + )); return state.pending.any( (pending) => EntitySyncKey.forStrategyOp(pending.op) == key, ) || diff --git a/lib/providers/strategy_provider.dart b/lib/providers/strategy_provider.dart index 742a14a8..adb2638c 100644 --- a/lib/providers/strategy_provider.dart +++ b/lib/providers/strategy_provider.dart @@ -46,7 +46,6 @@ import 'package:icarus/providers/collab/remote_library_provider.dart'; import 'package:icarus/providers/collab/cloud_media_upload_queue_provider.dart'; import 'package:icarus/providers/auth_provider.dart'; import 'package:icarus/providers/collab/remote_strategy_snapshot_provider.dart'; -import 'package:icarus/providers/collab/active_page_live_sync_provider.dart'; import 'package:icarus/providers/collab/strategy_op_queue_provider.dart'; import 'package:icarus/providers/strategy_page_session_provider.dart'; import 'package:icarus/providers/strategy_save_state_provider.dart'; @@ -85,17 +84,9 @@ enum PageCopyResult { imageUnavailable, } -/// What of a cloud page could not be copied onto a new page: images whose -/// pictures could not be copied yet; whether some of the copy could not be -/// stored to send (or the strategy was left first); and whether the new page -/// could not be added at all (the cloud could not be reached). -typedef NewPageCopyGaps = ({int imagesLeft, bool notSaved, bool pageNotAdded}); - class StrategyProvider extends Notifier { @override StrategyState build() { - _disposed = false; - ref.onDispose(() => _disposed = true); _registerPersistenceTrackingListeners(); ref.listen(authProvider, (previous, next) { final strategyId = state.strategyId; @@ -1074,12 +1065,40 @@ class StrategyProvider extends Notifier { // the original's picture under its own id first. It shares the stored // bytes, so nothing is uploaded again. if (element.kind == 'image') { - final picture = await _copyImagePicture( - strategyId: strategyId, - imageId: widgetId, - copyId: copyId, - ); - if (picture != null) return picture; + final CloudImageCopyResult picture; + try { + picture = + await ref.read(convexStrategyRepositoryProvider).copyImageAsset( + strategyPublicId: strategyId, + sourceAssetPublicId: widgetId, + targetAssetPublicId: copyId, + ); + } catch (error) { + log('Could not copy the picture of image $widgetId: $error'); + return PageCopyResult.unreachable; + } + switch (picture) { + case CloudImageCopyResult.uploading: + return PageCopyResult.imageUploading; + case CloudImageCopyResult.unavailable: + // An image this device placed may not have reached the server yet. + // Its upload only goes once the image itself is saved to send + // (referenceDurable); one whose save failed never will. + final stillUploading = ref + .read(cloudMediaUploadQueueProvider) + .jobsForStrategy(strategyId) + .any( + (job) => + job.assetPublicId == widgetId && + job.referenceDurable && + job.state != CloudMediaJobState.failed, + ); + return stillUploading + ? PageCopyResult.imageUploading + : PageCopyResult.imageUnavailable; + case CloudImageCopyResult.copied: + break; + } if (state.strategyId != strategyId) return PageCopyResult.unavailable; } // The canvas never draws the copy: its page shows it from the server. @@ -1105,48 +1124,6 @@ class StrategyProvider extends Notifier { return PageCopyResult.copied; } - /// Has the server give image [copyId] the picture of image [imageId], - /// sharing its stored bytes. Null once it has; otherwise why not. - Future _copyImagePicture({ - required String strategyId, - required String imageId, - required String copyId, - }) async { - final CloudImageCopyResult picture; - try { - picture = await ref.read(convexStrategyRepositoryProvider).copyImageAsset( - strategyPublicId: strategyId, - sourceAssetPublicId: imageId, - targetAssetPublicId: copyId, - ); - } catch (error) { - log('Could not copy the picture of image $imageId: $error'); - return PageCopyResult.unreachable; - } - switch (picture) { - case CloudImageCopyResult.copied: - return null; - case CloudImageCopyResult.uploading: - return PageCopyResult.imageUploading; - case CloudImageCopyResult.unavailable: - // An image this device placed may not have reached the server yet. - // Its upload only goes once the image itself is saved to send - // (referenceDurable); one whose save failed never will. - final stillUploading = ref - .read(cloudMediaUploadQueueProvider) - .jobsForStrategy(strategyId) - .any( - (job) => - job.assetPublicId == imageId && - job.referenceDurable && - job.state != CloudMediaJobState.failed, - ); - return stillUploading - ? PageCopyResult.imageUploading - : PageCopyResult.imageUnavailable; - } - } - /// The items on [page] as the server has them with the work still queued /// for it laid over, refused work waiting for the user's choice included, /// by id, with their sortIndexes. @@ -1536,13 +1513,11 @@ class StrategyProvider extends Notifier { return null; } - /// Adds a copy of the page on screen after it and turns to it. On a cloud - /// strategy, returns what of the page could not be copied, if anything. - Future addPage([String? name]) async { - if (!_currentStrategyCanEditPages()) return null; + Future addPage([String? name]) async { + if (!_currentStrategyCanEditPages()) return; if (_currentStrategyIsCloud()) { final snapshot = ref.read(remoteEditorSnapshotProvider).valueOrNull; - if (snapshot == null) return null; + if (snapshot == null) return; final pages = [...snapshot.pages] ..sortBySortIndex((item) => item.sortIndex); final pageID = const Uuid().v4(); @@ -1553,72 +1528,28 @@ class StrategyProvider extends Notifier { final sourceIndex = activeIndex >= 0 ? activeIndex : pages.length - 1; final nextIndex = sourceIndex + 1; final isAutoNamed = name == null; - // The new page copies the page on screen as it is drawn now, as a - // local "+" does, each item and lineup under a copy id (see - // page_copy_id.dart) so the turn to it glides rather than fades. - final strategyId = state.strategyId; - if (strategyId == null) return null; - final elements = activeIndex >= 0 - ? ref - .read(activePageLiveSyncProvider.notifier) - .elementRowsAsDrawn(activePageId!) - : const <({String publicId, CloudPayload payload, int sortIndex})>[]; - final lineUps = ref.read(lineUpProvider).graph; - final lineUpCopies = lineUps.copyOfLinks( - {for (final link in lineUps.links) link.id}, - newId: (id) => _storableCopyId( - strategyId, - id, - (copyId) => EntitySyncKey.lineup(pageID, copyId), - ), - ); - final deadline = DateTime.now().add(cloudPageCopyLandingWait); - // The page is added on the server directly, not queued: its copy can - // only go once the page exists, and the call says plainly whether it - // does. Offline, no page is added. - if (!await _addCloudPageNow( - strategyId: strategyId, - pageId: pageID, - name: name ?? 'Page ${nextIndex + 1}', - isAutoNamed: isAutoNamed, + final ack = await _enqueueCloudPageDescriptorOp(PageAddOp( + opId: const Uuid().v4(), + pagePublicId: pageID, + payload: { + 'name': name ?? 'Page ${nextIndex + 1}', + 'isAutoNamed': isAutoNamed, + 'isAttack': pages.isNotEmpty ? pages[sourceIndex].isAttack : true, + 'settings': ref.read(strategySettingsProvider).toJson(), + }, sortIndex: nextIndex, - isAttack: pages.isNotEmpty ? pages[sourceIndex].isAttack : true, - expectedRevision: snapshot.header.revision, - )) { - return (imagesLeft: 0, notSaved: false, pageNotAdded: true); - } - final gaps = await _copyPageRowsToCloudPage( - strategyId: strategyId, - pageId: pageID, - elements: elements, - lineUps: lineUpCopies, - deadline: deadline, - ); - if (!_disposed && state.strategyId == strategyId) { - final remaining = deadline.difference(DateTime.now()); - try { - await ref - .read(remoteEditorSnapshotProvider.notifier) - .refresh() - .timeout(remaining.isNegative ? Duration.zero : remaining); - } on TimeoutException { - // The page is shown with whatever the read has by then. - } - if (!_disposed && state.strategyId == strategyId) { - unawaited( - ref - .read(strategyPageSessionProvider.notifier) - .setActivePageAnimated( - pageID, - direction: PageTransitionDirection.forward, - ) - .catchError((Object error) { - log('Could not turn to new cloud page $pageID: $error'); - }), - ); - } + expectedStrategyRevision: snapshot.header.revision, + )); + if (ack?.isAck ?? false) { + await ref.read(remoteEditorSnapshotProvider.notifier).refresh(); + await ref + .read(strategyPageSessionProvider.notifier) + .setActivePageAnimated( + pageID, + direction: PageTransitionDirection.forward, + ); } - return gaps; + return; } final box = Hive.box(HiveBoxNames.strategiesBox); @@ -1627,9 +1558,9 @@ class StrategyProvider extends Notifier { await _syncCurrentPageToHive(); final strategyId = state.strategyId; - if (strategyId == null) return null; + if (strategyId == null) return; final strat = box.get(strategyId); - if (strat == null || strat.pages.isEmpty) return null; + if (strat == null || strat.pages.isEmpty) return; final orderedPages = [...strat.pages] ..sortBySortIndex((item) => item.sortIndex); @@ -1659,213 +1590,6 @@ class StrategyProvider extends Notifier { await box.put(updated.id, updated); await setActivePageAnimated(newPage.id); - return null; - } - - /// A copy id for [id] (see page_copy_id.dart), or a plain id when a change - /// to the copy, stored under [keyOf] its id, would pass Hive's - /// 255-character key limit. Only an item imported with an unusually long - /// id gets a plain one, and it then fades between pages rather than - /// gliding. - String _storableCopyId( - String strategyId, - String id, - EntitySyncKey Function(String copyId) keyOf, - ) { - final copyId = newPageCopyId(id); - final accountId = ref.read(strategyOpQueueProvider).accountId; - final fits = accountId != null && - DurableOutboxRecord.createStorageKey( - accountId: accountId, - strategyPublicId: strategyId, - entityKey: keyOf(copyId), - ).length <= - 255; - return fits ? copyId : const Uuid().v4(); - } - - bool _disposed = false; - - /// Waits, until [deadline], for the queued ops [ids] of strategy - /// [strategyId] to leave the queue, keeping it sending meanwhile (it may - /// be busy with an earlier batch). Stops early once one is refused, the - /// strategy is left or this is disposed. Only paces the turn to a new - /// page: the ops stay queued either way. - Future _untilSent( - String strategyId, - Set ids, - DateTime deadline, - ) async { - final queue = ref.read(strategyOpQueueProvider.notifier); - var nextFlush = DateTime.now(); - while (!_disposed && state.strategyId == strategyId) { - final queueState = ref.read(strategyOpQueueProvider); - if (queueState.strategyPublicId != strategyId || - queueState.attentionByEntityKey.values - .any((intent) => ids.contains(intent.pending.op.opId)) || - !queueState.pending.any((pending) => ids.contains(pending.op.opId))) { - return; - } - final now = DateTime.now(); - if (!now.isBefore(deadline)) return; - if (!now.isBefore(nextFlush)) { - unawaited(queue.flushNow()); - nextFlush = now.add(const Duration(milliseconds: 500)); - } - await Future.delayed(const Duration(milliseconds: 50)); - } - } - - /// Adds cloud page [pageId] on the server now, rather than through the - /// queue, so what follows knows it exists. A teammate's change to the - /// strategy since [expectedRevision] is read and the add tried once more. - /// False when the page could not be added. - Future _addCloudPageNow({ - required String strategyId, - required String pageId, - required String name, - required bool isAutoNamed, - required int sortIndex, - required bool isAttack, - required int expectedRevision, - }) async { - final repository = ref.read(convexStrategyRepositoryProvider); - final settings = ref.read(strategySettingsProvider).toJson(); - Future add(int revision) => repository.addPage( - strategyPublicId: strategyId, - pagePublicId: pageId, - name: name, - isAutoNamed: isAutoNamed, - sortIndex: sortIndex, - isAttack: isAttack, - expectedRevision: revision, - settings: settings, - ); - try { - await add(expectedRevision); - return true; - } catch (error) { - if (!isTypedConvexConflictError(error) || _disposed) { - log('Could not add cloud page $pageId: $error'); - return false; - } - } - try { - await ref.read(remoteEditorSnapshotProvider.notifier).refresh(); - final snapshot = ref.read(remoteEditorSnapshotProvider).valueOrNull; - if (_disposed || - snapshot == null || - snapshot.header.publicId != strategyId) { - return false; - } - await add(snapshot.header.revision); - return true; - } catch (error) { - log('Could not add cloud page $pageId: $error'); - return false; - } - } - - /// How long "+" on a cloud strategy waits, from the press, for the new - /// page's copies to be sent before showing it, so it opens full rather - /// than filling in. Past it, the page is shown and the queue keeps - /// sending. - @visibleForTesting - static Duration cloudPageCopyLandingWait = const Duration(seconds: 3); - - /// Sends copies of the page rows [elements] (under copy ids made here) and - /// the lineups [lineUps] (already under copy ids) to new cloud page - /// [pageId] of strategy [strategyId], then waits until [deadline] for - /// them to be sent. An image whose picture cannot be copied in time is - /// left out; leaving the strategy stops the copy. Returns what did not - /// make it. - Future _copyPageRowsToCloudPage({ - required String strategyId, - required String pageId, - required List<({String publicId, CloudPayload payload, int sortIndex})> - elements, - required LineUpGraph lineUps, - required DateTime deadline, - }) async { - final queue = ref.read(strategyOpQueueProvider.notifier); - // Ops go through the queue of the strategy open now, so none is queued - // once another is (the queue checks again as it writes each one). - bool stillOpen() => - !_disposed && - state.strategyId == strategyId && - ref.read(strategyOpQueueProvider).strategyPublicId == strategyId; - final sent = {}; - var imagesLeft = 0; - var notSaved = false; - Future send(StrategyOp op) async { - if (!stillOpen()) { - notSaved = true; - return; - } - if (await queue.enqueueOffCanvas(op, strategyPublicId: strategyId)) { - sent.add(op.opId); - } else { - notSaved = true; - } - } - - for (final element in elements) { - final copyId = _storableCopyId( - strategyId, - element.publicId, - (copyId) => EntitySyncKey.element(pageId, copyId), - ); - if (element.payload['kind'] == 'image') { - final remaining = deadline.difference(DateTime.now()); - final PageCopyResult? left; - try { - left = await _copyImagePicture( - strategyId: strategyId, - imageId: element.publicId, - copyId: copyId, - ).timeout(remaining.isNegative ? Duration.zero : remaining); - } on TimeoutException { - imagesLeft++; - continue; - } - if (left != null) { - imagesLeft++; - continue; - } - } - await send( - ElementAddOp( - opId: const Uuid().v4(), - elementPublicId: copyId, - pagePublicId: pageId, - payload: { - ...element.payload, - 'data': {...cloudPayloadData(element.payload), 'id': copyId}, - }, - sortIndex: element.sortIndex, - ), - ); - } - for (final (index, row) in cloudLineupRows(lineUps).rows.indexed) { - await send( - LineupAddOp( - opId: const Uuid().v4(), - lineupPublicId: row.publicId, - pagePublicId: pageId, - payload: row.payload, - sortIndex: index, - ), - ); - } - if (sent.isNotEmpty && stillOpen()) { - ref.read(strategySaveStateProvider.notifier) - ..markDirty() - ..setPendingCloudSync(true) - ..setCloudSyncError(null); - unawaited(queue.flushNow()); - await _untilSent(strategyId, sent, deadline); - } - return (imagesLeft: imagesLeft, notSaved: notSaved, pageNotAdded: false); } Future renamePage(String pageId, String newName) async { diff --git a/lib/widgets/global_shortcuts.dart b/lib/widgets/global_shortcuts.dart index 90aabd2a..4f3807af 100644 --- a/lib/widgets/global_shortcuts.dart +++ b/lib/widgets/global_shortcuts.dart @@ -22,7 +22,6 @@ import 'package:icarus/widgets/rotate_helpers.dart'; import 'package:icarus/config/platform_policy.dart'; import 'package:icarus/widgets/platform_feature_toast.dart'; import 'package:uuid/uuid.dart'; -import 'package:icarus/widgets/new_page_copy_toast.dart'; class GlobalShortcuts extends ConsumerStatefulWidget { const GlobalShortcuts({super.key, required this.child}); @@ -144,9 +143,7 @@ class _GlobalShortcutsState extends ConsumerState onInvoke: (intent) async { if (!capabilities.canAddPage) return null; _dismissDeleteMenu(); - showNewPageCopyGaps( - await ref.read(strategyProvider.notifier).addPage(), - ); + await ref.read(strategyProvider.notifier).addPage(); return null; }, ), diff --git a/lib/widgets/new_page_copy_toast.dart b/lib/widgets/new_page_copy_toast.dart deleted file mode 100644 index b46c263f..00000000 --- a/lib/widgets/new_page_copy_toast.dart +++ /dev/null @@ -1,28 +0,0 @@ -import 'package:icarus/const/settings.dart'; -import 'package:icarus/providers/strategy_provider.dart'; - -/// Says what of the page on screen a new cloud page could not copy, if -/// anything (see StrategyProvider.addPage). -void showNewPageCopyGaps(NewPageCopyGaps? gaps) { - if (gaps == null) return; - if (gaps.pageNotAdded) { - Settings.showToast( - message: "Couldn't reach the cloud, so no page was added.", - backgroundColor: Settings.tacticalVioletTheme.destructive, - ); - } else if (gaps.notSaved) { - Settings.showToast( - message: "Couldn't save all of the page's copy on this device, so some " - 'of it is missing from the new page.', - backgroundColor: Settings.tacticalVioletTheme.destructive, - ); - } else if (gaps.imagesLeft > 0) { - Settings.showToast( - message: gaps.imagesLeft == 1 - ? "An image couldn't be copied yet, so it isn't on the new page." - : "${gaps.imagesLeft} images couldn't be copied yet, so they aren't " - 'on the new page.', - backgroundColor: Settings.tacticalVioletTheme.primary, - ); - } -} diff --git a/lib/widgets/pages_bar.dart b/lib/widgets/pages_bar.dart index 1f79de23..6c1fecdd 100644 --- a/lib/widgets/pages_bar.dart +++ b/lib/widgets/pages_bar.dart @@ -19,7 +19,6 @@ import 'package:icarus/widgets/dialogs/delete_page_dialog.dart'; import 'package:icarus/widgets/dialogs/recently_deleted_dialog.dart'; import 'package:shadcn_ui/shadcn_ui.dart'; import 'package:toastification/toastification.dart'; -import 'package:icarus/widgets/new_page_copy_toast.dart'; const double _pagesBarCornerRadius = 12; const double _pagesBarFooterHeight = 48; @@ -217,7 +216,7 @@ class _PagesBarState extends ConsumerState { Future _addPage() async { final caps = ref.read(currentStrategyCapabilitiesProvider); if (!caps.canAddPage) return; - showNewPageCopyGaps(await ref.read(strategyProvider.notifier).addPage()); + await ref.read(strategyProvider.notifier).addPage(); } Future _selectPage(String id) async { diff --git a/test/strategy_op_queue_provider_test.dart b/test/strategy_op_queue_provider_test.dart index d53e39a4..c0df6cc7 100644 --- a/test/strategy_op_queue_provider_test.dart +++ b/test/strategy_op_queue_provider_test.dart @@ -780,54 +780,6 @@ void main() { expect(container.read(strategyOpQueueProvider).pending, hasLength(1)); }); - test( - 'work queued off the canvas for a strategy is dropped if another opens ' - 'before its write', () async { - final store = _BlockingStore(); - final container = ProviderContainer(overrides: [ - durableStrategyOutboxStoreProvider.overrideWithValue(store), - strategyOutboxSessionProvider.overrideWithValue( - const StrategyOutboxSession( - accountId: null, - isReady: false, - hasAuthIncident: false, - ), - ), - ]); - addTearDown(container.dispose); - final notifier = container.read(strategyOpQueueProvider.notifier) - ..setActiveStrategy('strategy-1', accountId: 'account-a'); - container - .read(cloudCollabModeProvider.notifier) - .setForceLocalFallback(true); - // An earlier write holds the queue while a copy for strategy 1 waits - // its turn; meanwhile strategy 2 opens. - final earlier = notifier.enqueue(_cloudElementOp()); - final copy = notifier.enqueueOffCanvas( - const ElementAddOp( - opId: 'copy-1', - elementPublicId: 'element-1~cp1~0f8fad5b-d9cb-469f-a165-70867728950e', - pagePublicId: 'page-2', - payload: {'value': 'copy'}, - sortIndex: 0, - ), - strategyPublicId: 'strategy-1', - ); - await Future.delayed(Duration.zero); - notifier.setActiveStrategy('strategy-2', accountId: 'account-a'); - store.allowWrite.complete(); - await earlier; - - expect(await copy, isFalse); - expect( - container - .read(strategyOpQueueProvider) - .pending - .where((pending) => pending.op.opId == 'copy-1'), - isEmpty, - ); - }); - test('replacement stays hidden until the durable record is written', () async { final store = _BlockingReplacementStore(); diff --git a/test/strategy_page_session_provider_test.dart b/test/strategy_page_session_provider_test.dart index 43a48c82..dfb79923 100644 --- a/test/strategy_page_session_provider_test.dart +++ b/test/strategy_page_session_provider_test.dart @@ -278,13 +278,8 @@ class _FakeStrategyOpQueueNotifier extends StrategyOpQueueNotifier { Future enqueueOffCanvas( StrategyOp op, { bool flushImmediately = false, - String? strategyPublicId, }) async { if (offCanvasStoreFails) return false; - if (strategyPublicId != null && - strategyPublicId != state.strategyPublicId) { - return false; - } final key = EntitySyncKey.forStrategyOp(op)!; await syncDesiredOpsForPage( pageId: key.pageId!, @@ -1497,50 +1492,42 @@ void main() { expect(queue.flushNowCount, 1); }); - test('cloud page add is added on the server with its descriptor and content', - () async { + test('cloud page add is persisted with its descriptor and content', () async { final page = _page('page-1', 0); final queue = _FakeStrategyOpQueueNotifier(); - final reader = _PageReader({}); - final remote = _FakeRemoteEditorNotifier(_editorSnapshot( - pages: [page], - activePage: _pageSnapshot(page), - shellRevision: 8, - )); - reader.onAddPage = (pageId, sortIndex) { - final added = _page(pageId, sortIndex); - remote.pageCatalog[pageId] = _pageSnapshot(added); - remote.initialSnapshot = _editorSnapshot( - pages: [page, added], - activePage: _pageSnapshot(page), - shellRevision: 9, - ); - }; final container = await _cloudContainer( - remote: remote, + remote: _FakeRemoteEditorNotifier(_editorSnapshot( + pages: [page], + activePage: _pageSnapshot(page), + shellRevision: 8, + )), queue: queue, - repository: reader, - ); - - final gaps = - await container.read(strategyProvider.notifier).addPage('Execute'); - - expect(gaps?.pageNotAdded, isFalse); - expect(reader.addedPages, hasLength(1)); - final added = reader.lastAddedPage!; - expect(added.name, 'Execute'); - expect(added.isAutoNamed, isFalse); - expect(added.sortIndex, 1); - expect(added.isAttack, isTrue); - expect(added.expectedRevision, 8); - expect(added.settings, container.read(strategySettingsProvider).toJson()); - // Nothing is queued for the page itself. + ); + + await container.read(strategyProvider.notifier).addPage('Execute'); + + final intent = container + .read(strategyOpQueueProvider) + .queuedByEntityKey + .entries + .single; + final pending = intent.value.pending; + expect(pending.op.entityType, StrategyOpEntityType.page); + expect(pending.op.kind, StrategyOpKind.add); + expect(pending.op.entityPublicId, isNotEmpty); + expect(pending.op.sortIndex, 1); + expect(pending.op.expectedRevision, 8); + expect(pending.op.payload, { + 'name': 'Execute', + 'isAutoNamed': false, + 'isAttack': true, + 'settings': container.read(strategySettingsProvider).toJson(), + }); expect( - queue.state.queuedByEntityKey.keys - .where((key) => key.kind == EntitySyncKeyKind.pageDescriptor), - isEmpty, + intent.key, + EntitySyncKey.pageDescriptor(pending.op.entityPublicId!), ); - await _settle(); + expect(queue.flushNowCount, 1); }); test('cloud page rename is persisted with the page revision', () async { @@ -11386,11 +11373,6 @@ void main() { final container = await _cloudContainer( remote: _FakeRemoteEditorNotifier( _editorSnapshot(pages: pages, activePage: onScreen), - pageCatalog: { - 'page-1': _pageSnapshot(pages[0]), - 'page-2': onScreen, - 'page-3': _pageSnapshot(pages[2], elements: nextPage), - }, ), queue: queue, repository: reader, @@ -11632,233 +11614,6 @@ void main() { expect(adds(container).single.pagePublicId, 'page-3'); }); - group('"+" on a cloud strategy', () { - /// Opens page 2 with its text, a lineup and an image on the canvas. - /// The server adds pages asked for, and answers every op sent; returns - /// every op sent. - Future<(ProviderContainer, List, _PageReader)> openToCopy({ - CloudImageCopyResult image = CloudImageCopyResult.copied, - }) async { - final (container, queue, reader) = await open(); - reader.imageCopy = image; - final sent = []; - final remote = container.read(remoteEditorSnapshotProvider.notifier) - as _FakeRemoteEditorNotifier; - reader.onAddPage = (pageId, sortIndex) { - final page = _page(pageId, sortIndex); - remote.pageCatalog[page.publicId] = _pageSnapshot(page); - remote.initialSnapshot = _editorSnapshot( - pages: [...pages, page], - activePage: remote.initialSnapshot.activePage!, - ); - }; - queue.onFlush = () { - sent.addAll([ - for (final intent in queue.state.queuedByEntityKey.values) - intent.pending.op, - ]); - queue.ackQueued(); - }; - container.read(placedImageProvider.notifier).fromHive([ - PlacedImage( - id: 'image', - position: const Offset(12, 34), - aspectRatio: 1.5, - scale: 100, - fileExtension: '.png', - ), - ]); - container.read(lineUpProvider.notifier).mergeRemote( - lineUpGraphFromCloudRows([ - CloudLineupRow.remote(_lineup('page-2', 'a')), - ]).graph, - ); - await _settle(); - sent.clear(); - return (container, sent, reader); - } - - setUp(() => StrategyProvider.cloudPageCopyLandingWait = - const Duration(seconds: 2)); - tearDown(() => StrategyProvider.cloudPageCopyLandingWait = - const Duration(seconds: 3)); - - /// The ops sent for pages the server was asked to add. - Iterable onNewPages( - List sent, - _PageReader reader, - ) => - sent - .whereType() - .where((op) => reader.addedPages.contains(op.pagePublicId)); - - test('adds the page, then copies the page on screen under copy ids', - () async { - final (container, sent, reader) = await openToCopy(); - - await container.read(strategyProvider.notifier).addPage(); - - expect(reader.addedPages, hasLength(1)); - expect(sent.whereType(), isEmpty); - final elements = onNewPages(sent, reader).toList(); - expect( - { - for (final op in elements) - pageCopyRoot(op.elementPublicId): op.payload['kind'], - }, - {'text-page-2': 'text', 'image': 'image'}, - ); - for (final op in elements) { - expect(op.elementPublicId, contains('~cp1~')); - expect(cloudPayloadData(op.payload)['id'], op.elementPublicId); - } - final text = elements.singleWhere( - (op) => pageCopyRoot(op.elementPublicId) == 'text-page-2'); - expect(cloudPayloadData(text.payload)['text'], 'two'); - final image = elements - .singleWhere((op) => pageCopyRoot(op.elementPublicId) == 'image'); - expect(reader.imageCopies, [('image', image.elementPublicId)]); - - final lineup = onNewPages(sent, reader).single; - final data = cloudPayloadData(lineup.payload); - expect( - _entries(data, 'links').map((link) => pageCopyRoot(link['id'])), - ['link-a'], - ); - expect(pageCopyRoot(_entries(data, 'origins').single['id']), 'a'); - // The page copied from is left as it was. - expect( - sent.where((op) => - op.pagePublicId == 'page-2' && - (op is ElementDeleteOp || op is LineupDeleteOp)), - isEmpty, - ); - await _settle(); - }); - - test('a page the cloud cannot add gets no copy, and says so', () async { - final (container, sent, reader) = await openToCopy(); - reader.addPageFailures.add(const SocketException('offline')); - - final gaps = await container.read(strategyProvider.notifier).addPage(); - - expect(gaps?.pageNotAdded, isTrue); - expect(sent.whereType(), isEmpty); - expect(sent.whereType(), isEmpty); - await _settle(); - }); - - test('a teammate\'s change to the strategy is read, then the page added', - () async { - final (container, sent, reader) = await openToCopy(); - reader.addPageFailures.add(const ConvexFunctionException( - code: ConvexErrorCode.conflict, - rawCode: 'CONFLICT', - message: 'stale revision', - )); - - final gaps = await container.read(strategyProvider.notifier).addPage(); - - expect(gaps?.pageNotAdded, isFalse); - expect(reader.addPageCalls, 2); - expect(onNewPages(sent, reader), hasLength(2)); - await _settle(); - }); - - test('leaves out an image still uploading and copies the rest', () async { - final (container, sent, reader) = - await openToCopy(image: CloudImageCopyResult.uploading); - - final gaps = await container.read(strategyProvider.notifier).addPage(); - - expect(gaps?.imagesLeft, 1); - expect( - onNewPages(sent, reader) - .map((op) => pageCopyRoot(op.elementPublicId)), - ['text-page-2'], - ); - expect(onNewPages(sent, reader), hasLength(1)); - await _settle(); - }); - - test('leaving the strategy while copying stops the copy', () async { - final (container, sent, reader) = await openToCopy(); - final picture = reader.imageCopyGate = Completer(); - - final adding = container.read(strategyProvider.notifier).addPage(); - await _until(() => reader.addedPages.isNotEmpty); - // While the image's picture is copied, another strategy opens. - container.read(strategyProvider.notifier).setFromState( - const StrategyState( - strategyId: 'other-strategy', - strategyName: 'Other', - source: StrategySource.cloud, - storageDirectory: null, - isOpen: true, - ), - ); - picture.complete(); - final gaps = await adding; - - expect(gaps?.notSaved, isTrue); - // The image's copy, due after the switch, is not queued. - expect( - container.read(strategyOpQueueProvider).pending.where((pending) => - pending.op is ElementAddOp && - reader.addedPages.contains(pending.op.pagePublicId) && - pageCopyRoot((pending.op as ElementAddOp).elementPublicId) == - 'image'), - isEmpty, - ); - expect(reader.imageCopies, hasLength(1)); - await _settle(); - }); - - test('an image whose picture takes too long is left out, in time', - () async { - final (container, sent, reader) = await openToCopy(); - StrategyProvider.cloudPageCopyLandingWait = - const Duration(milliseconds: 300); - reader.imageCopyGate = Completer(); - - final started = DateTime.now(); - final gaps = await container.read(strategyProvider.notifier).addPage(); - - expect(DateTime.now().difference(started).inSeconds, lessThan(2)); - expect(gaps?.imagesLeft, 1); - expect( - onNewPages(sent, reader) - .map((op) => pageCopyRoot(op.elementPublicId)), - ['text-page-2'], - ); - await _settle(); - }); - - test('a lineup whose copy id could not be stored gets a plain one', - () async { - final (container, sent, reader) = await openToCopy(); - final long = 'x' * 150; - container.read(lineUpProvider.notifier).mergeRemote( - lineUpGraphFromCloudRows([ - CloudLineupRow.remote(_lineup('page-2', long)), - ]).graph, - ); - await _settle(); - sent.clear(); - - final gaps = await container.read(strategyProvider.notifier).addPage(); - - expect(gaps?.notSaved, isFalse); - final lineup = onNewPages(sent, reader).single; - expect(lineup.lineupPublicId, isNot(contains('~cp1~'))); - expect( - _entries(cloudPayloadData(lineup.payload), 'links').single['id'], - lineup.lineupPublicId, - ); - await _settle(); - }); - }); - group('an image', () { /// Opens page 2 with a placed image on it, whose picture the server /// copies with [result] (null: the call fails, as when offline). @@ -12233,49 +11988,6 @@ class _PageReader extends Fake implements ConvexStrategyRepository { /// While set, copying an image's picture waits for it. Completer? imageCopyGate; - /// The pages added, by id; what each next add throws, in turn; and how - /// many adds were asked for. - final List addedPages = []; - final List addPageFailures = []; - int addPageCalls = 0; - - /// Called for each page added, as the server's read would then show it. - void Function(String pageId, int sortIndex)? onAddPage; - - @override - Future addPage({ - required String strategyPublicId, - required String pagePublicId, - required String name, - bool? isAutoNamed, - required int sortIndex, - required bool isAttack, - required int expectedRevision, - Map? settings, - }) async { - addPageCalls++; - lastAddedPage = ( - name: name, - isAutoNamed: isAutoNamed, - sortIndex: sortIndex, - isAttack: isAttack, - expectedRevision: expectedRevision, - settings: settings, - ); - if (addPageFailures.isNotEmpty) throw addPageFailures.removeAt(0); - addedPages.add(pagePublicId); - onAddPage?.call(pagePublicId, sortIndex); - } - - ({ - String name, - bool? isAutoNamed, - int sortIndex, - bool isAttack, - int expectedRevision, - Map? settings, - })? lastAddedPage; - /// The pictures copied, as (source, target) image ids. final List<(String, String)> imageCopies = []; From 374bdfd2586c4985345597a9f3b16609d4938334 Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 14:19:46 -0400 Subject: [PATCH 06/11] Ask the server to copy the page when "+" adds one in the cloud "+" queues the page add with the page on screen as its source; the server copies that page's content as it adds the page. Edits made just before are queued ahead of the add. Once the page lands the app turns to it, and says when images still uploading were left out. Co-Authored-By: Claude Opus 5.5 --- lib/collab/collab_models.dart | 13 ++- .../collab/strategy_op_queue_provider.dart | 9 +- lib/providers/strategy_provider.dart | 50 +++++++---- lib/widgets/global_shortcuts.dart | 5 +- lib/widgets/new_page_copy_toast.dart | 14 ++++ lib/widgets/pages_bar.dart | 5 +- test/collab_sync_models_test.dart | 31 +++++++ test/strategy_op_queue_provider_test.dart | 5 ++ test/strategy_page_session_provider_test.dart | 84 +++++++++++++++++++ 9 files changed, 198 insertions(+), 18 deletions(-) create mode 100644 lib/widgets/new_page_copy_toast.dart diff --git a/lib/collab/collab_models.dart b/lib/collab/collab_models.dart index aa213a2d..ce1994cf 100644 --- a/lib/collab/collab_models.dart +++ b/lib/collab/collab_models.dart @@ -323,13 +323,15 @@ sealed class StrategyOp { 'payload': payload, 'expectedStrategyRevision': expectedRevision, }, - PageAddOp() => { + PageAddOp(:final copyContentFromPagePublicId) => { 'opId': opId, 'type': type.wireName, 'pagePublicId': pagePublicId, 'payload': payload, 'sortIndex': sortIndex, 'expectedStrategyRevision': expectedRevision, + if (copyContentFromPagePublicId != null) + 'copyContentFromPagePublicId': copyContentFromPagePublicId, }, PagePatchOp() => { 'opId': opId, @@ -448,6 +450,8 @@ sealed class StrategyOp { sortIndex: _requiredInt(json['sortIndex']), expectedStrategyRevision: _requiredInt(json['expectedStrategyRevision']), + copyContentFromPagePublicId: + json['copyContentFromPagePublicId'] as String?, ), StrategyOpType.pagePatch => PagePatchOp( opId: opId, @@ -633,6 +637,7 @@ sealed class StrategyOp { :final payload, :final sortIndex, :final expectedStrategyRevision, + :final copyContentFromPagePublicId, ) => PageAddOp( opId: value, @@ -640,6 +645,7 @@ sealed class StrategyOp { payload: payload, sortIndex: sortIndex, expectedStrategyRevision: expectedStrategyRevision, + copyContentFromPagePublicId: copyContentFromPagePublicId, ), PagePatchOp( :final pagePublicId, @@ -821,6 +827,7 @@ final class PageAddOp extends StrategyOp { required this.payload, required this.sortIndex, required this.expectedStrategyRevision, + this.copyContentFromPagePublicId, }); @override final String opId; @@ -831,6 +838,10 @@ final class PageAddOp extends StrategyOp { @override final int sortIndex; final int expectedStrategyRevision; + + /// The page whose items and lineups the server copies onto this one as + /// it adds it ("+"), under copy ids (see page_copy_id.dart). + final String? copyContentFromPagePublicId; @override StrategyOpType get type => StrategyOpType.pageAdd; } diff --git a/lib/providers/collab/strategy_op_queue_provider.dart b/lib/providers/collab/strategy_op_queue_provider.dart index cb680a69..b1c19432 100644 --- a/lib/providers/collab/strategy_op_queue_provider.dart +++ b/lib/providers/collab/strategy_op_queue_provider.dart @@ -2676,6 +2676,7 @@ class StrategyOpQueueNotifier extends Notifier { payload: {...existing.payload, ...desired.payload}, sortIndex: existing.sortIndex, expectedStrategyRevision: existing.expectedStrategyRevision, + copyContentFromPagePublicId: existing.copyContentFromPagePublicId, ); } } @@ -2937,13 +2938,19 @@ class StrategyOpQueueNotifier extends Notifier { payload: payload, expectedStrategyRevision: revision, ), - PageAddOp(:final pagePublicId, :final payload, :final sortIndex) => + PageAddOp( + :final pagePublicId, + :final payload, + :final sortIndex, + :final copyContentFromPagePublicId, + ) => PageAddOp( opId: opId, pagePublicId: pagePublicId, payload: payload, sortIndex: sortIndex, expectedStrategyRevision: revision, + copyContentFromPagePublicId: copyContentFromPagePublicId, ), PagePatchOp(:final pagePublicId, :final payload) => PagePatchOp( opId: opId, diff --git a/lib/providers/strategy_provider.dart b/lib/providers/strategy_provider.dart index adb2638c..7760aaff 100644 --- a/lib/providers/strategy_provider.dart +++ b/lib/providers/strategy_provider.dart @@ -1513,11 +1513,15 @@ class StrategyProvider extends Notifier { return null; } - Future addPage([String? name]) async { - if (!_currentStrategyCanEditPages()) return; + /// Adds a copy of the page on screen after it, and turns to it. Returns + /// how many of the page's images the copy left out: in the cloud, an + /// image whose upload has not finished is not copied (see + /// convex/lib/contentCopy.ts). + Future addPage([String? name]) async { + if (!_currentStrategyCanEditPages()) return 0; if (_currentStrategyIsCloud()) { final snapshot = ref.read(remoteEditorSnapshotProvider).valueOrNull; - if (snapshot == null) return; + if (snapshot == null) return 0; final pages = [...snapshot.pages] ..sortBySortIndex((item) => item.sortIndex); final pageID = const Uuid().v4(); @@ -1528,6 +1532,15 @@ class StrategyProvider extends Notifier { final sourceIndex = activeIndex >= 0 ? activeIndex : pages.length - 1; final nextIndex = sourceIndex + 1; final isAutoNamed = name == null; + // Edits made just before "+" go ahead of the add, so the server's + // copy has them. + await ref.read(strategyPageSessionProvider.notifier).flushCurrentPage(); + final imagesOnScreen = activeIndex < 0 + ? const {} + : { + for (final image in ref.read(placedImageProvider).images) + pageCopyRoot(image.id), + }; final ack = await _enqueueCloudPageDescriptorOp(PageAddOp( opId: const Uuid().v4(), pagePublicId: pageID, @@ -1539,17 +1552,25 @@ class StrategyProvider extends Notifier { }, sortIndex: nextIndex, expectedStrategyRevision: snapshot.header.revision, + copyContentFromPagePublicId: + pages.isEmpty ? null : pages[sourceIndex].publicId, )); - if (ack?.isAck ?? false) { - await ref.read(remoteEditorSnapshotProvider.notifier).refresh(); - await ref - .read(strategyPageSessionProvider.notifier) - .setActivePageAnimated( - pageID, - direction: PageTransitionDirection.forward, - ); + if (!(ack?.isAck ?? false)) return 0; + await ref.read(remoteEditorSnapshotProvider.notifier).refresh(); + await ref + .read(strategyPageSessionProvider.notifier) + .setActivePageAnimated( + pageID, + direction: PageTransitionDirection.forward, + ); + if (ref.read(strategyPageSessionProvider).activePageId != pageID) { + return 0; } - return; + final imagesCopied = { + for (final image in ref.read(placedImageProvider).images) + pageCopyRoot(image.id), + }; + return imagesOnScreen.difference(imagesCopied).length; } final box = Hive.box(HiveBoxNames.strategiesBox); @@ -1558,9 +1579,9 @@ class StrategyProvider extends Notifier { await _syncCurrentPageToHive(); final strategyId = state.strategyId; - if (strategyId == null) return; + if (strategyId == null) return 0; final strat = box.get(strategyId); - if (strat == null || strat.pages.isEmpty) return; + if (strat == null || strat.pages.isEmpty) return 0; final orderedPages = [...strat.pages] ..sortBySortIndex((item) => item.sortIndex); @@ -1590,6 +1611,7 @@ class StrategyProvider extends Notifier { await box.put(updated.id, updated); await setActivePageAnimated(newPage.id); + return 0; } Future renamePage(String pageId, String newName) async { diff --git a/lib/widgets/global_shortcuts.dart b/lib/widgets/global_shortcuts.dart index 4f3807af..45e16560 100644 --- a/lib/widgets/global_shortcuts.dart +++ b/lib/widgets/global_shortcuts.dart @@ -22,6 +22,7 @@ import 'package:icarus/widgets/rotate_helpers.dart'; import 'package:icarus/config/platform_policy.dart'; import 'package:icarus/widgets/platform_feature_toast.dart'; import 'package:uuid/uuid.dart'; +import 'package:icarus/widgets/new_page_copy_toast.dart'; class GlobalShortcuts extends ConsumerStatefulWidget { const GlobalShortcuts({super.key, required this.child}); @@ -143,7 +144,9 @@ class _GlobalShortcutsState extends ConsumerState onInvoke: (intent) async { if (!capabilities.canAddPage) return null; _dismissDeleteMenu(); - await ref.read(strategyProvider.notifier).addPage(); + showImagesLeftOutOfNewPage( + await ref.read(strategyProvider.notifier).addPage(), + ); return null; }, ), diff --git a/lib/widgets/new_page_copy_toast.dart b/lib/widgets/new_page_copy_toast.dart new file mode 100644 index 00000000..7b14678a --- /dev/null +++ b/lib/widgets/new_page_copy_toast.dart @@ -0,0 +1,14 @@ +import 'package:icarus/const/settings.dart'; + +/// Says how many images a new page's copy left out, if any: in the cloud, +/// an image still uploading is not copied (see StrategyProvider.addPage). +void showImagesLeftOutOfNewPage(int imagesLeftOut) { + if (imagesLeftOut == 0) return; + Settings.showToast( + message: imagesLeftOut == 1 + ? "An image was still uploading, so it isn't on the new page." + : "$imagesLeftOut images were still uploading, so they aren't on " + 'the new page.', + backgroundColor: Settings.tacticalVioletTheme.primary, + ); +} diff --git a/lib/widgets/pages_bar.dart b/lib/widgets/pages_bar.dart index 6c1fecdd..7bbbcb80 100644 --- a/lib/widgets/pages_bar.dart +++ b/lib/widgets/pages_bar.dart @@ -19,6 +19,7 @@ import 'package:icarus/widgets/dialogs/delete_page_dialog.dart'; import 'package:icarus/widgets/dialogs/recently_deleted_dialog.dart'; import 'package:shadcn_ui/shadcn_ui.dart'; import 'package:toastification/toastification.dart'; +import 'package:icarus/widgets/new_page_copy_toast.dart'; const double _pagesBarCornerRadius = 12; const double _pagesBarFooterHeight = 48; @@ -216,7 +217,9 @@ class _PagesBarState extends ConsumerState { Future _addPage() async { final caps = ref.read(currentStrategyCapabilitiesProvider); if (!caps.canAddPage) return; - await ref.read(strategyProvider.notifier).addPage(); + showImagesLeftOutOfNewPage( + await ref.read(strategyProvider.notifier).addPage(), + ); } Future _selectPage(String id) async { diff --git a/test/collab_sync_models_test.dart b/test/collab_sync_models_test.dart index 65e975c3..a7aaaf13 100644 --- a/test/collab_sync_models_test.dart +++ b/test/collab_sync_models_test.dart @@ -62,6 +62,37 @@ void main() { expect(StrategyOp.fromJson(json), isA()); }); + test('a page add keeps the page it copies, through storage and retries', + () { + const op = PageAddOp( + opId: 'op-1', + pagePublicId: 'page-2', + payload: {'name': 'Page 2'}, + sortIndex: 1, + expectedStrategyRevision: 4, + copyContentFromPagePublicId: 'page-1', + ); + final json = op.toConvexJson(); + expect(json['copyContentFromPagePublicId'], 'page-1'); + final stored = StrategyOp.fromJson(json) as PageAddOp; + expect(stored.copyContentFromPagePublicId, 'page-1'); + expect( + (stored.withOpId('op-2') as PageAddOp).copyContentFromPagePublicId, + 'page-1', + ); + // A page added without copying sends no copy field, as before. + expect( + const PageAddOp( + opId: 'op-3', + pagePublicId: 'page-3', + payload: {}, + sortIndex: 2, + expectedStrategyRevision: 4, + ).toConvexJson().containsKey('copyContentFromPagePublicId'), + isFalse, + ); + }); + test('withOpId changes identity without changing typed intent', () { const original = LineupPatchOp( opId: 'op-2', diff --git a/test/strategy_op_queue_provider_test.dart b/test/strategy_op_queue_provider_test.dart index c0df6cc7..251a34cc 100644 --- a/test/strategy_op_queue_provider_test.dart +++ b/test/strategy_op_queue_provider_test.dart @@ -157,6 +157,7 @@ void main() { }, sortIndex: 1, expectedStrategyRevision: 4, + copyContentFromPagePublicId: 'page-1', ), flushImmediately: false, ); @@ -171,6 +172,10 @@ void main() { expect(intent.value.pending.op.opId, 'add-page'); expect(intent.value.pending.op.kind, StrategyOpKind.add); expect(intent.value.pending.op.expectedRevision, 4); + expect( + (intent.value.pending.op as PageAddOp).copyContentFromPagePublicId, + 'page-1', + ); }); test('restart while in flight replays the same event key', () async { diff --git a/test/strategy_page_session_provider_test.dart b/test/strategy_page_session_provider_test.dart index dfb79923..b00edce2 100644 --- a/test/strategy_page_session_provider_test.dart +++ b/test/strategy_page_session_provider_test.dart @@ -1527,9 +1527,93 @@ void main() { intent.key, EntitySyncKey.pageDescriptor(pending.op.entityPublicId!), ); + // The server copies the page on screen onto it. + expect((pending.op as PageAddOp).copyContentFromPagePublicId, 'page-1'); expect(queue.flushNowCount, 1); }); + test( + '"+" on a cloud strategy turns to the copy the server made, and counts ' + 'the images it left out', () async { + RemoteElement image(String pageId, String id) => RemoteElement( + publicId: id, + strategyPublicId: 'cloud-strategy', + pagePublicId: pageId, + elementType: 'image', + payload: cloudElementPayload(kind: 'image', data: { + ...cloudImagePayloadFromPlacedImage(PlacedImage( + id: id, + position: const Offset(10, 20), + aspectRatio: 1, + scale: ImageScalePolicy.defaultWidth, + fileExtension: '.png', + )), + 'elementType': 'image', + }), + sortIndex: 0, + revision: 1, + deleted: false, + ); + final page = _page('page-1', 0); + final remote = _FakeRemoteEditorNotifier(_editorSnapshot( + pages: [page], + activePage: _pageSnapshot(page, elements: [ + image('page-1', 'uploaded'), + image('page-1', 'uploading'), + ]), + shellRevision: 8, + )); + final queue = _FakeStrategyOpQueueNotifier(); + final container = await _cloudContainer(remote: remote, queue: queue); + await container + .read(strategyPageSessionProvider.notifier) + .initializeForStrategy( + strategyId: 'cloud-strategy', + source: StrategySource.cloud, + selectFirstPageIfNeeded: true, + ); + await _settle(); + expect(container.read(placedImageProvider).images, hasLength(2)); + PageAddOp? sent; + queue.onFlush = () async { + final add = queue.state.queuedByEntityKey.values + .map((intent) => intent.pending.op) + .whereType() + .firstOrNull; + if (add == null) return queue.ackQueued(); + sent = add; + // The server adds the page with a copy of page 1, leaving out the + // image whose upload has not finished. + final added = _page(sent!.pagePublicId, 1); + remote.pageCatalog[added.publicId] = _pageSnapshot(added, elements: [ + image( + added.publicId, + 'uploaded~cp1~0f8fad5b-d9cb-469f-a165-70867728950e', + ), + ]); + remote.initialSnapshot = _editorSnapshot( + pages: [page, added], + activePage: remote.initialSnapshot.activePage!, + shellRevision: 9, + ); + queue.ackQueued(); + }; + + final leftOut = await container.read(strategyProvider.notifier).addPage(); + + expect(sent!.copyContentFromPagePublicId, 'page-1'); + expect( + container.read(strategyPageSessionProvider).activePageId, + sent!.pagePublicId, + ); + expect( + container.read(placedImageProvider).images.map((image) => image.id), + ['uploaded~cp1~0f8fad5b-d9cb-469f-a165-70867728950e'], + ); + expect(leftOut, 1); + await _settle(); + }); + test('cloud page rename is persisted with the page revision', () async { final page = _page('page-1', 0, revision: 6); final queue = _FakeStrategyOpQueueNotifier(); From cb48e389b7476ac0777b6e8bc5654fe4c954bbb7 Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 14:42:34 -0400 Subject: [PATCH 07/11] Follow "+" to the server's answer, and say what the copy lacks "+" waits for its own add's answer rather than only the first send's, so a page added behind other work still opens; with no answer in five seconds it says the page will appear once it reaches the cloud. Edits to the copied page the server refused are named, since they aren't in the copy. Images left out are counted, not matched by id, as a copy of an item with a long id gets a plain one. Switching strategies mid-add stops it. Co-Authored-By: Claude Opus 5.5 --- lib/providers/strategy_provider.dart | 114 ++++++--- lib/widgets/global_shortcuts.dart | 2 +- lib/widgets/new_page_copy_toast.dart | 40 +++- lib/widgets/pages_bar.dart | 2 +- test/strategy_page_session_provider_test.dart | 218 +++++++++++++----- 5 files changed, 278 insertions(+), 98 deletions(-) diff --git a/lib/providers/strategy_provider.dart b/lib/providers/strategy_provider.dart index 7760aaff..01c9932a 100644 --- a/lib/providers/strategy_provider.dart +++ b/lib/providers/strategy_provider.dart @@ -84,6 +84,16 @@ enum PageCopyResult { imageUnavailable, } +/// What "+" could not finish at once (see StrategyProvider.addPage): images +/// the cloud copy left out because their uploads hadn't finished, edits to +/// the page the server refused (so not in its copy), and a page that hasn't +/// reached the cloud yet. +typedef NewPageGaps = ({ + int imagesLeftOut, + bool unsavedEditsLeftOut, + bool waitingForCloud, +}); + class StrategyProvider extends Notifier { @override StrategyState build() { @@ -405,6 +415,26 @@ class StrategyProvider extends Notifier { ..setCloudSyncError(null); } + /// The server's answer to queued op [opId], if it comes within [wait]. + /// None when it doesn't (offline, or behind other work): the op stays + /// queued and lands later. + Future _answerTo(String opId, Duration wait) async { + OpAck? answer(StrategyOpQueueState queue) => + queue.lastAcks.where((ack) => ack.opId == opId).firstOrNull; + final already = answer(ref.read(strategyOpQueueProvider)); + if (already != null) return already; + final answered = Completer(); + final subscription = ref.listen(strategyOpQueueProvider, (_, next) { + final ack = answer(next); + if (ack != null && !answered.isCompleted) answered.complete(ack); + }); + try { + return await answered.future.timeout(wait, onTimeout: () => null); + } finally { + subscription.close(); + } + } + Future _enqueueCloudPageDescriptorOp(StrategyOp op) async { await enqueueOps([op]); final queue = ref.read(strategyOpQueueProvider.notifier); @@ -1513,15 +1543,26 @@ class StrategyProvider extends Notifier { return null; } - /// Adds a copy of the page on screen after it, and turns to it. Returns - /// how many of the page's images the copy left out: in the cloud, an - /// image whose upload has not finished is not copied (see - /// convex/lib/contentCopy.ts). - Future addPage([String? name]) async { - if (!_currentStrategyCanEditPages()) return 0; + /// How long "+" waits for the server to add a cloud page before saying + /// it will appear once it gets there. + @visibleForTesting + static Duration cloudPageAddWait = const Duration(seconds: 5); + + /// Adds a copy of the page on screen after it, and turns to it. In the + /// cloud the server makes the copy (convex/lib/contentCopy.ts); what the + /// new page lacks, or that it hasn't landed yet, is returned for the + /// caller to say. + Future addPage([String? name]) async { + const none = ( + imagesLeftOut: 0, + unsavedEditsLeftOut: false, + waitingForCloud: false, + ); + if (!_currentStrategyCanEditPages()) return none; if (_currentStrategyIsCloud()) { + final strategyId = state.strategyId; final snapshot = ref.read(remoteEditorSnapshotProvider).valueOrNull; - if (snapshot == null) return 0; + if (strategyId == null || snapshot == null) return none; final pages = [...snapshot.pages] ..sortBySortIndex((item) => item.sortIndex); final pageID = const Uuid().v4(); @@ -1532,17 +1573,16 @@ class StrategyProvider extends Notifier { final sourceIndex = activeIndex >= 0 ? activeIndex : pages.length - 1; final nextIndex = sourceIndex + 1; final isAutoNamed = name == null; + final sourcePageId = pages.isEmpty ? null : pages[sourceIndex].publicId; // Edits made just before "+" go ahead of the add, so the server's // copy has them. await ref.read(strategyPageSessionProvider.notifier).flushCurrentPage(); - final imagesOnScreen = activeIndex < 0 - ? const {} - : { - for (final image in ref.read(placedImageProvider).images) - pageCopyRoot(image.id), - }; + if (state.strategyId != strategyId) return none; + final imagesOnScreen = + activeIndex < 0 ? 0 : ref.read(placedImageProvider).images.length; + final opId = const Uuid().v4(); final ack = await _enqueueCloudPageDescriptorOp(PageAddOp( - opId: const Uuid().v4(), + opId: opId, pagePublicId: pageID, payload: { 'name': name ?? 'Page ${nextIndex + 1}', @@ -1552,25 +1592,43 @@ class StrategyProvider extends Notifier { }, sortIndex: nextIndex, expectedStrategyRevision: snapshot.header.revision, - copyContentFromPagePublicId: - pages.isEmpty ? null : pages[sourceIndex].publicId, + copyContentFromPagePublicId: sourcePageId, )); - if (!(ack?.isAck ?? false)) return 0; + final answer = ack ?? await _answerTo(opId, cloudPageAddWait); + if (answer == null) { + // Offline, or behind other work: the page lands later. + return ( + imagesLeftOut: 0, + unsavedEditsLeftOut: false, + waitingForCloud: true, + ); + } + // A refusal waits in the sync panel like any other. + if (!answer.isAck || state.strategyId != strategyId) return none; + // Edits to the page the server refused are not in its copy. + final unsavedEditsLeftOut = ref + .read(strategyOpQueueProvider) + .attentionByEntityKey + .keys + .any((key) => key.pageId == sourcePageId); await ref.read(remoteEditorSnapshotProvider.notifier).refresh(); + if (state.strategyId != strategyId) return none; await ref .read(strategyPageSessionProvider.notifier) .setActivePageAnimated( pageID, direction: PageTransitionDirection.forward, ); - if (ref.read(strategyPageSessionProvider).activePageId != pageID) { - return 0; - } - final imagesCopied = { - for (final image in ref.read(placedImageProvider).images) - pageCopyRoot(image.id), - }; - return imagesOnScreen.difference(imagesCopied).length; + final turned = + ref.read(strategyPageSessionProvider).activePageId == pageID; + final imagesCopied = ref.read(placedImageProvider).images.length; + return ( + // Counted, not matched by id: a copy of an item with a long id + // gets a plain id (pageCopyId). + imagesLeftOut: turned ? max(0, imagesOnScreen - imagesCopied) : 0, + unsavedEditsLeftOut: unsavedEditsLeftOut, + waitingForCloud: false, + ); } final box = Hive.box(HiveBoxNames.strategiesBox); @@ -1579,9 +1637,9 @@ class StrategyProvider extends Notifier { await _syncCurrentPageToHive(); final strategyId = state.strategyId; - if (strategyId == null) return 0; + if (strategyId == null) return none; final strat = box.get(strategyId); - if (strat == null || strat.pages.isEmpty) return 0; + if (strat == null || strat.pages.isEmpty) return none; final orderedPages = [...strat.pages] ..sortBySortIndex((item) => item.sortIndex); @@ -1611,7 +1669,7 @@ class StrategyProvider extends Notifier { await box.put(updated.id, updated); await setActivePageAnimated(newPage.id); - return 0; + return none; } Future renamePage(String pageId, String newName) async { diff --git a/lib/widgets/global_shortcuts.dart b/lib/widgets/global_shortcuts.dart index 45e16560..dd21d740 100644 --- a/lib/widgets/global_shortcuts.dart +++ b/lib/widgets/global_shortcuts.dart @@ -144,7 +144,7 @@ class _GlobalShortcutsState extends ConsumerState onInvoke: (intent) async { if (!capabilities.canAddPage) return null; _dismissDeleteMenu(); - showImagesLeftOutOfNewPage( + showNewPageGaps( await ref.read(strategyProvider.notifier).addPage(), ); return null; diff --git a/lib/widgets/new_page_copy_toast.dart b/lib/widgets/new_page_copy_toast.dart index 7b14678a..114070d1 100644 --- a/lib/widgets/new_page_copy_toast.dart +++ b/lib/widgets/new_page_copy_toast.dart @@ -1,14 +1,32 @@ import 'package:icarus/const/settings.dart'; +import 'package:icarus/providers/strategy_provider.dart'; -/// Says how many images a new page's copy left out, if any: in the cloud, -/// an image still uploading is not copied (see StrategyProvider.addPage). -void showImagesLeftOutOfNewPage(int imagesLeftOut) { - if (imagesLeftOut == 0) return; - Settings.showToast( - message: imagesLeftOut == 1 - ? "An image was still uploading, so it isn't on the new page." - : "$imagesLeftOut images were still uploading, so they aren't on " - 'the new page.', - backgroundColor: Settings.tacticalVioletTheme.primary, - ); +/// Says what "+" could not finish at once, if anything (see +/// StrategyProvider.addPage). +void showNewPageGaps(NewPageGaps gaps) { + if (gaps.waitingForCloud) { + Settings.showToast( + message: "The new page hasn't reached the cloud yet. It will appear " + 'once it does.', + backgroundColor: Settings.tacticalVioletTheme.primary, + ); + return; + } + if (gaps.unsavedEditsLeftOut) { + Settings.showToast( + message: "Changes to the page you copied that didn't save aren't in the " + 'copy.', + backgroundColor: Settings.tacticalVioletTheme.destructive, + ); + } + final images = gaps.imagesLeftOut; + if (images > 0) { + Settings.showToast( + message: images == 1 + ? "An image was still uploading, so it isn't on the new page." + : "$images images were still uploading, so they aren't on the new " + 'page.', + backgroundColor: Settings.tacticalVioletTheme.primary, + ); + } } diff --git a/lib/widgets/pages_bar.dart b/lib/widgets/pages_bar.dart index 7bbbcb80..a5d6efa6 100644 --- a/lib/widgets/pages_bar.dart +++ b/lib/widgets/pages_bar.dart @@ -217,7 +217,7 @@ class _PagesBarState extends ConsumerState { Future _addPage() async { final caps = ref.read(currentStrategyCapabilitiesProvider); if (!caps.canAddPage) return; - showImagesLeftOutOfNewPage( + showNewPageGaps( await ref.read(strategyProvider.notifier).addPage(), ); } diff --git a/test/strategy_page_session_provider_test.dart b/test/strategy_page_session_provider_test.dart index b00edce2..4ddfbf7f 100644 --- a/test/strategy_page_session_provider_test.dart +++ b/test/strategy_page_session_provider_test.dart @@ -683,6 +683,9 @@ class _ServerRepository implements ConvexStrategyRepository { dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); } +const _copyUuid = '0f8fad5b-d9cb-469f-a165-70867728950e'; +const _copyUuid2 = '7c9e6679-7425-40de-944b-e07fc1f90ae7'; + RemotePage _page(String id, int index, {int revision = 1, String? name, bool isAttack = true}) { final now = DateTime.utc(2026); @@ -1532,9 +1535,7 @@ void main() { expect(queue.flushNowCount, 1); }); - test( - '"+" on a cloud strategy turns to the copy the server made, and counts ' - 'the images it left out', () async { + group('"+" on a cloud strategy', () { RemoteElement image(String pageId, String id) => RemoteElement( publicId: id, strategyPublicId: 'cloud-strategy', @@ -1554,64 +1555,167 @@ void main() { revision: 1, deleted: false, ); - final page = _page('page-1', 0); - final remote = _FakeRemoteEditorNotifier(_editorSnapshot( - pages: [page], - activePage: _pageSnapshot(page, elements: [ - image('page-1', 'uploaded'), - image('page-1', 'uploading'), - ]), - shellRevision: 8, - )); - final queue = _FakeStrategyOpQueueNotifier(); - final container = await _cloudContainer(remote: remote, queue: queue); - await container - .read(strategyPageSessionProvider.notifier) - .initializeForStrategy( - strategyId: 'cloud-strategy', - source: StrategySource.cloud, - selectFirstPageIfNeeded: true, + + /// Opens page 1, showing two images. [land] makes the server add the + /// page "+" sends, with the copy of page 1 it makes, and answers it. + Future< + ( + ProviderContainer, + _FakeStrategyOpQueueNotifier, + void Function(PageAddOp add, List copied) land, + )> open() async { + final page = _page('page-1', 0); + final remote = _FakeRemoteEditorNotifier(_editorSnapshot( + pages: [page], + activePage: _pageSnapshot(page, elements: [ + image('page-1', 'uploaded'), + image('page-1', 'uploading'), + ]), + shellRevision: 8, + )); + final queue = _FakeStrategyOpQueueNotifier(); + final container = await _cloudContainer(remote: remote, queue: queue); + await container + .read(strategyPageSessionProvider.notifier) + .initializeForStrategy( + strategyId: 'cloud-strategy', + source: StrategySource.cloud, + selectFirstPageIfNeeded: true, + ); + await _settle(); + expect(container.read(placedImageProvider).images, hasLength(2)); + void land(PageAddOp add, List copied) { + final added = _page(add.pagePublicId, 1); + remote.pageCatalog[added.publicId] = + _pageSnapshot(added, elements: copied); + remote.initialSnapshot = _editorSnapshot( + pages: [page, added], + activePage: remote.initialSnapshot.activePage!, + shellRevision: 9, ); - await _settle(); - expect(container.read(placedImageProvider).images, hasLength(2)); - PageAddOp? sent; - queue.onFlush = () async { - final add = queue.state.queuedByEntityKey.values - .map((intent) => intent.pending.op) - .whereType() - .firstOrNull; - if (add == null) return queue.ackQueued(); - sent = add; - // The server adds the page with a copy of page 1, leaving out the - // image whose upload has not finished. - final added = _page(sent!.pagePublicId, 1); - remote.pageCatalog[added.publicId] = _pageSnapshot(added, elements: [ - image( - added.publicId, - 'uploaded~cp1~0f8fad5b-d9cb-469f-a165-70867728950e', - ), - ]); - remote.initialSnapshot = _editorSnapshot( - pages: [page, added], - activePage: remote.initialSnapshot.activePage!, - shellRevision: 9, + queue.ackQueued(); + } + + return (container, queue, land); + } + + PageAddOp? queuedAdd(_FakeStrategyOpQueueNotifier queue) => + queue.state.queuedByEntityKey.values + .map((intent) => intent.pending.op) + .whereType() + .firstOrNull; + + tearDown(() => StrategyProvider.cloudPageAddWait = + const Duration(seconds: 5)); + + test('turns to the copy the server made, and counts the images it left out', + () async { + final (container, queue, land) = await open(); + PageAddOp? sent; + queue.onFlush = () async { + final add = queuedAdd(queue); + if (add == null) return queue.ackQueued(); + sent = add; + // The server leaves out the image whose upload has not finished. + // The copy of the other has a plain id, as a copy of an item whose + // id is too long to keep its root does. + land(add, [ + image(add.pagePublicId, '0f8fad5b-d9cb-469f-a165-70867728950e'), + ]); + }; + + final gaps = await container.read(strategyProvider.notifier).addPage(); + + expect(sent!.copyContentFromPagePublicId, 'page-1'); + expect( + container.read(strategyPageSessionProvider).activePageId, + sent!.pagePublicId, ); - queue.ackQueued(); - }; + expect(container.read(placedImageProvider).images, hasLength(1)); + expect(gaps, ( + imagesLeftOut: 1, + unsavedEditsLeftOut: false, + waitingForCloud: false, + )); + await _settle(); + }); - final leftOut = await container.read(strategyProvider.notifier).addPage(); + test('an answer that comes after the first send still turns to the page', + () async { + final (container, queue, land) = await open(); + // The add waits behind another send; the server answers later. + PageAddOp? sent; + queue.onFlush = () async { + final add = queuedAdd(queue); + if (add == null) return queue.ackQueued(); + sent = add; + Timer(const Duration(milliseconds: 200), () { + land(add, [image(add.pagePublicId, 'uploaded~cp1~$_copyUuid')]); + }); + }; - expect(sent!.copyContentFromPagePublicId, 'page-1'); - expect( - container.read(strategyPageSessionProvider).activePageId, - sent!.pagePublicId, - ); - expect( - container.read(placedImageProvider).images.map((image) => image.id), - ['uploaded~cp1~0f8fad5b-d9cb-469f-a165-70867728950e'], - ); - expect(leftOut, 1); - await _settle(); + final gaps = await container.read(strategyProvider.notifier).addPage(); + + expect( + container.read(strategyPageSessionProvider).activePageId, + sent!.pagePublicId, + ); + expect(gaps.waitingForCloud, isFalse); + expect(gaps.imagesLeftOut, 1); + await _settle(); + }); + + test('says when the page has not reached the cloud yet', () async { + final (container, queue, _) = await open(); + StrategyProvider.cloudPageAddWait = const Duration(milliseconds: 200); + + final gaps = await container.read(strategyProvider.notifier).addPage(); + + expect(gaps.waitingForCloud, isTrue); + expect(container.read(strategyPageSessionProvider).activePageId, + 'page-1'); + // The add stays queued, to land later. + expect(queuedAdd(queue), isNotNull); + await _settle(); + }); + + test('says when edits to the page were refused, so are not in its copy', + () async { + final (container, queue, land) = await open(); + queue.onFlush = () async { + final add = queuedAdd(queue); + if (add == null) return queue.ackQueued(); + land(add, [ + image(add.pagePublicId, 'uploaded~cp1~$_copyUuid'), + image(add.pagePublicId, 'uploading~cp1~$_copyUuid2'), + ]); + // The server refused an edit to page 1 sent ahead of the add. + queue.state = queue.state.copyWith(attentionByEntityKey: { + const EntitySyncKey.element('page-1', 'uploaded'): + const QueuedEntityIntent( + entityKey: EntitySyncKey.element('page-1', 'uploaded'), + pending: PendingOp( + op: ElementDeleteOp( + opId: 'refused-op', + pagePublicId: 'page-1', + elementPublicId: 'uploaded', + expectedElementRevision: 1, + ), + clientId: 'test-client', + ), + ), + }); + }; + + final gaps = await container.read(strategyProvider.notifier).addPage(); + + expect(gaps, ( + imagesLeftOut: 0, + unsavedEditsLeftOut: true, + waitingForCloud: false, + )); + await _settle(); + }); }); test('cloud page rename is persisted with the page revision', () async { From 95788f4cc622bb674688de928546a39750d2875d Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 14:47:07 -0400 Subject: [PATCH 08/11] Don't wait on a server that never answers in the page add test Co-Authored-By: Claude Opus 5.5 --- test/strategy_page_session_provider_test.dart | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/strategy_page_session_provider_test.dart b/test/strategy_page_session_provider_test.dart index 4ddfbf7f..b14d2067 100644 --- a/test/strategy_page_session_provider_test.dart +++ b/test/strategy_page_session_provider_test.dart @@ -1506,6 +1506,10 @@ void main() { )), queue: queue, ); + // This server never answers; "+" stops waiting at once. + StrategyProvider.cloudPageAddWait = Duration.zero; + addTearDown(() => + StrategyProvider.cloudPageAddWait = const Duration(seconds: 5)); await container.read(strategyProvider.notifier).addPage('Execute'); From 70b1b837be3db8902bda415dbc5e77799793b966 Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 14:58:05 -0400 Subject: [PATCH 09/11] Say what a page copy may lack when "+" is pressed What the copy may lack is known when "+" is pressed, from the page on screen: images still uploading, which the server leaves out, and edits to the page the server refused, which it never had. Saying so then, rather than guessing afterwards from what landed, holds whether the add lands at once or later, and isn't fooled by a teammate's changes. The wait for the server's answer now starts before sending, so a stalled send still answers "+" in time. A page copy too large to make gets its own message in the sync panel. Co-Authored-By: Claude Opus 5.5 --- lib/collab/cloud_sync_error_message.dart | 7 +- lib/collab/collab_models.dart | 5 + lib/providers/strategy_provider.dart | 138 ++++++++++-------- lib/widgets/new_page_copy_toast.dart | 20 +-- .../collab/cloud_sync_error_message_test.dart | 9 ++ test/strategy_page_session_provider_test.dart | 110 ++++++++------ 6 files changed, 178 insertions(+), 111 deletions(-) diff --git a/lib/collab/cloud_sync_error_message.dart b/lib/collab/cloud_sync_error_message.dart index b2a2b3c2..397b63b3 100644 --- a/lib/collab/cloud_sync_error_message.dart +++ b/lib/collab/cloud_sync_error_message.dart @@ -35,7 +35,8 @@ bool isSpecificAttentionReason(String error) { lower.contains(lineupOverlapMessage.toLowerCase()) || lower.contains(retiredLineupOpMessage.toLowerCase()) || lower.contains(teammateDeletedMessage.toLowerCase()) || - lower.contains(pageDeletedMessage.toLowerCase()); + lower.contains(pageDeletedMessage.toLowerCase()) || + lower.contains(pageTooLargeToCopyMessage.toLowerCase()); } /// A cloud change whose write to the durable outbox failed, or could not be @@ -125,6 +126,10 @@ String friendlyCloudSyncError(String raw) { return 'A saved cloud change is paused after repeated failures. Retry ' 'when the connection and account are healthy.'; } + if (lower.contains(pageTooLargeToCopyMessage.toLowerCase())) { + return 'A new page was too large for the cloud to copy, so it was not ' + 'added. Keep mine tries again; Use cloud drops it.'; + } if (lower.contains('too large for cloud sync')) { return 'A saved change is too large for cloud sync. It remains saved on ' 'this device. Reduce it, then choose Keep mine to retry, or Use ' diff --git a/lib/collab/collab_models.dart b/lib/collab/collab_models.dart index ce1994cf..a498c5b3 100644 --- a/lib/collab/collab_models.dart +++ b/lib/collab/collab_models.dart @@ -54,6 +54,11 @@ const teammateDeletedCannotRestoreMessage = /// sent again. const pageDeletedMessage = 'This page was deleted'; +/// The server's refusal of a page add whose copy of another page +/// (PageAddOp.copyContentFromPagePublicId) is too large to make in one go +/// (PAGE_TOO_LARGE_TO_COPY, convex/ops.ts). +const pageTooLargeToCopyMessage = 'This page is too large to copy.'; + /// How long the server keeps a deleted page restorable (the server's /// PAGE_TRASH_RETENTION_MS), for copy that promises it. const pageTrashRetentionDays = 30; diff --git a/lib/providers/strategy_provider.dart b/lib/providers/strategy_provider.dart index 01c9932a..fe0994d8 100644 --- a/lib/providers/strategy_provider.dart +++ b/lib/providers/strategy_provider.dart @@ -84,13 +84,13 @@ enum PageCopyResult { imageUnavailable, } -/// What "+" could not finish at once (see StrategyProvider.addPage): images -/// the cloud copy left out because their uploads hadn't finished, edits to -/// the page the server refused (so not in its copy), and a page that hasn't -/// reached the cloud yet. +/// What a new cloud page may lack, and whether it has reached the cloud +/// yet (see StrategyProvider.addPage): images whose uploads hadn't +/// finished, which the server leaves out of its copy, and edits to the page +/// the server refused, which it never had. typedef NewPageGaps = ({ - int imagesLeftOut, - bool unsavedEditsLeftOut, + int imagesUploading, + bool unsavedEdits, bool waitingForCloud, }); @@ -415,26 +415,53 @@ class StrategyProvider extends Notifier { ..setCloudSyncError(null); } - /// The server's answer to queued op [opId], if it comes within [wait]. - /// None when it doesn't (offline, or behind other work): the op stays - /// queued and lands later. - Future _answerTo(String opId, Duration wait) async { - OpAck? answer(StrategyOpQueueState queue) => - queue.lastAcks.where((ack) => ack.opId == opId).firstOrNull; - final already = answer(ref.read(strategyOpQueueProvider)); - if (already != null) return already; + /// Queues [op], sends it, and returns the server's answer if it comes + /// within [wait] of the call, sending included. None when it doesn't + /// (offline, or behind other work): the op stays queued and lands later. + Future _sendAndAwaitAnswer(StrategyOp op, Duration wait) async { final answered = Completer(); + void answer(OpAck? ack) { + if (!answered.isCompleted) answered.complete(ack); + } + final subscription = ref.listen(strategyOpQueueProvider, (_, next) { - final ack = answer(next); - if (ack != null && !answered.isCompleted) answered.complete(ack); + final ack = next.lastAcks.where((ack) => ack.opId == op.opId).firstOrNull; + if (ack != null) answer(ack); }); + final deadline = Timer(wait, () => answer(null)); + unawaited(_enqueueCloudPageDescriptorOp(op).then( + (ack) { + if (ack != null) answer(ack); + }, + onError: (Object error) => log('Could not send ${op.opId}: $error'), + )); try { - return await answered.future.timeout(wait, onTimeout: () => null); + return await answered.future; } finally { + deadline.cancel(); subscription.close(); } } + /// Images on the page on screen whose uploads haven't finished, as far + /// as this device knows: the server's, or this device's own, still on + /// their way. + int _imagesStillUploading(String strategyId) { + final assets = + ref.read(remoteEditorSnapshotProvider).valueOrNull?.assetsById ?? + const {}; + final uploads = + ref.read(cloudMediaUploadQueueProvider).jobsForStrategy(strategyId); + return ref.read(placedImageProvider).images.where((image) { + final status = assets[image.id]?.uploadStatus; + if (status == 'active') return false; + return status == 'pending' || + uploads.any((job) => + job.assetPublicId == image.id && + job.state != CloudMediaJobState.failed); + }).length; + } + Future _enqueueCloudPageDescriptorOp(StrategyOp op) async { await enqueueOps([op]); final queue = ref.read(strategyOpQueueProvider.notifier); @@ -1550,12 +1577,12 @@ class StrategyProvider extends Notifier { /// Adds a copy of the page on screen after it, and turns to it. In the /// cloud the server makes the copy (convex/lib/contentCopy.ts); what the - /// new page lacks, or that it hasn't landed yet, is returned for the + /// copy may lack, or that it hasn't landed yet, is returned for the /// caller to say. Future addPage([String? name]) async { const none = ( - imagesLeftOut: 0, - unsavedEditsLeftOut: false, + imagesUploading: 0, + unsavedEdits: false, waitingForCloud: false, ); if (!_currentStrategyCanEditPages()) return none; @@ -1578,57 +1605,54 @@ class StrategyProvider extends Notifier { // copy has them. await ref.read(strategyPageSessionProvider.notifier).flushCurrentPage(); if (state.strategyId != strategyId) return none; - final imagesOnScreen = - activeIndex < 0 ? 0 : ref.read(placedImageProvider).images.length; - final opId = const Uuid().v4(); - final ack = await _enqueueCloudPageDescriptorOp(PageAddOp( - opId: opId, - pagePublicId: pageID, - payload: { - 'name': name ?? 'Page ${nextIndex + 1}', - 'isAutoNamed': isAutoNamed, - 'isAttack': pages.isNotEmpty ? pages[sourceIndex].isAttack : true, - 'settings': ref.read(strategySettingsProvider).toJson(), - }, - sortIndex: nextIndex, - expectedStrategyRevision: snapshot.header.revision, - copyContentFromPagePublicId: sourcePageId, - )); - final answer = ack ?? await _answerTo(opId, cloudPageAddWait); + // What the copy may lack is known now, from what the page on screen + // holds: the server copies the page as it has it. + final gaps = activeIndex < 0 + ? none + : ( + imagesUploading: _imagesStillUploading(strategyId), + unsavedEdits: ref + .read(strategyOpQueueProvider) + .attentionByEntityKey + .keys + .any((key) => key.pageId == sourcePageId), + waitingForCloud: false, + ); + final answer = await _sendAndAwaitAnswer( + PageAddOp( + opId: const Uuid().v4(), + pagePublicId: pageID, + payload: { + 'name': name ?? 'Page ${nextIndex + 1}', + 'isAutoNamed': isAutoNamed, + 'isAttack': pages.isNotEmpty ? pages[sourceIndex].isAttack : true, + 'settings': ref.read(strategySettingsProvider).toJson(), + }, + sortIndex: nextIndex, + expectedStrategyRevision: snapshot.header.revision, + copyContentFromPagePublicId: sourcePageId, + ), + cloudPageAddWait, + ); if (answer == null) { // Offline, or behind other work: the page lands later. return ( - imagesLeftOut: 0, - unsavedEditsLeftOut: false, + imagesUploading: gaps.imagesUploading, + unsavedEdits: gaps.unsavedEdits, waitingForCloud: true, ); } // A refusal waits in the sync panel like any other. - if (!answer.isAck || state.strategyId != strategyId) return none; - // Edits to the page the server refused are not in its copy. - final unsavedEditsLeftOut = ref - .read(strategyOpQueueProvider) - .attentionByEntityKey - .keys - .any((key) => key.pageId == sourcePageId); + if (!answer.isAck || state.strategyId != strategyId) return gaps; await ref.read(remoteEditorSnapshotProvider.notifier).refresh(); - if (state.strategyId != strategyId) return none; + if (state.strategyId != strategyId) return gaps; await ref .read(strategyPageSessionProvider.notifier) .setActivePageAnimated( pageID, direction: PageTransitionDirection.forward, ); - final turned = - ref.read(strategyPageSessionProvider).activePageId == pageID; - final imagesCopied = ref.read(placedImageProvider).images.length; - return ( - // Counted, not matched by id: a copy of an item with a long id - // gets a plain id (pageCopyId). - imagesLeftOut: turned ? max(0, imagesOnScreen - imagesCopied) : 0, - unsavedEditsLeftOut: unsavedEditsLeftOut, - waitingForCloud: false, - ); + return gaps; } final box = Hive.box(HiveBoxNames.strategiesBox); diff --git a/lib/widgets/new_page_copy_toast.dart b/lib/widgets/new_page_copy_toast.dart index 114070d1..cc0598c7 100644 --- a/lib/widgets/new_page_copy_toast.dart +++ b/lib/widgets/new_page_copy_toast.dart @@ -1,8 +1,8 @@ import 'package:icarus/const/settings.dart'; import 'package:icarus/providers/strategy_provider.dart'; -/// Says what "+" could not finish at once, if anything (see -/// StrategyProvider.addPage). +/// Says what a new page's copy may lack, and when the page hasn't reached +/// the cloud yet, if either (see StrategyProvider.addPage). void showNewPageGaps(NewPageGaps gaps) { if (gaps.waitingForCloud) { Settings.showToast( @@ -10,22 +10,22 @@ void showNewPageGaps(NewPageGaps gaps) { 'once it does.', backgroundColor: Settings.tacticalVioletTheme.primary, ); - return; } - if (gaps.unsavedEditsLeftOut) { + if (gaps.unsavedEdits) { Settings.showToast( - message: "Changes to the page you copied that didn't save aren't in the " - 'copy.', + message: "Changes to the page you copied that didn't save won't be in " + 'the copy.', backgroundColor: Settings.tacticalVioletTheme.destructive, ); } - final images = gaps.imagesLeftOut; + final images = gaps.imagesUploading; if (images > 0) { Settings.showToast( message: images == 1 - ? "An image was still uploading, so it isn't on the new page." - : "$images images were still uploading, so they aren't on the new " - 'page.', + ? 'An image on the page you copied was still uploading, so the ' + 'copy may not have it.' + : '$images images on the page you copied were still uploading, so ' + 'the copy may not have them.', backgroundColor: Settings.tacticalVioletTheme.primary, ); } diff --git a/test/collab/cloud_sync_error_message_test.dart b/test/collab/cloud_sync_error_message_test.dart index bf96130b..d568c1d5 100644 --- a/test/collab/cloud_sync_error_message_test.dart +++ b/test/collab/cloud_sync_error_message_test.dart @@ -72,6 +72,15 @@ void main() { ); }); + test('explains a page copy too large to make', () { + final message = friendlyCloudSyncError(pageTooLargeToCopyMessage); + + expect(message, contains('too large for the cloud to copy')); + expect(message, contains('Keep mine')); + expect(message, contains('Use cloud')); + expect(isSpecificAttentionReason(pageTooLargeToCopyMessage), isTrue); + }); + test('lineup refusals and oversized work are specific attention reasons', () { expect(isSpecificAttentionReason(lineupPageMismatchMessage), isTrue); expect(isSpecificAttentionReason(retiredLineupOpMessage), isTrue); diff --git a/test/strategy_page_session_provider_test.dart b/test/strategy_page_session_provider_test.dart index b14d2067..7070e714 100644 --- a/test/strategy_page_session_provider_test.dart +++ b/test/strategy_page_session_provider_test.dart @@ -684,7 +684,6 @@ class _ServerRepository implements ConvexStrategyRepository { } const _copyUuid = '0f8fad5b-d9cb-469f-a165-70867728950e'; -const _copyUuid2 = '7c9e6679-7425-40de-944b-e07fc1f90ae7'; RemotePage _page(String id, int index, {int revision = 1, String? name, bool isAttack = true}) { @@ -733,6 +732,7 @@ RemotePageSnapshot _pageSnapshot( CloudPayload settings = const {}, List? elements, List lineups = const [], + Map assetsById = const {}, }) { final now = DateTime.utc(2026); return RemotePageSnapshot( @@ -748,7 +748,7 @@ RemotePageSnapshot _pageSnapshot( ? const [] : [_textElement(page.publicId, 'text-${page.publicId}', text)]), lineups: lineups, - assetsById: const {}, + assetsById: assetsById, ); } @@ -1560,7 +1560,18 @@ void main() { deleted: false, ); - /// Opens page 1, showing two images. [land] makes the server add the + RemoteImageAsset asset(String id, String uploadStatus) => RemoteImageAsset( + publicId: id, + fileExtension: '.png', + width: 64, + height: 64, + url: uploadStatus == 'active' ? 'https://media.test/$id.png' : null, + legacyStoragePath: null, + provider: 'r2', + uploadStatus: uploadStatus, + ); + + /// Opens page 1, showing two images, one still uploading. [land] makes the server add the /// page "+" sends, with the copy of page 1 it makes, and answers it. Future< ( @@ -1571,10 +1582,17 @@ void main() { final page = _page('page-1', 0); final remote = _FakeRemoteEditorNotifier(_editorSnapshot( pages: [page], - activePage: _pageSnapshot(page, elements: [ - image('page-1', 'uploaded'), - image('page-1', 'uploading'), - ]), + activePage: _pageSnapshot( + page, + elements: [ + image('page-1', 'uploaded'), + image('page-1', 'uploading'), + ], + assetsById: { + 'uploaded': asset('uploaded', 'active'), + 'uploading': asset('uploading', 'pending'), + }, + ), shellRevision: 8, )); final queue = _FakeStrategyOpQueueNotifier(); @@ -1612,8 +1630,8 @@ void main() { tearDown(() => StrategyProvider.cloudPageAddWait = const Duration(seconds: 5)); - test('turns to the copy the server made, and counts the images it left out', - () async { + test('turns to the copy the server made, and warns of an image still ' + 'uploading', () async { final (container, queue, land) = await open(); PageAddOp? sent; queue.onFlush = () async { @@ -1621,11 +1639,7 @@ void main() { if (add == null) return queue.ackQueued(); sent = add; // The server leaves out the image whose upload has not finished. - // The copy of the other has a plain id, as a copy of an item whose - // id is too long to keep its root does. - land(add, [ - image(add.pagePublicId, '0f8fad5b-d9cb-469f-a165-70867728950e'), - ]); + land(add, [image(add.pagePublicId, 'uploaded~cp1~$_copyUuid')]); }; final gaps = await container.read(strategyProvider.notifier).addPage(); @@ -1637,8 +1651,8 @@ void main() { ); expect(container.read(placedImageProvider).images, hasLength(1)); expect(gaps, ( - imagesLeftOut: 1, - unsavedEditsLeftOut: false, + imagesUploading: 1, + unsavedEdits: false, waitingForCloud: false, )); await _settle(); @@ -1665,7 +1679,6 @@ void main() { sent!.pagePublicId, ); expect(gaps.waitingForCloud, isFalse); - expect(gaps.imagesLeftOut, 1); await _settle(); }); @@ -1675,7 +1688,12 @@ void main() { final gaps = await container.read(strategyProvider.notifier).addPage(); - expect(gaps.waitingForCloud, isTrue); + // What the copy may lack is said all the same. + expect(gaps, ( + imagesUploading: 1, + unsavedEdits: false, + waitingForCloud: true, + )); expect(container.read(strategyPageSessionProvider).activePageId, 'page-1'); // The add stays queued, to land later. @@ -1683,41 +1701,47 @@ void main() { await _settle(); }); + test('a send that stalls still answers "+" in time', () async { + final (container, queue, _) = await open(); + StrategyProvider.cloudPageAddWait = const Duration(milliseconds: 200); + // The send never comes back. + queue.onFlush = () => Completer().future; + + final started = DateTime.now(); + final gaps = await container.read(strategyProvider.notifier).addPage(); + + expect(DateTime.now().difference(started).inSeconds, lessThan(2)); + expect(gaps.waitingForCloud, isTrue); + }); + test('says when edits to the page were refused, so are not in its copy', () async { final (container, queue, land) = await open(); + // The server refused an edit to page 1. + queue.state = queue.state.copyWith(attentionByEntityKey: { + const EntitySyncKey.element('page-1', 'uploaded'): + const QueuedEntityIntent( + entityKey: EntitySyncKey.element('page-1', 'uploaded'), + pending: PendingOp( + op: ElementDeleteOp( + opId: 'refused-op', + pagePublicId: 'page-1', + elementPublicId: 'uploaded', + expectedElementRevision: 1, + ), + clientId: 'test-client', + ), + ), + }); queue.onFlush = () async { final add = queuedAdd(queue); if (add == null) return queue.ackQueued(); - land(add, [ - image(add.pagePublicId, 'uploaded~cp1~$_copyUuid'), - image(add.pagePublicId, 'uploading~cp1~$_copyUuid2'), - ]); - // The server refused an edit to page 1 sent ahead of the add. - queue.state = queue.state.copyWith(attentionByEntityKey: { - const EntitySyncKey.element('page-1', 'uploaded'): - const QueuedEntityIntent( - entityKey: EntitySyncKey.element('page-1', 'uploaded'), - pending: PendingOp( - op: ElementDeleteOp( - opId: 'refused-op', - pagePublicId: 'page-1', - elementPublicId: 'uploaded', - expectedElementRevision: 1, - ), - clientId: 'test-client', - ), - ), - }); + land(add, [image(add.pagePublicId, 'uploaded~cp1~$_copyUuid')]); }; final gaps = await container.read(strategyProvider.notifier).addPage(); - expect(gaps, ( - imagesLeftOut: 0, - unsavedEditsLeftOut: true, - waitingForCloud: false, - )); + expect(gaps.unsavedEdits, isTrue); await _settle(); }); }); From 7372caf09830f3fc3023bd3f1bee74911916b157 Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 15:04:01 -0400 Subject: [PATCH 10/11] Name source edits refused in the same send as the page add Edits sent ahead of the add have their answers once the add does, so "+" checks again then for refusals its copy won't have. Co-Authored-By: Claude Opus 5.5 --- lib/providers/strategy_provider.dart | 24 +++++++++----- test/strategy_page_session_provider_test.dart | 31 +++++++++++++++++++ 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/lib/providers/strategy_provider.dart b/lib/providers/strategy_provider.dart index fe0994d8..b34dda7a 100644 --- a/lib/providers/strategy_provider.dart +++ b/lib/providers/strategy_provider.dart @@ -1605,17 +1605,20 @@ class StrategyProvider extends Notifier { // copy has them. await ref.read(strategyPageSessionProvider.notifier).flushCurrentPage(); if (state.strategyId != strategyId) return none; + // Edits to the page the server refused: the copy, made from the + // server's page, doesn't have them. + bool sourceEditsRefused() => ref + .read(strategyOpQueueProvider) + .attentionByEntityKey + .keys + .any((key) => key.pageId == sourcePageId); // What the copy may lack is known now, from what the page on screen // holds: the server copies the page as it has it. final gaps = activeIndex < 0 ? none : ( imagesUploading: _imagesStillUploading(strategyId), - unsavedEdits: ref - .read(strategyOpQueueProvider) - .attentionByEntityKey - .keys - .any((key) => key.pageId == sourcePageId), + unsavedEdits: sourceEditsRefused(), waitingForCloud: false, ); final answer = await _sendAndAwaitAnswer( @@ -1644,15 +1647,22 @@ class StrategyProvider extends Notifier { } // A refusal waits in the sync panel like any other. if (!answer.isAck || state.strategyId != strategyId) return gaps; + // Edits sent ahead of the add have their answers too by now, and the + // server may have refused some of them. + final landed = ( + imagesUploading: gaps.imagesUploading, + unsavedEdits: activeIndex >= 0 && sourceEditsRefused(), + waitingForCloud: false, + ); await ref.read(remoteEditorSnapshotProvider.notifier).refresh(); - if (state.strategyId != strategyId) return gaps; + if (state.strategyId != strategyId) return landed; await ref .read(strategyPageSessionProvider.notifier) .setActivePageAnimated( pageID, direction: PageTransitionDirection.forward, ); - return gaps; + return landed; } final box = Hive.box(HiveBoxNames.strategiesBox); diff --git a/test/strategy_page_session_provider_test.dart b/test/strategy_page_session_provider_test.dart index 7070e714..67e2a050 100644 --- a/test/strategy_page_session_provider_test.dart +++ b/test/strategy_page_session_provider_test.dart @@ -1744,6 +1744,37 @@ void main() { expect(gaps.unsavedEdits, isTrue); await _settle(); }); + + test('names an edit to the page refused in the same send as the add', + () async { + final (container, queue, land) = await open(); + queue.onFlush = () async { + final add = queuedAdd(queue); + if (add == null) return queue.ackQueued(); + land(add, [image(add.pagePublicId, 'uploaded~cp1~$_copyUuid')]); + // The server refused an edit to page 1 sent ahead of the add. + queue.state = queue.state.copyWith(attentionByEntityKey: { + const EntitySyncKey.element('page-1', 'uploaded'): + const QueuedEntityIntent( + entityKey: EntitySyncKey.element('page-1', 'uploaded'), + pending: PendingOp( + op: ElementDeleteOp( + opId: 'refused-op', + pagePublicId: 'page-1', + elementPublicId: 'uploaded', + expectedElementRevision: 1, + ), + clientId: 'test-client', + ), + ), + }); + }; + + final gaps = await container.read(strategyProvider.notifier).addPage(); + + expect(gaps.unsavedEdits, isTrue); + await _settle(); + }); }); test('cloud page rename is persisted with the page revision', () async { From e6c3d16f750e482fe9b6079609b35116eb2706e0 Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 16:09:16 -0400 Subject: [PATCH 11/11] Drop the "still uploading" warning from "+" Copies show their original's pictures now, so "+" leaves no image out and has nothing to warn about: a copy of an image still uploading shows it once the upload lands. Co-Authored-By: Claude Opus 5.5 --- lib/providers/strategy_provider.dart | 71 +++++-------------- lib/widgets/new_page_copy_toast.dart | 13 +--- test/strategy_page_session_provider_test.dart | 47 ++++++------ 3 files changed, 42 insertions(+), 89 deletions(-) diff --git a/lib/providers/strategy_provider.dart b/lib/providers/strategy_provider.dart index bf98f76a..be1974af 100644 --- a/lib/providers/strategy_provider.dart +++ b/lib/providers/strategy_provider.dart @@ -79,14 +79,9 @@ enum PageCopyResult { } /// What a new cloud page may lack, and whether it has reached the cloud -/// yet (see StrategyProvider.addPage): images whose uploads hadn't -/// finished, which the server leaves out of its copy, and edits to the page -/// the server refused, which it never had. -typedef NewPageGaps = ({ - int imagesUploading, - bool unsavedEdits, - bool waitingForCloud, -}); +/// yet (see StrategyProvider.addPage): edits to the page it copies that +/// the server refused, which its copy never had. +typedef NewPageGaps = ({bool unsavedEdits, bool waitingForCloud}); class StrategyProvider extends Notifier { @override @@ -437,25 +432,6 @@ class StrategyProvider extends Notifier { } } - /// Images on the page on screen whose uploads haven't finished, as far - /// as this device knows: the server's, or this device's own, still on - /// their way. - int _imagesStillUploading(String strategyId) { - final assets = - ref.read(remoteEditorSnapshotProvider).valueOrNull?.assetsById ?? - const {}; - final uploads = - ref.read(cloudMediaUploadQueueProvider).jobsForStrategy(strategyId); - return ref.read(placedImageProvider).images.where((image) { - final status = assets[image.id]?.uploadStatus; - if (status == 'active') return false; - return status == 'pending' || - uploads.any((job) => - job.assetPublicId == image.id && - job.state != CloudMediaJobState.failed); - }).length; - } - Future _enqueueCloudPageDescriptorOp(StrategyOp op) async { await enqueueOps([op]); final queue = ref.read(strategyOpQueueProvider.notifier); @@ -1542,11 +1518,7 @@ class StrategyProvider extends Notifier { /// copy may lack, or that it hasn't landed yet, is returned for the /// caller to say. Future addPage([String? name]) async { - const none = ( - imagesUploading: 0, - unsavedEdits: false, - waitingForCloud: false, - ); + const none = (unsavedEdits: false, waitingForCloud: false); if (!_currentStrategyCanEditPages()) return none; if (_currentStrategyIsCloud()) { final strategyId = state.strategyId; @@ -1569,20 +1541,14 @@ class StrategyProvider extends Notifier { if (state.strategyId != strategyId) return none; // Edits to the page the server refused: the copy, made from the // server's page, doesn't have them. - bool sourceEditsRefused() => ref - .read(strategyOpQueueProvider) - .attentionByEntityKey - .keys - .any((key) => key.pageId == sourcePageId); - // What the copy may lack is known now, from what the page on screen - // holds: the server copies the page as it has it. - final gaps = activeIndex < 0 - ? none - : ( - imagesUploading: _imagesStillUploading(strategyId), - unsavedEdits: sourceEditsRefused(), - waitingForCloud: false, - ); + bool sourceEditsRefused() => + activeIndex >= 0 && + ref + .read(strategyOpQueueProvider) + .attentionByEntityKey + .keys + .any((key) => key.pageId == sourcePageId); + final refusedBefore = sourceEditsRefused(); final answer = await _sendAndAwaitAnswer( PageAddOp( opId: const Uuid().v4(), @@ -1601,19 +1567,16 @@ class StrategyProvider extends Notifier { ); if (answer == null) { // Offline, or behind other work: the page lands later. - return ( - imagesUploading: gaps.imagesUploading, - unsavedEdits: gaps.unsavedEdits, - waitingForCloud: true, - ); + return (unsavedEdits: refusedBefore, waitingForCloud: true); } // A refusal waits in the sync panel like any other. - if (!answer.isAck || state.strategyId != strategyId) return gaps; + if (!answer.isAck || state.strategyId != strategyId) { + return (unsavedEdits: refusedBefore, waitingForCloud: false); + } // Edits sent ahead of the add have their answers too by now, and the // server may have refused some of them. final landed = ( - imagesUploading: gaps.imagesUploading, - unsavedEdits: activeIndex >= 0 && sourceEditsRefused(), + unsavedEdits: sourceEditsRefused(), waitingForCloud: false, ); await ref.read(remoteEditorSnapshotProvider.notifier).refresh(); diff --git a/lib/widgets/new_page_copy_toast.dart b/lib/widgets/new_page_copy_toast.dart index cc0598c7..08554a6d 100644 --- a/lib/widgets/new_page_copy_toast.dart +++ b/lib/widgets/new_page_copy_toast.dart @@ -1,7 +1,7 @@ import 'package:icarus/const/settings.dart'; import 'package:icarus/providers/strategy_provider.dart'; -/// Says what a new page's copy may lack, and when the page hasn't reached +/// Says what a new page's copy lacks, and when the page hasn't reached /// the cloud yet, if either (see StrategyProvider.addPage). void showNewPageGaps(NewPageGaps gaps) { if (gaps.waitingForCloud) { @@ -18,15 +18,4 @@ void showNewPageGaps(NewPageGaps gaps) { backgroundColor: Settings.tacticalVioletTheme.destructive, ); } - final images = gaps.imagesUploading; - if (images > 0) { - Settings.showToast( - message: images == 1 - ? 'An image on the page you copied was still uploading, so the ' - 'copy may not have it.' - : '$images images on the page you copied were still uploading, so ' - 'the copy may not have them.', - backgroundColor: Settings.tacticalVioletTheme.primary, - ); - } } diff --git a/test/strategy_page_session_provider_test.dart b/test/strategy_page_session_provider_test.dart index e1698169..9653e621 100644 --- a/test/strategy_page_session_provider_test.dart +++ b/test/strategy_page_session_provider_test.dart @@ -1508,8 +1508,8 @@ void main() { ); // This server never answers; "+" stops waiting at once. StrategyProvider.cloudPageAddWait = Duration.zero; - addTearDown(() => - StrategyProvider.cloudPageAddWait = const Duration(seconds: 5)); + addTearDown( + () => StrategyProvider.cloudPageAddWait = const Duration(seconds: 5)); await container.read(strategyProvider.notifier).addPage('Execute'); @@ -1540,7 +1540,8 @@ void main() { }); group('"+" on a cloud strategy', () { - RemoteElement image(String pageId, String id) => RemoteElement( + RemoteElement image(String pageId, String id, {String? assetId}) => + RemoteElement( publicId: id, strategyPublicId: 'cloud-strategy', pagePublicId: pageId, @@ -1552,6 +1553,7 @@ void main() { aspectRatio: 1, scale: ImageScalePolicy.defaultWidth, fileExtension: '.png', + assetId: assetId, )), 'elementType': 'image', }), @@ -1627,19 +1629,24 @@ void main() { .whereType() .firstOrNull; - tearDown(() => StrategyProvider.cloudPageAddWait = - const Duration(seconds: 5)); + tearDown( + () => StrategyProvider.cloudPageAddWait = const Duration(seconds: 5)); - test('turns to the copy the server made, and warns of an image still ' - 'uploading', () async { + test('turns to the copy the server made', () async { final (container, queue, land) = await open(); PageAddOp? sent; queue.onFlush = () async { final add = queuedAdd(queue); if (add == null) return queue.ackQueued(); sent = add; - // The server leaves out the image whose upload has not finished. - land(add, [image(add.pagePublicId, 'uploaded~cp1~$_copyUuid')]); + // The server copies both images, the one still uploading too: each + // copy shows its original's picture. + land(add, [ + image(add.pagePublicId, 'uploaded~cp1~$_copyUuid', + assetId: 'uploaded'), + image(add.pagePublicId, 'uploading~cp1~$_copyUuid', + assetId: 'uploading'), + ]); }; final gaps = await container.read(strategyProvider.notifier).addPage(); @@ -1649,12 +1656,11 @@ void main() { container.read(strategyPageSessionProvider).activePageId, sent!.pagePublicId, ); - expect(container.read(placedImageProvider).images, hasLength(1)); - expect(gaps, ( - imagesUploading: 1, - unsavedEdits: false, - waitingForCloud: false, - )); + expect( + container.read(placedImageProvider).images.map((i) => i.pictureId), + unorderedEquals(['uploaded', 'uploading']), + ); + expect(gaps, (unsavedEdits: false, waitingForCloud: false)); await _settle(); }); @@ -1688,14 +1694,9 @@ void main() { final gaps = await container.read(strategyProvider.notifier).addPage(); - // What the copy may lack is said all the same. - expect(gaps, ( - imagesUploading: 1, - unsavedEdits: false, - waitingForCloud: true, - )); - expect(container.read(strategyPageSessionProvider).activePageId, - 'page-1'); + expect(gaps, (unsavedEdits: false, waitingForCloud: true)); + expect( + container.read(strategyPageSessionProvider).activePageId, 'page-1'); // The add stays queued, to land later. expect(queuedAdd(queue), isNotNull); await _settle();