From b51b3aeacb260d0207444b5977b70c7696d1151f Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 8 Jun 2026 10:05:42 +0000 Subject: [PATCH] test(ensemble): cover navigateViewGroup clamp, CDN stale refresh, storage clear Extract @visibleForTesting helpers for recent regression fixes: - resolveNavigateViewGroupTabIndex (navigateViewGroup index clamping) - applyStaleRefreshOutcome (CDN race condition on cold start vs resume) - isCdnManifestIncomingNewer (CDN manifest freshness gating) - runEnsembleStorageClearDispatches (storage.clear binding ordering) Add focused unit tests that prove correctness without widget or network deps. Co-authored-by: Sharjeel Yunus --- modules/ensemble/lib/framework/action.dart | 5 +- .../ensemble/lib/framework/data_context.dart | 24 +++++++-- .../definition_providers/cdn_provider.dart | 36 ++++++++------ .../lib/framework/view/page_group.dart | 7 +++ .../test/cdn_manifest_freshness_test.dart | 24 +++++++++ modules/ensemble/test/cdn_provider_test.dart | 49 +++++++++++++++++++ .../ensemble_storage_clear_dispatch_test.dart | 40 +++++++++++++++ .../safe_view_group_payload_index_test.dart | 12 +++++ 8 files changed, 175 insertions(+), 22 deletions(-) create mode 100644 modules/ensemble/test/cdn_manifest_freshness_test.dart create mode 100644 modules/ensemble/test/ensemble_storage_clear_dispatch_test.dart diff --git a/modules/ensemble/lib/framework/action.dart b/modules/ensemble/lib/framework/action.dart index bafd15cc7..8c9f5b847 100644 --- a/modules/ensemble/lib/framework/action.dart +++ b/modules/ensemble/lib/framework/action.dart @@ -170,9 +170,8 @@ class NavigateViewGroupAction extends EnsembleAction { } else if (viewIndex != null) { final pageGroup = context.findAncestorWidgetOfExactType(); final menuLen = pageGroup?.menu.menuItems.length ?? 0; - final resolvedIndex = menuLen > 0 - ? safeViewGroupPayloadIndex(viewIndex, menuLen) - : viewIndex; + final resolvedIndex = + resolveNavigateViewGroupTabIndex(viewIndex, menuLen); if (payload != null) { // TODO: this is wrong. Can't mutate the scope like this scopeManager.dataContext.addDataContext(payload); diff --git a/modules/ensemble/lib/framework/data_context.dart b/modules/ensemble/lib/framework/data_context.dart index c275b34ec..0aca5f84a 100644 --- a/modules/ensemble/lib/framework/data_context.dart +++ b/modules/ensemble/lib/framework/data_context.dart @@ -746,6 +746,20 @@ class RemoteConfigInvokable with Invokable { List ensembleStorageClearDispatchKeys(Iterable allKeys) => allKeys.where((key) => !key.startsWith('enc_')).toList(); +/// Runs [dispatch] for each [keys] entry only after [clearFuture] completes. +@visibleForTesting +Future runEnsembleStorageClearDispatches( + Future clearFuture, + Iterable keys, + void Function(String key) dispatch, +) { + return clearFuture.whenComplete(() { + for (final key in keys) { + dispatch(key); + } + }); +} + /// Singleton handling user storage class EnsembleStorage with Invokable { static final EnsembleStorage _instance = EnsembleStorage._internal(); @@ -803,11 +817,11 @@ class EnsembleStorage with Invokable { // clearPublicStorage is async; binding listeners re-evaluate by reading // GetStorage. Dispatch only after removal completes so eval() does not // observe stale persisted values (see PR discussion on #2227). - unawaited(StorageManager().clearPublicStorage().whenComplete(() { - for (final key in keys) { - ScreenController().dispatchStorageChanges(context, key, null); - } - })); + unawaited(runEnsembleStorageClearDispatches( + StorageManager().clearPublicStorage(), + keys, + (key) => ScreenController().dispatchStorageChanges(context, key, null), + )); } @override diff --git a/modules/ensemble/lib/framework/definition_providers/cdn_provider.dart b/modules/ensemble/lib/framework/definition_providers/cdn_provider.dart index 1e955bd97..5c4c02945 100644 --- a/modules/ensemble/lib/framework/definition_providers/cdn_provider.dart +++ b/modules/ensemble/lib/framework/definition_providers/cdn_provider.dart @@ -202,6 +202,16 @@ class CdnDefinitionProvider extends DefinitionProvider { } } + /// After a stale CDN fetch updates [_artifactCache], either apply changes + /// immediately (artifact refresh on + app initialized) or defer to resume. + Future _applyStaleRefreshOutcome() async { + if (isArtifactRefreshEnabled() && Ensemble().getConfig() != null) { + await _handlePendingUpdate(); + } else { + _hasPendingUpdate = true; + } + } + /// Handles pending CDN updates when app resumes. /// CRITICAL: Must update appBundle and translations BEFORE firing refresh event /// to ensure screens rebuild with the new resources (fixes race condition). @@ -513,16 +523,7 @@ class CdnDefinitionProvider extends DefinitionProvider { // Save to persistent cache await _saveCachedState(jsonString); - // If artifact refresh is enabled and app is already initialized, - // immediately update appBundle and fire refresh event. - // This handles the cold start scenario where background refresh - // completes after initial render. - if (isArtifactRefreshEnabled() && Ensemble().getConfig() != null) { - await _handlePendingUpdate(); - } else { - // Mark for later refresh on next resume - _hasPendingUpdate = true; - } + await _applyStaleRefreshOutcome(); } catch (e) { if (kDebugMode) { debugPrint('⚠️ CDN Provider: Refresh failed: $e'); @@ -595,7 +596,8 @@ class CdnDefinitionProvider extends DefinitionProvider { return true; } - final shouldFetch = _isIncomingNewer(remoteLastUpdate, _lastUpdatedAt); + final shouldFetch = + isCdnManifestIncomingNewer(remoteLastUpdate, _lastUpdatedAt); _lastUpdatedAt = remoteLastUpdate; @@ -854,9 +856,6 @@ class CdnDefinitionProvider extends DefinitionProvider { // Helpers // -------------------------------------------------------- - static bool _isIncomingNewer(int? incoming, int? current) => - incoming != null && (current == null || incoming > current); - static Map? _asMap(dynamic value) { if (value is Map) return value; if (value is Map) return Map.from(value); @@ -925,6 +924,10 @@ class CdnDefinitionProvider extends DefinitionProvider { @visibleForTesting Future handlePendingUpdateForTesting() => _handlePendingUpdate(); + @visibleForTesting + Future applyStaleRefreshOutcomeForTesting() => + _applyStaleRefreshOutcome(); + @visibleForTesting bool get hasPendingUpdateForTesting => _hasPendingUpdate; @@ -961,3 +964,8 @@ class CdnDefinitionProvider extends DefinitionProvider { } } } + +/// Whether CDN [lastUpdateTime.json] indicates a manifest fetch is needed. +@visibleForTesting +bool isCdnManifestIncomingNewer(int? incoming, int? current) => + incoming != null && (current == null || incoming > current); diff --git a/modules/ensemble/lib/framework/view/page_group.dart b/modules/ensemble/lib/framework/view/page_group.dart index e3b7baac3..bf28b22a9 100644 --- a/modules/ensemble/lib/framework/view/page_group.dart +++ b/modules/ensemble/lib/framework/view/page_group.dart @@ -729,6 +729,13 @@ int safeViewGroupPayloadIndex(int index, int payloadLength) { return index; } +/// Resolves [viewIndex] for [NavigateViewGroupAction] before [PageController.jumpToPage]. +/// When the ancestor [PageGroup] has menu items, clamp to the tab range; otherwise pass +/// through (e.g. nested view groups without a menu). +@visibleForTesting +int resolveNavigateViewGroupTabIndex(int viewIndex, int menuLen) => + menuLen > 0 ? safeViewGroupPayloadIndex(viewIndex, menuLen) : viewIndex; + class ViewGroupNotifier extends ChangeNotifier { int _viewIndex = 0; Map? _payload; diff --git a/modules/ensemble/test/cdn_manifest_freshness_test.dart b/modules/ensemble/test/cdn_manifest_freshness_test.dart new file mode 100644 index 000000000..7e17e714f --- /dev/null +++ b/modules/ensemble/test/cdn_manifest_freshness_test.dart @@ -0,0 +1,24 @@ +import 'package:ensemble/framework/definition_providers/cdn_provider.dart'; +import 'package:flutter_test/flutter_test.dart'; + +void main() { + group('isCdnManifestIncomingNewer', () { + test('fetches when remote timestamp is newer than cached', () { + expect(isCdnManifestIncomingNewer(200, 100), isTrue); + }); + + test('skips fetch when remote is same or older', () { + expect(isCdnManifestIncomingNewer(100, 100), isFalse); + expect(isCdnManifestIncomingNewer(50, 100), isFalse); + }); + + test('fetches when there is no cached timestamp', () { + expect(isCdnManifestIncomingNewer(1, null), isTrue); + }); + + test('does not fetch when remote timestamp is missing', () { + expect(isCdnManifestIncomingNewer(null, 100), isFalse); + expect(isCdnManifestIncomingNewer(null, null), isFalse); + }); + }); +} diff --git a/modules/ensemble/test/cdn_provider_test.dart b/modules/ensemble/test/cdn_provider_test.dart index 8178cbe19..283d0d39b 100644 --- a/modules/ensemble/test/cdn_provider_test.dart +++ b/modules/ensemble/test/cdn_provider_test.dart @@ -171,6 +171,55 @@ void main() { }); }); + group('CDN stale refresh outcome', () { + test('applies updates immediately when artifact refresh is enabled', + () async { + final provider = CdnDefinitionProvider('test-app'); + final config = EnsembleConfig(definitionProvider: provider); + Ensemble().setEnsembleConfig(config); + + await provider.applyRuntimeManifestForTesting( + _manifestWithArtifactRefresh(_manifestWithResourceVersion('v1')), + ); + await config.updateAppBundle(); + + provider.rebuildManifestCacheForTesting( + _manifestWithArtifactRefresh(_manifestWithResourceVersion('v2')), + ); + + await provider.applyStaleRefreshOutcomeForTesting(); + + expect(provider.hasPendingUpdateForTesting, isFalse); + expect( + config.getResources()?[ResourceArtifactEntry.Scripts.name]['version'], + 'v2', + ); + }); + + test('defers updates when artifact refresh is disabled', () async { + final provider = CdnDefinitionProvider('test-app'); + final config = EnsembleConfig(definitionProvider: provider); + Ensemble().setEnsembleConfig(config); + + await provider.applyRuntimeManifestForTesting( + _manifestWithResourceVersion('v1'), + ); + await config.updateAppBundle(); + + provider.rebuildManifestCacheForTesting( + _manifestWithResourceVersion('v2'), + ); + + await provider.applyStaleRefreshOutcomeForTesting(); + + expect(provider.hasPendingUpdateForTesting, isTrue); + expect( + config.getResources()?[ResourceArtifactEntry.Scripts.name]['version'], + 'v1', + ); + }); + }); + group('CDN pending update ordering', () { test('handlePendingUpdate syncs app bundle from CDN cache and fires refresh', () async { diff --git a/modules/ensemble/test/ensemble_storage_clear_dispatch_test.dart b/modules/ensemble/test/ensemble_storage_clear_dispatch_test.dart new file mode 100644 index 000000000..c51632af6 --- /dev/null +++ b/modules/ensemble/test/ensemble_storage_clear_dispatch_test.dart @@ -0,0 +1,40 @@ +import 'package:ensemble/framework/data_context.dart'; +import 'package:flutter_test/flutter_test.dart'; + +void main() { + group('runEnsembleStorageClearDispatches', () { + test('dispatches only after clear future completes', () async { + final dispatched = []; + var storageCleared = false; + + final clearFuture = Future(() async { + await Future.delayed(const Duration(milliseconds: 10)); + storageCleared = true; + }); + + final done = runEnsembleStorageClearDispatches( + clearFuture, + ['session', 'theme'], + dispatched.add, + ); + + expect(dispatched, isEmpty); + await done; + expect(storageCleared, isTrue); + expect(dispatched, ['session', 'theme']); + }); + + test('still dispatches when clear future completes with error', () async { + final dispatched = []; + + final done = runEnsembleStorageClearDispatches( + Future.error(StateError('clear failed')), + ['a'], + dispatched.add, + ); + + await expectLater(done, throwsStateError); + expect(dispatched, ['a']); + }); + }); +} diff --git a/modules/ensemble/test/safe_view_group_payload_index_test.dart b/modules/ensemble/test/safe_view_group_payload_index_test.dart index f129b0bfa..3bd9d7f5a 100644 --- a/modules/ensemble/test/safe_view_group_payload_index_test.dart +++ b/modules/ensemble/test/safe_view_group_payload_index_test.dart @@ -2,6 +2,18 @@ import 'package:ensemble/framework/view/page_group.dart'; import 'package:flutter_test/flutter_test.dart'; void main() { + group('resolveNavigateViewGroupTabIndex', () { + test('clamps when menu has items', () { + expect(resolveNavigateViewGroupTabIndex(99, 3), 2); + expect(resolveNavigateViewGroupTabIndex(-1, 2), 0); + }); + + test('passes through when menu length is zero', () { + expect(resolveNavigateViewGroupTabIndex(5, 0), 5); + expect(resolveNavigateViewGroupTabIndex(-1, 0), -1); + }); + }); + group('safeViewGroupPayloadIndex', () { test('clamps to valid range for positive lengths', () { expect(safeViewGroupPayloadIndex(-1, 3), 0);