-
Notifications
You must be signed in to change notification settings - Fork 55
Caching integration settings and restoring last successful cache #190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements a caching mechanism for integration settings with a priority-based fallback system. When the Analytics SDK initializes, it now attempts to fetch settings from the network, but can gracefully fall back to cached settings, default settings, or an empty map if the network is unavailable.
Key Changes:
- Implements persistent caching for integration settings that survives app restarts
- Introduces a 4-tier priority system: network > cached > default > empty
- Adds
InMemoryStoretest implementation to avoid platform-specific file operations in tests
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/lib/state.dart | Modified IntegrationsState to persist settings to store and load cached settings on initialization |
| packages/core/lib/analytics.dart | Updated _fetchSettings() to implement fallback logic: network → cached → default → empty |
| packages/core/test/mocks/mock_store.dart | Added InMemoryStore class for testing without file system access |
| packages/core/test/mocks/mocks.dart | Changed store() to return real InMemoryStore and added mockStore() for mockito-based tests; added linkStream override to MockPlatform |
| packages/core/test/cached_settings_test.dart | Added comprehensive tests verifying all 4 priority levels of settings fallback |
| packages/core/test/analytics_test.dart | Added tests for settings persistence, loading, and fallback behavior across different scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await analytics4.init(); | ||
|
|
||
| // After initialization with network failure, we should have cached settings | ||
| final stateSettings4 = await analytics4.state.integrations.state; |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect use of await on a synchronous getter. analytics4.state.integrations.state returns a Map<String, dynamic> directly, not a Future. Remove the await keyword: final stateSettings4 = analytics4.state.integrations.state;
| final stateSettings4 = await analytics4.state.integrations.state; | |
| final stateSettings4 = analytics4.state.integrations.state; |
| static Store store() => InMemoryStore(storageJson: true); | ||
|
|
||
| // If tests need a mockito mock for specific verification |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider adding a comment explaining why store() now returns a real InMemoryStore instead of a mock, and when to use mockStore() vs store(). This will help other developers understand when to use each method.
| static Store store() => InMemoryStore(storageJson: true); | |
| // If tests need a mockito mock for specific verification | |
| /// Returns a real InMemoryStore for use in tests that require actual storage behavior. | |
| /// Use this when you want to test with a real store implementation. | |
| static Store store() => InMemoryStore(storageJson: true); | |
| /// Returns a mockito MockStore for tests that require specific verification or stubbing. | |
| /// Use this when you need to mock store interactions or verify method calls. |
| final integrations = state; | ||
| integrations[key] = settings; | ||
| state = integrations; |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addIntegration method directly modifies the map reference obtained from state getter. This mutates the map without creating a new instance, which can lead to unexpected behavior with state notifications. Consider creating a new map: state = {...integrations, key: settings}; to ensure proper state change detection.
| final integrations = state; | |
| integrations[key] = settings; | |
| state = integrations; | |
| state = { | |
| ...state, | |
| key: settings, | |
| }; |
| Future<Map<String, dynamic>> loadCachedSettings() async { | ||
| if (_cachedState != null) { | ||
| return _cachedState!; | ||
| } | ||
|
|
||
| final cachedSettings = await _store.getPersisted(_key); | ||
| if (cachedSettings != null) { | ||
| _cachedState = cachedSettings; | ||
| // Update in-memory state with cached settings | ||
| super.state = cachedSettings; | ||
| return cachedSettings; | ||
| } | ||
|
|
||
| return {}; | ||
| } |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loadCachedSettings method has a race condition. If called concurrently before _cachedState is set, multiple callers will all fetch from the store and could set different values. Consider using a Future to ensure only one load operation happens at a time, similar to how PersistedState handles this with _getCompleter.
|
|
||
| // For compatibility with the ready mechanism | ||
| Future<void> get ready => Future.value(); | ||
| void init(ErrorHandler errorHandler, bool storageJson) { |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The init method ignores the errorHandler and storageJson parameters. If these parameters are not needed, consider removing them from the signature, or document why they're unused. For consistency with other state classes, this method should either use these parameters or have a different signature.
| void init(ErrorHandler errorHandler, bool storageJson) { | |
| void init() { |
| await analytics3.init(); | ||
|
|
||
| // After initialization with cached settings + network fetch, we should have the network settings | ||
| final stateSettings3 = await analytics3.state.integrations.state; |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect use of await on a synchronous getter. analytics3.state.integrations.state returns a Map<String, dynamic> directly, not a Future. Remove the await keyword: final stateSettings3 = analytics3.state.integrations.state;
| final storedSettings = await mockStore1.getPersisted("integrations"); | ||
| expect(storedSettings, equals(networkSettings), | ||
| reason: "Network settings should be stored to cache"); |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test may be flaky due to the race condition in the IntegrationsState.state setter. The setPersisted call is not awaited, so the settings might not be persisted to storage yet when this assertion runs. Consider adding a small delay or awaiting the persistence to ensure the test is deterministic.
| Future<void> purge() async { | ||
| _storage.clear(); | ||
| } | ||
|
|
||
| @override |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purge method is not part of the Store interface. The Store mixin only defines getPersisted, setPersisted, deletePersisted, ready, and dispose. This method should be removed as it's not needed and violates the interface contract.
| Future<void> purge() async { | |
| _storage.clear(); | |
| } | |
| @override |
| set state(Map<String, dynamic> newState) { | ||
| super.state = newState; | ||
| // Persist to store when state is updated | ||
| _store.setPersisted(_key, newState); |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setPersisted call is not awaited, which could lead to race conditions. If the state is updated multiple times in quick succession, writes to the store could be lost or happen out of order. Consider either awaiting the persistence call or queuing writes to ensure data integrity. This is especially important since setPersisted returns a Future.
| await analytics2.init(); | ||
|
|
||
| // Verify default settings are used | ||
| final stateSettings2 = await analytics2.state.integrations.state; |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect use of await on a synchronous getter. analytics2.state.integrations.state returns a Map<String, dynamic> directly, not a Future. Remove the await keyword: final stateSettings2 = analytics2.state.integrations.state;
No description provided.