feat: Add theme support to Stac framework with cloud loading #393
Conversation
📝 WalkthroughWalkthroughThis PR extends the Stac framework to support asynchronous theme loading and generalizes caching/fetching mechanisms from screen-only to artifact-type aware operations. It introduces StacAppTheme utilities for loading themes from cloud/network/JSON sources, refactors StacCacheService and StacCloud to handle multiple artifact types, adds StacThemeConfig annotation, and modifies StacApp to accept FutureOr theme inputs with async resolution flow. Changes
Sequence DiagramsequenceDiagram
participant App as StacApp Builder
participant Resolver as Theme Resolver<br/>(_resolveThemes)
participant Loader as StacAppTheme<br/>(Cloud/Network/JSON)
participant MatApp as MaterialApp
participant UI as UI Renderer
App->>Resolver: Await theme resolution<br/>(FutureOr inputs)
Resolver->>Loader: Load theme from source<br/>(cloud/network/json)
rect rgb(220, 240, 255)
Note over App,Loader: Theme loading in progress
App->>UI: Show _ThemeFutureLoading<br/>(CircularProgressIndicator)
end
Loader->>Loader: Normalize & parse<br/>theme payload
Loader->>Resolver: Return StacTheme?
Resolver->>Resolver: Build _ResolvedStacThemes<br/>(theme, darkTheme)
rect rgb(220, 255, 220)
Note over Resolver,MatApp: Theme resolution complete
Resolver->>MatApp: Provide resolved themes<br/>(theme.parse(context),<br/>darkTheme.parse(context))
end
MatApp->>UI: Render with resolved theme
UI->>UI: Display app UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
packages/stac/lib/src/framework/stac_app.dart (2)
270-280: Consider resolving theme futures in parallel.Both theme futures are awaited sequentially. Using
Future.waitwould allow parallel resolution.return Future<_ResolvedStacThemes>(() async { - final resolvedTheme = - await (themeFuture ?? Future<StacTheme?>.value(themeValue)); - final resolvedDarkTheme = - await (darkThemeFuture ?? Future<StacTheme?>.value(darkThemeValue)); + final results = await Future.wait([ + themeFuture ?? Future<StacTheme?>.value(themeValue), + darkThemeFuture ?? Future<StacTheme?>.value(darkThemeValue), + ]); return _ResolvedStacThemes( - theme: resolvedTheme, - darkTheme: resolvedDarkTheme, + theme: results[0], + darkTheme: results[1], ); });
317-324: Loading widget has no theme context.This widget renders before
MaterialAppis built, so there's no inherited theme. TheMaterialwidget will use fallback colors which may cause a visual flash when the app loads. Consider specifying explicit colors.@override Widget build(BuildContext context) { - return const Material(child: Center(child: CircularProgressIndicator())); + return const ColoredBox( + color: Colors.white, // Or Colors.transparent for less jarring transition + child: Center(child: CircularProgressIndicator()), + ); }packages/stac/lib/src/framework/stac_app_theme.dart (1)
16-33: Silent null returns may hinder debugging.Multiple failure paths return
nullwithout any indication of what went wrong. Consider adding debug logging to help diagnose theme loading issues in development.static Future<StacTheme?> fromCloud({required String themeName}) async { final response = await StacCloud.fetchTheme(themeName: themeName); if (response == null) { + assert(() { debugPrint('StacAppTheme.fromCloud: fetchTheme returned null for "$themeName"'); return true; }()); return null; } final rawData = response.data; if (rawData is! Map<String, dynamic>) { + assert(() { debugPrint('StacAppTheme.fromCloud: response.data is not Map<String, dynamic>'); return true; }()); return null; }packages/stac/lib/src/services/stac_cache_service.dart (2)
19-29: UseStacArtifactTypeenum instead of string for type safety.The
artifactTypeparameter accepts any string, but only'screen'and'theme'are valid. Using theStacArtifactTypeenum would provide compile-time safety and prevent runtimeArgumentError.- static String _getCachePrefix(String artifactType) { - switch (artifactType) { - case 'screen': + static String _getCachePrefix(StacArtifactType artifactType) { + switch (artifactType) { + case StacArtifactType.screen: return 'stac_screen_cache_'; - case 'theme': + case StacArtifactType.theme: return 'stac_theme_cache_'; - default: - throw ArgumentError('Unknown artifact type: $artifactType'); } }This would require updating all method signatures to accept
StacArtifactTypeinstead ofString.
48-48:StacScreenCacheclass used for theme artifacts.The
StacScreenCacheclass is now used to cache both screens and themes, which is a naming inconsistency. Consider renaming toStacArtifactCachefor clarity, or document that the class is intentionally reused.Also applies to: 68-73
packages/stac/lib/src/models/stac_artifact_type.dart (1)
1-8: Inconsistent artifact type handling betweenStacCloudandStacCacheService.The
StacArtifactTypeenum is actively used throughoutStacCloud(via switch statements and type-safe parameters), but whenStacCloudcallsStacCacheServicemethods (e.g.,clearScreenCache,clearThemeCache), it passes hardcoded strings ('screen','theme') instead of enum values. Meanwhile,StacCacheService._getCachePrefix()switches on these string literals. Consider havingStacCacheServiceacceptStacArtifactTypeinstead ofStringto maintain type consistency and avoid string literal brittleness at the boundary.packages/stac/lib/src/services/stac_cloud.dart (3)
69-150: Tighten strategy handling in_fetchArtifactto avoid confusing dead branchThe overall flow (early handling of
networkOnly/cacheOnly, then switching on the remaining strategies) is good, but the finalcacheOnly/networkOnlycases in theswitchare now unreachable and contradict the comment:case StacCacheStrategy.cacheOnly: case StacCacheStrategy.networkOnly: // Already handled above return _fetchArtifactFromNetwork( artifactType: artifactType, artifactName: artifactName, saveToCache: false, );Because both strategies are already fully handled by the
ifbranches above (return/throw), this code will never run, but if someone refactors out the earlyifs later it would silently changecacheOnlyto “network-only, no cache”.Consider either:
- Moving
networkOnly/cacheOnlyhandling entirely into theswitchand dropping the earlyifs, or- Keeping the early
ifs and turning these two cases into an explicit unreachable/StateErrorso any future misuse is caught quickly.For example:
case StacCacheStrategy.optimistic: return _handleArtifactOptimistic( artifactType: artifactType, artifactName: artifactName, cachedArtifact: cachedArtifact, isCacheValid: isCacheValid, config: cacheConfig, ); case StacCacheStrategy.cacheOnly: case StacCacheStrategy.networkOnly: - // Already handled above - return _fetchArtifactFromNetwork( - artifactType: artifactType, - artifactName: artifactName, - saveToCache: false, - ); + // These should have been handled by the early returns above. + throw StateError( + 'Unexpected strategy ${cacheConfig.strategy} in _fetchArtifact; ' + 'cacheOnly/networkOnly should be handled before the switch.', + );Functionally nothing is broken today, but this makes the intent clearer and safer for future changes.
174-266: Strategy helpers read correctly and match the intended semantics
_handleArtifactNetworkFirst,_handleArtifactCacheFirst, and_handleArtifactOptimisticcorrectly implement the advertised behaviors (network-first with cache fallback, cache-first with optional background refresh and stale-while-revalidate, and optimistic with background update). Logging withartifactType.nameis helpful for debugging.The only (very minor) nit is that
StacScreenCacheis now effectively an artifact cache model for both screens and themes; if you ever generalize more artifact types, you might consider renaming it to something likeStacArtifactCachefor clarity, but that would be a larger cross-file change.
381-421: Theme fetch/clear helpers mirror screen APIs; consider removing hard-coded type stringsThe new
fetchTheme,clearThemeCache, andclearAllThemeCachehelpers align well with the existing screen APIs and the artifact-type abstraction.For the cache-clear methods, you’re still using string literals
'screen'/'theme':static Future<bool> clearScreenCache(String routeName) { return StacCacheService.removeArtifact(routeName, 'screen'); } static Future<bool> clearAllCache() { return StacCacheService.clearAllArtifacts('screen'); } static Future<bool> clearThemeCache(String themeName) { return StacCacheService.removeArtifact(themeName, 'theme'); } static Future<bool> clearAllThemeCache() { return StacCacheService.clearAllArtifacts('theme'); }Since you already have
_getArtifactTypeString, you can avoid duplicating these strings and keep everything enum-driven:static Future<bool> clearScreenCache(String routeName) { - return StacCacheService.removeArtifact(routeName, 'screen'); + return StacCacheService.removeArtifact( + routeName, + _getArtifactTypeString(StacArtifactType.screen), + ); } static Future<bool> clearAllCache() { - return StacCacheService.clearAllArtifacts('screen'); + return StacCacheService.clearAllArtifacts( + _getArtifactTypeString(StacArtifactType.screen), + ); } static Future<bool> clearThemeCache(String themeName) { - return StacCacheService.removeArtifact(themeName, 'theme'); + return StacCacheService.removeArtifact( + themeName, + _getArtifactTypeString(StacArtifactType.theme), + ); } static Future<bool> clearAllThemeCache() { - return StacCacheService.clearAllArtifacts('theme'); + return StacCacheService.clearAllArtifacts( + _getArtifactTypeString(StacArtifactType.theme), + ); }This keeps the cache-layer contract tied to the enum and avoids subtle drift if the underlying type strings ever change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/stac/lib/src/framework/framework.dart(1 hunks)packages/stac/lib/src/framework/stac_app.dart(7 hunks)packages/stac/lib/src/framework/stac_app_theme.dart(1 hunks)packages/stac/lib/src/models/stac_artifact_type.dart(1 hunks)packages/stac/lib/src/services/stac_cache_service.dart(4 hunks)packages/stac/lib/src/services/stac_cloud.dart(7 hunks)packages/stac_core/lib/annotations/annotations.dart(1 hunks)packages/stac_core/lib/annotations/stac_cloud_theme.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: analyze
🔇 Additional comments (9)
packages/stac_core/lib/annotations/annotations.dart (1)
3-4: Newstac_cloud_theme.dartexport looks consistentExporting the new annotation alongside
stac_screen.dartkeeps the annotations barrel file up to date and is aligned with how other annotations are exposed.packages/stac_core/lib/annotations/stac_cloud_theme.dart (1)
13-18: Annotation shape is simple and appropriate for intended useConst constructor with a required
themeNameand a singlefinalfield is a clean, idiomatic way to model this annotation and should work well for discovery/registration of theme builders.packages/stac/lib/src/framework/stac_app.dart (1)
113-114: Good backward-compatible API change.The change from
StacTheme?toFutureOr<StacTheme?>?maintains backward compatibility - existing synchronous theme assignments continue to work while enabling async loading.packages/stac/lib/src/framework/framework.dart (1)
5-5: LGTM!Export added to expose the new
StacAppThemeutility class through the framework's public API surface.packages/stac/lib/src/framework/stac_app_theme.dart (1)
39-49: LGTM!The
fromNetworkmethod correctly delegates toStacNetworkService.requestand handles null responses gracefully. TheBuildContextusage is appropriate as it's passed directly to the service.packages/stac/lib/src/services/stac_cache_service.dart (2)
130-139: LGTM!The cache validity logic is clear and handles edge cases properly - null artifact returns false, null maxAge means no expiration. The age comparison is straightforward.
97-110: LGTM!Good use of
Future.waitfor parallel key removal, and prefix-based filtering correctly isolates artifacts by type.packages/stac/lib/src/services/stac_cloud.dart (2)
26-52: Helper extraction for artifact URLs and params looks solidCentralizing
_baseUrl,_getFetchUrl,_getQueryParamName, and_getArtifactTypeStringkeeps screen/theme handling consistent and makes it easy to extend to new artifact types. No changes requested here.
268-331: Network + cache integration for artifacts looks consistent
_makeArtifactRequest,_fetchArtifactFromNetwork, and_buildArtifactCacheResponsenicely generalize the old screen-only path:
- Request URL and query param are derived from the artifact type.
- Cache read/write uses
_getArtifactTypeString(artifactType)consistently, so screens and themes share the same caching mechanism with type partitioning.- Cached responses are shaped identically to network ones (
name,stacJson,version), which keeps downstream consumers simple.No issues from a correctness standpoint; just ensure the Cloud endpoints for both screens and themes always return that
{ name, stacJson, version }shape so caching works uniformly.
| /// Example usage: | ||
| /// ```dart | ||
| /// @StacThemeAnnotation(themeName: 'darkTheme') | ||
| /// ThemeData buildDarkTheme() { | ||
| /// return ThemeData.dark(); | ||
| /// } | ||
| /// ``` |
There was a problem hiding this comment.
Doc example uses the wrong annotation name
The example shows @StacThemeAnnotation but this file defines StacCloudTheme. That mismatch can confuse users and docs/tools.
Consider updating the example to use the actual annotation:
-/// @StacThemeAnnotation(themeName: 'darkTheme')
+/// @StacCloudTheme(themeName: 'darkTheme')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Example usage: | |
| /// ```dart | |
| /// @StacThemeAnnotation(themeName: 'darkTheme') | |
| /// ThemeData buildDarkTheme() { | |
| /// return ThemeData.dark(); | |
| /// } | |
| /// ``` | |
| /// Example usage: | |
| /// |
🤖 Prompt for AI Agents
In packages/stac_core/lib/annotations/stac_cloud_theme.dart around lines 6 to
12, the documentation example uses the wrong annotation name
(`@StacThemeAnnotation`) which doesn't match the defined annotation
`StacCloudTheme`; update the example to use the actual annotation (e.g.,
`@StacCloudTheme(themeName: 'darkTheme')`) and ensure the annotated function
signature in the example matches the library's expected usage so docs and tools
are consistent.
There was a problem hiding this comment.
Can we name it StacThemeConfig?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any usages of the StacCloudTheme annotation in the codebase
echo "=== Searching for StacCloudTheme usages ==="
rg -n "StacCloudTheme" --type dart -C 2
echo -e "\n=== Searching for @StacCloudTheme annotation usage ==="
rg -n "@StacCloudTheme" --type dart -C 2
echo -e "\n=== Searching for stac_cloud_theme import references ==="
rg -n "stac_cloud_theme" --type dart -C 2
Length of output: 985
@divyanshub024 Sure, renaming to StacThemeConfig makes sense! Since this annotation isn't used anywhere in the codebase yet (it's new in this PR), the rename would be straightforward. Here's what would need to be updated:
-
Class name in
packages/stac_core/lib/annotations/stac_cloud_theme.dart:class StacCloudTheme→class StacThemeConfig- Constructor and doc references
-
File name:
- Rename
stac_cloud_theme.dart→stac_theme_config.dart
- Rename
-
Export statement in
packages/stac_core/lib/annotations/annotations.dart:export 'stac_cloud_theme.dart';→export 'stac_theme_config.dart';
-
Documentation example (which also needs fixing for the wrong name):
@StacThemeAnnotation(themeName: 'darkTheme')→@StacThemeConfig(themeName: 'darkTheme')
Would you like to make these changes, or should I provide the complete code blocks for each update?
| Widget _withResolvedThemes( | ||
| BuildContext context, | ||
| Widget Function(BuildContext, _ResolvedStacThemes) builder, | ||
| ) { | ||
| final resolved = _resolveThemes(); | ||
| if (resolved is Future<_ResolvedStacThemes>) { | ||
| return FutureBuilder<_ResolvedStacThemes>( | ||
| future: resolved, | ||
| builder: (futureContext, snapshot) { | ||
| if (snapshot.connectionState == ConnectionState.waiting) { | ||
| return const _ThemeFutureLoading(); | ||
| } | ||
| if (snapshot.hasError) { | ||
| return const _ThemeFutureLoading(); | ||
| } | ||
| final themes = snapshot.data; | ||
| if (themes == null) { | ||
| return const _ThemeFutureLoading(); | ||
| } | ||
| return builder(futureContext, themes); | ||
| }, | ||
| ); | ||
| } | ||
| return builder(context, resolved); | ||
| } |
There was a problem hiding this comment.
Future is recreated on every widget rebuild.
_resolveThemes() is called inside _withResolvedThemes during build(). When theme or darkTheme is a Future, a new Future object is created on each rebuild. FutureBuilder uses reference equality, so it will restart the async operation and show the loading state again whenever StacApp rebuilds.
Consider caching the resolved future in a stateful widget or using AsyncSnapshot's connectionState more carefully, or lift the resolution to an ancestor widget/state.
-class StacApp extends StatelessWidget {
+class StacApp extends StatefulWidget {
const StacApp({
...
});
+
+ @override
+ State<StacApp> createState() => _StacAppState();
+}
+
+class _StacAppState extends State<StacApp> {
+ late final FutureOr<_ResolvedStacThemes> _resolvedThemes;
+
+ @override
+ void initState() {
+ super.initState();
+ _resolvedThemes = _resolveThemes();
+ }
+
+ // Move build logic here, using _resolvedThemes instead of calling _resolveThemes()Committable suggestion skipped: line range outside the PR's diff.
| if (snapshot.hasError) { | ||
| return const _ThemeFutureLoading(); | ||
| } | ||
| final themes = snapshot.data; | ||
| if (themes == null) { | ||
| return const _ThemeFutureLoading(); |
There was a problem hiding this comment.
Error state is silently swallowed with no feedback.
When theme loading fails, the same loading indicator is shown indefinitely. Consider logging the error and/or falling back to a default theme rather than showing a perpetual loading state.
if (snapshot.hasError) {
+ // Log error for debugging
+ debugPrint('Theme loading failed: ${snapshot.error}');
- return const _ThemeFutureLoading();
+ // Fall back to no theme rather than infinite loading
+ return builder(futureContext, const _ResolvedStacThemes(theme: null, darkTheme: null));
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/stac/lib/src/framework/stac_app.dart around lines 295-300 the
snapshot.hasError branch currently returns the same loading widget and swallows
the error; change this to log the snapshot.error (using debugPrint or the app
logger) and return a sensible fallback (for example a default ThemeData-based
widget or an error/fallback _Theme widget) instead of _ThemeFutureLoading so the
app doesn't show a perpetual spinner; ensure snapshot.error and
snapshot.stackTrace are included in the log and that the fallback provides safe
default colors/fonts so UI remains usable.
| StacArtifactType.theme: {}, | ||
| }; | ||
|
|
||
| /// Fetches an artifact from Stac Cloud with intelligent caching. | ||
| /// |
There was a problem hiding this comment.
Fix _backgroundFetchInProgress value literals (currently map literals, not sets)
Right now the values in _backgroundFetchInProgress use {}, which is a Map literal in Dart, but the field type is Map<StacArtifactType, Set<String>>. This won’t compile:
static final Map<StacArtifactType, Set<String>> _backgroundFetchInProgress = {
StacArtifactType.screen: {},
StacArtifactType.theme: {},
};You need set literals here:
- static final Map<StacArtifactType, Set<String>> _backgroundFetchInProgress = {
- StacArtifactType.screen: {},
- StacArtifactType.theme: {},
- };
+ static final Map<StacArtifactType, Set<String>> _backgroundFetchInProgress = {
+ StacArtifactType.screen: <String>{},
+ StacArtifactType.theme: <String>{},
+ };Given _fetchAndUpdateArtifactInBackground does _backgroundFetchInProgress[artifactType]!, any missing key will also throw at runtime; if you expect more artifact types later, consider building this map from StacArtifactType.values to stay future-proof.
Also applies to: 337-378
🤖 Prompt for AI Agents
packages/stac/lib/src/services/stac_cloud.dart lines 54-58 (and similarly lines
337-378): the map is typed as Map<StacArtifactType, Set<String>> but uses {}
which creates Map literals and not Set<String> — replace the value literals with
explicit set literals like <String>{} (e.g. StacArtifactType.screen: <String>{},
StacArtifactType.theme: <String>{}); to be future-proof you can build the map
from StacArtifactType.values (e.g.
Map.fromEntries(StacArtifactType.values.map((t) => MapEntry(t, <String>{})))) or
ensure every enum key is initialized so
_backgroundFetchInProgress[artifactType]! never returns null (alternatively use
putIfAbsent before accessing).
| /// Example usage: | ||
| /// ```dart | ||
| /// @StacThemeAnnotation(themeName: 'darkTheme') | ||
| /// ThemeData buildDarkTheme() { | ||
| /// return ThemeData.dark(); | ||
| /// } | ||
| /// ``` |
There was a problem hiding this comment.
Can we name it StacThemeConfig?
| /// | ||
| /// Example usage: | ||
| /// ```dart | ||
| /// @StacThemeAnnotation(themeName: 'darkTheme') |
| /// Gets a cached artifact by its name and type. | ||
| /// | ||
| /// Returns `null` if the screen is not cached. | ||
| static Future<StacScreenCache?> getCachedScreen(String screenName) async { |
There was a problem hiding this comment.
I think we should also rename StacScreenCache model to StacCache
|
|
||
| /// Gets a cached screen by its name. | ||
| /// Gets the cache prefix for a given artifact type. | ||
| static String _getCachePrefix(String artifactType) { |
There was a problem hiding this comment.
Can we make artifactType as enum?
There was a problem hiding this comment.
I see there is already an enum for StacArtifactType, can we use that?
| } catch (e) { | ||
| // If there's an error reading from cache, return null | ||
| // and let the app fetch from network | ||
| return null; |
There was a problem hiding this comment.
Can we add logs wherever we return null? I forgot last time :)
| import 'package:stac_core/foundation/theme/stac_theme/stac_theme.dart'; | ||
|
|
||
| /// Provides helpers to load [StacTheme] definitions for [StacApp]. | ||
| class StacAppTheme { |
There was a problem hiding this comment.
Can we move these methods in StacTheme class and use StacTheme.fromCloud
There was a problem hiding this comment.
Also, we are not using function name fromCloud anywhere so we should rename it something else.
There was a problem hiding this comment.
@divyanshub024 its used in actual app code
title: 'AniWatch',
theme: StacAppTheme.fromCloud(themeName: "ani_watch_dark"),
homeBuilder:
(context) =>
Stac(routeName: "onboarding_screen"),
);
There was a problem hiding this comment.
StacAppTheme(name: 'light_theme`)
| static Future<bool> isCacheValid({ | ||
| required String artifactName, | ||
| required String artifactType, | ||
| Duration? maxAge, | ||
| }) async { | ||
| final cachedArtifact = await getCachedArtifact(artifactName, artifactType); | ||
| return isCacheValidSync(cachedArtifact, maxAge); | ||
| } | ||
|
|
There was a problem hiding this comment.
Are we using this function anywhere?
| return null; | ||
| } | ||
|
|
||
| final themePayload = _normalizeThemeJson(rawData['stacJson']); |
There was a problem hiding this comment.
Please change the name of _normalizeThemeJson 😆
Add comprehensive theme support to the Stac framework, enabling themes to be loaded from Stac Cloud similar to screens. This includes: - Add StacArtifactType enum to support both screens and themes - Add StacCloudTheme annotation for marking theme builder methods - Add StacAppTheme helper class for loading themes from cloud, network, or JSON - Extend StacApp to support FutureOr<StacTheme?> for async theme loading - Refactor StacCacheService to be artifact-type agnostic (supports screens and themes) - Refactor StacCloud to support fetching both screens and themes with unified caching Themes can now be loaded asynchronously from Stac Cloud using the same caching strategies as screens (optimistic, cache-first, network-first, etc.), with proper loading states handled in StacApp.
fixes for PR review comments
Potatomonsta
force-pushed
the
mn/themeing
branch
from
86a5f24 to
3538b9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/stac/lib/src/models/stac_artifact_cache.dart (1)
66-69:toString()still references the old class name.The output says
StacScreenCache(...)but the class is nowStacArtifactCache.🔎 Suggested fix
@override String toString() { - return 'StacScreenCache(name: $name, version: $version, cachedAt: $cachedAt)'; + return 'StacArtifactCache(name: $name, version: $version, cachedAt: $cachedAt)'; }
♻️ Duplicate comments (4)
packages/stac/lib/src/services/stac_cloud.dart (2)
47-50: Use explicit set literals for clarity and safety.While Dart may infer
{}asSet<String>from the map's value type, using explicit<String>{}prevents ambiguity and ensures correctness if the context changes. Additionally, consider building fromStacArtifactType.valuesto stay future-proof when new artifact types are added.🔎 Suggested fix
- static final Map<StacArtifactType, Set<String>> _backgroundFetchInProgress = { - StacArtifactType.screen: {}, - StacArtifactType.theme: {}, - }; + static final Map<StacArtifactType, Set<String>> _backgroundFetchInProgress = { + for (final type in StacArtifactType.values) type: <String>{}, + };
332-334: Force-unwrap on map access may throw if a new artifact type is added.
_backgroundFetchInProgress[artifactType]!assumes all enum values exist as keys. If a newStacArtifactTypeis added without updating the map initializer, this will throw at runtime. UseputIfAbsentor the suggested map comprehension to guarantee all enum values are present.🔎 Defensive access pattern
- final inProgressSet = _backgroundFetchInProgress[artifactType]!; + final inProgressSet = _backgroundFetchInProgress.putIfAbsent( + artifactType, + () => <String>{}, + );packages/stac/lib/src/framework/stac_app.dart (2)
283-307: Future is recreated on every widget rebuild.
_resolveThemes()is invoked duringbuild(), creating a newFutureinstance on each rebuild. SinceFutureBuilderuses reference equality, this causes the async operation to restart and the loading indicator to reappear wheneverStacApprebuilds.Convert
StacAppto aStatefulWidgetand cache the resolved future ininitState().🔎 Suggested refactor to StatefulWidget
-class StacApp extends StatelessWidget { +class StacApp extends StatefulWidget { const StacApp({ super.key, // ... existing parameters }); + + @override + State<StacApp> createState() => _StacAppState(); +} + +class _StacAppState extends State<StacApp> { + late final FutureOr<_ResolvedStacThemes> _resolvedThemes; + + @override + void initState() { + super.initState(); + _resolvedThemes = _resolveThemes(); + } + + // Move _resolveThemes and build logic here, referencing widget.theme, widget.darkTheme
295-300: Error state shows perpetual loading indicator.When theme loading fails, the same
_ThemeFutureLoadingwidget is returned, leaving users stuck. Log the error and fall back to null themes so the app remains usable.🔎 Suggested fix
if (snapshot.hasError) { + debugPrint('Theme loading failed: ${snapshot.error}'); - return const _ThemeFutureLoading(); + return builder(futureContext, const _ResolvedStacThemes(theme: null, darkTheme: null)); }
🧹 Nitpick comments (7)
packages/stac_core/lib/annotations/stac_theme_config.dart (1)
13-19: Consider documenting theme name constraints and uniqueness.While the implementation is correct, the documentation could be enhanced to clarify:
- Whether
themeNamevalues must be unique across the application- Any naming conventions or restrictions for
themeName(e.g., non-empty, no special characters)- Whether annotated methods should be static or instance methods
These clarifications would help developers use the annotation correctly without needing to reference external documentation.
packages/stac/lib/src/framework/stac_app.dart (1)
270-280: Resolve theme futures in parallel.Currently
themeFutureanddarkThemeFutureare awaited sequentially. UseFuture.waitto resolve them concurrently, reducing total loading time when both are async.🔎 Parallel resolution
return Future<_ResolvedStacThemes>(() async { - final resolvedTheme = - await (themeFuture ?? Future<StacTheme?>.value(themeValue)); - final resolvedDarkTheme = - await (darkThemeFuture ?? Future<StacTheme?>.value(darkThemeValue)); + final results = await Future.wait([ + themeFuture ?? Future<StacTheme?>.value(themeValue), + darkThemeFuture ?? Future<StacTheme?>.value(darkThemeValue), + ]); + final resolvedTheme = results[0]; + final resolvedDarkTheme = results[1]; return _ResolvedStacThemes( theme: resolvedTheme, darkTheme: resolvedDarkTheme, ); });packages/stac/lib/src/models/stac_artifact_cache.dart (1)
7-9: Update documentation to reflect the artifact-centric rename.The doc comment still references "cached screen" but the class now represents any artifact type (screens, themes, etc.).
🔎 Suggested fix
-/// Model representing a cached screen from Stac Cloud. +/// Model representing a cached artifact from Stac Cloud. /// -/// This model stores the screen data along with metadata for caching purposes. +/// This model stores artifact data (screens, themes, etc.) along with metadata for caching purposes.packages/stac/lib/src/services/stac_cache_service.dart (4)
44-46: Add logging when no cached data is found.As previously requested, logging helps with debugging cache misses.
🔎 Suggested fix
if (cachedData == null) { + Log.d('No cached data found for ${artifactType.name} $artifactName'); return null; }
79-81: Add logging for cache save failures.Silent failures make debugging difficult. Log the error before returning
false.🔎 Suggested fix
} catch (e) { + Log.w('Failed to save artifact $name (${artifactType.name}): $e'); return false; }
93-97: Add logging for cache removal failures.Consistent with
getCachedArtifact, log errors when removal fails.🔎 Suggested fix
} catch (e) { + Log.w('Failed to remove artifact $artifactName (${artifactType.name}): $e'); return false; }
109-113: Add logging for cache clear failures.Log errors when clearing all artifacts of a type fails.
🔎 Suggested fix
} catch (e) { + Log.w('Failed to clear all ${artifactType.name} artifacts: $e'); return false; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/stac/lib/src/framework/framework.dartpackages/stac/lib/src/framework/stac_app.dartpackages/stac/lib/src/framework/stac_app_theme.dartpackages/stac/lib/src/models/models.dartpackages/stac/lib/src/models/stac_artifact_cache.dartpackages/stac/lib/src/models/stac_artifact_cache.g.dartpackages/stac/lib/src/models/stac_artifact_type.dartpackages/stac/lib/src/services/stac_cache_service.dartpackages/stac/lib/src/services/stac_cloud.dartpackages/stac_core/lib/annotations/annotations.dartpackages/stac_core/lib/annotations/stac_theme_config.dart
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/stac_core/lib/annotations/annotations.dart
- packages/stac/lib/src/framework/stac_app_theme.dart
- packages/stac/lib/src/models/stac_artifact_type.dart
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-19T20:20:55.561Z
Learnt from: akhil-ge0rge
Repo: StacDev/stac PR: 399
File: packages/stac/lib/src/parsers/theme/stac_tool_tip_theme_data_parser.dart:9-26
Timestamp: 2025-12-19T20:20:55.561Z
Learning: In Flutter, TooltipThemeData.height is deprecated since v3.30.0-0.1.pre. Replace uses with TooltipThemeData.constraints: BoxConstraints(minHeight: value) to achieve the same minimum height. Apply this pattern to review any Dart/Flutter code that uses TooltipThemeData.height, not just this file.
Applied to files:
packages/stac/lib/src/framework/framework.dartpackages/stac_core/lib/annotations/stac_theme_config.dartpackages/stac/lib/src/services/stac_cache_service.dartpackages/stac/lib/src/models/models.dartpackages/stac/lib/src/models/stac_artifact_cache.g.dartpackages/stac/lib/src/models/stac_artifact_cache.dartpackages/stac/lib/src/framework/stac_app.dartpackages/stac/lib/src/services/stac_cloud.dart
🔇 Additional comments (4)
packages/stac_core/lib/annotations/stac_theme_config.dart (1)
1-12: Previous issues addressed correctly.The annotation name in the documentation example now correctly uses
@StacThemeConfig, and the rename fromStacCloudThemetoStacThemeConfighas been implemented as requested in the previous review.packages/stac/lib/src/framework/framework.dart (1)
5-5: LGTM!The new export for
stac_app_theme.dartcorrectly exposes the theme loading utilities as part of the framework's public API.packages/stac/lib/src/models/stac_artifact_cache.g.dart (1)
1-23: LGTM!Generated serialization code correctly reflects the renamed
StacArtifactCachemodel with proper field mappings.packages/stac/lib/src/services/stac_cloud.dart (1)
23-24: Verify hardcoded base URL is appropriate.The base URL points to a Firebase Cloud Functions endpoint. Confirm this is intentional for production or consider making it configurable (e.g., via
StacService.options).
| /// ```dart | ||
| /// @StacThemeConfig(themeName: 'darkTheme') | ||
| /// ThemeData buildDarkTheme() { | ||
| /// return ThemeData.dark(); | ||
| /// } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for StacTheme type definition and usage patterns to confirm expected return type for theme builder methods
echo "=== Searching for StacTheme type definition ==="
rg -n "class StacTheme" --type dart -C 3
echo -e "\n=== Searching for theme builder method patterns ==="
rg -n "@StacThemeConfig" --type dart -A 5
echo -e "\n=== Searching for StacAppTheme usage to understand theme type expectations ==="
rg -n "StacAppTheme\.(fromCloud|fromNetwork|fromJson)" --type dart -C 3Repository: StacDev/stac
Length of output: 4233
🏁 Script executed:
#!/bin/bash
# Read the full annotation file to understand the expected return type
echo "=== Full content of stac_theme_config.dart ==="
cat -n packages/stac_core/lib/annotations/stac_theme_config.dart
echo -e "\n=== Search for actual @StacThemeConfig decorator usage (excluding comments) ==="
rg -n "@StacThemeConfig" --type dart -B 3 -A 10
echo -e "\n=== Check imports in the annotation file ==="
head -n 20 packages/stac_core/lib/annotations/stac_theme_config.dartRepository: StacDev/stac
Length of output: 2704
🏁 Script executed:
#!/bin/bash
# Search for how themes are built and registered
echo "=== Search for theme builder or theme registration patterns ==="
rg -n "buildTheme|buildDark|buildLight|themeBuilder" --type dart -C 2
echo -e "\n=== Search for code that processes @StacThemeConfig ==="
rg -n "StacThemeConfig|themeRegistry|registerTheme" --type dart -C 2
echo -e "\n=== Look at StacTheme to understand its structure ==="
head -n 100 packages/stac_core/lib/foundation/theme/stac_theme/stac_theme.dartRepository: StacDev/stac
Length of output: 6479
🏁 Script executed:
#!/bin/bash
# Check if there's any conversion between ThemeData and StacTheme
echo "=== Search for ThemeData to StacTheme conversion or relationship ==="
rg -n "toThemeData|fromThemeData|ThemeData" packages/stac_core/lib/foundation/theme/stac_theme/ --type dart -C 1
echo -e "\n=== Check if StacTheme has a method to convert to ThemeData ==="
grep -n "ThemeData" packages/stac_core/lib/foundation/theme/stac_theme/stac_theme.dart | head -20
echo -e "\n=== Look for any documentation or README about themes ==="
find packages/stac_core -name "README*" -o -name "*.md" | xargs grep -l -i "theme" 2>/dev/null || echo "No theme-related docs found"
echo -e "\n=== Check the structure of StacTheme more - look for conversion methods ==="
sed -n '1,150p' packages/stac_core/lib/foundation/theme/stac_theme/stac_theme.dart | tail -50Repository: StacDev/stac
Length of output: 24269
Correct the documented example return type for @StacThemeConfig methods.
Theme builder methods annotated with @StacThemeConfig should return StacTheme, not ThemeData. The framework exclusively uses Stac model types (@JsonSerializable() classes) for theme configuration. Update the example to:
@StacThemeConfig(themeName: 'darkTheme')
StacTheme buildDarkTheme() {
return StacTheme(...);
}Also update the annotation's documentation to clarify the expected return type explicitly.
🤖 Prompt for AI Agents
In packages/stac_core/lib/annotations/stac_theme_config.dart around lines 7 to
11, the documentation example shows theme builder methods returning ThemeData
but the framework expects StacTheme; update the example return type and body to
use StacTheme instead of ThemeData and change the annotation doc text to
explicitly state annotated methods must return StacTheme (a @JsonSerializable()
Stac model) so callers and docs are consistent.
| @@ -1,2 +1,2 @@ | |||
| export 'package:stac/src/models/stac_cache_config.dart'; | |||
| export 'package:stac/src/models/stac_screen_cache.dart'; | |||
| export 'package:stac/src/models/stac_artifact_cache.dart'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'StacScreenCache' --type dartRepository: StacDev/stac
Length of output: 195
🏁 Script executed:
cat -n packages/stac/lib/src/models/stac_artifact_cache.dart | sed -n '60,75p'Repository: StacDev/stac
Length of output: 561
Update the toString() method to reflect the renamed class.
The export change itself is not a breaking change—no external references to StacScreenCache exist in the codebase. However, the toString() method in stac_artifact_cache.dart line 68 still outputs the old class name 'StacScreenCache(...)' instead of 'StacArtifactCache(...)'. Update the string to match the renamed class.
🤖 Prompt for AI Agents
In packages/stac/lib/src/models/stac_artifact_cache.dart around line 68, the
toString() method still returns the old class name 'StacScreenCache(...)';
update that literal to 'StacArtifactCache(...)' so the printed representation
matches the renamed class, and run tests/lint to confirm no other string
references remain.
|
Currently, we need to import all these files. Can we just import the |
| /// return ThemeData.dark(); | ||
| /// } | ||
| /// ``` | ||
| class StacThemeConfig { |
| import 'package:stac_core/foundation/theme/stac_theme/stac_theme.dart'; | ||
|
|
||
| /// Provides helpers to load [StacTheme] definitions for [StacApp]. | ||
| class StacAppTheme { |
There was a problem hiding this comment.
StacAppTheme(name: 'light_theme`)
| static Future<StacTheme?> fromCloud({required String themeName}) async { | ||
| final response = await StacCloud.fetchTheme(themeName: themeName); | ||
| if (response == null) { | ||
| return null; | ||
| } | ||
|
|
||
| final rawData = response.data; | ||
| if (rawData is! Map<String, dynamic>) { |
There was a problem hiding this comment.
On error the it keep showing the loader. We should show the default theme.
|
Add docs and example in movie app |
Description
This PR adds comprehensive theme support to the Stac framework, enabling themes to be loaded from Stac Cloud similar to how screens are currently loaded. The implementation includes async theme loading, intelligent caching, and proper context handling.
Key Changes
New Features:
StacArtifactTypeenum to support both screens and themes as fetchable artifactsStacCloudThemeannotation for marking theme builder methodsStacAppThemehelper class with methods to load themes from:fromCloud)fromNetwork)fromJson)StacAppto supportFutureOr<StacTheme?>for boththemeanddarkThemeparameters, enabling async theme loading_ThemeFutureLoadingwidget when themes are being fetchedRefactoring:
StacCacheServiceto be artifact-type agnostic:getCachedArtifact,saveArtifact, etc.)'screen'and'theme'artifact typesStacCloudto support fetching both screens and themes:_fetchArtifactmethodfetchThememethod as public APIclearThemeCache,clearAllThemeCache)Type of Change
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.