From b36ee8573c867aaa9e1686369bbf9b52ccdf183a Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Wed, 11 Nov 2026 18:00:48 -0500 Subject: [PATCH 1/9] Reload the strategy that is open, not a share link left behind A signed-in reader who opened a view link, went home, opened their own strategy and reloaded was left on the editor's loading screen with a home button that took no clicks. The address and the held share code still named the link while the tab's store named their own strategy, so the reload opened two editors and one lost. The address and the held code now follow what is open: with the reader's own strategy open the link is not what a reload reopens, and it is again once its strategy is back on screen. A failed load takes its own editor off the stack, and the loading screen has a working home button. --- lib/providers/share_link_provider.dart | 53 ++++++++--- lib/services/open_cloud_strategy_store.dart | 10 ++- lib/strategy_view.dart | 98 ++++++++++++++++----- lib/widgets/strategy_view_skeleton.dart | 50 +++++++++-- test/pending_share_code_store_test.dart | 2 +- test/strategy_view_skeleton_test.dart | 42 +++++++++ test/web_editor_reload_test.dart | 48 ++++++++++ 7 files changed, 260 insertions(+), 43 deletions(-) diff --git a/lib/providers/share_link_provider.dart b/lib/providers/share_link_provider.dart index 282626201..8cdd85dbb 100644 --- a/lib/providers/share_link_provider.dart +++ b/lib/providers/share_link_provider.dart @@ -18,9 +18,12 @@ final pendingShareCodeStoreProvider = Provider( (ref) => createPendingShareCodeStore(), ); -/// The page's own URL on web (where a share link arrives), null on native. -/// Overridden in tests. -final sharePageUrlProvider = Provider((ref) => kIsWeb ? Uri.base : null); +/// Reads the page's own URL on web (where a share link arrives), null on +/// native. Read each time it is needed: the router and this controller both +/// change it while the page stays open. Overridden in tests. +final sharePageUrlProvider = Provider( + (ref) => () => kIsWeb ? Uri.base : null, +); /// Replaces the address bar URL without navigating. Overridden in tests. final replaceBrowserUrlProvider = @@ -281,6 +284,25 @@ class ShareLinkController extends Notifier { return redeemPendingIfPossible(source: 'add_to_library', explicit: true); } + /// Whether a page reload reopens the held link. The editor says so as the + /// open strategy changes (see openCloudStrategyRecorderProvider): no while + /// the reader has a strategy of their own open, because a reload reopens + /// that one, and yes again once the strategy they read through the link is + /// back. Without this a reload restored both, and the two editors fought. + /// The link stays held in memory either way, so going back to its strategy + /// in this tab still reads through it. + void reopenOnReload(bool reopen) { + final token = state; + if (token == null) return; + final store = ref.read(pendingShareCodeStoreProvider); + if (reopen) { + store.write(token); + } else { + store.clear(); + } + _pointPageUrlAt(reopen ? token : null); + } + /// Whether [token]'s attempt is stale: a newer code arrived, or sign-in /// redeemed this one while the attempt was waiting on the server. bool _superseded(String token, int generation) => @@ -322,24 +344,31 @@ class ShareLinkController extends Notifier { } state = null; ref.read(pendingShareCodeStoreProvider).clear(); - _dropSharePathFromPageUrl(); + _pointPageUrlAt(null); } - /// Once a code is done with, the page must not keep its /share/ URL, + /// Makes the page's URL the link for [code], or drops the link from it + /// when [code] is null. A reload opens whatever link the URL names, so + /// once a code is done with the page must not keep its /share/ URL, /// or every reload would redeem it again (and, after the link is disabled, /// report a failure to someone who still has access). The fragment stays: /// Flutter's router keeps the open route there. - void _dropSharePathFromPageUrl() { - final page = ref.read(sharePageUrlProvider); - if (page == null || - !isIcarusShareUri(page, currentOrigin: currentShareOrigin())) { - return; - } + void _pointPageUrlAt(String? code) { + final page = ref.read(sharePageUrlProvider)(); + if (page == null) return; + final origin = currentShareOrigin(); + final isLink = isIcarusShareUri(page, currentOrigin: origin); + final alreadyThere = code == null + ? !isLink + : isLink && + extractIcarusShareCode(page.toString(), currentOrigin: origin) == + code; + if (alreadyThere) return; ref.read(replaceBrowserUrlProvider)(Uri( scheme: page.scheme, host: page.host, port: page.hasPort ? page.port : null, - path: '/', + path: code == null ? '/' : '/share/$code', fragment: page.fragment.isEmpty ? null : page.fragment, )); } diff --git a/lib/services/open_cloud_strategy_store.dart b/lib/services/open_cloud_strategy_store.dart index b878423e9..17b036b13 100644 --- a/lib/services/open_cloud_strategy_store.dart +++ b/lib/services/open_cloud_strategy_store.dart @@ -45,16 +45,24 @@ final openCloudStrategyStoreProvider = Provider( /// editor goes away the store is cleared. A strategy open through a share /// link is not recorded: its page keeps the /share/ URL, and a reload /// reopens it through the link. +/// +/// A reload reopens one strategy, so the two never both hold: while the +/// reader's own strategy is recorded, the link is told not to reopen, and +/// it is told to again when its strategy is the open one. final openCloudStrategyRecorderProvider = Provider.autoDispose((ref) { final store = ref.watch(openCloudStrategyStoreProvider); void record() { final open = ref.read(strategyProvider); final id = open.strategyId; - final throughLink = ref.read(shareLinkViewProvider)?.strategyPublicId == id; + final throughLink = + id != null && ref.read(shareLinkViewProvider)?.strategyPublicId == id; + final link = ref.read(shareLinkControllerProvider.notifier); if (open.source == StrategySource.cloud && id != null && !throughLink) { store.write(id); + link.reopenOnReload(false); } else { store.clear(); + if (throughLink) link.reopenOnReload(true); } } diff --git a/lib/strategy_view.dart b/lib/strategy_view.dart index f8fe632c7..cd07c5686 100644 --- a/lib/strategy_view.dart +++ b/lib/strategy_view.dart @@ -182,7 +182,7 @@ class _StrategyViewState extends ConsumerState if (!viewingThroughLink && !await _waitForCloudWorkspace()) { // Signed out, or the cloud is unreachable: nothing failed to // load, and the library says which, so go back without an error. - if (mounted) Navigator.maybePop(context); + if (mounted) _leaveAfterFailedLoad(); return; } await ref.read(strategyProvider.notifier).openCloudStrategy( @@ -206,6 +206,13 @@ class _StrategyViewState extends ConsumerState // Refused: the access-lost dialog says so over the skeleton, and // leads out (see StrategyAccessWatcher). if (ref.read(strategyAccessLostProvider)) return; + // Another strategy was opened while this one loaded, in an editor of + // its own: nothing failed that the reader should hear about. + final open = ref.read(strategyProvider); + if (open.isOpen && open.strategyId != strategyId) { + if (mounted) _leaveAfterFailedLoad(); + return; + } final notKept = ref.read(remoteEditorSnapshotProvider).error is StrategyNotKeptOffline; if (mounted) { @@ -217,7 +224,7 @@ class _StrategyViewState extends ConsumerState ? Settings.tacticalVioletTheme.secondary : Settings.tacticalVioletTheme.destructive, ); - Navigator.maybePop(context); + _leaveAfterFailedLoad(); } } finally { if (mounted) { @@ -226,6 +233,19 @@ class _StrategyViewState extends ConsumerState } } + /// Takes this editor off the screen after its strategy failed to open. + /// Another screen may have been pushed over it meanwhile (a share link + /// opening its own editor), and a plain pop would close that one and + /// leave this one behind it, showing its loading screen for good. + void _leaveAfterFailedLoad() { + final route = ModalRoute.of(context); + if (route == null || route.isCurrent) { + Navigator.maybePop(context); + } else if (route.isActive) { + Navigator.of(context).removeRoute(route); + } + } + /// Resolves [cloudWorkspaceOutcomeProvider] once it settles. After a page /// reload the editor can open before sign-in is restored, so a cloud /// strategy waits here rather than failing on an unready backend. @@ -299,12 +319,31 @@ class _StrategyViewState extends ConsumerState onLeave: _leaveToLibrary, child: Scaffold( // The skeleton is the desktop editor's shape, panels and all. + // The way back to the library works from the first frame: a load + // that never finishes must not hold the reader here. body: touch - ? const Center(child: WingDotLoader()) + ? Stack( + children: [ + const Center(child: WingDotLoader()), + SafeArea( + child: Padding( + padding: const EdgeInsets.all(6), + child: _LeaveButton( + onPressed: _leaveToLibrary, + backTooltip: widget.backTooltip, + ), + ), + ), + ], + ) : StrategyViewSkeleton( strategyName: widget.initialStrategyName, mapValue: widget.initialMapValue, isAttack: widget.initialIsAttack, + leading: _LeaveButton( + onPressed: _leaveToLibrary, + backTooltip: widget.backTooltip, + ), ), ), ); @@ -333,25 +372,9 @@ class _StrategyViewState extends ConsumerState Row( children: [ const SizedBox(width: 6), - ShadTooltip( - builder: (context) => - Text(widget.backTooltip ?? - context.l10n.strategyViewBackToLibraryTooltip), - child: ShadIconButton.ghost( - width: 28, - height: 28, - foregroundColor: - Settings.tacticalVioletTheme.mutedForeground, - hoverForegroundColor: - Settings.tacticalVioletTheme.foreground, - onPressed: _leaveToLibrary, - icon: Icon( - widget.backTooltip == null - ? LucideIcons.house300 - : LucideIcons.chevronLeft300, - size: 18, - ), - ), + _LeaveButton( + onPressed: _leaveToLibrary, + backTooltip: widget.backTooltip, ), const IcarusWordmark(), const Expanded( @@ -472,6 +495,37 @@ class _StrategyViewState extends ConsumerState /// The bug-report link at the end of the editor strip: text when there is /// room, the glyph alone when there isn't. +/// The editor's way out, at the left of the window strip: the house back to +/// the library, or a back arrow when [backTooltip] names somewhere else. +class _LeaveButton extends StatelessWidget { + const _LeaveButton({required this.onPressed, this.backTooltip}); + + final VoidCallback onPressed; + final String? backTooltip; + + @override + Widget build(BuildContext context) { + return ShadTooltip( + builder: (context) => Text( + backTooltip ?? context.l10n.strategyViewBackToLibraryTooltip, + ), + child: ShadIconButton.ghost( + width: 28, + height: 28, + foregroundColor: Settings.tacticalVioletTheme.mutedForeground, + hoverForegroundColor: Settings.tacticalVioletTheme.foreground, + onPressed: onPressed, + icon: Icon( + backTooltip == null + ? LucideIcons.house300 + : LucideIcons.chevronLeft300, + size: 18, + ), + ), + ); + } +} + class _DiscordLink extends StatelessWidget { const _DiscordLink(); diff --git a/lib/widgets/strategy_view_skeleton.dart b/lib/widgets/strategy_view_skeleton.dart index 6e72afcfa..32c9da3df 100644 --- a/lib/widgets/strategy_view_skeleton.dart +++ b/lib/widgets/strategy_view_skeleton.dart @@ -16,14 +16,42 @@ class StrategyViewSkeleton extends StatelessWidget { this.strategyName, this.mapValue, this.isAttack = true, + this.leading, }); final String? strategyName; final MapValue? mapValue; final bool isAttack; + /// The editor's leave button. It sits where the editor will show it and + /// is the one part of the skeleton that takes clicks. + final Widget? leading; + @override Widget build(BuildContext context) { + final leading = this.leading; + if (leading == null) return _buildSkeleton(showLeadingBlock: true); + return Stack( + children: [ + _buildSkeleton(showLeadingBlock: false), + Positioned( + top: 0, + left: 0, + height: kWindowStripHeight, + child: Row( + mainAxisSize: MainAxisSize.min, + children: [ + const MacTrafficLightInset(), + const SizedBox(width: 6), + leading, + ], + ), + ), + ], + ); + } + + Widget _buildSkeleton({required bool showLeadingBlock}) { final resolvedMap = mapValue ?? MapValue.ascent; return ExcludeSemantics( child: IgnorePointer( @@ -34,6 +62,7 @@ class StrategyViewSkeleton extends StatelessWidget { strategyName: strategyName, mapValue: resolvedMap, isAttack: isAttack, + showLeadingBlock: showLeadingBlock, ), Expanded( child: Stack( @@ -161,6 +190,7 @@ class _SkeletonTopBar extends StatelessWidget { const _SkeletonTopBar({ required this.mapValue, required this.isAttack, + required this.showLeadingBlock, this.strategyName, }); @@ -168,20 +198,26 @@ class _SkeletonTopBar extends StatelessWidget { final MapValue mapValue; final bool isAttack; + /// False when the real leave button is drawn over this spot. + final bool showLeadingBlock; + @override Widget build(BuildContext context) { final title = strategyName?.trim(); return AppWindowStrip( child: Stack( children: [ - const Row( + Row( children: [ - SizedBox(width: 6), - _SkeletonBlock(width: 28, height: 28, radius: 8), - IcarusWordmark(), - Spacer(), - _SkeletonBlock(width: 200, height: 28, radius: 8), - SizedBox(width: 10), + const SizedBox(width: 6), + if (showLeadingBlock) + const _SkeletonBlock(width: 28, height: 28, radius: 8) + else + const SizedBox(width: 28), + const IcarusWordmark(), + const Spacer(), + const _SkeletonBlock(width: 200, height: 28, radius: 8), + const SizedBox(width: 10), ], ), Center( diff --git a/test/pending_share_code_store_test.dart b/test/pending_share_code_store_test.dart index 15084378d..a9c0e64f3 100644 --- a/test/pending_share_code_store_test.dart +++ b/test/pending_share_code_store_test.dart @@ -145,7 +145,7 @@ void main() { pendingShareCodeStoreProvider .overrideWithValue(store ?? MemoryPendingShareCodeStore()), authProvider.overrideWith(() => _SettableAuthProvider(auth)), - sharePageUrlProvider.overrideWithValue(pageUrl), + sharePageUrlProvider.overrideWithValue(() => pageUrl), replaceBrowserUrlProvider.overrideWithValue(replacedUrls.add), if (repository != null) convexStrategyRepositoryProvider.overrideWithValue(repository), diff --git a/test/strategy_view_skeleton_test.dart b/test/strategy_view_skeleton_test.dart index 9886ce440..56dffdb4b 100644 --- a/test/strategy_view_skeleton_test.dart +++ b/test/strategy_view_skeleton_test.dart @@ -18,6 +18,48 @@ class _FixedPreferences extends AppPreferencesNotifier { } void main() { + testWidgets('the leave button works while the skeleton shows', + (tester) async { + // The rest of the skeleton takes no clicks. Without a live way out, a + // load that never finished held the reader on this screen. + CoordinateSystem(playAreaSize: const Size(1120, 630)); + var left = 0; + await tester.pumpWidget( + ProviderScope( + overrides: [ + appPreferencesProvider.overrideWith(_FixedPreferences.new), + ], + child: ShadApp( + themeMode: ThemeMode.dark, + darkTheme: ShadThemeData( + brightness: Brightness.dark, + colorScheme: Settings.tacticalVioletTheme, + ), + home: MediaQuery( + data: const MediaQueryData( + size: Size(800, 630), + disableAnimations: true, + ), + child: StrategyViewSkeleton( + leading: IconButton( + onPressed: () => left++, + icon: const Icon(Icons.home), + ), + ), + ), + ), + ), + ); + await tester.pump(); + + final button = tester.getRect(find.byType(IconButton)); + final strip = tester.getRect(find.byType(AppWindowStrip)); + expect(button.center.dy, closeTo(strip.center.dy, 0.5)); + await tester.tap(find.byType(IconButton)); + expect(left, 1); + expect(tester.takeException(), isNull); + }); + testWidgets('loading skeleton fits the minimum desktop window', (tester) async { // DotGrid reads the play area at paint time. diff --git a/test/web_editor_reload_test.dart b/test/web_editor_reload_test.dart index db5d407c1..3b8d2c921 100644 --- a/test/web_editor_reload_test.dart +++ b/test/web_editor_reload_test.dart @@ -5,8 +5,11 @@ import 'package:icarus/config/platform_policy.dart'; import 'package:icarus/const/routes.dart'; import 'package:icarus/providers/auth_provider.dart'; import 'package:icarus/providers/library_workspace_provider.dart'; +import 'package:icarus/providers/share_link_provider.dart'; import 'package:icarus/providers/strategy_provider.dart'; import 'package:icarus/services/open_cloud_strategy_store.dart'; +import 'package:icarus/share/pending_share_code_store.dart'; +import 'package:icarus/share/share_link_format.dart'; import 'package:icarus/strategy/strategy_page_models.dart'; import 'package:icarus/strategy_view.dart'; import 'package:icarus/widgets/editor_toolbar.dart'; @@ -130,6 +133,51 @@ void main() { expect(store.read(), isNull, reason: 'leaving the editor clears it'); }); + test('a reload reopens the strategy that is open, not a link left behind', + () { + // A reader opened a view link, then their own strategy, in one tab. Both + // used to stay set for the reload: the link in the address and the held + // code, their own strategy in the store. The reload opened two editors + // and one was left on its loading screen. + const code = 'ICR-2345-6789-ABCD-EFGH'; + var page = icarusProductionShareOrigin.replace( + path: '/share/$code', + fragment: '/strategy_view', + ); + final open = MemoryOpenCloudStrategyStore(); + final heldCode = MemoryPendingShareCodeStore()..write(code); + final strategy = _SettableStrategy(); + final container = ProviderContainer(overrides: [ + openCloudStrategyStoreProvider.overrideWithValue(open), + pendingShareCodeStoreProvider.overrideWithValue(heldCode), + sharePageUrlProvider.overrideWithValue(() => page), + replaceBrowserUrlProvider.overrideWithValue((url) => page = url), + strategyProvider.overrideWith(() => strategy), + ]); + addTearDown(container.dispose); + container.read(shareLinkViewProvider.notifier).state = + (strategyPublicId: 'shared', token: code); + container.listen(openCloudStrategyRecorderProvider, (_, __) {}); + + strategy.open('shared', StrategySource.cloud); + expect(open.read(), isNull); + expect(heldCode.read(), code); + expect(page.path, '/share/$code'); + + strategy.open('mine', StrategySource.cloud); + expect(open.read(), 'mine'); + expect(heldCode.read(), isNull, reason: 'a reload must not reopen it'); + expect(page.path, '/', reason: 'the address follows what is open'); + expect(page.fragment, '/strategy_view', reason: 'the route stays'); + + // Back to the shared strategy in the same tab: the link still reads it, + // and it is what a reload reopens again. + strategy.open('shared', StrategySource.cloud); + expect(open.read(), isNull); + expect(heldCode.read(), code); + expect(page.path, '/share/$code'); + }); + testWidgets('the web beta toolbar never offers a local Save', (tester) async { for (final (policy, expected) in [ (PlatformPolicy.webBeta, findsNothing), From c61f1c7e9958371de1071e1c5671478a50724bef Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Wed, 11 Nov 2026 18:23:47 -0500 Subject: [PATCH 2/9] Drop a load the reader left, and name the link on screen Review of the first commit found three things. The loading screen's home button lets a reader leave before the strategy has loaded. The read went on, and its late answer opened the strategy with no editor showing it and started its live reads. Closing a strategy now discards a read still on its way, and a strategy that was closed or gave way to another while it loaded does not open after the fact. A load only counts as having given way when a strategy opened since it started, so a real failure with an older strategy still open is reported. The address is restored from the link whose strategy is on screen, not from the held code, which can be a later link that could not be opened. --- .../remote_strategy_snapshot_provider.dart | 5 +++ lib/providers/share_link_provider.dart | 12 ++++--- lib/providers/strategy_provider.dart | 5 +-- lib/strategy_view.dart | 33 +++++++++++-------- .../remote_editor_snapshot_provider_test.dart | 29 ++++++++++++++++ test/web_editor_reload_test.dart | 16 ++++++++- 6 files changed, 78 insertions(+), 22 deletions(-) diff --git a/lib/providers/collab/remote_strategy_snapshot_provider.dart b/lib/providers/collab/remote_strategy_snapshot_provider.dart index 8e7e79beb..c1a3f6989 100644 --- a/lib/providers/collab/remote_strategy_snapshot_provider.dart +++ b/lib/providers/collab/remote_strategy_snapshot_provider.dart @@ -164,6 +164,9 @@ class RemoteEditorSnapshotNotifier } else { await _refreshFromServer(); } + // Closed, or another strategy opened, while this one was being read: + // there is nothing left to watch for it. + if (_activeStrategyPublicId != strategyPublicId) return; await _startShellSubscription(strategyPublicId); final pageId = _activePagePublicId; if (pageId != null) { @@ -383,6 +386,8 @@ class RemoteEditorSnapshotNotifier _pagesToOpen(shell).firstOrNull ?? shell.pages.firstOrNull?.publicId; void clear() { + // A read still on its way must not bring the strategy back. + _readEpoch += 1; _activeStrategyPublicId = null; _activePagePublicId = null; _shareToken = null; diff --git a/lib/providers/share_link_provider.dart b/lib/providers/share_link_provider.dart index 8cdd85dbb..a859ee6a2 100644 --- a/lib/providers/share_link_provider.dart +++ b/lib/providers/share_link_provider.dart @@ -292,15 +292,17 @@ class ShareLinkController extends Notifier { /// The link stays held in memory either way, so going back to its strategy /// in this tab still reads through it. void reopenOnReload(bool reopen) { - final token = state; - if (token == null) return; + if (state == null) return; + // The link whose strategy is on screen. A code that arrived later and + // could not be opened may be the one held. + final viewed = reopen ? ref.read(shareLinkViewProvider)?.token : null; final store = ref.read(pendingShareCodeStoreProvider); - if (reopen) { - store.write(token); + if (viewed != null) { + store.write(viewed); } else { store.clear(); } - _pointPageUrlAt(reopen ? token : null); + _pointPageUrlAt(viewed); } /// Whether [token]'s attempt is stale: a newer code arrived, or sign-in diff --git a/lib/providers/strategy_provider.dart b/lib/providers/strategy_provider.dart index 3f65d8a70..625bc43ef 100644 --- a/lib/providers/strategy_provider.dart +++ b/lib/providers/strategy_provider.dart @@ -355,10 +355,11 @@ class StrategyProvider extends Notifier { .openStrategy(strategyID, activePagePublicId: pageId); final snapshotState = ref.read(remoteEditorSnapshotProvider); final snapshot = snapshotState.valueOrNull; - if (snapshot == null) { + if (snapshot == null || snapshot.header.publicId != strategyID) { // Returning silently here used to leave the editor on an eternal // skeleton with no feedback. Throw so callers can surface the failure - // and back out. + // and back out. The same goes for a strategy that was closed, or gave + // way to another, while it loaded: it must not open after the fact. throw StateError( 'Cloud strategy snapshot unavailable for $strategyID' '${snapshotState.hasError ? ': ${snapshotState.error}' : ''}', diff --git a/lib/strategy_view.dart b/lib/strategy_view.dart index cd07c5686..c4a2584cb 100644 --- a/lib/strategy_view.dart +++ b/lib/strategy_view.dart @@ -170,6 +170,9 @@ class _StrategyViewState extends ConsumerState return; } + // What was open before this load: only a strategy opened since then + // means this load gave way to another. + final openBefore = ref.read(strategyProvider).strategyId; try { switch (source) { case StrategySource.local: @@ -190,6 +193,9 @@ class _StrategyViewState extends ConsumerState pageId: widget.initialPageId, ); } + // The reader left while it loaded (the home button works from the + // first frame); the load was dropped with the editor. + if (!mounted) return; final loadedStrategy = ref.read(strategyProvider); if (loadedStrategy.strategyId != strategyId || loadedStrategy.strategyName == null) { @@ -203,29 +209,28 @@ class _StrategyViewState extends ConsumerState error: error, stackTrace: stackTrace, ); + if (!mounted) return; // Refused: the access-lost dialog says so over the skeleton, and // leads out (see StrategyAccessWatcher). if (ref.read(strategyAccessLostProvider)) return; // Another strategy was opened while this one loaded, in an editor of // its own: nothing failed that the reader should hear about. - final open = ref.read(strategyProvider); - if (open.isOpen && open.strategyId != strategyId) { - if (mounted) _leaveAfterFailedLoad(); + final open = ref.read(strategyProvider).strategyId; + if (open != null && open != strategyId && open != openBefore) { + _leaveAfterFailedLoad(); return; } final notKept = ref.read(remoteEditorSnapshotProvider).error is StrategyNotKeptOffline; - if (mounted) { - Settings.showToast( - message: notKept - ? context.l10n.syncStatusStrategyNeedsConnection - : context.l10n.strategyViewLoadFailed, - backgroundColor: notKept - ? Settings.tacticalVioletTheme.secondary - : Settings.tacticalVioletTheme.destructive, - ); - _leaveAfterFailedLoad(); - } + Settings.showToast( + message: notKept + ? context.l10n.syncStatusStrategyNeedsConnection + : context.l10n.strategyViewLoadFailed, + backgroundColor: notKept + ? Settings.tacticalVioletTheme.secondary + : Settings.tacticalVioletTheme.destructive, + ); + _leaveAfterFailedLoad(); } finally { if (mounted) { setState(() => _isInitialLoadPending = false); diff --git a/test/remote_editor_snapshot_provider_test.dart b/test/remote_editor_snapshot_provider_test.dart index 8779603ed..1df397814 100644 --- a/test/remote_editor_snapshot_provider_test.dart +++ b/test/remote_editor_snapshot_provider_test.dart @@ -77,6 +77,35 @@ void main() { ); }); + test('a strategy closed while it was still opening stays closed', () async { + // The editor's loading screen has a working home button, so a reader can + // leave before the first read answers. The late answer used to open the + // strategy anyway, with no editor showing it, and start its live reads. + final repository = _Repository(); + final container = ProviderContainer(overrides: [ + convexStrategyRepositoryProvider.overrideWithValue(repository), + authProvider.overrideWith(_ReadyAuthProvider.new), + strategyOpQueueProvider.overrideWith(_IdleOpQueue.new), + cloudMediaUploadQueueProvider.overrideWith(_IdleMediaQueue.new), + cloudMediaAccountIdProvider.overrideWithValue(null), + ]); + addTearDown(container.dispose); + final remote = container.read(remoteEditorSnapshotProvider.notifier); + await container.read(remoteEditorSnapshotProvider.future); + + final gate = Completer(); + repository.pageReadGate = gate; + final opening = remote.openStrategy('strategy', activePagePublicId: 'a'); + await _settle(); + remote.clear(); + gate.complete(); + await opening; + + expect(container.read(remoteEditorSnapshotProvider).valueOrNull, isNull); + expect(repository.shells.hasListener, isFalse); + expect(repository.watchedPages, isEmpty); + }); + test( 'a page answer counts as live only on the connection it came on, and ' 'never from the copy', () async { diff --git a/test/web_editor_reload_test.dart b/test/web_editor_reload_test.dart index 3b8d2c921..5109ac2b3 100644 --- a/test/web_editor_reload_test.dart +++ b/test/web_editor_reload_test.dart @@ -134,7 +134,7 @@ void main() { }); test('a reload reopens the strategy that is open, not a link left behind', - () { + () async { // A reader opened a view link, then their own strategy, in one tab. Both // used to stay set for the reload: the link in the address and the held // code, their own strategy in the store. The reload opened two editors @@ -153,6 +153,10 @@ void main() { sharePageUrlProvider.overrideWithValue(() => page), replaceBrowserUrlProvider.overrideWithValue((url) => page = url), strategyProvider.overrideWith(() => strategy), + // Signed in, the cloud not ready yet: a new code waits, held. + authProvider.overrideWith( + () => _FixedAuth(_auth(ConvexAuthStatus.configuring)), + ), ]); addTearDown(container.dispose); container.read(shareLinkViewProvider.notifier).state = @@ -176,6 +180,16 @@ void main() { expect(open.read(), isNull); expect(heldCode.read(), code); expect(page.path, '/share/$code'); + + // A second code arrived and could not be opened, so it is the one held. + // The address still names the link whose strategy is on screen. + strategy.open('mine', StrategySource.cloud); + await container + .read(shareLinkControllerProvider.notifier) + .redeemToken('ICR-XXXX-XXXX-XXXX-XXXX'); + strategy.open('shared', StrategySource.cloud); + expect(heldCode.read(), code); + expect(page.path, '/share/$code'); }); testWidgets('the web beta toolbar never offers a local Save', (tester) async { From 487b82d1d36e1b232e87512207c1d36930258f72 Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Wed, 11 Nov 2026 18:37:34 -0500 Subject: [PATCH 3/9] Keep a link that never opened, and drop a local load the reader left A second review found two more. Opening a strategy set aside any held share code, including one that had not opened yet (it arrived offline). The reload was that link's retry, and it was lost. Only a link that has opened is set aside now. On desktop, a strategy still being read from disk when the reader left and opened another could finish late and replace the one they moved to. A local load that gave way to a newer open or a close opens nothing. --- lib/providers/share_link_provider.dart | 16 +++++++++------- lib/providers/strategy_provider.dart | 11 +++++++++++ lib/strategy_view.dart | 4 ++-- test/web_editor_reload_test.dart | 13 +++++++------ 4 files changed, 29 insertions(+), 15 deletions(-) diff --git a/lib/providers/share_link_provider.dart b/lib/providers/share_link_provider.dart index a859ee6a2..975367678 100644 --- a/lib/providers/share_link_provider.dart +++ b/lib/providers/share_link_provider.dart @@ -292,17 +292,19 @@ class ShareLinkController extends Notifier { /// The link stays held in memory either way, so going back to its strategy /// in this tab still reads through it. void reopenOnReload(bool reopen) { - if (state == null) return; - // The link whose strategy is on screen. A code that arrived later and - // could not be opened may be the one held. - final viewed = reopen ? ref.read(shareLinkViewProvider)?.token : null; + final token = state; + // Only a link that has opened. One still waiting to open (it arrived + // offline, say) keeps its code and its address: a reload is its retry. + if (token == null || ref.read(shareLinkViewProvider)?.token != token) { + return; + } final store = ref.read(pendingShareCodeStoreProvider); - if (viewed != null) { - store.write(viewed); + if (reopen) { + store.write(token); } else { store.clear(); } - _pointPageUrlAt(viewed); + _pointPageUrlAt(reopen ? token : null); } /// Whether [token]'s attempt is stale: a newer code arrived, or sign-in diff --git a/lib/providers/strategy_provider.dart b/lib/providers/strategy_provider.dart index 625bc43ef..dd5b94a1b 100644 --- a/lib/providers/strategy_provider.dart +++ b/lib/providers/strategy_provider.dart @@ -100,6 +100,11 @@ typedef NewPageGaps = ({ }); class StrategyProvider extends Notifier { + /// Bumped when a strategy starts to open and when the open one is closed. + /// A local load that finds it moved on gave way to something newer (the + /// reader left, or opened another strategy) and opens nothing. + int _openAttempt = 0; + @override StrategyState build() { _registerPersistenceTrackingListeners(); @@ -349,6 +354,7 @@ class StrategyProvider extends Notifier { } cancelPendingSave(); ref.read(strategySaveStateProvider.notifier).reset(); + _openAttempt += 1; await ref .read(remoteEditorSnapshotProvider.notifier) @@ -797,6 +803,7 @@ class StrategyProvider extends Notifier { } Future clearCurrentStrategy() async { + _openAttempt += 1; cancelPendingSave(); ref.read(strategyThemeProvider.notifier).fromStrategy(); ref.read(strategySaveStateProvider.notifier).reset(); @@ -2045,6 +2052,7 @@ class StrategyProvider extends Notifier { throw StateError('This strategy is moving to the cloud.'); } cancelPendingSave(); + final attempt = ++_openAttempt; final newStrat = Hive.box(HiveBoxNames.strategiesBox) .values .where((StrategyData strategy) { @@ -2085,6 +2093,9 @@ class StrategyProvider extends Notifier { final newDir = kIsWeb ? null : await setStorageDirectory(migratedStrategy.id); + // Left, or another strategy opened, while this one was being read from + // disk: opening it now would replace what the reader moved on to. + if (attempt != _openAttempt) return; state = StrategyState( strategyId: migratedStrategy.id, strategyName: migratedStrategy.name, diff --git a/lib/strategy_view.dart b/lib/strategy_view.dart index c4a2584cb..862560de1 100644 --- a/lib/strategy_view.dart +++ b/lib/strategy_view.dart @@ -498,8 +498,6 @@ class _StrategyViewState extends ConsumerState } } -/// The bug-report link at the end of the editor strip: text when there is -/// room, the glyph alone when there isn't. /// The editor's way out, at the left of the window strip: the house back to /// the library, or a back arrow when [backTooltip] names somewhere else. class _LeaveButton extends StatelessWidget { @@ -531,6 +529,8 @@ class _LeaveButton extends StatelessWidget { } } +/// The bug-report link at the end of the editor strip: text when there is +/// room, the glyph alone when there isn't. class _DiscordLink extends StatelessWidget { const _DiscordLink(); diff --git a/test/web_editor_reload_test.dart b/test/web_editor_reload_test.dart index 5109ac2b3..5d7327663 100644 --- a/test/web_editor_reload_test.dart +++ b/test/web_editor_reload_test.dart @@ -181,14 +181,15 @@ void main() { expect(heldCode.read(), code); expect(page.path, '/share/$code'); - // A second code arrived and could not be opened, so it is the one held. - // The address still names the link whose strategy is on screen. - strategy.open('mine', StrategySource.cloud); + // A second link arrived and has not opened yet (the cloud was not + // ready). It is not left behind: opening a strategy keeps its code, so + // a reload still retries it. + const waiting = 'ICR-XXXX-XXXX-XXXX-XXXX'; await container .read(shareLinkControllerProvider.notifier) - .redeemToken('ICR-XXXX-XXXX-XXXX-XXXX'); - strategy.open('shared', StrategySource.cloud); - expect(heldCode.read(), code); + .redeemToken(waiting); + strategy.open('mine', StrategySource.cloud); + expect(heldCode.read(), waiting); expect(page.path, '/share/$code'); }); From fc63ecbf80674b0349602ea11a2c542f3df9add1 Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Wed, 11 Nov 2026 18:48:10 -0500 Subject: [PATCH 4/9] Check a cloud open again after its folder is made On desktop a cloud strategy makes its storage folder before it is marked open. A reader who left in that moment had it open again with no editor showing. The open now checks once more that nothing newer took its place. --- lib/providers/strategy_provider.dart | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/providers/strategy_provider.dart b/lib/providers/strategy_provider.dart index dd5b94a1b..58a0f029b 100644 --- a/lib/providers/strategy_provider.dart +++ b/lib/providers/strategy_provider.dart @@ -354,7 +354,7 @@ class StrategyProvider extends Notifier { } cancelPendingSave(); ref.read(strategySaveStateProvider.notifier).reset(); - _openAttempt += 1; + final attempt = ++_openAttempt; await ref .read(remoteEditorSnapshotProvider.notifier) @@ -375,6 +375,10 @@ class StrategyProvider extends Notifier { final storageDirectory = kIsWeb ? null : (await setStorageDirectory(snapshot.header.publicId)).path; + // Closed, or another strategy opened, while its folder was being made. + if (attempt != _openAttempt) { + throw StateError('Opening $strategyID gave way to something newer.'); + } state = state.copyWith( strategyId: snapshot.header.publicId, strategyName: snapshot.header.name, From d540f02ee97bab435b7bb6f8d472c1db98131aac Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Wed, 11 Nov 2026 19:20:49 -0500 Subject: [PATCH 5/9] Stop a superseded open at every step A strategy that gave way while its pages were being read still pointed the upload queue at itself (cloud) or marked the save state saved (local), which is now another strategy's. And a reader who left while the editor waited for sign-in to come back could have the load start during the editor's exit animation. Both stop now. --- lib/providers/strategy_provider.dart | 6 ++++++ lib/strategy_view.dart | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/lib/providers/strategy_provider.dart b/lib/providers/strategy_provider.dart index 58a0f029b..edc93e4ab 100644 --- a/lib/providers/strategy_provider.dart +++ b/lib/providers/strategy_provider.dart @@ -393,6 +393,9 @@ class StrategyProvider extends Notifier { selectFirstPageIfNeeded: true, preferredPageId: pageId, ); + // Gave way while its pages were read: the uploads belong to what is + // open now, or to nothing. + if (attempt != _openAttempt) return; // Each page the editor reads is checked for images to upload as it // arrives (see RemoteEditorSnapshotNotifier). unawaited( @@ -2114,6 +2117,9 @@ class StrategyProvider extends Notifier { selectFirstPageIfNeeded: true, preferredPageId: pageId, ); + // Gave way while its pages were read: the save state now belongs to + // another strategy, and marking it saved could hide that one's edits. + if (attempt != _openAttempt) return; ref.read(strategySaveStateProvider.notifier).markPersisted(); } diff --git a/lib/strategy_view.dart b/lib/strategy_view.dart index 862560de1..d52ff9f8e 100644 --- a/lib/strategy_view.dart +++ b/lib/strategy_view.dart @@ -132,6 +132,10 @@ class _StrategyViewState extends ConsumerState bool _isInitialLoadPending = false; bool _hasInitialLoadCompleted = false; + /// Set when the reader leaves for the library. The editor stays mounted + /// while it animates out, and a load still waiting to start must not. + bool _hasLeft = false; + @override void initState() { super.initState(); @@ -188,6 +192,8 @@ class _StrategyViewState extends ConsumerState if (mounted) _leaveAfterFailedLoad(); return; } + // Left while waiting for sign-in to come back. + if (_hasLeft || !mounted) return; await ref.read(strategyProvider.notifier).openCloudStrategy( strategyId, pageId: widget.initialPageId, @@ -279,6 +285,7 @@ class _StrategyViewState extends ConsumerState .read(agentFilterProvider.notifier) .updateFilterState(FilterState.all); ref.read(deleteMenuProvider.notifier).requestClose(); + _hasLeft = true; if (mounted) { Navigator.pop(context); } From 83cb3ff9365beaee6401d0d5c727420e938157d4 Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Wed, 11 Nov 2026 19:26:16 -0500 Subject: [PATCH 6/9] Make every wait inside an open able to end it Three review rounds each found one more wait after which an open the reader had left, or replaced with another, carried on: marking itself open, marking another strategy saved, pointing the upload queue at itself. The checks were written by hand after each wait, so the next wait added would have had the same gap. An open now carries one attempt (OpenAttempt) and waits only through it. A wait that returns after the open gave way ends the open there, in one place, so nothing after any wait can run for a superseded open. The page session's part of an open takes the same attempt. The editor's own initial load waits the same way against the reader having left. A test reads the four functions that make up an open and fails on any await that does not go through the helper. --- .../strategy_page_session_provider.dart | 9 +- lib/providers/strategy_provider.dart | 219 ++++++++++-------- lib/strategy/open_attempt.dart | 34 +++ lib/strategy_view.dart | 49 ++-- test/open_attempt_test.dart | 105 +++++++++ 5 files changed, 297 insertions(+), 119 deletions(-) create mode 100644 lib/strategy/open_attempt.dart create mode 100644 test/open_attempt_test.dart diff --git a/lib/providers/strategy_page_session_provider.dart b/lib/providers/strategy_page_session_provider.dart index 25046a2c1..ae07e0c27 100644 --- a/lib/providers/strategy_page_session_provider.dart +++ b/lib/providers/strategy_page_session_provider.dart @@ -5,6 +5,7 @@ import 'dart:math' show max; import 'package:flutter/foundation.dart' show listEquals, setEquals; import 'package:flutter/widgets.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:icarus/strategy/open_attempt.dart'; import 'package:hive_ce/hive.dart'; import 'package:icarus/collab/cloud_lineup_rows.dart'; import 'package:icarus/collab/collab_models.dart'; @@ -330,13 +331,17 @@ class StrategyPageSessionNotifier extends Notifier { required StrategySource source, required bool selectFirstPageIfNeeded, String? preferredPageId, + OpenAttempt? attempt, }) async { + // The open this is a step of, if any. It stops here too when the open + // gave way (see [OpenAttempt]). + final opening = attempt ?? OpenAttempt.unchallenged; _pageSessionGeneration++; // As in reset: a switch this supersedes leaves its transition here. ref.read(transitionProvider.notifier).complete(); state = state.copyWith(transitionState: PageTransitionState.idle); final pageSource = _resolvePageSource(strategyId, source); - final pageIds = await pageSource.listPageIds(); + final pageIds = await opening.wait(pageSource.listPageIds()); // Drawn from this computer's copy, the page the copy could show is the // only one there is to open (see RemoteEditorSnapshotNotifier). final copiedPage = source == StrategySource.cloud @@ -368,7 +373,7 @@ class StrategyPageSessionNotifier extends Notifier { ); if (selected != null) { - await _rehydrateActivePageFromSource(selected); + await opening.wait(_rehydrateActivePageFromSource(selected)); } // Snapshots that arrived while the strategy was opening went unheard: // catch up on pages that came back while it was closed. diff --git a/lib/providers/strategy_provider.dart b/lib/providers/strategy_provider.dart index edc93e4ab..dc290d468 100644 --- a/lib/providers/strategy_provider.dart +++ b/lib/providers/strategy_provider.dart @@ -14,6 +14,7 @@ import 'package:icarus/const/line_provider.dart'; import 'package:icarus/const/maps.dart'; import 'package:icarus/providers/image_provider.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:icarus/strategy/open_attempt.dart'; import 'package:icarus/const/hive_boxes.dart'; import 'package:icarus/const/settings.dart'; import 'package:icarus/l10n/l10n.dart'; @@ -101,10 +102,22 @@ typedef NewPageGaps = ({ class StrategyProvider extends Notifier { /// Bumped when a strategy starts to open and when the open one is closed. - /// A local load that finds it moved on gave way to something newer (the - /// reader left, or opened another strategy) and opens nothing. int _openAttempt = 0; + /// Runs one attempt to open a strategy. Every wait in [steps] goes through + /// the attempt it is given (see [OpenAttempt]), so an open the reader left, + /// or replaced by opening another strategy, stops at that wait and changes + /// nothing after it. Giving way is not a failure: this returns normally, + /// and what is open afterwards is whatever took its place. + Future _open(Future Function(OpenAttempt opening) steps) async { + final attempt = ++_openAttempt; + try { + await steps(OpenAttempt(() => attempt == _openAttempt)); + } on OpenGaveWay { + // The newer open, or the close, owns the outcome. + } + } + @override StrategyState build() { _registerPersistenceTrackingListeners(); @@ -354,55 +367,54 @@ class StrategyProvider extends Notifier { } cancelPendingSave(); ref.read(strategySaveStateProvider.notifier).reset(); - final attempt = ++_openAttempt; - await ref - .read(remoteEditorSnapshotProvider.notifier) - .openStrategy(strategyID, activePagePublicId: pageId); - final snapshotState = ref.read(remoteEditorSnapshotProvider); - final snapshot = snapshotState.valueOrNull; - if (snapshot == null || snapshot.header.publicId != strategyID) { - // Returning silently here used to leave the editor on an eternal - // skeleton with no feedback. Throw so callers can surface the failure - // and back out. The same goes for a strategy that was closed, or gave - // way to another, while it loaded: it must not open after the fact. - throw StateError( - 'Cloud strategy snapshot unavailable for $strategyID' - '${snapshotState.hasError ? ': ${snapshotState.error}' : ''}', + return _open((opening) async { + await opening.wait( + ref + .read(remoteEditorSnapshotProvider.notifier) + .openStrategy(strategyID, activePagePublicId: pageId), ); - } + final snapshotState = ref.read(remoteEditorSnapshotProvider); + final snapshot = snapshotState.valueOrNull; + if (snapshot == null) { + // Returning silently here used to leave the editor on an eternal + // skeleton with no feedback. Throw so callers can surface the + // failure and back out. + throw StateError( + 'Cloud strategy snapshot unavailable for $strategyID' + '${snapshotState.hasError ? ': ${snapshotState.error}' : ''}', + ); + } - final storageDirectory = kIsWeb - ? null - : (await setStorageDirectory(snapshot.header.publicId)).path; - // Closed, or another strategy opened, while its folder was being made. - if (attempt != _openAttempt) { - throw StateError('Opening $strategyID gave way to something newer.'); - } - state = state.copyWith( - strategyId: snapshot.header.publicId, - strategyName: snapshot.header.name, - source: StrategySource.cloud, - storageDirectory: storageDirectory, - isOpen: true, - ); + final storageDirectory = kIsWeb + ? null + : (await opening.wait(setStorageDirectory(snapshot.header.publicId))) + .path; + state = state.copyWith( + strategyId: snapshot.header.publicId, + strategyName: snapshot.header.name, + source: StrategySource.cloud, + storageDirectory: storageDirectory, + isOpen: true, + ); - await ref.read(strategyPageSessionProvider.notifier).initializeForStrategy( - strategyId: snapshot.header.publicId, - source: StrategySource.cloud, - selectFirstPageIfNeeded: true, - preferredPageId: pageId, - ); - // Gave way while its pages were read: the uploads belong to what is - // open now, or to nothing. - if (attempt != _openAttempt) return; - // Each page the editor reads is checked for images to upload as it - // arrives (see RemoteEditorSnapshotNotifier). - unawaited( - ref.read(cloudMediaUploadQueueProvider.notifier).setActiveStrategy( - snapshot.header.publicId, - ), - ); + await opening.wait( + ref.read(strategyPageSessionProvider.notifier).initializeForStrategy( + strategyId: snapshot.header.publicId, + source: StrategySource.cloud, + selectFirstPageIfNeeded: true, + preferredPageId: pageId, + attempt: opening, + ), + ); + // Each page the editor reads is checked for images to upload as it + // arrives (see RemoteEditorSnapshotNotifier). + unawaited( + ref.read(cloudMediaUploadQueueProvider.notifier).setActiveStrategy( + snapshot.header.publicId, + ), + ); + }); } Future switchPage(String pageID) async { @@ -2059,68 +2071,73 @@ class StrategyProvider extends Notifier { throw StateError('This strategy is moving to the cloud.'); } cancelPendingSave(); - final attempt = ++_openAttempt; - final newStrat = Hive.box(HiveBoxNames.strategiesBox) - .values - .where((StrategyData strategy) { - return strategy.id == id; - }).firstOrNull; - if (newStrat == null) { - return; - } - _withoutPersistenceTracking(() { - ref.read(actionProvider.notifier).resetActionState(); - }); + return _open((opening) async { + final newStrat = Hive.box(HiveBoxNames.strategiesBox) + .values + .where((StrategyData strategy) { + return strategy.id == id; + }).firstOrNull; - List pageImageData = []; - for (final page in newStrat.pages) { - pageImageData.addAll(page.imageData); - } - if (!kIsWeb) { - List allImageIds = []; + if (newStrat == null) { + return; + } + _withoutPersistenceTracking(() { + ref.read(actionProvider.notifier).resetActionState(); + }); + + List pageImageData = []; for (final page in newStrat.pages) { - allImageIds.addAll(page.imageData.map((image) => image.pictureId)); - for (final link in page.lineUpLinks) { - allImageIds.addAll(link.images.map((image) => image.id)); + pageImageData.addAll(page.imageData); + } + if (!kIsWeb) { + List allImageIds = []; + for (final page in newStrat.pages) { + allImageIds.addAll(page.imageData.map((image) => image.pictureId)); + for (final link in page.lineUpLinks) { + allImageIds.addAll(link.images.map((image) => image.id)); + } } + await opening.wait( + ref + .read(placedImageProvider.notifier) + .deleteUnusedImages(newStrat.id, allImageIds), + ); } - await ref - .read(placedImageProvider.notifier) - .deleteUnusedImages(newStrat.id, allImageIds); - } - // We clear previous data to avoid artifacts when loading a new strategy - final migratedStrategy = StrategyMigrator.migrateToCurrentVersion(newStrat); - - if (migratedStrategy != newStrat) { - await Hive.box(HiveBoxNames.strategiesBox) - .put(migratedStrategy.id, migratedStrategy); - } + // We clear previous data to avoid artifacts when loading a new strategy + final migratedStrategy = + StrategyMigrator.migrateToCurrentVersion(newStrat); - final newDir = - kIsWeb ? null : await setStorageDirectory(migratedStrategy.id); - // Left, or another strategy opened, while this one was being read from - // disk: opening it now would replace what the reader moved on to. - if (attempt != _openAttempt) return; - state = StrategyState( - strategyId: migratedStrategy.id, - strategyName: migratedStrategy.name, - source: StrategySource.local, - storageDirectory: newDir?.path, - isOpen: true, - ); - ref.read(strategySaveStateProvider.notifier).reset(); - await ref.read(strategyPageSessionProvider.notifier).initializeForStrategy( - strategyId: migratedStrategy.id, - source: StrategySource.local, - selectFirstPageIfNeeded: true, - preferredPageId: pageId, + if (migratedStrategy != newStrat) { + await opening.wait( + Hive.box(HiveBoxNames.strategiesBox) + .put(migratedStrategy.id, migratedStrategy), ); - // Gave way while its pages were read: the save state now belongs to - // another strategy, and marking it saved could hide that one's edits. - if (attempt != _openAttempt) return; - ref.read(strategySaveStateProvider.notifier).markPersisted(); + } + + final newDir = kIsWeb + ? null + : await opening.wait(setStorageDirectory(migratedStrategy.id)); + state = StrategyState( + strategyId: migratedStrategy.id, + strategyName: migratedStrategy.name, + source: StrategySource.local, + storageDirectory: newDir?.path, + isOpen: true, + ); + ref.read(strategySaveStateProvider.notifier).reset(); + await opening.wait( + ref.read(strategyPageSessionProvider.notifier).initializeForStrategy( + strategyId: migratedStrategy.id, + source: StrategySource.local, + selectFirstPageIfNeeded: true, + preferredPageId: pageId, + attempt: opening, + ), + ); + ref.read(strategySaveStateProvider.notifier).markPersisted(); + }); } /// The page settings a new strategy starts with: the user's defaults. diff --git a/lib/strategy/open_attempt.dart b/lib/strategy/open_attempt.dart new file mode 100644 index 000000000..0a128c834 --- /dev/null +++ b/lib/strategy/open_attempt.dart @@ -0,0 +1,34 @@ +/// One attempt to open a strategy in the editor. +/// +/// Opening takes several waits (the disk, the server, the pages), and the +/// reader can leave or open something else during any of them. An open that +/// gave way must do nothing further: not mark itself open, not touch the save +/// state or the upload queue, which by then belong to whatever replaced it. +/// +/// So every wait inside an open goes through [wait], which ends the open +/// right there if it gave way. test/open_attempt_test.dart fails on an +/// `await` in an open that does not. +class OpenAttempt { + OpenAttempt(this._isCurrent); + + /// An attempt nothing supersedes, for a caller that is not an open. + static final unchallenged = OpenAttempt(() => true); + + final bool Function() _isCurrent; + + /// Waits for [step], then throws [OpenGaveWay] if this open was closed or + /// replaced meanwhile. Whoever started the attempt catches it. + Future wait(Future step) async { + final result = await step; + if (!_isCurrent()) throw const OpenGaveWay(); + return result; + } +} + +/// Ends an open that was closed, or replaced by a newer one, while it waited. +class OpenGaveWay implements Exception { + const OpenGaveWay(); + + @override + String toString() => 'The open gave way to something newer.'; +} diff --git a/lib/strategy_view.dart b/lib/strategy_view.dart index d52ff9f8e..ce6a1fb56 100644 --- a/lib/strategy_view.dart +++ b/lib/strategy_view.dart @@ -133,9 +133,18 @@ class _StrategyViewState extends ConsumerState bool _hasInitialLoadCompleted = false; /// Set when the reader leaves for the library. The editor stays mounted - /// while it animates out, and a load still waiting to start must not. + /// while it animates out, so being mounted does not mean still wanted. bool _hasLeft = false; + /// How the initial load waits. A reader who left meanwhile gets nothing + /// more from it: the load ends at that wait. test/open_attempt_test.dart + /// fails on an `await` in the load that does not go through here. + Future _unlessLeft(Future step) async { + final result = await step; + if (_hasLeft || !mounted) throw const _LeftWhileLoading(); + return result; + } + @override void initState() { super.initState(); @@ -180,34 +189,37 @@ class _StrategyViewState extends ConsumerState try { switch (source) { case StrategySource.local: - await ref - .read(strategyProvider.notifier) - .loadFromHive(strategyId, pageId: widget.initialPageId); + await _unlessLeft( + ref + .read(strategyProvider.notifier) + .loadFromHive(strategyId, pageId: widget.initialPageId), + ); case StrategySource.cloud: final viewingThroughLink = ref.read(shareLinkViewProvider)?.strategyPublicId == strategyId; - if (!viewingThroughLink && !await _waitForCloudWorkspace()) { + if (!viewingThroughLink && + !await _unlessLeft(_waitForCloudWorkspace())) { // Signed out, or the cloud is unreachable: nothing failed to // load, and the library says which, so go back without an error. - if (mounted) _leaveAfterFailedLoad(); + _leaveAfterFailedLoad(); return; } - // Left while waiting for sign-in to come back. - if (_hasLeft || !mounted) return; - await ref.read(strategyProvider.notifier).openCloudStrategy( - strategyId, - pageId: widget.initialPageId, - ); + await _unlessLeft( + ref.read(strategyProvider.notifier).openCloudStrategy( + strategyId, + pageId: widget.initialPageId, + ), + ); } - // The reader left while it loaded (the home button works from the - // first frame); the load was dropped with the editor. - if (!mounted) return; final loadedStrategy = ref.read(strategyProvider); if (loadedStrategy.strategyId != strategyId || loadedStrategy.strategyName == null) { throw StateError('Strategy "$strategyId" was not found.'); } _hasInitialLoadCompleted = true; + } on _LeftWhileLoading { + // The home button works from the first frame; the load went with the + // editor. } catch (error, stackTrace) { developer.log( 'Error loading strategy: $error', @@ -215,7 +227,7 @@ class _StrategyViewState extends ConsumerState error: error, stackTrace: stackTrace, ); - if (!mounted) return; + if (_hasLeft || !mounted) return; // Refused: the access-lost dialog says so over the skeleton, and // leads out (see StrategyAccessWatcher). if (ref.read(strategyAccessLostProvider)) return; @@ -505,6 +517,11 @@ class _StrategyViewState extends ConsumerState } } +/// Ends the editor's initial load once the reader has left it. +class _LeftWhileLoading implements Exception { + const _LeftWhileLoading(); +} + /// The editor's way out, at the left of the window strip: the house back to /// the library, or a back arrow when [backTooltip] names somewhere else. class _LeaveButton extends StatelessWidget { diff --git a/test/open_attempt_test.dart b/test/open_attempt_test.dart new file mode 100644 index 000000000..4fab64910 --- /dev/null +++ b/test/open_attempt_test.dart @@ -0,0 +1,105 @@ +import 'dart:async'; +import 'dart:io'; + +import 'package:flutter_test/flutter_test.dart'; +import 'package:icarus/strategy/open_attempt.dart'; + +/// The body of the function whose declaration starts with [signature], from +/// its opening brace to the matching closing one. +String _bodyOf(String path, String signature) { + final source = File(path).readAsStringSync(); + final start = source.indexOf(signature); + expect(start, isNot(-1), reason: '$signature is no longer in $path'); + // The parameter list can hold braces of its own (named parameters), so + // the body starts at the brace after `async`. + final open = source.indexOf('{', source.indexOf(') async', start)); + var depth = 0; + for (var i = open; i < source.length; i++) { + if (source[i] == '{') depth++; + if (source[i] == '}' && --depth == 0) return source.substring(open, i + 1); + } + fail('unbalanced braces after $signature'); +} + +/// Each `await` in [body] that is not followed by [through]. +List _bareWaits(String body, String through) { + final withoutComments = body.replaceAll(RegExp(r'//.*'), ''); + return [ + for (final match in RegExp(r'\bawait\b\s*').allMatches(withoutComments)) + if (!withoutComments.startsWith(through, match.end)) + withoutComments + .substring(match.start) + .split('\n') + .first + .trim(), + ]; +} + +void main() { + test('a wait returns what it waited for while the open is current', + () async { + final opening = OpenAttempt(() => true); + expect(await opening.wait(Future.value(7)), 7); + }); + + test('a wait ends an open that gave way while it waited', () async { + var current = true; + final opening = OpenAttempt(() => current); + final step = Completer(); + final waited = opening.wait(step.future); + // The reader leaves, or opens another strategy, before the step answers. + current = false; + step.complete(7); + await expectLater(waited, throwsA(isA())); + }); + + // Three rounds of review each found one more wait inside an open after + // which a superseded open went on to change something: mark itself open, + // mark another strategy saved, point the upload queue at itself. The rule + // that ends that is mechanical, so it is checked mechanically: inside the + // functions that make up an open, nothing waits except through the helper + // that stops a superseded open. + group('every wait inside an open can end it', () { + const provider = 'lib/providers/strategy_provider.dart'; + const session = 'lib/providers/strategy_page_session_provider.dart'; + const editor = 'lib/strategy_view.dart'; + + for (final (path, signature, through) in [ + (provider, 'Future openCloudStrategy(', 'opening.wait('), + (provider, 'Future loadFromHive(', 'opening.wait('), + (session, 'Future initializeForStrategy(', 'opening.wait('), + (editor, 'Future _loadInitialStrategy(', '_unlessLeft('), + ]) { + test('$signature in $path', () { + expect( + _bareWaits(_bodyOf(path, signature), through), + isEmpty, + reason: 'Wait with `await $through...)` here. A plain await lets ' + 'an open the reader already left carry on to its next step.', + ); + }); + } + + test('the page step of both opens is given the attempt', () { + for (final signature in [ + 'Future openCloudStrategy(', + 'Future loadFromHive(', + ]) { + final body = _bodyOf(provider, signature); + expect(body, contains('initializeForStrategy(')); + expect( + body, + contains('attempt: opening'), + reason: 'Without it the page session waits on its own and cannot ' + 'be stopped when $signature gives way.', + ); + } + }); + + test('the check itself catches a plain await', () { + const body = '{\n await opening.wait(a());\n await b();\n' + ' // await in a comment\n}'; + expect(_bareWaits(body, 'opening.wait('), ['await b();']); + }); + }); +} From d7cfacaca51dae685e8ff31f96f2bb4b22cc05ec Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Wed, 11 Nov 2026 20:19:43 -0500 Subject: [PATCH 7/9] Stop the first page read with its open, and say whether an open took A second review of the single-check shape found three things. The page an open starts on was read and put on the canvas by a helper that waited on its own, so it could replace the canvas of whatever the reader had moved on to. It now takes the open's attempt, like the steps above it, and the test that reads those functions lists it and says what it does not see. An open that gives way returns without opening. Both opens now answer whether they took, so mouse back and forward no longer record a place the reader never reached, and exporting from a card does not go on to save the canvas of a different strategy. A held share code, the stored code and the address now always name the same link. Before, a second link held while the cloud was not ready left the address on the first, and a reload retried the wrong one. --- lib/providers/share_link_provider.dart | 4 ++ .../strategy_page_session_provider.dart | 65 ++++++++++++------- lib/providers/strategy_provider.dart | 22 ++++--- lib/strategy/open_attempt.dart | 11 +++- lib/widgets/mouse_navigation.dart | 17 +++-- lib/widgets/strategy_tile/strategy_tile.dart | 6 +- test/mouse_navigation_test.dart | 56 +++++++++++++++- test/open_attempt_test.dart | 28 ++++++-- test/web_editor_reload_test.dart | 11 +++- 9 files changed, 169 insertions(+), 51 deletions(-) diff --git a/lib/providers/share_link_provider.dart b/lib/providers/share_link_provider.dart index 975367678..df721ff32 100644 --- a/lib/providers/share_link_provider.dart +++ b/lib/providers/share_link_provider.dart @@ -336,10 +336,14 @@ class ShareLinkController extends Notifier { /// finishing late cannot clear a newer code that arrived meanwhile. int _generation = 0; + /// The held code, the stored code and the address say the same link from + /// here on: a reload opens the link in the address, so an address left on + /// an older link would retry that one and drop this one. void _hold(String token) { _generation += 1; state = token; ref.read(pendingShareCodeStoreProvider).write(token); + _pointPageUrlAt(token); } void _release(int generation) { diff --git a/lib/providers/strategy_page_session_provider.dart b/lib/providers/strategy_page_session_provider.dart index ae07e0c27..2d719dde2 100644 --- a/lib/providers/strategy_page_session_provider.dart +++ b/lib/providers/strategy_page_session_provider.dart @@ -373,7 +373,9 @@ class StrategyPageSessionNotifier extends Notifier { ); if (selected != null) { - await opening.wait(_rehydrateActivePageFromSource(selected)); + await opening.wait( + _rehydrateActivePageFromSource(selected, attempt: opening), + ); } // Snapshots that arrived while the strategy was opening went unheard: // catch up on pages that came back while it was closed. @@ -1190,11 +1192,15 @@ class StrategyPageSessionNotifier extends Notifier { } } + /// Reads the page an open starts on and puts it on the canvas. A step of + /// the open [attempt] belongs to, so it stops like one (see [OpenAttempt]). Future _rehydrateActivePageFromSource( String pageId, { + required OpenAttempt attempt, _RemotePageHydrationKey? hydrationKey, bool preserveTextDrafts = false, }) async { + final opening = attempt; final strategyState = ref.read(strategyProvider); final strategyId = strategyState.strategyId; final source = strategyState.source; @@ -1213,14 +1219,17 @@ class StrategyPageSessionNotifier extends Notifier { ); } final pageSource = _resolvePageSource(strategyId, source); - final pageData = await pageSource.loadPage(pageId); - await _applyLoadedPageData( - pageData, - strategyId: strategyId, - source: source, - hydrationKey: hydrationKey, - preserveTextDrafts: preserveTextDrafts, - loadedRemoteSnapshot: pageSource.loadedRemoteSnapshot, + final pageData = await opening.wait(pageSource.loadPage(pageId)); + await opening.wait( + _applyLoadedPageData( + pageData, + strategyId: strategyId, + source: source, + hydrationKey: hydrationKey, + preserveTextDrafts: preserveTextDrafts, + loadedRemoteSnapshot: pageSource.loadedRemoteSnapshot, + attempt: opening, + ), ); } @@ -1233,9 +1242,14 @@ class StrategyPageSessionNotifier extends Notifier { RemoteEditorSnapshot? loadedRemoteSnapshot, ({MapValue map, StrategyThemeState theme})? preservedMetadata, bool Function()? canApply, + OpenAttempt? attempt, }) async { - final availablePageIds = - await _resolvePageSource(strategyId, source).listPageIds(); + // The open this page is read for, if it is one: a page read for an open + // that gave way must not replace the canvas of what took its place. + final opening = attempt ?? OpenAttempt.unchallenged; + final availablePageIds = await opening.wait( + _resolvePageSource(strategyId, source).listPageIds(), + ); if (canApply != null && !canApply()) return; final preserveHistory = source == StrategySource.cloud && _lastHydratedRemotePageKey?.strategyPublicId == strategyId && @@ -1257,13 +1271,15 @@ class StrategyPageSessionNotifier extends Notifier { ? Map.from(ref.read(textDraftProvider)) : const {}; try { - await applyStrategyEditorPageData( - ref, - pageData, - themeProfileId: themeProfileId, - themeOverridePalette: themeOverridePalette, - preserveHistory: preserveHistory, - mapOverride: preservedMetadata?.map, + await opening.wait( + applyStrategyEditorPageData( + ref, + pageData, + themeProfileId: themeProfileId, + themeOverridePalette: themeOverridePalette, + preserveHistory: preserveHistory, + mapOverride: preservedMetadata?.map, + ), ); for (final entry in retainedTextDrafts.entries) { ref.read(textDraftProvider.notifier).setDraft(entry.key, entry.value); @@ -1291,11 +1307,14 @@ class StrategyPageSessionNotifier extends Notifier { : hydrationKey, ); } finally { - state = state.copyWith( - activePageId: pageData.pageId, - isApplyingPage: false, - ); - _resumePendingRemoteReapplyIfPossible(); + // After an open gave way the session belongs to what replaced it. + if (opening.isCurrent) { + state = state.copyWith( + activePageId: pageData.pageId, + isApplyingPage: false, + ); + _resumePendingRemoteReapplyIfPossible(); + } } } diff --git a/lib/providers/strategy_provider.dart b/lib/providers/strategy_provider.dart index dc290d468..7cb7f9c35 100644 --- a/lib/providers/strategy_provider.dart +++ b/lib/providers/strategy_provider.dart @@ -107,14 +107,17 @@ class StrategyProvider extends Notifier { /// Runs one attempt to open a strategy. Every wait in [steps] goes through /// the attempt it is given (see [OpenAttempt]), so an open the reader left, /// or replaced by opening another strategy, stops at that wait and changes - /// nothing after it. Giving way is not a failure: this returns normally, - /// and what is open afterwards is whatever took its place. - Future _open(Future Function(OpenAttempt opening) steps) async { + /// nothing after it. Giving way is not a failure, so nothing is thrown: + /// the answer is whether the strategy opened. False means something else + /// is open now (or nothing), and a caller that goes on to act on "its" + /// strategy must look first. + Future _open(Future Function(OpenAttempt opening) steps) async { final attempt = ++_openAttempt; try { - await steps(OpenAttempt(() => attempt == _openAttempt)); + return await steps(OpenAttempt(() => attempt == _openAttempt)); } on OpenGaveWay { // The newer open, or the close, owns the outcome. + return false; } } @@ -358,8 +361,9 @@ class StrategyProvider extends Notifier { } /// Opens the cloud strategy [strategyID] in the editor, on [pageId] when - /// it has that page. - Future openCloudStrategy(String strategyID, {String? pageId}) async { + /// it has that page. False when it gave way to a newer open or a close + /// before it finished (see [_open]); throws when it could not be read. + Future openCloudStrategy(String strategyID, {String? pageId}) async { // A cloud copy still being built by a move from this device is the // mover's: an edit made in it would be lost if the move fails. if (ref.read(movingToCloudProvider).contains(strategyID)) { @@ -414,6 +418,7 @@ class StrategyProvider extends Notifier { snapshot.header.publicId, ), ); + return true; }); } @@ -2063,7 +2068,7 @@ class StrategyProvider extends Notifier { } /// Opens strategy [id] on its first page, or on [pageId] when given. - Future loadFromHive(String id, {String? pageId}) async { + Future loadFromHive(String id, {String? pageId}) async { _requireLocalLibrary('open'); // Edits made while a strategy moves to the cloud would go with the // local original the move deletes. @@ -2080,7 +2085,7 @@ class StrategyProvider extends Notifier { }).firstOrNull; if (newStrat == null) { - return; + return false; } _withoutPersistenceTracking(() { ref.read(actionProvider.notifier).resetActionState(); @@ -2137,6 +2142,7 @@ class StrategyProvider extends Notifier { ), ); ref.read(strategySaveStateProvider.notifier).markPersisted(); + return true; }); } diff --git a/lib/strategy/open_attempt.dart b/lib/strategy/open_attempt.dart index 0a128c834..f42215d44 100644 --- a/lib/strategy/open_attempt.dart +++ b/lib/strategy/open_attempt.dart @@ -6,8 +6,10 @@ /// state or the upload queue, which by then belong to whatever replaced it. /// /// So every wait inside an open goes through [wait], which ends the open -/// right there if it gave way. test/open_attempt_test.dart fails on an -/// `await` in an open that does not. +/// right there if it gave way. test/open_attempt_test.dart reads the +/// functions an open is made of and fails on an `await` in them that does +/// not; a function they call that waits on its own has to be given the +/// attempt and added to that list. class OpenAttempt { OpenAttempt(this._isCurrent); @@ -16,11 +18,14 @@ class OpenAttempt { final bool Function() _isCurrent; + /// Whether this open is still the one wanted. + bool get isCurrent => _isCurrent(); + /// Waits for [step], then throws [OpenGaveWay] if this open was closed or /// replaced meanwhile. Whoever started the attempt catches it. Future wait(Future step) async { final result = await step; - if (!_isCurrent()) throw const OpenGaveWay(); + if (!isCurrent) throw const OpenGaveWay(); return result; } } diff --git a/lib/widgets/mouse_navigation.dart b/lib/widgets/mouse_navigation.dart index 527c91012..96fa4a029 100644 --- a/lib/widgets/mouse_navigation.dart +++ b/lib/widgets/mouse_navigation.dart @@ -319,6 +319,9 @@ class _MouseNavigationState extends ConsumerState { if (strategyOpen) { // Already in the strategy view; swap strategies in place, exactly // like the quick switcher does. + // An open that gave way to a newer one (or to a close) did not + // move here, and history must not record that it did. + var opened = false; final bool switched = await guardUnsavedStrategyExit( context: navigator.context, ref: ref, @@ -326,15 +329,15 @@ class _MouseNavigationState extends ConsumerState { onContinue: () async { _resetStrategyUiState(); final strategies = ref.read(strategyProvider.notifier); - switch (source) { - case StrategySource.local: - await strategies.loadFromHive(strategyId); - case StrategySource.cloud: - await strategies.openCloudStrategy(strategyId); - } + opened = switch (source) { + StrategySource.local => + await strategies.loadFromHive(strategyId), + StrategySource.cloud => + await strategies.openCloudStrategy(strategyId), + }; }, ); - return switched && mounted; + return switched && opened && mounted; } if (source == StrategySource.cloud) { diff --git a/lib/widgets/strategy_tile/strategy_tile.dart b/lib/widgets/strategy_tile/strategy_tile.dart index 0ac81364c..cd77b339b 100644 --- a/lib/widgets/strategy_tile/strategy_tile.dart +++ b/lib/widgets/strategy_tile/strategy_tile.dart @@ -599,7 +599,11 @@ class _StrategyTileState extends ConsumerState { return; } - await ref.read(strategyProvider.notifier).loadFromHive(_strategyId); + // The export saves the open canvas first, so it must be this + // strategy's: not one the reader opened while this one loaded. + final opened = + await ref.read(strategyProvider.notifier).loadFromHive(_strategyId); + if (!opened) return; await StrategyImportExportService(ref).exportFile(_strategyId); } diff --git a/test/mouse_navigation_test.dart b/test/mouse_navigation_test.dart index 9c3298a42..00d0cae6c 100644 --- a/test/mouse_navigation_test.dart +++ b/test/mouse_navigation_test.dart @@ -207,6 +207,51 @@ void main() { expect(strategies.opened, ['${recorded.name}:shared-id']); }); } + + testWidgets('a back whose open gave way does not count as having moved', + (tester) async { + // An open that gives way to a newer one returns without opening. Back + // used to record the move anyway, and history gained a place the + // reader never reached. + final strategies = _RecordingStrategies()..givesWay.add('shared-id'); + await tester.pumpWidget( + ProviderScope( + overrides: [ + platformPolicyProvider.overrideWithValue(PlatformPolicy.desktop), + isCloudWorkspaceAvailableProvider.overrideWithValue(true), + strategyProvider.overrideWith(() => strategies), + ], + child: ShadApp( + navigatorKey: appNavigatorKey, + navigatorObservers: [mouseNavigationRouteObserver], + builder: (_, child) => MouseNavigation(child: child!), + home: const Scaffold(body: Text('library')), + ), + ), + ); + strategies.show('shared-id', StrategySource.local); + await tester.pump(); + strategies.show('other-id', StrategySource.local); + await tester.pump(); + + Future back(int pointer) async { + await tester.sendEventToBinding( + PointerDownEvent( + pointer: pointer, + kind: PointerDeviceKind.mouse, + buttons: kBackMouseButton, + position: const Offset(400, 300), + ), + ); + await tester.pumpAndSettle(); + } + + await back(1); + expect(strategies.state.strategyId, 'other-id'); + // Still one step back from where the reader is: back tries it again. + await back(2); + expect(strategies.opened, ['local:shared-id', 'local:shared-id']); + }); }); } @@ -214,6 +259,9 @@ void main() { class _RecordingStrategies extends StrategyProvider { final opened = []; + /// Strategies whose open gives way before it finishes. + final givesWay = {}; + @override StrategyState build() => const StrategyState(); @@ -227,14 +275,18 @@ class _RecordingStrategies extends StrategyProvider { } @override - Future loadFromHive(String id, {String? pageId}) async { + Future loadFromHive(String id, {String? pageId}) async { opened.add('local:$id'); + if (givesWay.contains(id)) return false; show(id, StrategySource.local); + return true; } @override - Future openCloudStrategy(String id, {String? pageId}) async { + Future openCloudStrategy(String id, {String? pageId}) async { opened.add('cloud:$id'); + if (givesWay.contains(id)) return false; show(id, StrategySource.cloud); + return true; } } diff --git a/test/open_attempt_test.dart b/test/open_attempt_test.dart index 4fab64910..e6d5c32f5 100644 --- a/test/open_attempt_test.dart +++ b/test/open_attempt_test.dart @@ -59,15 +59,24 @@ void main() { // that ends that is mechanical, so it is checked mechanically: inside the // functions that make up an open, nothing waits except through the helper // that stops a superseded open. + // + // What this promises: no plain `await` in the functions listed here. What + // it does not: it reads only those functions. A function they call that + // waits and then writes on its own is not seen (the first version missed + // the page read for exactly that reason), so a new step of an open has to + // take the attempt and be added to the list. The two tests after the + // loop check that the steps known today are handed it. group('every wait inside an open can end it', () { const provider = 'lib/providers/strategy_provider.dart'; const session = 'lib/providers/strategy_page_session_provider.dart'; const editor = 'lib/strategy_view.dart'; for (final (path, signature, through) in [ - (provider, 'Future openCloudStrategy(', 'opening.wait('), - (provider, 'Future loadFromHive(', 'opening.wait('), + (provider, 'Future openCloudStrategy(', 'opening.wait('), + (provider, 'Future loadFromHive(', 'opening.wait('), (session, 'Future initializeForStrategy(', 'opening.wait('), + (session, 'Future _rehydrateActivePageFromSource(', 'opening.wait('), + (session, 'Future _applyLoadedPageData(', 'opening.wait('), (editor, 'Future _loadInitialStrategy(', '_unlessLeft('), ]) { test('$signature in $path', () { @@ -80,10 +89,21 @@ void main() { }); } + test('the page read under the page step is given the attempt', () { + final initialize = + _bodyOf(session, 'Future initializeForStrategy('); + expect(initialize, contains('_rehydrateActivePageFromSource(')); + expect(initialize, contains('attempt: opening')); + final rehydrate = + _bodyOf(session, 'Future _rehydrateActivePageFromSource('); + expect(rehydrate, contains('_applyLoadedPageData(')); + expect(rehydrate, contains('attempt: opening')); + }); + test('the page step of both opens is given the attempt', () { for (final signature in [ - 'Future openCloudStrategy(', - 'Future loadFromHive(', + 'Future openCloudStrategy(', + 'Future loadFromHive(', ]) { final body = _bodyOf(provider, signature); expect(body, contains('initializeForStrategy(')); diff --git a/test/web_editor_reload_test.dart b/test/web_editor_reload_test.dart index 5d7327663..702f37db7 100644 --- a/test/web_editor_reload_test.dart +++ b/test/web_editor_reload_test.dart @@ -182,15 +182,20 @@ void main() { expect(page.path, '/share/$code'); // A second link arrived and has not opened yet (the cloud was not - // ready). It is not left behind: opening a strategy keeps its code, so - // a reload still retries it. + // ready). The address names it from then on, with the stored code, so + // a reload retries that link and not the older one. const waiting = 'ICR-XXXX-XXXX-XXXX-XXXX'; await container .read(shareLinkControllerProvider.notifier) .redeemToken(waiting); + expect(heldCode.read(), waiting); + expect(page.path, '/share/$waiting'); + + // It is not left behind when a strategy opens: the reload is its retry. strategy.open('mine', StrategySource.cloud); expect(heldCode.read(), waiting); - expect(page.path, '/share/$code'); + expect(page.path, '/share/$waiting'); + expect(open.read(), 'mine'); }); testWidgets('the web beta toolbar never offers a local Save', (tester) async { From 5c7745abcedbf48c16b95a8f52d76b0343b5265d Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Wed, 11 Nov 2026 21:27:00 -0500 Subject: [PATCH 8/9] Keep the editor when its replacement opens in it When another strategy opened while an editor's own was still loading, the editor treated giving way as a failed load and took itself off the screen. That is right when the other strategy has an editor of its own above. It is wrong when a share link, or back and forward, swapped the strategy in place: the editor that went was the one showing it, and the strategy stayed open with nothing on screen for it. Giving way is now settled before the failure path, and the editor stays when it is the one showing what opened. A local open also clears the cloud read, as closing does, so a cloud strategy still being read cannot answer into a local one. --- lib/providers/strategy_provider.dart | 4 +++ lib/strategy_view.dart | 39 ++++++++++++++++++++++++---- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/lib/providers/strategy_provider.dart b/lib/providers/strategy_provider.dart index 7cb7f9c35..f8aed944e 100644 --- a/lib/providers/strategy_provider.dart +++ b/lib/providers/strategy_provider.dart @@ -2124,6 +2124,10 @@ class StrategyProvider extends Notifier { final newDir = kIsWeb ? null : await opening.wait(setStorageDirectory(migratedStrategy.id)); + // A cloud strategy still being read, or the one this replaces in + // place, has no say in a local one: its late answer, or a link its + // owner disables, must not reach this editor. + ref.read(remoteEditorSnapshotProvider.notifier).clear(); state = StrategyState( strategyId: migratedStrategy.id, strategyName: migratedStrategy.name, diff --git a/lib/strategy_view.dart b/lib/strategy_view.dart index ce6a1fb56..3d770eba5 100644 --- a/lib/strategy_view.dart +++ b/lib/strategy_view.dart @@ -35,6 +35,7 @@ import 'package:icarus/widgets/strategy_quick_switcher.dart'; import 'package:icarus/widgets/touch_strategy_viewer.dart'; import 'package:icarus/widgets/turn_phone_notice.dart'; import 'package:icarus/widgets/map_selector.dart'; +import 'package:icarus/widgets/mouse_navigation.dart'; import 'package:icarus/widgets/pages_bar.dart'; import 'package:icarus/widgets/editor_toolbar.dart'; import 'package:icarus/widgets/strategy_presence.dart'; @@ -214,6 +215,8 @@ class _StrategyViewState extends ConsumerState final loadedStrategy = ref.read(strategyProvider); if (loadedStrategy.strategyId != strategyId || loadedStrategy.strategyName == null) { + // Giving way to another strategy is not a failed load. + if (_gaveWayTo(loadedStrategy.strategyId, openBefore)) return; throw StateError('Strategy "$strategyId" was not found.'); } _hasInitialLoadCompleted = true; @@ -231,11 +234,9 @@ class _StrategyViewState extends ConsumerState // Refused: the access-lost dialog says so over the skeleton, and // leads out (see StrategyAccessWatcher). if (ref.read(strategyAccessLostProvider)) return; - // Another strategy was opened while this one loaded, in an editor of - // its own: nothing failed that the reader should hear about. - final open = ref.read(strategyProvider).strategyId; - if (open != null && open != strategyId && open != openBefore) { - _leaveAfterFailedLoad(); + // Another strategy was opened while this one loaded: nothing failed + // that the reader should hear about. + if (_gaveWayTo(ref.read(strategyProvider).strategyId, openBefore)) { return; } final notKept = ref.read(remoteEditorSnapshotProvider).error @@ -256,6 +257,34 @@ class _StrategyViewState extends ConsumerState } } + /// Whether [open], the strategy open now, took this editor's place while + /// its own was loading: it is another strategy, opened since the load + /// began ([openBefore] is what was open then). If so, this settles where + /// it shows. + /// + /// It shows in an editor of its own above this one when it was opened from + /// outside (a share link arriving as this editor starts), and then this + /// editor goes. Or it shows in this very editor, when a share link or + /// back and forward swapped strategies in place: then this editor stays + /// and shows it. Popping here would leave it open with nothing showing it. + bool _gaveWayTo(String? open, String? openBefore) { + if (open == null || + open == widget.initialStrategyId || + open == openBefore) { + return false; + } + final route = ModalRoute.of(context); + final top = mouseNavigationRouteObserver.topRoute; + final anotherEditorShowsIt = + top != route && top?.settings.name == Routes.strategyView; + if (!anotherEditorShowsIt) { + _hasInitialLoadCompleted = true; + } else if (route != null && route.isActive) { + Navigator.of(context).removeRoute(route); + } + return true; + } + /// Takes this editor off the screen after its strategy failed to open. /// Another screen may have been pushed over it meanwhile (a share link /// opening its own editor), and a plain pop would close that one and From d41cf6825bc35bdaa346cda5d7809d1c1387dbe7 Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Wed, 11 Nov 2026 21:40:13 -0500 Subject: [PATCH 9/9] Clear the cloud read on a local open only when there is one Clearing it on every local open also ran where no cloud is set up, which three local-library tests showed by failing. --- lib/providers/strategy_provider.dart | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/providers/strategy_provider.dart b/lib/providers/strategy_provider.dart index f8aed944e..abaecfd3e 100644 --- a/lib/providers/strategy_provider.dart +++ b/lib/providers/strategy_provider.dart @@ -2127,7 +2127,8 @@ class StrategyProvider extends Notifier { // A cloud strategy still being read, or the one this replaces in // place, has no say in a local one: its late answer, or a link its // owner disables, must not reach this editor. - ref.read(remoteEditorSnapshotProvider.notifier).clear(); + final remote = ref.read(remoteEditorSnapshotProvider.notifier); + if (remote.activeStrategyPublicId != null) remote.clear(); state = StrategyState( strategyId: migratedStrategy.id, strategyName: migratedStrategy.name,