-
-
Notifications
You must be signed in to change notification settings - Fork 82
Refactor movie app to new Stac structure #381
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Updated Dart SDK version in pubspec.yaml to ^3.8.0. - Upgraded dependencies including stac and stac_core. - Removed unused JSON screen files and light theme configuration. - Introduced new constants for API endpoints and asset paths. - Added default Stac options for initialization. - Implemented onboarding, home, and detail screens using Stac widgets. - Refactored movie carousel widget to use Stac's JSON serialization. - Enhanced theme management with a dedicated dark theme implementation.
- Removed the deprecated app_theme.dart file and updated references to StacThemeData in various screens. - Adjusted theme usage in detail, home, and onboarding screens to utilize StacThemeData for consistency. - Cleaned up pubspec.lock by removing unused freezed_annotation dependency.
WalkthroughReplaces three JSON screen assets with Dart/STAC screen implementations, centralizes API/assets/strings constants and theme, adds Stac initialization options, introduces a JSON-serializable StacMovieCarousel model with generated serializers and parser update, and updates pubspec/dependencies and project defaults. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as main.dart
participant Options as defaultStacOptions
participant Theme as darkTheme
participant Stac as STAC Runtime
participant Screen as STAC Screen (onboarding/home/detail)
participant API as AppApi
App->>Options: request defaultStacOptions
Options-->>App: StacOptions
App->>Theme: request darkTheme
Theme-->>App: StacTheme
App->>Stac: Stac.initialize(options:, dio:, parsers:)
Stac-->>Stac: configure globals (options, theme, parsers)
App->>Stac: navigate to route (onboarding_screen)
Stac->>Screen: instantiate Dart screen builder
Screen->>API: build URLs / request endpoints via AppApi helpers
API-->>Screen: return URLs / endpoint strings
Screen->>Stac: render widget tree (DynamicView, lists, taps → setValue/navigate)
Stac-->>App: perform navigation / render updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/movie_app/pubspec.yaml (1)
66-67: Remove unused JSON asset constants fromexamples/movie_app/lib/constants/app_assets.dart.The migration to Dart-based STAC implementations is complete—no code attempts to load the removed JSON assets, and
Stac(routeName: 'onboarding_screen')pattern confirms the transition. However, the constantsonboardingScreenJson,homeScreenJson, anddetailScreenJson(lines 14-21 in app_assets.dart) remain defined but completely unused, pointing to now-removed assets. Remove these unused constants and the associated// JSON Screenscomment to eliminate dead code.
🧹 Nitpick comments (3)
examples/movie_app/lib/constants/app_api.dart (1)
57-63: Normalize image paths before concatenationTMDb poster/profile paths usually arrive with a leading slash, so the current interpolation yields
...//path, which is easy to miss when debugging CDN issues. Normalize the incoming path once and reuse it so the generated URL is consistent regardless of input formatting.+ static String _normalizeImagePath(String path) => + path.startsWith('/') ? path.substring(1) : path; + /// Get full poster image URL from poster path - static String getPosterImageUrl(String posterPath) => - '$imageBaseUrl/$posterPath'; + static String getPosterImageUrl(String posterPath) => + '$imageBaseUrl/${_normalizeImagePath(posterPath)}'; /// Get full profile image URL from profile path - static String getProfileImageUrl(String profilePath) => - '$imageBaseUrl/$profilePath'; + static String getProfileImageUrl(String profilePath) => + '$imageBaseUrl/${_normalizeImagePath(profilePath)}';examples/movie_app/stac/home_screen.dart (1)
23-183: Consider extracting a reusable section builder.Each section differs only by the title and endpoint; factoring this into a helper (e.g.,
_buildMoviesSection({required String title, required String url})) would cut the repetition and make future tweaks safer.examples/movie_app/stac/detail_screen.dart (1)
1-292: Rundart format.CI failed the formatting check on this file. Please run
melos run format(ordart format) so the pipeline passes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
examples/movie_app/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
examples/movie_app/assets/jsons/screens/detail_screen.json(0 hunks)examples/movie_app/assets/jsons/screens/home_screen.json(0 hunks)examples/movie_app/assets/jsons/screens/onboarding_screen.json(0 hunks)examples/movie_app/lib/constants/app_api.dart(1 hunks)examples/movie_app/lib/constants/app_assets.dart(1 hunks)examples/movie_app/lib/constants/app_constants.dart(1 hunks)examples/movie_app/lib/constants/app_strings.dart(1 hunks)examples/movie_app/lib/default_stac_options.dart(1 hunks)examples/movie_app/lib/main.dart(3 hunks)examples/movie_app/lib/themes/app_theme.dart(1 hunks)examples/movie_app/lib/widgets/movie_carousel/movie_carousel.dart(1 hunks)examples/movie_app/lib/widgets/movie_carousel/movie_carousel.g.dart(1 hunks)examples/movie_app/lib/widgets/movie_carousel/movie_carousel_parser.dart(1 hunks)examples/movie_app/pubspec.yaml(3 hunks)examples/movie_app/stac/detail_screen.dart(1 hunks)examples/movie_app/stac/home_screen.dart(1 hunks)examples/movie_app/stac/onboarding_screen.dart(1 hunks)
💤 Files with no reviewable changes (3)
- examples/movie_app/assets/jsons/screens/onboarding_screen.json
- examples/movie_app/assets/jsons/screens/detail_screen.json
- examples/movie_app/assets/jsons/screens/home_screen.json
🧰 Additional context used
🪛 GitHub Actions: all_plugins
examples/movie_app/lib/default_stac_options.dart
[error] 1-1: Dart format check failed. File is not formatted according to 'dart format'. Run 'melos run format' to fix.
examples/movie_app/stac/detail_screen.dart
[error] 1-1: Dart format check failed. File is not formatted according to 'dart format'. Run 'melos run format' to fix.
🔇 Additional comments (5)
examples/movie_app/pubspec.yaml (1)
37-38: Verify stac/stac_core version compatibility and confirm migration requirements.The upgrade from
stac: ^1.0.0-dev.6tostac: ^1.0.1with newstac_core: ^1.0.0requires verification:
- Version compatibility: Confirmed—
stac: ^1.0.1explicitly declaresstac_core: ^1.0.0as a dependency (same major version).- Migration impact: The significant transitive change is
stac_coreintroduction (absent in dev.6, introduced in dev.7) andstac_frameworkupgrade from^0.3.0→^0.4.0between dev.6 and dev.7. Detailed breaking changes or API modifications could not be verified from public sources.- JSON serialization:
json_annotation,build_runner, andjson_serializablewere already present in dev.6 (not newly added).Manually review the changelog at https://github.com/StacDev/stac/releases for breaking changes between 1.0.0-dev.6 and 1.0.1, and verify that the application code is compatible with any stac_framework or stac_core API changes.
examples/movie_app/lib/themes/app_theme.dart (1)
1-119: Theme configuration looks solidNice job centralizing the dark theme with a complete color scheme, typography, and button styles—this keeps the Stac screens consistent out of the box.
examples/movie_app/lib/widgets/movie_carousel/movie_carousel_parser.dart (1)
6-18: Parser migration looks solid.The parser now works directly with
StacMovieCarousel, and the deserialization/parsing flow still matches the expected request contract. Nicely done.examples/movie_app/lib/main.dart (1)
3-41: Initialization update looks good.Supplying
defaultStacOptions, the parser list, and sharing the centralizeddarkThemekeeps app startup consistent with the new structure. 👍examples/movie_app/lib/constants/app_assets.dart (1)
1-21: Nice asset centralization.Keeping all image and screen JSON paths in one place reduces drift and makes future updates painless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
♻️ Duplicate comments (2)
examples/movie_app/stac/onboarding_screen.dart (2)
16-21: Remove conflicting width constraint.The
StacContaineron line 21 specifieswidth: 1000while the parentStacPositionedalready definesleft: 0andright: 0. This creates conflicting constraints that will trigger a runtime assertion, as previously noted.
58-67: Remove excessive button width constraint.The
StacSizedBoxon line 60 specifieswidth: 1000, which is both excessive and conflicts with the parent's left/right constraints. This was already flagged in the previous review.The navigation action on line 65 correctly routes to the home screen.
🧹 Nitpick comments (1)
examples/movie_app/stac/detail_screen.dart (1)
86-86: Format vote_average for better display.The raw vote_average might display excessive decimal places (e.g., "8.543210"). Consider formatting to one decimal place for cleaner UI.
If STAC supports expression formatting, you could use:
data: '{{vote_average | number_format(1)}}',Otherwise, this formatting should be handled server-side or via a custom STAC formatter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/movie_app/stac/detail_screen.dart(1 hunks)examples/movie_app/stac/onboarding_screen.dart(1 hunks)
🔇 Additional comments (10)
examples/movie_app/stac/onboarding_screen.dart (3)
1-2: LGTM!The imports are appropriate for this onboarding screen implementation.
4-5: LGTM!The screen annotation and function definition correctly follow the STAC framework pattern.
23-56: LGTM!The gradient overlay and text styling are well-structured. The gradient correctly uses transparent alpha (
#00050608) for a smooth fade effect, and the text properly leverages theme styling with accent spans.examples/movie_app/stac/detail_screen.dart (7)
1-5: LGTM!The imports and function signature are properly structured. The @StacScreen annotation correctly registers this as a named screen.
6-20: LGTM!The scaffold and app bar configuration is correct. The transparent app bar with
extendBodyBehindAppBar: trueproperly overlays the content, and the back button navigation is properly implemented.
117-117: Verify intentionally disabled buttons.Both the "Watch Trailer" and "Add to Watchlist" buttons have
onPressed: null, making them non-interactive. Confirm this is intentional for this refactor phase.Also applies to: 130-130
234-234: Verify handling of missing profile_path.Cast members without profile images will have null profile_path values, resulting in malformed image URLs. Ensure proper fallback handling similar to the poster_path concern.
267-273: LGTM! Navigation pattern is well-structured.The onTap action correctly updates the movie_id context variable and navigates to the detail screen, enabling users to explore similar movies.
278-278: Verify handling of missing poster_path for similar movies.Similar movies without posters will have null data.poster_path values. Ensure this is handled consistently with the other image loading scenarios.
32-32: Add error handling and fallback for missing poster_path.The concern is valid. Evidence shows
widget.movies[index]['poster_path']is accessed directly without null checks atexamples/movie_app/lib/widgets/movie_carousel/movie_carousel_parser.dart:71, which produces malformed URLs if the field is missing. Additionally, the STAC framework's handling of missing{{poster_path}}template variables indetail_screen.dart:32is unclear.Recommended mitigations:
- Add null coalescing:
widget.movies[index]['poster_path'] ?? 'fallback_path'- Define a placeholder image constant in AppAssets
- Add
errorBuilderorloadingBuildertoImage.network()for graceful degradation- Verify The Movie DB API response contract guarantees
poster_pathpresence
Description
Refactor movie app to new Stac structure
Type of Change
Summary by CodeRabbit
New Features
Refactor
Chores