Conversation
WalkthroughAdds Ukrainian localization and many locale keys; introduces multiple Freezed network models (core, first/second stage, payload) with generated serializers; extends launch/rocket/links models and resource mappings; refactors Launch screen into Bloc + stateful content and many UI widgets; adds url_launcher, tests, and CI concurrency plus a new integration test. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as LaunchScreenBlocContent
participant Bloc as LaunchBloc
participant Repo as LaunchesRepository
participant API as Network API
participant Content as LaunchScreenContent
User->>UI: open(flightNumber)
UI->>Bloc: dispatch LaunchLoadEvent(flightNumber)
Bloc->>Repo: loadLaunchFull(flightNumber)
Repo->>API: GET /launches/{id}
API-->>Repo: NetworkLaunchFullModel JSON
Repo->>Bloc: mapped LaunchFullResource
Bloc-->>UI: LaunchSuccessState(resource)
UI->>Content: build(resource)
Content->>Content: render status, rocket, payload, timeline, links, site, ships
alt API error
API-->>Repo: error
Repo->>Bloc: LaunchFailure
Bloc-->>UI: LaunchErrorState
UI->>User: show ErrorContent (retry -> LaunchLoadEvent)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
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 |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #38 +/- ##
==========================================
+ Coverage 72.03% 80.83% +8.80%
==========================================
Files 69 96 +27
Lines 1137 1931 +794
==========================================
+ Hits 819 1561 +742
- Misses 318 370 +52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
lib/data/network/model/launch/full/network_launch_full_model.dart (2)
49-50: Don’t mark a nullable field as requiredrequired List? cores is misleading; remove required or make the field non-nullable with a default.
- required List<NetworkCore>? cores, + List<NetworkCore>? cores,Alternative (if you prefer non-nullable):
- required List<NetworkCore>? cores, + @Default(<NetworkCore>[]) List<NetworkCore> cores,
59-63: Fix type: block should be int?, not String?As-is, JSON deserialization will fail when the API returns an integer for block.
- @JsonKey(name: 'block') String? block, + @JsonKey(name: 'block') int? block,test/data/network/model/network_launch_full_model_test.dart (1)
141-145: Fix toJson expectations: don't cast JSON back to model typestoJson should yield JSON-serializable maps. Casting json['rocket'] to NetworkRocketModel will fail. Assert on the nested map instead.
Apply:
- expect((json['rocket'] as NetworkRocketModel).name, 'Falcon 1'); + expect(json['rocket'], isA<Map<String, dynamic>>()); + expect((json['rocket'] as Map<String, dynamic>)['rocket_name'], 'Falcon 1');lib/models/launch/launch_full_resource.dart (1)
9-25: Enforce immutability for ships (mutable List breaks @immutable intent).ships is mutable and externally modifiable, which can cause subtle state/equality bugs. Wrap it as unmodifiable at construction and in copyWith. This removes the const constructor (trade-off is worth the safety).
Apply:
-class LaunchFullResource extends Equatable { - const LaunchFullResource({ +class LaunchFullResource extends Equatable { + LaunchFullResource({ required this.id, required this.flightNumber, this.missionName, this.launchDate, this.staticFireDate, this.isTentative, this.tentativeMaxPrecision, this.details, this.tbd, this.launchWindow, this.rocket, this.launchSuccess, this.links, this.launchSite, - required this.ships, - }); + required List<String> ships, + }) : ships = List.unmodifiable(ships); @@ - final List<String> ships; + final List<String> ships; @@ - ships: ships ?? this.ships, + ships: ships != null ? List.unmodifiable(ships) : this.ships,If you rely on const constructions elsewhere, I can suggest a factory that preserves const for already-unmodifiable inputs. Want that?
Also applies to: 41-41, 74-76
lib/data/network/model/launch/full/network_launch_full_model.freezed.dart (1)
1569-1572: Critical:NetworkCore.blocktyped asString?— likely runtime cast error.SpaceX v3 returns numeric
block.json_serializablewill generatejson['block'] as String?, failing when it’s anint. Change the source model (network_launch_full_model.dart) so nestedNetworkCore.blockisint?, then re-run codegen.Apply equivalent changes in the source (illustrative diff against the generated file; do not hand-edit generated code):
- @JsonKey(name: 'block') - String? get block; + @JsonKey(name: 'block') + int? get block; ... - @JsonKey(name: 'block') String? block, + @JsonKey(name: 'block') int? block,Also applies to: 1647-1656, 1969-1972, 2108-2112
🧹 Nitpick comments (116)
pubspec.yaml (1)
49-49: url_launcher schemes are http(s) only; add LinkLauncher abstraction
grep search confirmed noUri.parsecalls with tel:, sms:, mailto:, geo:, maps:, market:, itms: or itms-apps schemes—only http/https are used. Recommend introducing a small LinkLauncher wrapper aroundlaunchUrlto enable mocking in tests.integration_test/app_test.dart (1)
17-17: Assertion relax is fine; prefer locale-agnostic finderUsing findsAtLeast(1) is OK, but relying on English text can be brittle with multi-locale runs. Prefer keys/semantics or pulling the localized string from AppLocalizations in the test.
integration_test/launch_navigation_test.dart (2)
10-10: Remove no-op setUpsetUp(() {}) is redundant here.
- setUp(() {}); +
18-21: Avoid data-dependent hard-coded KeyKey('FalconSat1') ties the test to specific seed data. Prefer a stable key (e.g., ValueKey('launch_$id')) or find by semantics/label built from domain data.
Would you like a patch adding stable ValueKey wiring to LaunchItem (e.g., ValueKey('launch_${model.flightNumber}')) and updating the test accordingly?
lib/l10n/intl_uk.arb (2)
145-148: Polish obvious UA wording issuesA few strings read awkwardly or contain grammatical slips in Ukrainian.
-"borderRadiusButtonTitle": "Радіус кордону", +"borderRadiusButtonTitle": "Радіус заокруглення", -"borderSideButtonTitle": "Кордона сторона", +"borderSideButtonTitle": "Лінія межі", -"gridFins": "Сітчасті кермові", +"gridFins": "Решітчасті керма", -"landingLegs": "Ноги приземлення", +"landingLegs": "Посадкові опори", -"firstStage": "🚀 Перша ступінь", +"firstStage": "🚀 Перший ступінь", -"recoveryShips": "Судна порятунку", +"recoveryShips": "Рятувальні судна",If you prefer, I can review the full UA set for consistency (cases, gender, and aerospace terminology).
Also applies to: 239-244, 219-223, 255-258
97-104: Consider ICU plurals for daysUkrainian has complex plurals; ICU messages will render correctly for 1/2–4/5+.
-"daysSinceTodayTitle": "{days} днів тому", +"daysSinceTodayTitle": "{days, plural, one {# день тому} few {# дні тому} many {# днів тому} other {# днів тому}}", -"daysFromTodayTitle": "Через {days} днів", +"daysFromTodayTitle": "{days, plural, one {Через # день} few {Через # дні} many {Через # днів} other {Через # днів}}",Note: Adjust other locales accordingly to keep key parity.
lib/features/launch/widget/feature_chip_widget.dart (2)
17-25: Confirm Color.withValues() SDK support or use withOpacity() for broader compatibilitywithValues(alpha: ...) requires a fairly recent Flutter SDK. If your min SDK might be older, switch to withOpacity().
Apply:
- ? theme.colorScheme.primary.withValues(alpha: 0.1) + ? theme.colorScheme.primary.withOpacity(0.1) ... - ? theme.colorScheme.primary.withValues(alpha: 0.3) + ? theme.colorScheme.primary.withOpacity(0.3) - : theme.colorScheme.outline.withValues(alpha: 0.2), + : theme.colorScheme.outline.withOpacity(0.2),Also applies to: 33-44
27-49: Add Semantics for accessibilityExpose selection state to screen readers.
- child: Row( + child: Semantics( + label: label, + selected: active, + child: Row( mainAxisSize: MainAxisSize.min, children: [ Icon( active ? Icons.check_circle : Icons.cancel, size: 14, color: active ? theme.colorScheme.primary : theme.colorScheme.onSurfaceVariant, ), const SizedBox(width: 4), Text( label, style: TextStyle( fontSize: 12, color: active ? theme.colorScheme.primary : theme.colorScheme.onSurfaceVariant, fontWeight: FontWeight.w500, ), ), ], - ), + ), + ),lib/features/launch/widget/animated_link_button_widget.dart (2)
28-41: Optional: AnimatedContainer is static hereNo properties change over time; a plain Container suffices unless you plan to animate.
29-33: SDK check for Color.withValues()Same note as other file; consider withOpacity() for backward compatibility.
lib/l10n/intl_de.arb (1)
196-197: Add placeholders metadata for flightNumberImproves generated API clarity and tooling.
- "flightNumber": "Flug #{number}" + "flightNumber": "Flug #{number}", + "@flightNumber": { + "placeholders": { "number": {} } + }lib/l10n/intl_pt.arb (2)
196-197: Add placeholders metadata for flightNumberMatch other locales and aid codegen.
- "flightNumber": "Voo #{number}" + "flightNumber": "Voo #{number}", + "@flightNumber": { + "placeholders": { "number": {} } + }
177-177: Clarify “not available” wording“N/D” can be unclear; consider a full phrase.
- "notAvailable": "N/D", + "notAvailable": "Não disponível",lib/l10n/intl_en.arb (1)
154-158: Add a brief description for @transparentButtonTitle.Metadata helps translators; even a one-liner keeps consistency with nearby entries.
- "@transparentButtonTitle": {}, + "@transparentButtonTitle": { + "description": "Label for transparent button style" + },lib/l10n/app_localizations_uk.dart (1)
12-12: Polish several Ukrainian UI strings (more idiomatic).Improves clarity and correctness; safe, user-facing text quality.
- String get appTitle => 'шаблон_flutter_bloc_app'; + String get appTitle => 'Шаблон Flutter BLoC'; - String get borderRadiusButtonTitle => 'Радіус кордону'; + String get borderRadiusButtonTitle => 'Радіус кутів'; - String get borderSideButtonTitle => 'Кордона сторона'; + String get borderSideButtonTitle => 'Товщина обвідки'; - String get gridFins => 'Сітчасті кермові'; + String get gridFins => 'Решітчасті керма'; - String get landingLegs => 'Ноги приземлення'; + String get landingLegs => 'Посадкові опори'; - String get recoveryShips => 'Судна порятунку'; + String get recoveryShips => 'Рятувальні судна'; - String get siteIdLabel => 'ID сайту:'; + String get siteIdLabel => 'ID майданчика:';Also applies to: 154-158, 232-236, 244-246, 295-295
.github/workflows/ci.yml (1)
101-116: Speed up integration job with pub cache.Mirror the unit-test job caching to shave minutes off CI.
steps: - name: Checkout uses: actions/checkout@v5 - name: Setup Flutter uses: subosito/flutter-action@v2 with: channel: stable + + - name: Cache pub dependencies + uses: actions/cache@v4 + with: + path: ${{ env.HOME }}/.pub-cache + key: ${{ runner.os }}-pub-${{ hashFiles('**/pubspec.lock') }} + restore-keys: ${{ runner.os }}-pub-test/repository/launches_repository_test.dart (2)
33-39: Prefer Exception over Error in tests.Throwing Error is for VM/runtime failures; app/repo code should surface Exceptions.
- when(repository.getLaunches()).thenAnswer((_) => Future.error(Error())); + when(repository.getLaunches()).thenAnswer((_) => Future.error(Exception('failed'))); ... - when(repository.getLaunch(flightNumber)) - .thenAnswer((_) => Future.error(Error())); + when(repository.getLaunch(flightNumber)) + .thenAnswer((_) => Future.error(Exception('failed')));Also applies to: 53-61
97-107: Validate the new ships field explicitly.Add an assertion to document the contract (non-null, empty by default).
test('returns full launch', () { when(repository.getLaunch(flightNumber)) .thenAnswer((_) => Future.value(mockFullLaunch)); expect( repository.getLaunch(flightNumber), completion(equals(mockFullLaunch)), ); + expect(mockFullLaunch.ships, isEmpty); });test/models/payload/payload_resource_test.dart (1)
45-48: Strengthen equality test with non-const lists.Current equality may pass due to constant canonicalization of list literals. Add a case where two instances use non-const lists to ensure deep equality behaves as intended at runtime.
Example add-on test:
test('value equality with non-const lists', () { final r1 = const PayloadResource( payloadId: 'X', noradId: null, customers: null, ).copyWith( noradId: [1, 2], customers: ['A'], ); final r2 = const PayloadResource( payloadId: 'X', noradId: null, customers: null, ).copyWith( noradId: [1, 2], customers: ['A'], ); expect(r1, equals(r2)); });If equality fails, you may need Equatable’s deep collection support or to normalize lists.
Also applies to: 50-52
lib/features/launch/info_item.dart (2)
2-2: Make constructor const (cheap win for immutability/perf).- InfoItem(this.label, this.value); + const InfoItem(this.label, this.value);
2-2: Consider named parameters to prevent argument swaps.Low-risk API hardening if usage is internal. If public, defer to a follow-up.
- const InfoItem(this.label, this.value); + const InfoItem({required this.label, required this.value});test/models/first_stage_ext_test.dart (3)
68-68: Test name mismatch: uses empty list, not null.- test('should handle null cores correctly', () { + test('should handle empty cores correctly', () {
72-72: Prefer isEmpty matcher over raw list equality.- expect(resource.cores, <dynamic>[]); + expect(resource.cores, isEmpty);
69-73: Add a case for truly null cores (if the model allows it).Covers
cores == nullvs[]semantics intoResource().Example:
test('should handle null cores correctly', () { const nullModel = NetworkFirstStageModel(cores: null); final resource = nullModel.toResource(); expect(resource.cores, isEmpty); // or isNull, per design });test/data/network/model/core/network_core_model_test.dart (1)
34-39: EnsuretoJsonalways includes nulls or use round-trip equivalence
- Default
includeIfNullistrue, soexpect(result, json)currently passes (fuchsia.googlesource.com)- To avoid brittleness if null-omission is ever enabled, either add
@JsonSerializable(includeIfNull: true)toNetworkCoreModelor refactor the test to round-trip viafromJson(toJson())and compare modelstest/models/launch/launch_site_ext_test.dart (1)
28-30: Use isNull matcher for clarity.- expect(resource.siteId, null); - expect(resource.siteName, null); - expect(resource.siteNameLong, null); + expect(resource.siteId, isNull); + expect(resource.siteName, isNull); + expect(resource.siteNameLong, isNull);test/models/launch/rocket_resource_test.dart (1)
61-64: Consider immutability checksOptionally assert that nested lists are unmodifiable to protect state objects from accidental mutation.
test/models/payload/payload_ext_test.dart (1)
44-76: Add immutability assertions for list fieldsEnsure toResource returns unmodifiable lists for noradId/customers to prevent downstream mutation.
Apply this diff to add a test:
test('should handle null fields correctly', () { const model = NetworkPayloadModel( payloadId: null, noradId: null, reused: null, customers: null, nationality: null, manufacturer: null, payloadType: null, payloadMassKg: null, payloadMassLbs: null, orbit: null, ); final resource = model.toResource(); expect( resource, equals( const PayloadResource( payloadId: null, noradId: null, reused: null, customers: null, nationality: null, manufacturer: null, payloadType: null, payloadMassKg: null, payloadMassLbs: null, orbit: null, ), ), ); }); + + test('toResource should return unmodifiable lists', () { + final model = NetworkPayloadModel.fromJson(json); + final resource = model.toResource(); + expect(() => resource.customers?.add('NASA'), throwsUnsupportedError); + expect(() => resource.noradId?.add(99999), throwsUnsupportedError); + });If this fails, update the mapping (models/payload/payload_ext.dart) or the resource to wrap lists with List.unmodifiable or EquatableListView.
test/models/core/core_ext_test.dart (2)
8-19: Type the JSON literal to Map<String, dynamic> (and make it const)Prevents implicit dynamic map issues and enables compile-time const.
- final json = { + const Map<String, dynamic> json = {
44-76: Add a mixed/nulls test caseYou have “all set” and “all null” cases. Consider a mixed case (some nulls, some set) to catch partial mapping regressions.
test/models/launch/rocket_resource_ext_test.dart (1)
52-65: Consider adding an “empty lists” caseA rocket with non-null stages but empty cores/payloads would harden behavior around empty collections vs nulls.
test/models/second_stage_resource_test.dart (1)
55-62: Avoid asserting Equatable.props directlyAsserting props ties tests to implementation details. Prefer asserting public fields or equality semantics only.
Example:
expect(resource1.block, 1); expect(resource1.payloads, [payload1, payload2]); // deep equalslib/models/payload/payload_ext.dart (1)
4-19: Correct field mapping; consider defensive copies for list fieldsTo prevent downstream mutation affecting equality, defensively copy list fields when present.
return PayloadResource( payloadId: payloadId, - noradId: noradId, + noradId: noradId == null ? null : List.unmodifiable(noradId), reused: reused, - customers: customers, + customers: customers == null ? null : List.unmodifiable(customers), nationality: nationality, manufacturer: manufacturer, payloadType: payloadType, payloadMassKg: payloadMassKg, payloadMassLbs: payloadMassLbs, orbit: orbit, );test/features/launch/bloc/launch_screen_test.dart (2)
77-79: Avoid tautological byType checksYou pump LaunchScreenBlocContent directly, so asserting its presence adds little. Prefer asserting specific child widgets/text relevant to the state.
Also applies to: 97-99
81-99: Consider adding a Ukrainian locale testPR mentions UA localization; mirror the PT test with Locale('uk') and assert the correct translation.
test/models/first_stage_resource_test.dart (1)
54-57: Add an explicit “empty list” caseYou cover null; add a cores: [] case to ensure props is [[]] and equality behaves as expected.
lib/data/network/model/launch/full/network_launch_full_model.dart (3)
89-91: Tighten norad_id to List?Improves type safety and matches resource/tests; add a custom converter if the API mixes types.
- @JsonKey(name: 'norad_id') List<dynamic>? noradId, + @JsonKey(name: 'norad_id') List<int>? noradId,
111-117: Use double? for orbital numeric fields that can be fractionallongitude and inclination_deg are often non-integers; current ints risk cast errors or precision loss.
- @JsonKey(name: 'longitude') int? longitude, + @JsonKey(name: 'longitude') double? longitude, ... - @JsonKey(name: 'inclination_deg') int? inclinationDeg, + @JsonKey(name: 'inclination_deg') double? inclinationDeg,
46-84: Reduce model duplication across network layerYou now have standalone models (e.g., NetworkFirstStageModel/NetworkCoreModel). Consider reusing them here to avoid drift (types like block) and regenerate only one source of truth.
lib/models/core/core_ext.dart (1)
4-4: Minor naming clarity for the extension.Consider a more descriptive name like NetworkCoreModelExt or a short doc comment for discoverability.
lib/models/launch/second_stage_resource.dart (1)
7-13: Make payloads truly immutable to prevent hash/equality drift.Lists are mutable; mutating payloads after construction can change equality/hash behavior. Copy to an unmodifiable list in the constructor.
Apply:
- const SecondStageResource({this.block, this.payloads}); + const SecondStageResource({this.block, List<PayloadResource>? payloads}) + : payloads = payloads == null ? null : List.unmodifiable(payloads);If you intend null and empty to be equivalent in equality, also adjust props accordingly:
- List<Object?> get props => [block, payloads]; + List<Object?> get props => [block, payloads ?? const <PayloadResource>[]];test/models/core/core_resource_test.dart (2)
45-48: Nit: avoid asserting on hashCode.hashCode is an implementation detail; equality is sufficient. Consider dropping the hashCode assertion to reduce brittleness.
- expect(core1.hashCode, equals(core2.hashCode)); + // Equality is sufficient; hashCode is an implementation detail.
54-70: Props-order test is tightly coupled to implementation details.This will fail on harmless props reordering. Prefer checking individual fields or using contains/hasLength matchers for stability.
test/models/launch/launch_full_resource_test.dart (2)
19-21: Stringly-typed dates risk parsing/locale issues.If possible, model launchDate/staticFireDate as DateTime in the resource and format at the UI layer.
63-84: Reduce coupling to props order.Asserting the entire props list (including nulls) makes tests brittle. Prefer asserting fields directly or using matchers against a subset.
test/models/launch/launch_site_resource_test.dart (1)
33-47: Avoid hard-coupling tests to Equatable.propsAsserting the exact contents/order of props can be brittle if implementation details change. Consider relying on equality/inequality and adding behavior-focused tests instead (e.g., mapping usage), or keep one minimal props check and drop the null-props assertion.
lib/models/launch/first_stage_ext.dart (1)
7-9: Return an immutable listDefensively copy to an unmodifiable list to preserve immutability guarantees of resources.
Apply:
FirstStageResource toResource() { return FirstStageResource( - cores: cores?.map((e) => e.toResource()).toList(), + cores: cores == null + ? null + : List.unmodifiable(cores!.map((e) => e.toResource())), ); }test/data/network/model/network_launch_full_model_test.dart (1)
136-139: Optional: assert static_fire_date_utc in serialized JSONThis guards the rename by verifying JSON key mapping.
You can add:
expect(json['static_fire_date_utc'], '2007-03-10T00:00:00.000Z');lib/models/launch/rocket_resource.dart (1)
3-6: Unify import styleMixing package and relative imports for sibling models can cause duplicate-type issues in some tooling. Use package imports consistently.
Apply:
-import 'package:flutter_bloc_app_template/models/launch/second_stage_resource.dart'; - -import 'first_stage_resource.dart'; +import 'package:flutter_bloc_app_template/models/launch/second_stage_resource.dart'; +import 'package:flutter_bloc_app_template/models/launch/first_stage_resource.dart';test/data/network/model/network_first_stage_model_test.dart (1)
5-34: Optional: add null/empty cores casesConsider tests for cores: null and cores: [] to cover edge cases in parsing and serialization.
lib/features/launch/widget/mission_overview_card_widget.dart (3)
20-22: Prefer withOpacity over withValues for broader SDK compatibilityUnless you’ve added a custom Color extension,
withValuesisn’t guaranteed across Flutter SDKs. UsewithOpacityto avoid build/runtime surprises.- theme.colorScheme.primaryContainer.withValues(alpha: 0.5), - theme.colorScheme.primaryContainer.withValues(alpha: 0.1), + theme.colorScheme.primaryContainer.withOpacity(0.5), + theme.colorScheme.primaryContainer.withOpacity(0.1), ... - color: theme.colorScheme.primary.withValues(alpha: 0.1), + color: theme.colorScheme.primary.withOpacity(0.1), ... - color: theme.colorScheme.surface.withValues(alpha: 0.7), + color: theme.colorScheme.surface.withOpacity(0.7),Also applies to: 37-38, 58-58
62-62: Treat empty strings as “no details”Current null-check lets empty/whitespace through. Normalize to show the fallback in that case.
- details ?? loc.noDetails, + (details?.trim().isNotEmpty ?? false) ? details! : loc.noDetails,
34-44: Add semantics for accessibilityExpose a semantic label/role for the info icon to improve TalkBack/VoiceOver.
- Container( + Semantics( + label: loc.missionOverview, + button: false, + child: Container( padding: const EdgeInsets.all(8), decoration: BoxDecoration( color: theme.colorScheme.primary.withOpacity(0.1), borderRadius: BorderRadius.circular(12), ), child: Icon( Icons.info_outline, color: theme.colorScheme.primary, ), - ), + ), + ),lib/features/launch/widget/info_grid_widget.dart (3)
21-27: Use theme-aware color for labelsAvoid hard-coded greys; respect theming (light/dark/high-contrast).
- Text( - item.label, - style: TextStyle( - fontSize: 12, - color: Colors.grey[600], - ), - ), + Text( + item.label, + style: Theme.of(context).textTheme.labelSmall?.copyWith( + color: Theme.of(context).colorScheme.onSurfaceVariant, + ), + ),
16-18: Consider making tile width configurableA fixed 150px can clip localized/long values on narrow screens. Expose a
tileWidthparameter with a sensible default.-class InfoGridWidget extends StatelessWidget { - const InfoGridWidget({super.key, required this.items}); +class InfoGridWidget extends StatelessWidget { + const InfoGridWidget({super.key, required this.items, this.tileWidth = 160}); final List<InfoItem> items; + final double tileWidth; ... - (item) => SizedBox( - width: 150, + (item) => SizedBox( + width: tileWidth,
29-35: Guard against overflow in valuesLong strings (e.g., orbits, IDs) can overflow. Clamp lines and add ellipsis.
- Text( - item.value, - style: const TextStyle( - fontSize: 14, - fontWeight: FontWeight.w600, - ), - ), + Text( + item.value, + maxLines: 2, + overflow: TextOverflow.ellipsis, + style: const TextStyle( + fontSize: 14, + fontWeight: FontWeight.w600, + ), + ),test/data/network/model/payload/network_payload_model_test.dart (1)
6-17: Minor: make the input JSON const for safetyImmutability reduces accidental mutations in tests.
- final json = { + const json = { 'payload_id': 'FalconSAT-2', 'norad_id': [12345, 67890], 'reused': false, 'customers': ['DARPA'], 'nationality': 'United States', 'manufacturer': 'SSTL', 'payload_type': 'Satellite', 'payload_mass_kg': 20, 'payload_mass_lbs': 44, 'orbit': 'LEO', };lib/features/launch/widget/links_card_widget.dart (3)
53-55: Hide the card when there are no linksAvoid rendering an empty card when all links are null.
final linkItems = [ ... ]; + + if (linkItems.isEmpty) return const SizedBox.shrink(); return Card(
54-66: Match Card shape with inner Container and clip the gradientPrevents subtle corner mismatches and ensures proper clipping.
- return Card( - child: Container( + return Card( + shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(20)), + clipBehavior: Clip.antiAlias, + child: Container( decoration: BoxDecoration( gradient: LinearGradient(
59-61: Confirm Flutter version for Color.withValues or switch to withOpacityColor.withValues is relatively new; use withOpacity for broader compatibility if your min SDK doesn’t support it.
- theme.colorScheme.surfaceContainerHighest.withValues(alpha: 0.5), - theme.colorScheme.surfaceContainerHighest.withValues(alpha: 0.2), + theme.colorScheme.surfaceContainerHighest.withOpacity(0.5), + theme.colorScheme.surfaceContainerHighest.withOpacity(0.2), @@ - color: theme.colorScheme.primary.withValues(alpha: 0.1), + color: theme.colorScheme.primary.withOpacity(0.1),Also applies to: 77-77
lib/data/network/model/launch/network_second_stage_model.dart (1)
8-18: Freezed class style and JSON configTwo tweaks for robustness:
- Prefer
classoverabstract classwith Freezed (common style and avoids older patterns).- Add
@JsonSerializable(explicitToJson: true)so nested lists calltoJson()without manual mapping.@freezed -abstract class NetworkSecondStageModel with _$NetworkSecondStageModel { +@JsonSerializable(explicitToJson: true) +class NetworkSecondStageModel with _$NetworkSecondStageModel { const factory NetworkSecondStageModel({ int? block, List<NetworkPayloadModel>? payloads, }) = _NetworkSecondStageModel; const NetworkSecondStageModel._();test/data/network/model/network_second_stage_model_test.dart (2)
45-53: Use the generated toJson for round-trip testingThis validates the generator output directly and reduces boilerplate.
- test('toJson should convert correctly', () { - final model = NetworkSecondStageModel.fromJson(json); - final result = { - 'block': model.block, - 'payloads': model.payloads?.map((p) => p.toJson()).toList(), - }; - - expect(result, json); - }); + test('toJson should convert correctly', () { + final model = NetworkSecondStageModel.fromJson(json); + expect(model.toJson(), equals(json)); + });
63-70: Avoid dynamic list typing in expectationUse
isEmptyfor clarity and to avoid type inference surprises.- final updated = model.copyWith(block: 2, payloads: []); - - expect(updated.block, 2); - expect(updated.payloads, <dynamic>[]); + final updated = model.copyWith(block: 2, payloads: []); + expect(updated.block, 2); + expect(updated.payloads, isEmpty);lib/features/launch/widget/status_section_widget.dart (2)
13-71: Add Semantics for screen readersAnnounce the mission status for accessibility.
- return Container( + return Semantics( + container: true, + label: success ? loc.missionSuccessful : loc.missionFailed, + child: Container( padding: const EdgeInsets.all(16), decoration: BoxDecoration( @@ - ), - child: Row( + ), + child: Row( children: [ @@ - ), - ); + ), + );
26-26: Confirm Color.withValues availability or use withOpacitySwitch to withOpacity if your minimum Flutter SDK doesn’t include withValues.
- color: (success ? Colors.green : Colors.red).withValues(alpha: 0.3), + color: (success ? Colors.green : Colors.red).withOpacity(0.3), @@ - color: Colors.white.withValues(alpha: 0.2), + color: Colors.white.withOpacity(0.2), @@ - color: Colors.white.withValues(alpha: 0.9), + color: Colors.white.withOpacity(0.9),Also applies to: 37-37, 62-62
lib/features/launch/widget/recovery_ships_card_widget.dart (2)
15-17: Match Card shape with inner Container and clipKeeps the gradient corners perfectly aligned.
- return Card( - child: Container( + return Card( + shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(20)), + clipBehavior: Clip.antiAlias, + child: Container(
20-22: Confirm Color.withValues availability or use withOpacityUse withOpacity for wider SDK compatibility.
- Colors.cyan.withValues(alpha: 0.1), - Colors.cyan.withValues(alpha: 0.05), + Colors.cyan.withOpacity(0.1), + Colors.cyan.withOpacity(0.05), @@ - color: Colors.cyan.withValues(alpha: 0.1), + color: Colors.cyan.withOpacity(0.1), @@ - color: Colors.cyan.withValues(alpha: 0.3), + color: Colors.cyan.withOpacity(0.3),Also applies to: 38-38, 66-66
lib/data/network/model/launch/network_launch_model.dart (2)
36-37: Ensure nested models serialize correctly (toJson).If these models are ever serialized, add explicitToJson to avoid nested objects being left as dynamic Maps.
Apply this diff on the factory to enable nested serialization:
abstract class NetworkRocketModel with _$NetworkRocketModel { - const factory NetworkRocketModel({ + @JsonSerializable(explicitToJson: true) + const factory NetworkRocketModel({ @JsonKey(name: 'rocket_id') required String id, @JsonKey(name: 'rocket_type') String? type, @JsonKey(name: 'rocket_name') String? name, @JsonKey(name: 'first_stage') NetworkFirstStageModel? firstStage, @JsonKey(name: 'second_stage') NetworkSecondStageModel? secondStage, }) = _NetworkRocketModel;Please confirm whether toJson is used for NetworkRocketModel anywhere; if yes, re-run build_runner.
54-56: Consider stronger types for URLs and add mapping tests.For articleLink/wikipedia/videoLink/presskit, using Uri? instead of String? can prevent invalid URLs at the edge. Also add JSON (de)serialization tests for these new keys.
Would you like me to add a small test asserting reddit_launch/video_link/presskit mapping?
integration_test/launch_test.dart (3)
21-37: Stub the bloc stream, not only the state.Some widgets read
bloc.stream(e.g., BlocBuilder) and won’t rebuild from a staticstate. Stub the stream to a matching success state.Apply:
when(() => launchBloc.state).thenReturn( const LaunchSuccessState( launch: mockLaunchResource, ), ); + when(() => launchBloc.stream).thenAnswer( + (_) => Stream.value( + const LaunchSuccessState(launch: mockLaunchResource), + ), + );If
LaunchScreenBlocContentreads onlycontext.read<LaunchBloc>().state, this is harmless but safer for future changes.
17-19: Close the mocked bloc in tearDown.Avoid leaking subscriptions/resources even for mocks.
Add:
setUp(() { launchBloc = MockLaunchBloc(); }); +tearDown(() { + launchBloc.close(); +});
50-77: Reduce repetition by parameterizing locale assertions.Loop over locale/expectations to shorten and ease maintenance.
I can provide a parameterized helper if you’d like.
lib/features/launch/widget/launch_site_card_widget.dart (1)
14-16: Clip the gradient to the card’s rounded shape.Without
clipBehavior/shapeon Card, the gradient container’s radius can visually mismatch ink/press states.Apply:
- return Card( - child: Container( + return Card( + clipBehavior: Clip.antiAlias, + shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(20)), + child: Container( decoration: BoxDecoration( gradient: LinearGradient( ... - borderRadius: BorderRadius.circular(20), + borderRadius: BorderRadius.circular(20),Also applies to: 25-26
lib/features/launch/widget/payload_card_widget.dart (1)
82-114: Render customers only when non-empty.Skip the Wrap when the list is empty.
Apply:
- if (payload.customers != null) + if ((payload.customers?.isNotEmpty ?? false)) Wrap( spacing: 8, runSpacing: 8, children: payload.customers!lib/models/payload/payload_resource.dart (2)
26-27: Confirm numeric types for payload mass.SpaceX data often exposes mass as non-integer. Consider
double?(ornum?) forpayloadMassKg/payloadMassLbsto avoid truncation.If upstream network model uses
double?, align the resource type and UI formatting accordingly.
20-23: List equality semantics.Equatable compares lists by
==(identity). If deep equality ofnoradId/customersmatters, wrap withUnmodifiableListViewand compare using collection equality, or switch to Freezed data classes.Do current tests rely on deep list equality? If yes, I can provide a small refactor.
lib/data/network/model/launch/full/network_launch_full_model.g.dart (1)
28-29: Casting ships to List is correct; consider aligning missionId too.The change to cast ships as List tightens types. For consistency/type-safety against the API, consider also mapping missionId to List (currently List).
lib/data/network/model/payload/network_payload_model.dart (1)
16-17: Consider double for payload mass fields (API often returns fractional values).If the source API supplies fractional masses, switch to double to avoid truncation; regenerate after change.
Apply:
- @JsonKey(name: 'payload_mass_kg') int? payloadMassKg, - @JsonKey(name: 'payload_mass_lbs') int? payloadMassLbs, + @JsonKey(name: 'payload_mass_kg') double? payloadMassKg, + @JsonKey(name: 'payload_mass_lbs') double? payloadMassLbs,lib/features/launch/widget/rocket_card_widget.dart (4)
16-16: firstOrNull may not be available without SDK support/import.If targeting a Dart SDK without Iterable.firstOrNull, replace with a safe fallback.
- final core = rocket.firstStage?.cores?.firstOrNull; + final core = (rocket.firstStage?.cores?.isNotEmpty ?? false) + ? rocket.firstStage!.cores!.first + : null;
25-27: Color.withValues requires recent Flutter; use withOpacity if needed.To support older stable channels, you can switch to withOpacity.
- theme.colorScheme.tertiaryContainer.withValues(alpha: 0.5), - theme.colorScheme.tertiaryContainer.withValues(alpha: 0.1), + theme.colorScheme.tertiaryContainer.withOpacity(0.5), + theme.colorScheme.tertiaryContainer.withOpacity(0.1), ... - color: theme.colorScheme.tertiary.withValues(alpha: 0.1), + color: theme.colorScheme.tertiary.withOpacity(0.1),Also applies to: 40-41
67-69: Duplicate label in value (“Block … Block N”).The value should only contain the number (or a simple formatted token), not the label again.
- InfoItem(loc.rocketBlock, - '${loc.rocketBlock} ${core?.block!}'), + InfoItem(loc.rocketBlock, '${core!.block!}'),
100-121: Spacer gaps appear when preceding chips are absent; prefer Wrap with spacing.Prevents leading/trailing 8px gaps and simplifies layout.
- SingleChildScrollView( - scrollDirection: Axis.horizontal, - child: Row( - children: [ - if (core?.gridfins != null) - FeatureChipWidget( - label: loc.gridFins, - active: core!.gridfins!, - ), - const SizedBox(width: 8), - if (core?.legs != null) - FeatureChipWidget( - label: loc.landingLegs, - active: core!.legs!, - ), - const SizedBox(width: 8), - if (core?.reused != null) - FeatureChipWidget( - label: loc.reused, - active: core!.reused!, - ), - ], - ), - ), + SingleChildScrollView( + scrollDirection: Axis.horizontal, + child: Wrap( + spacing: 8, + children: [ + if (core?.gridfins != null) + FeatureChipWidget( + label: loc.gridFins, + active: core!.gridfins!, + ), + if (core?.legs != null) + FeatureChipWidget( + label: loc.landingLegs, + active: core!.legs!, + ), + if (core?.reused != null) + FeatureChipWidget( + label: loc.reused, + active: core!.reused!, + ), + ], + ), + ),lib/data/network/model/launch/network_launch_model.g.dart (3)
42-49: Nested stage parsing looks correct; ensure serialization doesn't leak raw objectsDeserialization is fine. However, in ToJson you currently emit the object refs (see Lines 57–58). If any caller uses
toJson()directly (e.g., caching) instead ofjsonEncode, nested objects will remain non-encodable. Prefer@JsonSerializable(explicitToJson: true)on the parent models so generatedtoJsoncalls.toJson()on nested types.Apply at the source (not in this generated file), e.g.:
-@JsonSerializable() +@JsonSerializable(explicitToJson: true) class NetworkRocketModel { ... } -@JsonSerializable() +@JsonSerializable(explicitToJson: true) class NetworkLaunchModel { ... } -@JsonSerializable() +@JsonSerializable(explicitToJson: true) class NetworkLaunchLinksModel { ... }
57-58: Prefer emitting JSON maps for nested stagesSame note as above: writing
instance.firstStage/secondStagedirectly can leave raw objects in the returned map. WithexplicitToJson: true, this becomesinstance.firstStage?.toJson()andinstance.secondStage?.toJson().
69-71: New link fields parsed correctly; consider end-to-end coverage
reddit_launch,video_link, andpresskitare parsed. Verify downstream resource mapping (LinksResource) includes these if product needs them; otherwise they’ll be silently dropped.lib/models/core/core_resource.dart (1)
6-17: Optional: add ergonomics (copyWith, toString) and an empty factoryThese help call sites and testing without adding runtime cost.
Example:
class CoreResource extends Equatable { const CoreResource({ @@ }); + + CoreResource copyWith({ + String? coreSerial, + int? flight, + int? block, + bool? gridfins, + bool? legs, + bool? reused, + bool? landSuccess, + bool? landingIntent, + String? landingType, + String? landingVehicle, + }) { + return CoreResource( + coreSerial: coreSerial ?? this.coreSerial, + flight: flight ?? this.flight, + block: block ?? this.block, + gridfins: gridfins ?? this.gridfins, + legs: legs ?? this.legs, + reused: reused ?? this.reused, + landSuccess: landSuccess ?? this.landSuccess, + landingIntent: landingIntent ?? this.landingIntent, + landingType: landingType ?? this.landingType, + landingVehicle: landingVehicle ?? this.landingVehicle, + ); + } + + static const empty = CoreResource();test/models/launch_ext_model_tests.dart (5)
103-113: Consider asserting new link fields if surfaced in LinksResourceIf
LinksResourceexposesredditLaunch,videoLink, orpresskit, add expectations to prevent regressions. If intentionally omitted from domain, ignore.
67-72: You include ships in the input but don’t assert mappingIf
LaunchFullResourceexposesships, add an expectation to cover it.Example:
- 'ships': ['ELSBETH3', 'GOQUEST', 'GOSEARCHER'], + 'ships': ['ELSBETH3', 'GOQUEST', 'GOSEARCHER'], @@ - final launchResource = networkLaunchModel.toResource(); + final launchResource = networkLaunchModel.toResource(); + expect( + launchResource.ships, + ['ELSBETH3', 'GOQUEST', 'GOSEARCHER'], + );
185-191: Time-delta test is flaky; freeze “now”Using
DateTime.now()makes this off-by-one across day boundaries. Freeze the clock for determinism.Apply:
+import 'package:clock/clock.dart'; @@ - final date = DateTime.now().add(const Duration(days: 5)); - final launchDays = date.formatDateWithDays(); + withClock(Clock.fixed(DateTime.utc(2030, 01, 01, 12)), () { + final date = clock.now().add(const Duration(days: 5)); + final launchDays = date.formatDateWithDays(); - expect(launchDays, isA<From>()); - expect((launchDays as From).formattedDate, '4'); + expect(launchDays, isA<From>()); + expect((launchDays as From).formattedDate, '5'); + });
193-199: Same flakiness for “Since”Freeze the clock to avoid boundary drift.
- final date = DateTime.now().subtract(const Duration(days: 5)); - final launchDays = date.formatDateWithDays(); + withClock(Clock.fixed(DateTime.utc(2030, 01, 06, 12)), () { + final date = clock.now().subtract(const Duration(days: 5)); + final launchDays = date.formatDateWithDays(); - expect(launchDays, isA<Since>()); - expect((launchDays as Since).formattedDate, '5'); + expect(launchDays, isA<Since>()); + expect((launchDays as Since).formattedDate, '5'); + });
222-228: Make the fixture UTC to match “UTC” labelUse a UTC DateTime to avoid timezone surprises.
- final date = DateTime(2025, 12, 25, 14, 30); + final date = DateTime.utc(2025, 12, 25, 14, 30); final formattedTime = date.formatDate(format: 'MMMM dd, yyyy - HH:mm UTC');lib/models/launch/launch_full_resource.dart (1)
30-41: Consider carrying raw DateTime alongside formatted strings.Storing launchDate/staticFireDate as String? limits sorting, timezone transforms, and reformatting. Consider adding DateTime? raw fields (or a value object) and derive presentation in the UI layer.
I can draft a minimal change that adds raw fields while keeping these Strings for backward compatibility.
lib/features/launch/widget/timeline_widgets.dart (3)
21-33: Align Card shape with gradient clipping.The child Container has its own borderRadius while Card uses its default shape (can cause visual mismatch). Let Card define the radius and clip; drop the child’s borderRadius.
- return Card( - child: Container( + return Card( + clipBehavior: Clip.antiAlias, + shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(20)), + child: Container( decoration: BoxDecoration( gradient: LinearGradient( @@ - ), - borderRadius: BorderRadius.circular(20), + ), ),
134-146: Guard against long text overflow.Titles/dates can be long (localized). Add maxLines/ellipsis.
- Text( - title, + Text( + title, style: theme.textTheme.titleMedium?.copyWith( fontWeight: FontWeight.bold, ), + maxLines: 1, + overflow: TextOverflow.ellipsis, ), - Text( - subtitle, + Text( + subtitle, style: theme.textTheme.bodySmall?.copyWith( color: theme.colorScheme.onSurfaceVariant, ), + maxLines: 1, + overflow: TextOverflow.ellipsis, ),
62-75: Hard-coded completed: true — consider data-driven states.Both “Static fire” and “Launch” are always rendered as completed. If data is missing or tentative, reflect that in UI (or hide the item).
I can add optional booleans (e.g., staticFireCompleted, launchCompleted) with sensible defaults and update call sites.
lib/models/launch/extension/launch_ext.dart (1)
65-67: Optional: localizable, consistent time pattern.launchTime uses a fixed pattern. Consider DateFormat.yMMMEd().add_jm() to respect locale or reuse a shared date formatter.
I can extract a small DateFormats helper to centralize patterns.
lib/features/launch/widget/stat_card_widget.dart (3)
73-85: Color.withValues may require newer Flutter; prefer withOpacity for broad SDK compatibilityReduce risk of build breaks on older channels.
- gradient: LinearGradient( + gradient: LinearGradient( colors: [ - color.withValues(alpha: 0.1), - color.withValues(alpha: 0.05), + color.withOpacity(0.10), + color.withOpacity(0.05), ], @@ - border: Border.all( - color: color.withValues(alpha: 0.3), + border: Border.all( + color: color.withOpacity(0.30), width: 1, ),If the project guarantees a Flutter version that supports
withValues, feel free to keep it.
100-106: Improve value text contrast in dark/light themesUsing the raw accent color for text may fail contrast in some themes. Consider deriving from the theme or clamping luminance.
Example:
- style: TextStyle( + style: TextStyle( fontSize: 14, fontWeight: FontWeight.bold, - color: color, + color: Theme.of(context).brightness == Brightness.dark + ? Colors.white + : color, ),
87-111: Add semantics for accessibilityExpose label/value to screen readers for each card.
Wrap the Column with:
Semantics( label: '$label', value: value, child: Column( // ... ), )lib/features/launch/launch_screen.dart (3)
124-131: System UI overlay style is hardcoded to light iconsOn light backgrounds this can reduce visibility. Make it adaptive or set it on the SliverAppBar.
Example:
final overlay = Theme.of(context).brightness == Brightness.dark ? SystemUiOverlayStyle.light : SystemUiOverlayStyle.dark;Then either drop the global AnnotatedRegion or set
systemOverlayStyle: overlayon SliverAppBar.
120-123: Deduplicate deep payload chainingStore
payloadonce and reuse. Reduces risk of NPEs and improves readability.@@ - final images = <String>[]; + final images = <String>[]; + final payload = + widget.resource.rocket?.secondStage?.payloads?.firstOrNull; @@ - if (widget.resource.rocket?.secondStage?.payloads - ?.firstOrNull != - null) ...[ + if (payload != null) ...[ const SizedBox(height: 20), - QuickStatsSectionWidget( - payload: widget - .resource.rocket!.secondStage!.payloads!.first, - rocketName: widget.resource.rocket?.rocketName, - ), + QuickStatsSectionWidget( + payload: payload, + rocketName: widget.resource.rocket?.rocketName, + ), const SizedBox(height: 20), - PayloadCardWidget( - payload: widget - .resource.rocket!.secondStage!.payloads!.first, - ), + PayloadCardWidget(payload: payload), ],Also applies to: 351-364
145-147: Color.withValues appears multiple times; confirm SDK supportIf your min Flutter SDK doesn’t include
Color.withValues, switch towithOpacityfor compatibility.Also applies to: 158-160, 215-216, 282-285, 299-300, 303-307
lib/data/network/model/launch/full/network_launch_full_model.freezed.dart (2)
24-25: Prefer strong typing formissionId.If API returns strings, switch
missionIdfromList<dynamic>?toList<String>?in the source and regenerate.- @JsonKey(name: 'mission_id') - List<dynamic>? get missionId; + @JsonKey(name: 'mission_id') + List<String>? get missionId;Also applies to: 786-804, 256-261
2486-2494: Considerdouble?for payload mass fields.If payload masses can be non-integer, use
double?to avoid cast issues/loss.- @JsonKey(name: 'payload_mass_kg') int? get payloadMassKg; - @JsonKey(name: 'payload_mass_lbs') int? get payloadMassLbs; + @JsonKey(name: 'payload_mass_kg') double? get payloadMassKg; + @JsonKey(name: 'payload_mass_lbs') double? get payloadMassLbs;Also applies to: 2900-2903
lib/data/network/model/payload/network_payload_model.freezed.dart (1)
28-31: Optional: allow non-integer masses.Switch
payloadMassKg/payloadMassLbstodouble?in the source if API may return decimals; regenerate to avoidas intcasts failing.- @JsonKey(name: 'payload_mass_kg') int? payloadMassKg; - @JsonKey(name: 'payload_mass_lbs') int? payloadMassLbs; + @JsonKey(name: 'payload_mass_kg') double? payloadMassKg; + @JsonKey(name: 'payload_mass_lbs') double? payloadMassLbs;Also applies to: 450-456
lib/l10n/app_localizations.dart (1)
387-650: Add missing description metadata for ambiguous keys in translation ARBsintl_de.arb and intl_pt.arb currently lack any
@…descriptionentries for these message IDs, which can lead to mistranslations:
- payload
- payloadTitle
- type
- rocketType
- id
- flight
Add succinct
descriptionfields in the ARB metadata for each of these keys (e.g. inlib/l10n/intl_en.arb), then regenerate or propagate to the other locale files so translators have clear context.lib/l10n/app_localizations_en.dart (2)
215-216: Avoid emoji in localized copy; move to UI for accessibility.Screen readers may read the emoji literally. Prefer keeping copy text-only and add the emoji in the widget tree with proper Semantics.
Apply:
- String get firstStage => '🚀 First Stage'; + String get firstStage => 'First Stage';
296-298: Format flight numbers with locale-aware grouping.Improves readability for large numbers and uses the existing intl import.
Apply:
- String flightNumber(Object number) { - return 'Flight #$number'; - } + String flightNumber(Object number) { + final n = int.tryParse('$number'); + final formatted = n == null + ? '$number' + : intl.NumberFormat.decimalPattern(localeName).format(n); + return 'Flight #$formatted'; + }lib/l10n/app_localizations_pt.dart (4)
238-241: Spell out “not available” instead of abbreviation.“N/D” can be unclear to users. Recommend a full phrase.
- String get notAvailable => 'N/D'; + String get notAvailable => 'Não disponível';
274-276: More natural PT phrasing for “Watch Video”.- String get watchVideo => 'Assistir Vídeo'; + String get watchVideo => 'Assistir ao vídeo';
295-298: Prefer PT idiom for flight numbering.- String flightNumber(Object number) { - return 'Voo #$number'; - } + String flightNumber(Object number) { + return 'Voo nº $number'; + }
166-170: Follow-up: older PT keys remain untranslated (“Appearance”, “About”).Not part of this hunk, but worth aligning for consistency later.
lib/l10n/app_localizations_de.dart (4)
271-273: Use “und” instead of ampersand in running text.- String get linksResources => 'Links & Ressourcen'; + String get linksResources => 'Links und Ressourcen';
247-249: UI convention: prefer “ID” over “Kennung”.Common UI label and matches
siteIdLabel.- String get id => 'Kennung'; + String get id => 'ID';
295-298: More idiomatic DE numbering for flights.- String flightNumber(Object number) { - return 'Flug #$number'; - } + String flightNumber(Object number) { + return 'Flug Nr. $number'; + }
166-170: Follow-up: some older DE keys are still English (“Appearance”, “About”).Not in this diff, but consider localizing them for consistency.
| String formatDate({String format = 'hh:mm a, MMM yyyy'}) { | ||
| if (this == null) return ''; | ||
| return DateFormat('hh:mm a, MMM yyyy').format(this!); | ||
| return DateFormat(format).format(this!); | ||
| } |
There was a problem hiding this comment.
Timezone bug: label says “UTC” but time isn’t converted to UTC.
formatDate doesn’t convert to UTC, yet launchDate is formatted with “UTC” in the pattern. Convert when labeling UTC.
Apply:
- String formatDate({String format = 'hh:mm a, MMM yyyy'}) {
+ String formatDate({String format = 'hh:mm a, MMM yyyy', bool useUtc = false}) {
if (this == null) return '';
- return DateFormat(format).format(this!);
+ final dt = useUtc ? this!.toUtc() : this!;
+ return DateFormat(format).format(dt);
}And update the UTC-labeled usage:
- launchDate: launchDate.formatDate(format: 'MMMM dd, yyyy - HH:mm UTC'),
+ launchDate: launchDate.formatDate(
+ format: "MMMM dd, yyyy - HH:mm 'UTC'",
+ useUtc: true,
+ ),Also applies to: 80-82
| @@ -0,0 +1,115 @@ | |||
| import 'package:bloc_test/bloc_test.dart'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stabilize tests by stubbing the bloc stream
Some Bloc widgets subscribe to bloc.stream; stubbing avoids flakiness.
+import 'dart:async';
import 'package:bloc_test/bloc_test.dart'; setUp(() {
launchBloc = MockLaunchBloc();
+ when(() => launchBloc.stream).thenAnswer((_) => Stream<LaunchState>.empty());
});Also applies to: 39-41
🤖 Prompt for AI Agents
In test/features/launch/bloc/launch_screen_test.dart around lines 1 and also
apply to lines 39-41, the test fails to stub the bloc.stream which causes
flakiness because widgets subscribe to the stream; fix by stubbing the bloc
stream using whenListen (from bloc_test) or equivalent before rendering the
widget—provide a Stream.of the desired sequence of states and set the
initialState argument to match the bloc's current state so the widget receives
predictable events; apply the same whenListen stub to the other test locations
(lines 39-41).
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (23)
test/features/launch/widget/recovery_ships_card_widget_test.dart (4)
14-14: Use pumpAndSettle() to deflake widget stabilization.
Prevents race conditions in CI if there are implicit animations or async rebuilds.- await tester.pump(); + await tester.pumpAndSettle(); @@ - await tester.pump(); + await tester.pumpAndSettle(); @@ - await tester.pump(); + await tester.pumpAndSettle();Also applies to: 27-27, 49-49
9-18: Avoid brittle SizedBox assertion for “renders nothing”.
Asserting exactly one SizedBox is fragile; assert absence of header/icons instead while keeping the Card check.- expect(find.byType(SizedBox), findsOne); - expect(find.byType(Card), findsNothing); + expect(find.text(S.current.recoveryShips), findsNothing); + expect(find.byIcon(Icons.directions_boat), findsNothing); + expect(find.byIcon(Icons.anchor), findsNothing); + expect(find.byType(Card), findsNothing);
20-40: Assert Card presence and prefer const list literal.
Adds a stronger structural check and a small immutability win.- final ships = ['GO Ms. Tree', 'GO Ms. Chief', 'GO Ms. Independence']; + const ships = ['GO Ms. Tree', 'GO Ms. Chief', 'GO Ms. Independence']; @@ - await tester.pump(); + await tester.pumpAndSettle(); @@ + // Card should render when there are ships + expect(find.byType(Card), findsOneWidget);
42-55: Mirror the multi-ship assertions: const list and Card presence.
Keeps tests consistent and slightly more robust.- final ships = ['GO Ms. Tree']; + const ships = ['GO Ms. Tree']; @@ - await tester.pump(); + await tester.pumpAndSettle(); @@ + expect(find.byType(Card), findsOneWidget);test/features/launch/widget/feature_chip_widget_test.dart (4)
23-25: Prefer deterministic icon lookupUsing find.byType(Icon) can return an unrelated Icon. Query the specific icon instead.
- final icon = tester.widget<Icon>(find.byType(Icon)); - expect(icon.icon, Icons.check_circle); + expect(find.byIcon(Icons.check_circle), findsOneWidget);
54-55: Same here: assert the specific inactive icon- final icon = tester.widget<Icon>(find.byType(Icon)); - expect(icon.icon, Icons.cancel); + expect(find.byIcon(Icons.cancel), findsOneWidget);
17-17: Unnecessary pumpAndSettleNo async animations here. Removing saves test time.
- await tester.pumpAndSettle();
48-48: Unnecessary pumpAndSettleSame rationale as above.
- await tester.pumpAndSettle();lib/features/launch/info_item.dart (4)
5-9: Make constructor const and the Key optional/nullable.
Allows const instantiation where possible and avoids forcing a Key at call sites.- InfoItem(this.label, this.value, this.key); + const InfoItem(this.label, this.value, [this.key]); final String label; final String value; - final Key key; + final Key? key;
4-4: Annotate as immutable and enable Equatable stringify for better logs.
Minor DX improvements.+@immutable class InfoItem extends Equatable { + @override + bool? get stringify => true;
7-9: Consider a clearer name thankey(e.g.,tileKeyoruiKey).
Avoids confusion with Flutter widgetkeyparams when reading call sites.
11-16: Remove UI key from Equatable props
Including the UIkeyties value equality to a rendering concern. Recommend removing it:- List<Object?> get props => [ - label, - value, - key, - ]; + List<Object?> get props => [label, value];test/features/launch/widget/payload_card_widget_test.dart (5)
24-36: Stabilize i18n lookups and drop redundant pumpUse l10n from the widget context (safer than S.current across tests) and replace the extra pump with pumpAndSettle.
testWidgets('renders title and subtitle correctly', (tester) async { await tester.pumpLocalizedWidget( child: PayloadCardWidget( payload: payload, ), locale: const Locale('en'), ); - - await tester.pump(); - expect(find.text(S.current.payloadTitle), findsOneWidget); + await tester.pumpAndSettle(); + final l10n = + S.of(tester.element(find.byType(PayloadCardWidget))); + expect(find.text(l10n.payloadTitle), findsOneWidget); - expect(find.text('${payload.payloadId} • ${payload.payloadType}'), + expect(find.text('${payload.payloadId} • ${payload.payloadType}'), findsOneWidget); });Repeat the l10n pattern in other tests that use S.current.
38-58: Prefer keyed assertions over text to reduce brittlenessText can change with copy edits or formatting; stable ValueKeys make tests resilient. If the widget can expose keys for all info rows, assert on them here.
// Expand the tile - await tester.tap(find.text(S.current.payloadTitle)); + await tester.tap(find.text(S.current.payloadTitle)); await tester.pumpAndSettle(); - expect(find.text(payload.payloadId!), findsOneWidget); - expect(find.text(payload.payloadType!), findsOneWidget); - expect(find.text('${payload.payloadMassKg} kg'), findsOneWidget); - expect(find.text(payload.orbit!), findsOneWidget); - expect(find.text(payload.manufacturer!), findsOneWidget); - expect(find.text(payload.nationality!), findsOneWidget); + expect(find.byKey(const Key('info_id')), findsOneWidget); + expect(find.byKey(const Key('info_type')), findsOneWidget); + expect(find.byKey(const Key('info_mass')), findsOneWidget); + expect(find.byKey(const Key('info_orbit')), findsOneWidget); + expect(find.byKey(const Key('info_manufacturer')), findsOneWidget); + expect(find.byKey(const Key('info_nationality')), findsOneWidget);If these keys don’t exist yet, consider adding them in the widget as static consts.
60-77: Make customer checks resilient to different renderingsAvoid null-assert and allow for chips/comma-separated layouts by using textContaining.
- for (final customer in payload.customers!) { - expect(find.text(customer), findsOneWidget); - } + final customers = payload.customers ?? const <String>[]; + for (final c in customers) { + expect(find.textContaining(c), findsOneWidget); + }
79-99: Also assert absence of optional sections when missingStrengthen this test by verifying that optional info rows aren’t rendered.
await tester.pumpAndSettle(); expect(find.text(emptyPayload.payloadId!), findsOneWidget); expect(find.text(emptyPayload.payloadType!), findsOneWidget); expect(find.byKey(const Key('info_id')), findsOneWidget); expect(find.byKey(const Key('info_type')), findsOneWidget); + expect(find.byKey(const Key('info_mass')), findsNothing); + expect(find.byKey(const Key('info_orbit')), findsNothing); + expect(find.byKey(const Key('info_manufacturer')), findsNothing); + expect(find.byKey(const Key('info_nationality')), findsNothing); + expect(find.byKey(const Key('info_customers')), findsNothing);If keys aren’t available, add negative text assertions (e.g., expect(find.text('kg'), findsNothing)) as a fallback.
1-7: De-fragile test helper importThe deep relative path to pumpLocalizedWidget (../../../bloc/utils.dart) is brittle. Consider moving common test utilities under a top-level test_support/helpers.dart and importing from there.
test/features/launch/widget/launch_site_card_widget_test.dart (1)
10-11: Unnecessary binding initialization
TestWidgetsFlutterBinding.ensureInitialized()is redundant in widget tests when usingtester.pumpWidget(...)helpers. Safe to remove.- TestWidgetsFlutterBinding.ensureInitialized();test/features/launch/widget/stat_card_widget_test.dart (2)
1-59: File name/content mismatch and duplicationThis file tests
LaunchSiteCardWidgetbut is namedstat_card_widget_test.dart. It also largely duplicateslaunch_site_card_widget_test.dart. Prefer to either:
- Rename to
launch_site_card_widget_test.dartand consolidate tests in a single file, or- Replace contents with actual
StatCardWidgettests if that’s the intent.Reduces confusion and keeps CI time lean.
1-2: Consistent imports forLocaleBoth
dart:uiandpackage:flutter/material.dartprovideLocale. For consistency with the other test, import style can be unified, but it’s non-blocking.test/features/launch/launch_screen_test.dart (3)
1-12: Use AppLocalizations instead of hard-coded strings to reduce brittlenessAsserting raw text literals will break on copy tweaks. Prefer l10n lookups so the tests track the source of truth.
Apply this diff:
@@ import 'package:flutter_test/flutter_test.dart'; import 'package:mocktail/mocktail.dart'; +import 'package:flutter_gen/gen_l10n/app_localizations.dart'; @@ testWidgets('render launch details UI on success state', (tester) async { @@ await tester.pumpAndSettle(); - expect(find.byType(LaunchScreenBlocContent), findsNWidgets(1)); - expect(find.text('Mission Successful'), findsOneWidget); + expect(find.byType(LaunchScreenBlocContent), findsOneWidget); + final l10nEn = AppLocalizations.of( + tester.element(find.byType(LaunchScreenBlocContent)), + )!; + expect(find.text(l10nEn.missionSuccessful), findsOneWidget); }); @@ testWidgets('render launch details UI on success state localized', (tester) async { @@ await tester.pumpAndSettle(); - expect(find.byType(LaunchScreenBlocContent), findsNWidgets(1)); - expect(find.text('Missão bem-sucedida'), findsOneWidget); + expect(find.byType(LaunchScreenBlocContent), findsOneWidget); + final l10nPt = AppLocalizations.of( + tester.element(find.byType(LaunchScreenBlocContent)), + )!; + expect(find.text(l10nPt.missionSuccessful), findsOneWidget); }); @@ testWidgets('renders error', (tester) async { @@ await tester.pumpAndSettle(); - expect(find.text('Try Again'), findsOneWidget); + final l10nEn = AppLocalizations.of( + tester.element(find.byType(LaunchScreenBlocContent)), + )!; + expect(find.text(l10nEn.tryAgain), findsOneWidget); });Note: adjust the l10n getter names if they differ.
Also applies to: 62-79, 81-99, 101-113
77-79: Prefer findsOneWidget over findsNWidgets(1)Clearer intent and more idiomatic in Flutter tests.
-expect(find.byType(LaunchScreenBlocContent), findsNWidgets(1)); +expect(find.byType(LaunchScreenBlocContent), findsOneWidget);Also applies to: 97-99
81-99: Add a quick Ukrainian locale assertion to cover the newly added l10nSince this PR adds Ukrainian, add a minimal check to prevent regressions.
Example snippet:
testWidgets('render launch details UI on success state (uk)', (tester) async { when(() => launchBloc.state).thenReturn(const LaunchSuccessState(launch: mockLaunchResource)); await tester.pumpLocalizedWidgetWithBloc<LaunchBloc>( bloc: launchBloc, child: const LaunchScreenBlocContent(flightNumber: 1), locale: const Locale('uk'), ); await tester.pumpAndSettle(); final l10nUk = AppLocalizations.of( tester.element(find.byType(LaunchScreenBlocContent)), )!; expect(find.text(l10nUk.missionSuccessful), findsOneWidget); });Want me to push this test?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
integration_test/launch_test.dart(1 hunks)lib/features/launch/info_item.dart(1 hunks)lib/features/launch/widget/info_grid_widget.dart(1 hunks)lib/features/launch/widget/launch_site_card_widget.dart(1 hunks)lib/features/launch/widget/payload_card_widget.dart(1 hunks)lib/features/launch/widget/rocket_card_widget.dart(1 hunks)test/features/launch/launch_screen_test.dart(1 hunks)test/features/launch/widget/feature_chip_widget_test.dart(1 hunks)test/features/launch/widget/launch_site_card_widget_test.dart(1 hunks)test/features/launch/widget/payload_card_widget_test.dart(1 hunks)test/features/launch/widget/recovery_ships_card_widget_test.dart(1 hunks)test/features/launch/widget/stat_card_widget_test.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/features/launch/widget/rocket_card_widget.dart
- lib/features/launch/widget/info_grid_widget.dart
- lib/features/launch/widget/launch_site_card_widget.dart
- lib/features/launch/widget/payload_card_widget.dart
- integration_test/launch_test.dart
⏰ 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). (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Flutter Integration Tests (31)
- GitHub Check: App
🔇 Additional comments (10)
test/features/launch/widget/recovery_ships_card_widget_test.dart (1)
1-7: Imports and localization test harness look correct.
All required deps are present and used. Good setup for localized widget tests.test/features/launch/widget/feature_chip_widget_test.dart (1)
5-16: Good test scaffoldingClear arrange–act–assert structure and const usage; looks solid.
lib/features/launch/info_item.dart (1)
4-17: LGTM overall — simple, immutable value object fits the use case.
Fields are final and Equatable use is correct.test/features/launch/widget/payload_card_widget_test.dart (1)
9-22: LGTM: Clear setup and representative payload fixtureGood coverage for expanded/empty states with realistic data.
test/features/launch/widget/launch_site_card_widget_test.dart (3)
22-33: LGTM: title renders with localizationCovers the localized title path and locale wiring via
pumpLocalizedWidget.
35-49: LGTM: full site details renderValidates name, long name, and labeled siteId.
51-67: LGTM: minimal data behaviorGood assertion that raw
siteIddoesn’t render without the label.test/features/launch/widget/stat_card_widget_test.dart (2)
21-29: LGTM: title renders with localizationConfirms locale wiring for the title.
31-42: LGTM: full site details renderMatches expectations for name, long name, and labeled id.
test/features/launch/launch_screen_test.dart (1)
43-61: LGTM: loading-state path covered wellThe initial state test is concise and asserts the spinner correctly.
| final container = tester.widget<Container>(find.byType(Container).first); | ||
| final decoration = container.decoration as BoxDecoration; | ||
| final borderColor = (decoration.border as Border).top.color; | ||
|
|
||
| // Use the same theme primary color with alpha as in widget | ||
| final theme = ThemeData().colorScheme; | ||
| final expectedColor = theme.primary.withValues(alpha: 0.3); | ||
| expect(borderColor, equals(expectedColor)); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Scope Container lookup to the widget and use Theme.of(context); also avoid Color.withValues
Picking the first Container in the whole tree is brittle and may crash on cast. Scope the search to FeatureChipWidget. Also use withOpacity(0.3) for broader Flutter compatibility.
- final container = tester.widget<Container>(find.byType(Container).first);
- final decoration = container.decoration as BoxDecoration;
- final borderColor = (decoration.border as Border).top.color;
-
- // Use the same theme primary color with alpha as in widget
- final theme = ThemeData().colorScheme;
- final expectedColor = theme.primary.withValues(alpha: 0.3);
+ final featureChip = find.byType(FeatureChipWidget);
+ final container = tester.widget<Container>(
+ find.descendant(of: featureChip, matching: find.byType(Container)).first,
+ );
+ final decoration = container.decoration as BoxDecoration;
+ final borderColor = (decoration.border as Border).top.color;
+
+ // Use the same theme as the rendered tree
+ final theme = Theme.of(tester.element(featureChip)).colorScheme;
+ final expectedColor = theme.primary.withOpacity(0.3);
expect(borderColor, equals(expectedColor));Run to check if withValues is used elsewhere and replace with withOpacity if needed:
🏁 Script executed:
#!/bin/bash
rg -n "withValues\s*\(" -g '!**/build/**' -SLength of output: 6352
Scope Container lookup in the test and replace .withValues usage across codebase
- In test/features/launch/widget/feature_chip_widget_test.dart, scope the Container lookup to the FeatureChipWidget, use Theme.of(...) and withOpacity(0.3) instead of ThemeData().colorScheme and withValues(alpha: 0.3) (as shown in the diff).
- Replace every
.withValues(alpha: x)(and colorScheme.*.withValues(alpha: x)) in lib/features/launch/** with.withOpacity(x)` for broader Flutter compatibility; verify no remaining instances viarg -n "withValues\s*\(" -g '!**/build/**'
🤖 Prompt for AI Agents
In test/features/launch/widget/feature_chip_widget_test.dart around lines 27 to
35, the test is using a global Container lookup and
ThemeData().colorScheme.withValues(...) which is incorrect; scope the Container
lookup to the FeatureChipWidget instance (use a finder that targets the
FeatureChipWidget or its descendant Container), obtain the theme via
Theme.of(tester.element(find.byType(FeatureChipWidget))).colorScheme and replace
the .withValues(alpha: 0.3) usage with .withOpacity(0.3); then update every
occurrence of colorScheme.*.withValues(...) or .withValues(alpha: x) under
lib/features/launch/** to .withOpacity(x) and run a search (e.g., ripgrep) to
verify no remaining withValues calls outside build folders.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…lutter_bloc_app_template into feature/launch_details_screen
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/features/launch/launch_screen_test.dart (1)
41-45: Nice: stream/close stubs added to avoid MissingStubErrorThis resolves the prior bloc stream subscription issue and ensures proper teardown.
🧹 Nitpick comments (3)
test/features/launch/launch_screen_test.dart (3)
49-51: Rename test description or stub the actual initial stateThe description says “initial” but the stubbed state is
LaunchLoadingState. Either rename to “loading state” or, if anInitialstate exists, stub that instead.- 'renders CircularProgressIndicator ' - 'when launch state is initial', (tester) async { + 'renders CircularProgressIndicator when launch state is loading', (tester) async {
81-81: Use findsOneWidget for clarity
findsNWidgets(1)is equivalent but less idiomatic here.- expect(find.byType(LaunchScreenBlocContent), findsNWidgets(1)); + expect(find.byType(LaunchScreenBlocContent), findsOneWidget);
82-82: Avoid brittle assertions on hard-coded localized stringsAsserting literal text couples tests to translation wording. Prefer asserting against the localization getter (e.g., AppLocalizations) or a widget Key.
Also applies to: 102-102, 116-116
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/ci.yml(2 hunks)integration_test/launch_navigation_test.dart(1 hunks)lib/features/launch/launch_screen.dart(3 hunks)lib/models/launch/second_stage_ext.dart(1 hunks)test/features/launch/launch_screen_test.dart(1 hunks)test/features/launch/widget/stat_card_widget_test.dart(1 hunks)test/models/launch/rocket_resource_test.dart(1 hunks)test/models/second_stage_ext_test.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- lib/models/launch/second_stage_ext.dart
- .github/workflows/ci.yml
- test/models/launch/rocket_resource_test.dart
- integration_test/launch_navigation_test.dart
- test/models/second_stage_ext_test.dart
- test/features/launch/widget/stat_card_widget_test.dart
- lib/features/launch/launch_screen.dart
| rocket: mockRocket, | ||
| launchSuccess: true, | ||
| links: mockLinks, | ||
| ships: [], |
There was a problem hiding this comment.
Fix const construction: make the empty list const
mockLaunchResource is const, so all nested values must be const. ships: [] will fail to compile in a const context.
Apply this diff (adjust the generic to match the field’s element type if needed):
- ships: [],
+ ships: const [],📝 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.
| ships: [], | |
| ships: const [], |
🤖 Prompt for AI Agents
In test/features/launch/launch_screen_test.dart around line 33, the const
mockLaunchResource contains a non-const empty list (ships: []), which breaks
const construction; change it to a const empty list (e.g., ships: const []) and,
if the field has a specific element type, use a typed const (e.g., ships: const
<Ship>[]), so all nested values are const and the file compiles.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
lib/models/launch/extension/launch_ext.dart (2)
27-30: Timezone bug: label says “UTC” but the formatter doesn’t convert to UTCformatDate currently formats the given DateTime as-is. When labeling as UTC elsewhere, this yields incorrect times.
Apply:
- String formatDate({String format = 'hh:mm a, MMM yyyy'}) { + String formatDate({ + String format = 'hh:mm a, MMM yyyy', + bool useUtc = false, + }) { if (this == null) return ''; - return DateFormat(format).format(this!); + final dt = useUtc ? this!.toUtc() : this!; + return DateFormat(format).format(dt); }
81-83: Quote literal “UTC” and actually format in UTCEnsure the label matches the timezone used.
Apply:
- launchDate: launchDate.formatDate(format: 'MMMM dd, yyyy - HH:mm UTC'), + launchDate: launchDate.formatDate( + format: "MMMM dd, yyyy - HH:mm 'UTC'", + useUtc: true, + ), - staticFireDate: staticFireDate.formatDate(format: 'MMMM dd, yyyy'), + staticFireDate: staticFireDate.formatDate(format: 'MMMM dd, yyyy'),(Note: staticFireDate stays local by design; keep or switch to UTC based on UX intent.)
🧹 Nitpick comments (13)
test/features/launch/info_item_test.dart (4)
1-1: Prefer foundation import for Key-only usage.Lighter dependency;
KeyandValueKeylive influtter/foundation.dart.-import 'package:flutter/widgets.dart'; +import 'package:flutter/foundation.dart';
8-9: Deduplicate constant keys to reduce noise and improve clarity.Define shared consts once and reuse.
group('InfoItem', () { - test('supports value equality', () { - final item1 = InfoItem('Label', 'Value', const Key('testKey')); - final item2 = InfoItem('Label', 'Value', const Key('testKey')); + // Shared keys for all tests + const testKey = Key('testKey'); + const otherKey = Key('otherKey'); + + test('supports value equality', () { + final item1 = InfoItem('Label', 'Value', testKey); + final item2 = InfoItem('Label', 'Value', testKey); ... - final base = InfoItem('Label', 'Value', const Key('testKey')); + final base = InfoItem('Label', 'Value', testKey); ... - final differentLabel = InfoItem('OtherLabel', 'Value', const Key('testKey')); - final differentValue = InfoItem('Label', 'OtherValue', const Key('testKey')); - final differentKey = InfoItem('Label', 'Value', const Key('otherKey')); + final differentLabel = InfoItem('OtherLabel', 'Value', testKey); + final differentValue = InfoItem('Label', 'OtherValue', testKey); + final differentKey = InfoItem('Label', 'Value', otherKey); ... - final item = InfoItem('Label', 'Value', const Key('testKey')); + final item = InfoItem('Label', 'Value', testKey); - expect(item.props, equals(['Label', 'Value', const Key('testKey')])); + expect(item.props, equals(['Label', 'Value', testKey]));Also applies to: 16-23, 30-32
29-33: Avoid brittle assertion onpropsshape.Asserting exact
propsties tests to an internal representation. Prefer order-checked containment to stay resilient to added fields.- expect(item.props, equals(['Label', 'Value', testKey])); + expect(item.props, containsAllInOrder(['Label', 'Value', testKey]));
7-13: Optional: Add a Set semantics test to validate hash/equality in hashed collections.This guards against regressions where equals/hashCode diverge.
test('supports value equality', () { final item1 = InfoItem('Label', 'Value', testKey); final item2 = InfoItem('Label', 'Value', testKey); expect(item1, equals(item2)); expect(item1.hashCode, equals(item2.hashCode)); }); + + test('works as a Set key (deduplication)', () { + final a = InfoItem('Label', 'Value', testKey); + final b = InfoItem('Label', 'Value', testKey); + final s = {a, b}; + expect(s.length, 1); + expect(s.contains(b), isTrue); + });test/models/launch/extension/launch_ext_test.dart (1)
35-39: Stabilize locale-dependent expectationDateFormat('MMM') depends on the default locale. Pin it in tests to avoid CI flakiness.
Apply:
@@ -import 'package:flutter_test/flutter_test.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:intl/intl.dart'; @@ void main() { + setUpAll(() { + Intl.defaultLocale = 'en_US'; + });lib/models/launch/links_resource.dart (1)
12-16: Consider deep-equality for flickrImages in EquatableIf UI state equality relies on LinksResource, list identity vs. content changes may cause unnecessary rebuilds or missed updates.
Option A (simple): hash the list contents in props
+import 'package:collection/collection.dart'; @@ @override List<Object?> get props => [ @@ - flickrImages, + const DeepCollectionEquality().hash(flickrImages), ];Option B: store an unmodifiable copy and compare by contents where needed.
Also applies to: 23-26, 35-38
lib/features/launch/launch_screen.dart (5)
124-131: Prefer AppBar/SliverAppBar systemOverlayStyle over global AnnotatedRegionAvoids white status bar icons on light surfaces when collapsed and scopes the effect to the app bar.
- return AnnotatedRegion<SystemUiOverlayStyle>( - value: const SystemUiOverlayStyle( - statusBarColor: Colors.transparent, - statusBarIconBrightness: Brightness.light, - ), - child: Scaffold( + return Scaffold( extendBodyBehindAppBar: true, body: CustomScrollView( @@ - SliverAppBar( + SliverAppBar( + systemOverlayStyle: SystemUiOverlayStyle.light,If you keep AnnotatedRegion, ensure the collapsed AppBar icon contrast is acceptable on
colorScheme.surface.Also applies to: 134-141
122-123: Make fallback list const to avoid new allocsMinor, but keeps types crisp and avoids reallocation.
- final images = widget.resource.links?.flickrImages ?? []; + final images = widget.resource.links?.flickrImages ?? const <String>[];
193-209: Disable paging when ≤1 image and reduce image flickerImproves UX for single/no-image cases and prevents frame jumps on reloads.
- PageView.builder( + PageView.builder( controller: _pageController, + physics: images.length <= 1 + ? const NeverScrollableScrollPhysics() + : const BouncingScrollPhysics(), onPageChanged: (index) { setState(() => _currentImageIndex = index); }, itemCount: images.length, itemBuilder: (context, index) { return Stack( fit: StackFit.expand, children: [ - Image.network( + Image.network( images[index], fit: BoxFit.cover, + gaplessPlayback: true, errorBuilder: (context, error, stackTrace) => const SizedBox.shrink(), ),
280-301: Add semantics for page indicatorsImprove accessibility by exposing current position to screen readers.
- child: Row( + child: Semantics( + label: 'Image carousel position', + value: '${_currentImageIndex + 1} of ${images.length}', + child: Row( mainAxisAlignment: MainAxisAlignment.center, children: images.asMap().entries.map((entry) { @@ - }).toList(), - ), + }).toList(), + ), + ),
382-391: Consider rendering Timeline when only one date is presentCurrently requires both
launchDateandstaticFireDate. UX may benefit from showing whichever exists.- if (widget.resource.launchDate != null && - widget.resource.staticFireDate != null) ...[ + if (widget.resource.launchDate != null || + widget.resource.staticFireDate != null) ...[ const SizedBox(height: 20), TimelineCardWidget( - launchDate: widget.resource.launchDate!, - staticFireDate: widget.resource.staticFireDate!, + launchDate: widget.resource.launchDate, + staticFireDate: widget.resource.staticFireDate, launchSuccess: widget.resource.launchSuccess ?? false, ),Ensure
TimelineCardWidgethandles nulls accordingly.test/models/launch_ext_model_tests.dart (2)
218-223: Avoid time-based test flakiness by freezing "now"Using
DateTime.now()can flake around midnight/DST. Freeze time withpackage:clock.- final date = DateTime.now().add(const Duration(days: 5)); - final launchDays = date.formatDateWithDays(); + final fixedNow = DateTime.utc(2025, 01, 01, 12); + final date = fixedNow.add(const Duration(days: 5)); + final launchDays = withClock(Clock.fixed(fixedNow), () => date.formatDateWithDays()); @@ - final date = DateTime.now().subtract(const Duration(days: 5)); - final launchDays = date.formatDateWithDays(); + final fixedNow = DateTime.utc(2025, 01, 01, 12); + final date = fixedNow.subtract(const Duration(days: 5)); + final launchDays = withClock(Clock.fixed(fixedNow), () => date.formatDateWithDays());Add imports:
import 'package:clock/clock.dart';Also applies to: 226-231
165-174: Prefer constructing real network models over Mockito for data classesIf
NetworkRocketModelis a Freezed/data class, create it via factory/JSON instead of mocking, reducing reliance on dynamic stubbing.Example:
final networkRocketModel = NetworkRocketModel(name: 'Falcon 9', type: 'Reusable');If not feasible, keep the mock.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
Makefile(1 hunks)lib/data/network/model/launch/network_launch_model.dart(3 hunks)lib/data/network/model/launch/network_launch_model.freezed.dart(28 hunks)lib/data/network/model/launch/network_launch_model.g.dart(3 hunks)lib/features/launch/launch_screen.dart(3 hunks)lib/models/launch/extension/launch_ext.dart(6 hunks)lib/models/launch/links_resource.dart(2 hunks)test/data/network/model/network_launch_full_model_test.dart(3 hunks)test/features/launch/info_item_test.dart(1 hunks)test/models/launch/extension/launch_ext_test.dart(1 hunks)test/models/launch_ext_model_tests.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/data/network/model/launch/network_launch_model.g.dart
- lib/data/network/model/launch/network_launch_model.freezed.dart
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-11T19:29:38.429Z
Learnt from: ashtanko
PR: ashtanko/flutter_bloc_app_template#30
File: lib/di/app_bloc_providers.dart:21-26
Timestamp: 2025-08-11T19:29:38.429Z
Learning: In the flutter_bloc_app_template project, immediate event dispatch using cascade notation (..add()) in BlocProvider creation is the preferred pattern. The blocs should dispatch initial events immediately upon creation rather than deferring them to widget initialization or post-frame callbacks.
Applied to files:
lib/features/launch/launch_screen.dart
⏰ 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). (2)
- GitHub Check: App
- GitHub Check: Flutter Integration Tests (31)
🔇 Additional comments (14)
Makefile (1)
8-8: EnsureDARTandFLUTTERenv-vars override correct executables
Unable to verify SDK resolution in this environment (bothdartandfluttercommands not found); please manually confirm that usingDART/FLUTTERvariables in the Makefile picks up the intended Dart/Flutter SDKs for all developer setups and CI pipelines.test/features/launch/info_item_test.dart (2)
7-13: LGTM: Equality and hashCode assertions are solid.Covers the core Equatable contract well.
15-27: Confirm: Should Key participate in value equality?Including
keyinpropsmeans identical label/value with a different key are not equal. This affects diffing (e.g., Set/Map membership, list de-duping). Verify this is intentional for your UI model; otherwise consider excludingkeyfromprops.test/models/launch/extension/launch_ext_test.dart (2)
8-14: LGTM: day-difference behavior matches current implementationAsserting "4" for now + 5 days aligns with floor semantics in formatDateWithDays().
76-101: LGTM: network → resource mapping assertions are solidCoverage for id/flightNumber/time/links/rocket looks good.
lib/data/network/model/launch/network_launch_model.dart (2)
36-38: LGTM: staged rocket fields are correctly added and keyedfirst_stage and second_stage wiring looks correct for Freezed/JSON.
54-58: LGTM: extended links fields (reddit/video/presskit/flickr) match API keysTypes and JsonKey names align with SpaceX v3 schema patterns.
lib/models/launch/extension/launch_ext.dart (1)
66-68: LGTM: launchDays + launchTime mappingConsistent use of the new formatter; once the UTC fix lands, this remains valid.
lib/features/launch/launch_screen.dart (4)
26-35: Good use of immediate event dispatch on Bloc creationMatches the project preference to dispatch initial events via cascade (
..add(...)) in the provider.
253-272: Good guard against empty mission patch URLAvoids futile network calls when the URL is empty.
111-116: Properly disposing controllersPrevents leaks; good lifecycle hygiene.
51-71: Dart SDK constraint already supports Dart 3 pattern matching pubspec.yaml specifiessdk: ">=3.2.0 <4.0.0", satisfying the requirement.test/models/launch_ext_model_tests.dart (2)
254-260: If labeling output as UTC, ensure the time is actually converted to UTCThe test expects '... 14:30 UTC' from a local
DateTime(2025, 12, 25, 14, 30). Verify thatformatDate()converts to UTC before appending 'UTC'; otherwise results vary by environment.Option A: pass a UTC
DateTime.utc(...)in the test. Option B: adjust implementation to call.toUtc()before formatting when'UTC'is part of the format.
48-128: Solid coverage of nested mappingsGood assertions for rocket, links, site, and date formatting in
LaunchFullResource.toResource().
| color: Colors.black.withValues(alpha: 0.6), | ||
| borderRadius: BorderRadius.circular(8), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix compile errors: Color.withValues(...) is not a Flutter API
Use withOpacity(...) (or withAlpha(...)) instead. Current code won’t compile on stable Flutter.
Apply this diff:
- color: Colors.black.withValues(alpha: 0.6),
+ color: Colors.black.withOpacity(0.6),
@@
- color: Colors.black.withValues(alpha: 0.5),
+ color: Colors.black.withOpacity(0.5),
@@
- Colors.black.withValues(alpha: 0.20),
- Colors.black.withValues(alpha: 0.70),
+ Colors.black.withOpacity(0.20),
+ Colors.black.withOpacity(0.70),
@@
- color: Colors.white.withValues(alpha: 0.1),
+ color: Colors.white.withOpacity(0.1),
@@
- : Colors.white.withValues(alpha: 0.4),
+ : Colors.white.withOpacity(0.4),
@@
- color: colorScheme.primaryContainer
- .withValues(alpha: 0.9),
+ color:
+ colorScheme.primaryContainer.withOpacity(0.9),
@@
- BoxShadow(
- color: Colors.black.withValues(alpha: 0.2),
+ BoxShadow(
+ color: Colors.black.withOpacity(0.2),To catch any other occurrences:
Also applies to: 158-159, 216-218, 248-252, 295-297, 311-316
🏁 Script executed:
#!/bin/bash
rg -n "withValues\(" -S libLength of output: 6216
Replace all .withValues(alpha: …) calls with .withOpacity(…)
The non-existent withValues API causes compile errors on stable Flutter. A global search (rg -n "withValues\(" -S lib) uncovered occurrences in:
- lib/features/launch/launch_screen.dart
- lib/features/launch/widget/feature_chip_widget.dart
- lib/features/launch/widget/launch_site_card_widget.dart
- lib/features/launch/widget/payload_card_widget.dart
- lib/features/launch/widget/stat_card_widget.dart
- lib/features/launch/widget/mission_overview_card_widget.dart
- lib/features/launch/widget/status_section_widget.dart
- lib/features/launch/widget/recovery_ships_card_widget.dart
- lib/features/launch/widget/timeline_widgets.dart
- lib/features/launch/widget/links_card_widget.dart
- lib/features/launch/widget/rocket_card_widget.dart
- lib/features/launch/widget/animated_link_button_widget.dart
In each case, replace:
.someColor.withValues(alpha: 0.x)with:
.someColor.withOpacity(0.x)🤖 Prompt for AI Agents
In lib/features/launch/launch_screen.dart around lines 145 to 146, the code
calls a non-existent Colors.withValues(alpha: …) API which causes compile
errors; replace .withValues(alpha: x) with .withOpacity(x). Apply the same
replacement for all other occurrences in the launch feature files listed in the
review (search project for "withValues(") so every .withValues(alpha: 0.x)
becomes .withOpacity(0.x).
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Summary by CodeRabbit
New Features
Localization
Models & API
Tests
Chores