-
-
Notifications
You must be signed in to change notification settings - Fork 82
feat: Add Screen Caching & Offline Support to Stac #386
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
Conversation
WalkthroughThis PR introduces a comprehensive caching mechanism for Stac by adding configuration models, persistent storage service, five caching strategies (networkFirst, cacheFirst, optimistic, cacheOnly, networkOnly), integration into StacCloud fetch operations, public API extensions to the Stac widget, and documentation on caching behavior and configuration. Changes
Sequence DiagramsequenceDiagram
participant Stac
participant StacCloud
participant StacCacheService
participant Network
participant SharedPreferences
Stac->>StacCloud: fetchScreen(routeName, cacheConfig)
activate StacCloud
alt optimistic (default)
StacCloud->>StacCacheService: getCachedScreen(routeName)
activate StacCacheService
StacCacheService->>SharedPreferences: read cache entry
deactivate StacCacheService
alt cache exists
StacCloud-->>Stac: return cached data immediately
alt refreshInBackground enabled
StacCloud->>Network: fetch in background (async)
Network-->>StacCloud: new data
StacCloud->>StacCacheService: saveScreen (if version newer)
end
else cache missing
StacCloud->>Network: fetch from network
Network-->>StacCloud: response
StacCloud->>StacCacheService: saveScreen(screenName, stacJson, version)
StacCacheService->>SharedPreferences: store cache entry
StacCloud-->>Stac: return fetched data
end
else cacheFirst
StacCloud->>StacCacheService: isCacheValid(screenName, maxAge)
alt valid cache exists
StacCloud->>StacCacheService: getCachedScreen(routeName)
StacCloud-->>Stac: return cached data
else cache missing or expired
StacCloud->>Network: fetch from network
Network-->>StacCloud: response
StacCloud->>StacCacheService: saveScreen()
StacCloud-->>Stac: return fetched data
end
else networkFirst
StacCloud->>Network: try network first
alt success
Network-->>StacCloud: response
StacCloud->>StacCacheService: saveScreen()
StacCloud-->>Stac: return network data
else failure
StacCloud->>StacCacheService: getCachedScreen(routeName)
StacCloud-->>Stac: return cached data (fallback)
end
end
deactivate StacCloud
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/stac/lib/src/models/stac_cache_config.dart (1)
1-22: Clarify strategy docs and consider allowingmaxAgeto be cleared incopyWithOverall the enum and config API look good and read well. Two small polish suggestions:
- The
networkFirstdoc reads as “Always fetch from network, update cache in background.” If the actual behavior is “try network, then fall back to cache on failure/offline”, it may be worth making that explicit to distinguish it fromnetworkOnlyand avoid confusion for users familiar with common “network first” semantics.- In
copyWith,maxAgeusesmaxAge ?? this.maxAge, which means there’s no way to intentionally resetmaxAgeback tonullviacopyWith. If you expect callers to sometimes remove a time‑based expiry and fall back to “version only” invalidation, consider the usual sentinel approach (e.g.,Object? maxAge = _sentinel) socopyWith(maxAge: null)can clear it.Neither is blocking, but adjusting them would make the API more predictable for consumers.
Also applies to: 48-92
packages/stac/lib/src/models/models.dart (1)
1-2: Barrel exports expose caching models as part of the public APIRe‑exporting
StacCacheConfigandStacScreenCachethroughmodels.dartaligns with the goal of making caching configurable from user code. Just ensure you’re comfortable withStacScreenCachebeing a stable public type (vs. an internal detail), since changing it later would be a breaking change.docs/concepts/caching.mdx (1)
214-220: Clarify the "Initial Load" column for accuracy.The strategy comparison table for
optimisticshows "Network → Cache" for initial load, which could be misinterpreted. On first load (no cache), it fetches from network. On subsequent loads with valid/stale cache, it returns cache immediately and fetches in background.Consider clarifying:
-| `optimistic` | Network → Cache | Cache (bg update) | ✅ Yes | Fast UX | +| `optimistic` | Network (then cache) | Cache + bg update | ✅ Yes | Fast UX |This better conveys that the initial load is network-bound, while subsequent loads serve cache immediately.
packages/stac/lib/src/services/stac_cache_service.dart (1)
17-19: Minor race condition in SharedPreferences initialization.If multiple async calls invoke
_sharedPrefsbefore_prefsis assigned, each will callSharedPreferences.getInstance(). While benign (SharedPreferences returns the same singleton), it's slightly wasteful.Consider a
Completer-based approach for cleaner single initialization:static Completer<SharedPreferences>? _prefsCompleter; static Future<SharedPreferences> get _sharedPrefs async { if (_prefsCompleter == null) { _prefsCompleter = Completer<SharedPreferences>(); _prefsCompleter!.complete(SharedPreferences.getInstance()); } return _prefsCompleter!.future; }packages/stac/lib/src/services/stac_cloud.dart (1)
92-96: Remove unreachable switch cases.The
cacheOnlyandnetworkOnlycases in the switch statement are unreachable since they're handled by early returns at lines 48-63. This is dead code that can be removed for clarity.case StacCacheStrategy.optimistic: return _handleOptimistic( routeName, cachedScreen, isCacheValid, cacheConfig, ); - - case StacCacheStrategy.cacheOnly: - case StacCacheStrategy.networkOnly: - // Already handled above - return _fetchFromNetwork(routeName, saveToCache: false); } }Since the switch is on an enum and all cases must be handled, you'll need to keep the cases but can simplify by asserting they're unreachable or relying on the enum exhaustiveness.
packages/stac/lib/src/framework/stac.dart (1)
376-379: Consider defensive null handling for response data.While
snapshot.hasDataensuressnapshot.datais non-null, accessingdata['stacJson']assumes a specific response structure. If the API response format changes or is malformed, this will throw a runtime exception.Consider adding defensive checks:
if (snapshot.hasData) { final responseData = snapshot.data!.data; final jsonString = responseData?['stacJson'] as String?; if (jsonString == null) { return errorWidget ?? const SizedBox(); } return StacService.fromJson(jsonDecode(jsonString), context) ?? const SizedBox(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
examples/counter_example/pubspec.lockis excluded by!**/*.lockexamples/movie_app/ios/Podfile.lockis excluded by!**/*.lockexamples/movie_app/pubspec.lockis excluded by!**/*.lockexamples/stac_gallery/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
docs/concepts/caching.mdx(1 hunks)docs/docs.json(1 hunks)examples/counter_example/macos/Flutter/GeneratedPluginRegistrant.swift(1 hunks)examples/movie_app/macos/Flutter/GeneratedPluginRegistrant.swift(1 hunks)examples/stac_gallery/macos/Flutter/GeneratedPluginRegistrant.swift(1 hunks)packages/stac/lib/src/framework/stac.dart(8 hunks)packages/stac/lib/src/models/models.dart(1 hunks)packages/stac/lib/src/models/stac_cache_config.dart(1 hunks)packages/stac/lib/src/models/stac_screen_cache.dart(1 hunks)packages/stac/lib/src/models/stac_screen_cache.g.dart(1 hunks)packages/stac/lib/src/services/services.dart(1 hunks)packages/stac/lib/src/services/stac_cache_service.dart(1 hunks)packages/stac/lib/src/services/stac_cloud.dart(2 hunks)packages/stac/lib/stac.dart(1 hunks)packages/stac/pubspec.yaml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
examples/stac_gallery/macos/Flutter/GeneratedPluginRegistrant.swift (2)
examples/counter_example/macos/Flutter/GeneratedPluginRegistrant.swift (1)
RegisterGeneratedPlugins(12-16)examples/movie_app/macos/Flutter/GeneratedPluginRegistrant.swift (1)
RegisterGeneratedPlugins(12-16)
examples/movie_app/macos/Flutter/GeneratedPluginRegistrant.swift (2)
examples/counter_example/macos/Flutter/GeneratedPluginRegistrant.swift (1)
RegisterGeneratedPlugins(12-16)examples/stac_gallery/macos/Flutter/GeneratedPluginRegistrant.swift (1)
RegisterGeneratedPlugins(13-18)
🪛 LanguageTool
docs/concepts/caching.mdx
[style] ~104-~104: In American English, “you” and “have” do not usually form a contraction unless they’re followed by a past participle.
Context: ... Offline-only mode, airplane mode, when you've pre-cached screens. ### 5. Network Onl...
(IVE_I_HAVE_AMERICAN_STYLE)
🔇 Additional comments (18)
packages/stac/pubspec.yaml (1)
17-27: SharedPreferences dependency looks appropriate; just confirm SDK / platform matrixAdding
shared_preferences: ^2.5.3fits the new caching service; please just double‑check this version is compatible with your declared Flutter/Dart SDKs and all platforms you intend to support (especially web/desktop), since this package wires in platform plugins.examples/movie_app/macos/Flutter/GeneratedPluginRegistrant.swift (1)
8-15: MacOS SharedPreferences plugin registration is consistentImporting
shared_preferences_foundationand registeringSharedPreferencesPluginalongside the other plugins matches the pattern in the other example apps; no issues from a wiring perspective. Just ensure these registrant files stay in sync with Flutter’s generated output after future plugin changes.examples/counter_example/macos/Flutter/GeneratedPluginRegistrant.swift (1)
8-15: Counter example macOS registrant wiring looks goodThe added
shared_preferences_foundationimport andSharedPreferencesPluginregistration align with the other examples and the new shared_preferences dependency; nothing to change here.examples/stac_gallery/macos/Flutter/GeneratedPluginRegistrant.swift (1)
8-17: Gallery macOS registrant correctly adds SharedPreferencesThe shared_preferences foundation import and registration are added without disturbing existing plugins (including WebView); this is consistent with the other macOS examples.
packages/stac/lib/src/models/stac_screen_cache.g.dart (1)
1-23: Generated JSON mapping forStacScreenCachelooks correctThe (de)serialization functions cleanly mirror the expected fields (
name,stacJson,versionas int,cachedAtas ISO 8601). This is fine as long as all writers use ISO 8601 strings forcachedAt.docs/docs.json (1)
37-44: Newconcepts/cachingdocs entry is wired correctlyThe new navigation entry under “Concepts” looks consistent with the existing structure. Just confirm that
docs/concepts/caching.mdx(or equivalent) exists and uses the same slug so the link doesn’t 404.packages/stac/lib/stac.dart (1)
2-2: LGTM!The new export for
models.dartcorrectly exposes the caching configuration types (StacCacheConfig,StacCacheStrategy,StacScreenCache) through the public API, enabling consumers to configure caching behavior.packages/stac/lib/src/services/services.dart (1)
1-2: LGTM!The cache service export follows the existing barrel pattern and correctly exposes
StacCacheServicefor public use.docs/concepts/caching.mdx (1)
1-232: Well-documented caching feature.The documentation comprehensively covers all cache strategies, configuration options, and usage patterns with clear examples. The structure with behavior summaries and "Best for" recommendations is helpful for users choosing the right strategy.
packages/stac/lib/src/models/stac_screen_cache.dart (1)
1-86: LGTM!The
StacScreenCachemodel is well-designed:
- Immutable with
finalfields andconstconstructor- Complete JSON serialization with both map and string variants
- Proper equality semantics using all fields
copyWithfor immutable updatesThe lack of error handling in
fromJsonStringis acceptable sinceStacCacheService.getCachedScreenwraps calls in try-catch.packages/stac/lib/src/services/stac_cache_service.dart (2)
24-40: LGTM - Good defensive error handling.The
getCachedScreenmethod properly catches exceptions and returnsnull, allowing the app to gracefully fall back to network fetching. This resilience pattern is consistently applied throughout the service.
109-122: Nice use of parallel deletion.Using
Future.waitfor clearing cache entries in parallel is more efficient than sequential awaits, especially when there are many cached screens.packages/stac/lib/src/services/stac_cloud.dart (3)
221-255: Good background fetch implementation.The
_fetchAndUpdateInBackgroundmethod has solid design:
- Deduplication via
_backgroundFetchInProgressSet prevents parallel fetches for the same screen- Version comparison ensures cache is only updated when server has newer content
finallyblock guarantees cleanup even on errors- Silent failure is appropriate for background operations
15-22: Good network configuration.The Dio configuration with explicit timeouts (10s connect, 30s receive) and HTTPS URL provides reasonable defaults for mobile network conditions.
147-165: Verify optimistic strategy behavior aligns with documentation.The
_handleOptimisticmethod returns cache if it exists and is either valid ORstaleWhileRevalidateis true. However, the defaultStacCacheConfighasstaleWhileRevalidate: false, so expired cache won't be returned by default.This means with default config:
- Valid cache → return immediately, background refresh
- Expired cache → fetch from network (not instant)
Confirm this matches the documented behavior: "Returns cached data instantly (even if expired)" at line 35 of caching.mdx. The "even if expired" claim is only true if
staleWhileRevalidate: true.packages/stac/lib/src/framework/stac.dart (3)
110-118: LGTM - Consistent default cache configuration.The default
cacheConfigusingStacCacheStrategy.optimisticmatches the default inStacCloud.fetchScreenand aligns with the PR objective of providing fast perceived performance with instant loading.
387-394: Good use of Material wrapper.Wrapping
CircularProgressIndicatorinMaterialensures proper theme inheritance and avoids rendering issues when the widget tree lacks aMaterialancestor.
42-99: Excellent class documentation.The comprehensive documentation with usage examples, caching explanation, and references to related types significantly improves developer experience. This is a great model for API documentation.
Description
StacCacheConfigandStacCacheStrategyto control how screens are cached and fetched.optimistic(default),cacheFirst,networkFirst,cacheOnly, andnetworkOnly.maxAge,refreshInBackground, andstaleWhileRevalidatefor finer cache control.Stacwidget when using Stac Cloud, including version-based updates for cached screens.caching.mdxto document strategies, configuration, and behavior.Motivation
Related Issues
Closes #200
Type of Change
Summary by CodeRabbit
New Features
fromJson,fromAssets,fromNetwork.Documentation
✏️ Tip: You can customize this high-level summary in your review settings.