feat: Add theme support to Stac framework with cloud loading #393
Conversation
📝 WalkthroughWalkthroughThis PR introduces multi-source theming via StacAppTheme (DSL/Cloud/Network/JSON), migrates StacApp to a StatefulWidget with async theme resolution and loading state, generalizes caching to artifact-type aware APIs and model (StacCache/StacArtifactType), and adds a StacThemeRef annotation. Changes
Sequence Diagram(s)sequenceDiagram
participant App as StacApp (Widget)
participant State as _StacAppState (State)
participant Resolver as _resolveThemes()
participant AppTheme as StacAppTheme
participant Source as Theme Source (Cloud/Network/DSL/JSON)
participant UI as MaterialApp / UI
App->>State: initState()
activate State
State->>Resolver: _resolveThemes()
activate Resolver
Resolver->>AppTheme: theme.resolve()
activate AppTheme
alt DSL
AppTheme->>Source: return DSL theme
else Cloud
AppTheme->>Source: fromCloud(themeName)
else Network
AppTheme->>Source: fromNetwork(context, request)
else JSON
AppTheme->>Source: fromJson(payload)
end
Source-->>AppTheme: StacTheme? (async)
deactivate AppTheme
Resolver-->>State: _ResolvedStacThemes
deactivate Resolver
State->>UI: setState() → build()
alt Resolved
State->>UI: MaterialApp / MaterialApp.router with parsed themes
else Loading / Error
State->>UI: CircularProgressIndicator or fallback
end
deactivate State
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-12-19T20:20:55.561ZApplied to files:
🔇 Additional comments (7)
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?
| 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).
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| /// 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?
| 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 { |
|
Add docs and example in movie app |
- Introduced a new "Theming" concept in the documentation, detailing how to implement and manage themes in Stac applications. - Updated the main application example to utilize cloud-based themes instead of local theme definitions. - Removed the deprecated app_theme.dart file to streamline theme management. - Enhanced StacApp to support various theme loading methods, including cloud, network, and JSON sources.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/stac_core/lib/annotations/stac_theme_ref.dart (1)
6-12: Documentation example has inconsistencies.The example code shows
@StacThemeConfigbut the actual class isStacThemeRef. Additionally, the return type should beStacTheme, notThemeData, as confirmed by the usage inexamples/movie_app/stac/app_theme.dart.🔎 Proposed fix
/// Example usage: /// ```dart -/// @StacThemeConfig(themeName: 'darkTheme') -/// ThemeData buildDarkTheme() { -/// return ThemeData.dark(); +/// @StacThemeRef(name: 'darkTheme') +/// StacTheme get darkTheme => StacTheme( +/// brightness: StacBrightness.dark, +/// // ... theme configuration +/// ); /// } /// ```packages/stac/lib/src/framework/stac_app.dart (1)
275-303: Future is recreated on every widget rebuild.The
_resolveThemes()method is called inside_withResolvedThemesduringbuild(). SinceStacAppis aStatelessWidget, each rebuild creates a newFutureobject.FutureBuilderuses reference equality, so it will restart the async operation and show the loading state again wheneverStacApprebuilds (e.g., when a parent rebuilds).Consider converting
StacAppto aStatefulWidgetand caching the resolved future ininitState().🔎 Proposed approach
-class StacApp extends StatelessWidget { +class StacApp extends StatefulWidget { const StacApp({ // ... existing constructor params }); + + @override + State<StacApp> createState() => _StacAppState(); +} + +class _StacAppState extends State<StacApp> { + late final Future<_ResolvedStacThemes> _resolvedThemes; + + @override + void initState() { + super.initState(); + _resolvedThemes = _resolveThemes(); + } + + // Use _resolvedThemes in build instead of calling _resolveThemes()
🧹 Nitpick comments (2)
packages/stac/lib/src/framework/stac_app.dart (2)
249-273: Consider resolving themes in parallel.The theme resolution awaits each theme sequentially. Since
themeFutureanddarkThemeFutureare independent, resolving them in parallel withFuture.waitwould improve loading performance.🔎 Proposed optimization
return Future<_ResolvedStacThemes>(() async { - final resolvedTheme = - await (themeFuture ?? Future<StacTheme?>.value(null)); - final resolvedDarkTheme = - await (darkThemeFuture ?? Future<StacTheme?>.value(null)); + final results = await Future.wait([ + themeFuture ?? Future<StacTheme?>.value(null), + darkThemeFuture ?? Future<StacTheme?>.value(null), + ]); return _ResolvedStacThemes( - theme: resolvedTheme, - darkTheme: resolvedDarkTheme, + theme: results[0], + darkTheme: results[1], ); });
313-320: Consider customizable loading indicator.The
_ThemeFutureLoadingwidget shows a plainCircularProgressIndicator. For better UX, consider allowing customization of the loading widget or using a minimal blank screen to avoid visual jarring.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
docs/concepts/theming.mdxdocs/docs.jsonexamples/movie_app/lib/main.dartexamples/movie_app/lib/themes/app_theme.dartexamples/movie_app/stac/app_theme.dartexamples/stac_gallery/lib/main.dartpackages/stac/lib/src/framework/stac_app.dartpackages/stac/lib/src/framework/stac_app_theme.dartpackages/stac_core/lib/annotations/annotations.dartpackages/stac_core/lib/annotations/stac_theme_ref.dart
💤 Files with no reviewable changes (1)
- examples/movie_app/lib/themes/app_theme.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stac/lib/src/framework/stac_app_theme.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_core/lib/annotations/stac_theme_ref.dartexamples/stac_gallery/lib/main.dartexamples/movie_app/stac/app_theme.dartexamples/movie_app/lib/main.dartpackages/stac/lib/src/framework/stac_app.dartpackages/stac_core/lib/annotations/annotations.dart
🪛 LanguageTool
docs/concepts/theming.mdx
[style] ~279-~279: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ... use @StacThemeRef:** - Only when you want to generate JSON and deploy themes to Stac...
(REP_WANT_TO_VB)
🔇 Additional comments (11)
examples/movie_app/lib/main.dart (1)
37-37: LGTM!The migration to
StacAppTheme(name: "movie_app_dark")correctly uses the new cloud-based theming API. The theme name matches the@StacThemeRefannotation defined instac/app_theme.dart.packages/stac_core/lib/annotations/annotations.dart (1)
4-4: LGTM!The export of
stac_theme_ref.dartfollows the existing pattern and correctly exposes the newStacThemeRefannotation through the barrel file.docs/docs.json (1)
43-44: LGTM!The new theming documentation page is correctly added to the navigation structure under the Concepts group.
examples/movie_app/stac/app_theme.dart (3)
1-19: Well-structured theme definition.The theme is cleanly organized with the
@StacThemeRefannotation for cloud deployment. The separation of concerns with helper methods (_buildTheme,_buildTextTheme, etc.) follows best practices and aligns with the documentation examples.
35-113: Comprehensive text theme implementation.The text theme covers all Material Design text styles with consistent patterns. Good use of the
_textStylehelper to reduce duplication.
115-158: Button and utility helpers look good.The button styles and helper functions are well-implemented with appropriate defaults.
examples/stac_gallery/lib/main.dart (2)
5-5: Import simplified to barrel file.This change aligns with the reviewer feedback requesting simplified imports through a single barrel import.
44-45: Correct usage of StacAppTheme.json API.The migration to
StacAppTheme.json(payload: ...)properly wraps the theme payloads from the cubit state using the new theming API.docs/concepts/theming.mdx (2)
1-116: Comprehensive theming documentation.The documentation thoroughly covers all four theme sources with clear examples and workflow explanations. The Cloud Themes section with the CLI deployment workflow is particularly helpful.
397-415: Good best practices and error handling sections.The guidance on when to use each theme source and the error handling behavior documentation are valuable additions.
packages/stac/lib/src/framework/stac_app.dart (1)
286-292: Error handling improvement looks good.The error handling now properly logs a warning and falls back to null themes instead of showing a perpetual loading state. This addresses the previously flagged concern.
docs/concepts/theming.mdx
Outdated
| Helper functions (shared for both approaches): | ||
|
|
||
| ```dart | ||
| StacTheme _buildTheme({ | ||
| required StacBrightness brightness, | ||
| required StacColorScheme colorScheme, | ||
| }) { | ||
| return StacTheme( | ||
| brightness: brightness, | ||
| colorScheme: colorScheme, | ||
| textTheme: _buildTextTheme(), | ||
| filledButtonTheme: _buildFilledButtonTheme(), | ||
| outlinedButtonTheme: _buildOutlinedButtonTheme(), | ||
| dividerTheme: _buildDividerTheme(), | ||
| ); | ||
| } | ||
| ``` | ||
|
|
||
| Helper functions (shared for both approaches): | ||
|
|
There was a problem hiding this comment.
Duplicate section header.
There's a duplicate "Helper functions (shared for both approaches):" label on lines 179 and 197.
🔎 Proposed fix
Remove the duplicate header on line 197:
}-Helper functions (shared for both approaches):
StacTextTheme _buildTextTheme() {🤖 Prompt for AI Agents
In docs/concepts/theming.mdx around lines 179 to 198, there's a duplicated
section header "Helper functions (shared for both approaches):" appearing at
lines 179 and 197; remove the duplicate header at line 197 so only the first
occurrence remains and ensure surrounding markdown/code spacing remains correct
(delete the redundant header line and any stray blank line introduced).
|
@Potatomonsta PR checks are failing, please check |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
examples/movie_app/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
pubspec.yaml
pubspec.yaml
Outdated
|
|
||
| dev_dependencies: | ||
| melos: ^6.3.2 | ||
| melos: ^7.3.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
melos 7.3.0 breaking changes from version 6 dart flutter
💡 Result:
Summary of breaking changes when upgrading from Melos 6.x → 7.3.0:
-
Melos now uses Dart/
pubworkspaces (migration from Melos-managed overrides). This is a breaking change: Melos no longer writes/relies on pubspec_overrides.yaml; workspaces are used instead. [1][2] -
melos.yaml removed — workspace configuration moves into the root pubspec.yaml (root becomes a workspace package). You must migrate settings from melos.yaml into the root pubspec.yaml. [1][2]
-
You must list your packages in the root pubspec.yaml workspace section (a packages list) and add resolution: workspace to each package that should resolve to workspace packages. (i.e., add workspace: { packages: [...] } in root pubspec and resolution: workspace in child pubspecs). [2][0]
-
Some command behavior changed to operate at the workspace root (e.g.,
pub getruns in the workspace root). Review bootstrap/exec behavior after migrating. [1][2] -
Miscellaneous non-breaking features in 7.3.0: groups config for scripts and an option to opt-out of IntelliJ runConfiguration generation (not breaking but new features to be aware of). [1]
Sources:
- Melos changelog (pub.dev) — 7.0.0 → 7.3.0 notes. [1]
- Community migration instructions / discussion (migration details & examples). [2]
References:
[1] Melos changelog (pub.dev).
[2] Melos workspace migration discussion / example (forum).
🏁 Script executed:
# Search for CI configuration files
find . -type f \( -name "*.yml" -o -name "*.yaml" \) | grep -E "(\.github|\.gitlab|\.cirrus|workflow)" | head -20Repository: StacDev/stac
Length of output: 150
🏁 Script executed:
# Search for melos configuration
find . -type f -name "melos.yaml" -o -name "pubspec.yaml" | xargs grep -l "melos" 2>/dev/nullRepository: StacDev/stac
Length of output: 70
🏁 Script executed:
# Check git commit history or PR description to understand CI failures context
git log --oneline -20 2>/dev/null || echo "No git history available"Repository: StacDev/stac
Length of output: 85
🏁 Script executed:
# Check if melos.yaml exists
fd "melos\.yaml" . 2>/dev/null && echo "melos.yaml found" || echo "melos.yaml not found"Repository: StacDev/stac
Length of output: 85
🏁 Script executed:
# Check if pubspec_overrides.yaml exists
fd "pubspec_overrides\.yaml" . 2>/dev/null && echo "pubspec_overrides.yaml found" || echo "pubspec_overrides.yaml not found"Repository: StacDev/stac
Length of output: 355
🏁 Script executed:
# Read root pubspec.yaml to see if workspace configuration is present
head -50 ./pubspec.yamlRepository: StacDev/stac
Length of output: 152
🏁 Script executed:
# Check the CI workflow file to understand what might be failing
cat ./.github/workflows/all_plugins.yamlRepository: StacDev/stac
Length of output: 1879
🏁 Script executed:
# Search for any melos commands in scripts or CI files
rg "melos" . --type yaml --type json -A 2 2>/dev/null | head -50Repository: StacDev/stac
Length of output: 1591
Melos v7.3.0 upgrade is incomplete and will cause CI failures.
This major version upgrade requires several breaking changes that have not been applied:
- Remove deprecated melos.yaml — Melos v7 no longer uses melos.yaml; configuration must migrate to the root pubspec.yaml.
- Remove/migrate pubspec_overrides.yaml — The 6 pubspec_overrides.yaml files (in packages and examples) are incompatible with v7's workspace system. These must be removed and replaced with workspace resolution configuration.
- Add workspace configuration to root pubspec.yaml — The root pubspec.yaml must include a
workspacesection listing all packages withresolution: workspacein each package's pubspec. - Update CI workflow — The
.github/workflows/all_plugins.yamluses a pinned melos-action v3.0.1, which predates v7 and will not work with the upgraded dependency. Update themelos-versionto match ^7.3.0.
These missing migration steps are likely causing the failing CI checks.
🤖 Prompt for AI Agents
In pubspec.yaml around line 7, the Melos v7 upgrade is incomplete and will break
CI: remove deprecated melos.yaml from the repo, delete the six
pubspec_overrides.yaml files in packages/ and examples/ (they are incompatible
with Melos v7), add a workspace section to the root pubspec.yaml that lists
every package path and ensure each package's pubspec.yaml sets resolution:
workspace for workspace dependency resolution, and update
.github/workflows/all_plugins.yaml to set melos-version to ^7.3.0 (or a specific
7.x tag) so the CI melos-action matches the upgraded Melos dependency.
ae50bfd to
3e98d2c
Compare
This reverts commit 3e98d2c.
| final StacTheme? darkTheme; | ||
| } | ||
|
|
||
| class _ThemeFutureLoading extends StatelessWidget { |
There was a problem hiding this comment.
Rename this to _LoadingWidget
| class _ResolvedStacThemes { | ||
| const _ResolvedStacThemes({required this.theme, required this.darkTheme}); | ||
|
|
||
| final StacTheme? theme; | ||
| final StacTheme? darkTheme; | ||
| } | ||
|
|
There was a problem hiding this comment.
Can we make this as a record?
typedef ResolvedStacThemes = ({StacTheme? theme, StacTheme? darkTheme});
| Future<StacTheme?> resolve() async { | ||
| switch (_source) { | ||
| case _ThemeSource.dsl: | ||
| return Future.value(_dslTheme); |
There was a problem hiding this comment.
Do we need Future.Value? We can return _dslTheme directly.
| final _ThemeSource _source; | ||
| final BuildContext? _context; | ||
| final StacNetworkRequest? _request; | ||
| final dynamic _jsonPayload; |
There was a problem hiding this comment.
Can we use Object? for slightly more type safety, since dynamic disables static analysis.
| class StacScreenCache { | ||
| /// Creates a [StacScreenCache] instance. | ||
| const StacScreenCache({ | ||
| class StacArtifactCache { |
There was a problem hiding this comment.
Can we rename this to StacCache?
|
|
||
| @override | ||
| String toString() { | ||
| return 'StacScreenCache(name: $name, version: $version, cachedAt: $cachedAt)'; |
| StacArtifactType.theme: {}, | ||
| }; | ||
|
|
||
| /// Fetches an artifact from Stac Cloud with intelligent caching. | ||
| /// |
| /// | ||
| /// Example usage: | ||
| /// ```dart | ||
| /// @StacThemeConfig(themeName: 'darkTheme') |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/stac/lib/src/models/stac_artifact_cache.g.dart (1)
1-23: Run the code generator to fix formatting issues.The pipeline is reporting formatting issues in this generated file. Since this is a generated file, you should regenerate it using the build_runner to ensure proper formatting.
Run the following to regenerate:
#!/bin/bash # Regenerate the file and format it melos run build_runner melos run formatpackages/stac/lib/src/services/stac_cache_service.dart (1)
1-131: Run formatter to fix formatting issues.The pipeline is reporting formatting issues in this file. Please run the formatter to ensure code style compliance.
Run the following command:
#!/bin/bash # Format the file melos run format
♻️ Duplicate comments (1)
packages/stac/lib/src/services/stac_cloud.dart (1)
47-50: Critical: Fix Set literal syntax in_backgroundFetchInProgressinitialization.The map values use
{}which createsMapliterals, notSet<String>literals. This will cause type errors at runtime when trying to callSetmethods likeadd()andremove().🔎 Fix the literal syntax
static final Map<StacArtifactType, Set<String>> _backgroundFetchInProgress = { - StacArtifactType.screen: {}, - StacArtifactType.theme: {}, + StacArtifactType.screen: <String>{}, + StacArtifactType.theme: <String>{}, };Alternatively, for future-proofing against new artifact types:
- static final Map<StacArtifactType, Set<String>> _backgroundFetchInProgress = { - StacArtifactType.screen: {}, - StacArtifactType.theme: {}, - }; + static final Map<StacArtifactType, Set<String>> _backgroundFetchInProgress = + Map.fromEntries( + StacArtifactType.values.map((type) => MapEntry(type, <String>{})), + );Based on past review comments, this issue was previously flagged but remains unresolved.
🧹 Nitpick comments (1)
packages/stac/lib/src/framework/stac_app.dart (1)
291-291: Consider using a more descriptive typedef name.The typedef
_ResolvedStacThemesuses a record type, which is good. However, since this is now addressing a past comment that suggested making it a record, you might consider if the name clearly conveys that it's a resolved/awaited result.Based on past review comments requesting record usage, this implementation looks good.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/stac/lib/src/framework/stac_app.dartpackages/stac/lib/src/framework/stac_app_theme.dartpackages/stac/lib/src/models/stac_artifact_cache.dartpackages/stac/lib/src/models/stac_artifact_cache.g.dartpackages/stac/lib/src/services/stac_cache_service.dartpackages/stac/lib/src/services/stac_cloud.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/services/stac_cloud.dartpackages/stac/lib/src/framework/stac_app.dartpackages/stac/lib/src/framework/stac_app_theme.dartpackages/stac/lib/src/services/stac_cache_service.dartpackages/stac/lib/src/models/stac_artifact_cache.g.dartpackages/stac/lib/src/models/stac_artifact_cache.dart
🪛 GitHub Actions: all_plugins
packages/stac/lib/src/services/stac_cache_service.dart
[error] 1-1: Formatting issues detected by melos format. The file 'stac_cache_service.dart' is not formatted correctly. See diff output from the formatter.
packages/stac/lib/src/models/stac_artifact_cache.g.dart
[error] 1-1: Formatting issues detected by melos format. The file 'stac_artifact_cache.g.dart' is not formatted correctly. See diff output from the formatter.
🔇 Additional comments (7)
packages/stac/lib/src/models/stac_artifact_cache.dart (1)
11-84: LGTM! Class rename completed successfully.The rename from
StacScreenCachetoStacCacheis complete and consistent throughout the file. All references (constructors, factories, methods, documentation) have been properly updated to reflect the more generic artifact-centric naming.packages/stac/lib/src/framework/stac_app.dart (1)
6-97: LGTM! StatefulWidget conversion and async theme resolution implemented correctly.The migration from
StatelessWidgettoStatefulWidgetis well-executed:
- Theme resolution happens once in
initState(avoiding rebuild issues)- Proper
mountedchecks before callingsetState- Error handling with logging and fallback to null themes
- Loading state properly managed
This implementation addresses all the concerns raised in previous reviews.
Also applies to: 142-178
packages/stac/lib/src/services/stac_cache_service.dart (1)
34-55: LGTM! Artifact-type aware caching implemented correctly.The refactoring to support multiple artifact types (screens, themes) is well-implemented:
- Clear separation via
_getCachePrefixhelper- Type-safe with
StacArtifactTypeenum- Proper error logging with artifact type context
- Consistent null returns on failures
packages/stac/lib/src/framework/stac_app_theme.dart (2)
26-89: LGTM! Multi-source theme loading implemented well.The
StacAppThemewrapper provides a clean API for loading themes from multiple sources:
- Clear constructor separation (dsl, cloud, network, json)
- Type-safe routing with private
_ThemeSourceenum- Consistent async resolution pattern
- Proper null handling for optional sources
The implementation is intuitive and follows good Flutter patterns.
141-163: LGTM! JSON parsing error handling implemented correctly.The
_themeJsonDynamicToMaphelper properly handles all the edge cases:
- Nested
stacJsonunwrapping- Direct Map passthrough
- String parsing with try-catch (addresses previous feedback)
- Error logging on parse failures
This addresses the concern raised in past reviews about
FormatExceptionhandling.packages/stac/lib/src/services/stac_cloud.dart (2)
26-44: LGTM! Artifact-type aware cloud fetching implemented correctly.The refactoring to support multiple artifact types is well-structured:
- Clear URL and query parameter routing via helper methods
- Consistent artifact-type parameter threading through all methods
- Proper cache integration with type awareness
- Good separation of concerns
The generalized
_fetchArtifactapproach with type-specific wrappers (fetchScreen,fetchTheme) provides a clean public API.Also applies to: 61-140
371-411: LGTM! Theme-specific cache management methods added.The new
fetchThemeand theme cache clearing methods (clearThemeCache,clearAllThemeCache) mirror the existing screen methods, providing a consistent API surface for theme artifact management.
- Changed StacApp from StatelessWidget to StatefulWidget to manage theme resolution state. - Introduced a loading state while themes are being resolved. - Refactored theme resolution logic to improve clarity and efficiency. - Updated StacArtifactCache to StacCache for consistency across caching models. - Adjusted related methods and types to reflect the new StacCache naming convention.
Potatomonsta
force-pushed
the
mn/themeing
branch
from
1b077a1 to
87fc897
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/stac/lib/src/services/stac_cloud.dart (1)
47-50: Critical: Fix Map initialization - values must be Set literals, not Map literals.The field is typed as
Map<StacArtifactType, Set<String>>but the values use{}which createsMapliterals in Dart, notSet<String>. This will cause type errors at runtime whenaddorremoveis called (e.g., line 334).🔎 Proposed fix
static final Map<StacArtifactType, Set<String>> _backgroundFetchInProgress = { - StacArtifactType.screen: {}, - StacArtifactType.theme: {}, + StacArtifactType.screen: <String>{}, + StacArtifactType.theme: <String>{}, };For future-proofing when more artifact types are added:
- static final Map<StacArtifactType, Set<String>> _backgroundFetchInProgress = { - StacArtifactType.screen: {}, - StacArtifactType.theme: {}, - }; + static final Map<StacArtifactType, Set<String>> _backgroundFetchInProgress = + Map.fromEntries( + StacArtifactType.values.map((type) => MapEntry(type, <String>{})), + );
🧹 Nitpick comments (5)
packages/stac/lib/src/framework/stac_app.dart (2)
142-166: Consider implementingdidUpdateWidgetto handle theme changes.If the parent widget rebuilds with different
themeordarkThemevalues, the current implementation won't re-resolve the themes. The resolved themes will remain stale until the widget is completely recreated.🔎 Proposed implementation
@override void initState() { super.initState(); _themesFuture = _resolveThemes(); _themesFuture! .then((themes) { if (mounted) { setState(() { _resolvedThemes = themes; }); } }) .catchError((error) { if (mounted) { Log.w('Failed to resolve theme: $error'); setState(() { _resolvedThemes = (theme: null, darkTheme: null); }); } }); } + + @override + void didUpdateWidget(StacApp oldWidget) { + super.didUpdateWidget(oldWidget); + if (widget.theme != oldWidget.theme || widget.darkTheme != oldWidget.darkTheme) { + _themesFuture = _resolveThemes(); + _themesFuture! + .then((themes) { + if (mounted) { + setState(() { + _resolvedThemes = themes; + }); + } + }) + .catchError((error) { + if (mounted) { + Log.w('Failed to resolve theme: $error'); + setState(() { + _resolvedThemes = (theme: null, darkTheme: null); + }); + } + }); + } + }
267-289: Consider parallelizing theme resolution for better performance.The current implementation awaits
themeFutureanddarkThemeFuturesequentially. If both themes are loaded from network or cloud, this doubles the loading time unnecessarily.🔎 Proposed parallel resolution
Future<_ResolvedStacThemes> _resolveThemes() { final themeInput = widget.theme; final darkThemeInput = widget.darkTheme; // Both themes are optional, so we need to handle null cases final Future<StacTheme?>? themeFuture = themeInput?.resolve(); final Future<StacTheme?>? darkThemeFuture = darkThemeInput?.resolve(); // If both are null, return immediately with null themes if (themeFuture == null && darkThemeFuture == null) { return Future.value((theme: null, darkTheme: null)); } - return Future<_ResolvedStacThemes>(() async { - final resolvedTheme = - await (themeFuture ?? Future<StacTheme?>.value(null)); - final resolvedDarkTheme = - await (darkThemeFuture ?? Future<StacTheme?>.value(null)); - - return (theme: resolvedTheme, darkTheme: resolvedDarkTheme); - }); + // Resolve both themes in parallel + return Future.wait([ + themeFuture ?? Future<StacTheme?>.value(null), + darkThemeFuture ?? Future<StacTheme?>.value(null), + ]).then((results) => (theme: results[0], darkTheme: results[1])); }packages/stac/lib/src/framework/stac_app_theme.dart (2)
78-89: Consider wrapping DSL theme inFuture.valuefor consistency.The
dslcase returns_dslThemedirectly while other cases returnFutures. While Dart's type system allows this, explicitly wrapping it improves clarity and makes the async contract more obvious.Future<StacTheme?> resolve() async { switch (_source) { case _ThemeSource.dsl: - return _dslTheme; + return Future.value(_dslTheme); case _ThemeSource.cloud: return fromCloud(themeName: name!); case _ThemeSource.network: return fromNetwork(context: _context!, request: _request!); case _ThemeSource.json: return fromJson(_jsonPayload); } }
141-163: RecursivestacJsonunwrapping could be clearer.Lines 145-147 recursively call
_themeJsonDynamicToMapwhen astacJsonkey is found. While this handles one common nesting pattern, the recursion depth is unbounded and could theoretically loop infinitely on circular structures (though unlikely in practice with JSON).Consider either:
- Adding a depth parameter to limit recursion
- Unwrapping only once explicitly
- Adding a comment explaining the expected nesting pattern
Example with explicit single unwrap
static Map<String, dynamic>? _themeJsonDynamicToMap(dynamic payload) { if (payload == null) { return null; } // Unwrap nested 'stacJson' key once if present if (payload is Map<String, dynamic>) { final nested = payload['stacJson']; if (nested != null) { payload = nested; } } if (payload is Map<String, dynamic>) { return payload; } if (payload is String) { try { final decoded = jsonDecode(payload); if (decoded is Map<String, dynamic>) { return decoded; } } catch (e) { Log.w('Unexpected error parsing theme JSON: $e'); return null; } } return null; }packages/stac/lib/src/services/stac_cache_service.dart (1)
60-113: Consider adding error logging for consistency.The
saveArtifact,removeArtifact, andclearAllArtifactsmethods catch exceptions but returnfalsewithout logging, unlikegetCachedArtifactwhich logs warnings (lines 50-52). Adding logs would help diagnose issues.static Future<bool> saveArtifact({ required String name, required String stacJson, required int version, required StacArtifactType artifactType, }) async { try { final prefs = await _sharedPrefs; final cachePrefix = _getCachePrefix(artifactType); final cacheKey = '$cachePrefix$name'; final artifactCache = StacCache( name: name, stacJson: stacJson, version: version, cachedAt: DateTime.now(), ); return prefs.setString(cacheKey, artifactCache.toJsonString()); } catch (e) { + Log.w('Failed to save artifact $name (${artifactType.name}): $e'); return false; } } static Future<bool> removeArtifact( String artifactName, StacArtifactType artifactType, ) async { try { final prefs = await _sharedPrefs; final cachePrefix = _getCachePrefix(artifactType); final cacheKey = '$cachePrefix$artifactName'; return prefs.remove(cacheKey); } catch (e) { + Log.w('Failed to remove artifact $artifactName (${artifactType.name}): $e'); return false; } } static Future<bool> clearAllArtifacts(StacArtifactType artifactType) async { try { final prefs = await _sharedPrefs; final keys = prefs.getKeys(); final cachePrefix = _getCachePrefix(artifactType); final cacheKeys = keys.where((key) => key.startsWith(cachePrefix)); await Future.wait(cacheKeys.map(prefs.remove)); return true; } catch (e) { + Log.w('Failed to clear all artifacts (${artifactType.name}): $e'); return false; } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/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_cache.dartpackages/stac/lib/src/models/stac_cache.g.dartpackages/stac/lib/src/services/stac_cache_service.dartpackages/stac/lib/src/services/stac_cloud.dart
✅ Files skipped from review due to trivial changes (1)
- packages/stac/lib/src/models/stac_cache.g.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stac/lib/src/models/models.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/stac_app.dartpackages/stac/lib/src/models/stac_cache.dartpackages/stac/lib/src/framework/stac_app_theme.dartpackages/stac/lib/src/services/stac_cloud.dartpackages/stac/lib/src/services/stac_cache_service.dart
| const StacAppTheme.network({ | ||
| required BuildContext context, | ||
| required StacNetworkRequest request, | ||
| }) : _source = _ThemeSource.network, | ||
| name = null, | ||
| _context = context, | ||
| _request = request, | ||
| _jsonPayload = null, | ||
| _dslTheme = null; |
There was a problem hiding this comment.
Storing BuildContext in a field may cause issues.
The _context field stores a BuildContext that could become invalid if the creating widget is unmounted before resolve() is called. If the context is used after its widget is disposed, it may reference a deactivated element.
Consider passing the BuildContext at resolution time rather than construction time, or document that resolution must happen synchronously.
Alternative API approach
Option 1: Accept context in resolve() instead:
/// Resolves the theme based on the configured source.
Future<StacTheme?> resolve([BuildContext? context]) async {
switch (_source) {
case _ThemeSource.network:
if (context == null) {
throw ArgumentError('Context required for network theme resolution');
}
return fromNetwork(context: context, request: _request!);
// ... other cases
}
}Option 2: Document the requirement clearly:
/// Creates a [StacAppTheme] wrapper for fetching a theme from network.
///
/// ⚠️ IMPORTANT: [resolve] must be called synchronously (e.g., in initState)
/// while the [context] is still valid. Do not store this wrapper for later use.
const StacAppTheme.network({
required BuildContext context,
required StacNetworkRequest request,
}) : // ...- Updated the description to clarify the use of DSL and Cloud sources for theme management. - Removed redundant sections on DSL themes and streamlined the explanation of theme loading methods. - Adjusted the numbering of theme loading methods for consistency. - Enhanced best practices and error handling sections for better guidance.
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
New Features
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.