Implement cores #42
Implement cores #42
Conversation
WalkthroughAdds end-to-end “cores” feature: network service/data source, domain repository, Bloc (events/states/logic), UI screen and widgets, DI wiring, navigation entry, utilities, and localizations. Refactors core/stage models, introduces mission and stage-core types, updates resources and extensions, and adds comprehensive tests (unit, widget, integration) plus fixtures. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as CoresScreen
participant Bloc as CoresBloc
participant Repo as CoresRepository
participant DS as CoresDataSource
participant Svc as CoresService
participant API as SpaceX API
User->>UI: Open tab / pull to refresh / filter
UI->>Bloc: CoresEvent.load/refresh/filter
Bloc->>Repo: getCores(hasId, limit, offset)
Repo->>DS: getCores(...)
DS->>Svc: GET /cores?id&limit&offset
Svc->>API: HTTP request
API-->>Svc: JSON cores[]
Svc-->>DS: List<NetworkCoreModel>
DS-->>Repo: ApiResult.success(cores)
Repo->>Repo: map to List<CoreResource>
Repo-->>Bloc: List<CoreResource>
Bloc->>Bloc: filter (search/status)
Bloc-->>UI: CoresState.success/empty/notFound
alt Error
API-->>Svc: Error
Svc-->>DS: throws
DS-->>Repo: ApiResult.error(msg)
Repo-->>Bloc: throws Exception
Bloc-->>UI: CoresState.error(message)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (15)
🚧 Files skipped from review as they are similar to previous changes (10)
👮 Files not reviewed due to content moderation or server errors (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (10)
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 #42 +/- ##
==========================================
+ Coverage 84.90% 85.68% +0.77%
==========================================
Files 150 164 +14
Lines 3194 3549 +355
==========================================
+ Hits 2712 3041 +329
- Misses 482 508 +26 ☔ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/models/stage/first_stage_ext.dart (1)
7-9: Apply null-aware chaining to.toList()after?.map()in mapping extensions
In the following files, replace patterns likemissions: missions?.map((m) => m.toResource()).toList(),with
- missions: missions?.map((m) => m.toResource()).toList(), + missions: missions?.map((m) => m.toResource())?.toList(),– lib/models/core/core_ext.dart (missions)
– lib/models/rocket/rocket_ext.dart (payloadWeights)
– lib/models/stage/first_stage_ext.dart (cores)
– lib/models/stage/second_stage_ext.dart (payloads – includegrowable: falseas before)If downstream code requires a non-null list, append
?? const []after?.toList(). Do not edit generated*.g.dartfiles.
🧹 Nitpick comments (32)
test/data/network/fixtures/cores/cores.json (1)
1-62: Broaden fixture variance for stronger parsing tests.Consider adding at least one item with: non-zero reuse/rtls/asds counts, water_landing: true, and a different status value to exercise edge cases.
lib/models/stage/stage_core_resource.dart (1)
4-43: Consider enums for landingType/landingVehicle.If the set of values is bounded, using enums (with mapping extensions) prevents typos and eases refactors.
lib/data/network/model/core/network_core_model.dart (2)
6-22: Call nested toJson explicitly to be safe.Annotate the factory with JsonSerializable(explicitToJson: true) so nested missions serialize explicitly (minor, but future-proof).
Apply this minimal diff:
@freezed abstract class NetworkCoreModel with _$NetworkCoreModel { - const factory NetworkCoreModel({ + @JsonSerializable(explicitToJson: true) + const factory NetworkCoreModel({ @JsonKey(name: 'core_serial') String? coreSerial, int? block, String? status, @JsonKey(name: 'original_launch') String? originalLaunch, @JsonKey(name: 'original_launch_unix') int? originalLaunchUnix, List<NetworkMission>? missions, @JsonKey(name: 'reuse_count') int? reuseCount, @JsonKey(name: 'rtls_attempts') int? rtlsAttempts, @JsonKey(name: 'rtls_landings') int? rtlsLandings, @JsonKey(name: 'asds_attempts') int? asdsAttempts, @JsonKey(name: 'asds_landings') int? asdsLandings, @JsonKey(name: 'water_landing') bool? waterLanding, String? details, }) = _NetworkCoreModel;
30-41: Optional: mirror annotation on NetworkMission.Not strictly required, but adding JsonSerializable keeps annotations consistent.
@freezed abstract class NetworkMission with _$NetworkMission { - const factory NetworkMission({ + @JsonSerializable() + const factory NetworkMission({ String? name, int? flight, }) = _NetworkMission;lib/models/stage/first_stage_resource.dart (1)
7-13: Consider enforcing immutability of the cores list.To prevent accidental external mutation, wrap the list or document expectations. Example:
class FirstStageResource extends Equatable { const FirstStageResource({this.cores}); - final List<StageCoreResource>? cores; + final List<StageCoreResource>? cores; + + // Optionally expose an unmodifiable view via a getter: + // List<StageCoreResource>? get coresUnmodifiable => + // cores == null ? null : List<StageCoreResource>.unmodifiable(cores!);test/data/network/model/stage/network_stage_core_model_test.dart (1)
48-55: Add a guard to ensure original remains unchanged in copyWith test.Minor addition to assert immutability of the source.
final updated = model.copyWith(coreSerial: 'B1014', flight: 2); expect(updated.coreSerial, 'B1014'); expect(updated.flight, 2); expect(updated.block, model.block); + expect(model.coreSerial, 'B1013'); // original unchanged + expect(model.flight, 1);test/models/launch/rocket_resource_test.dart (1)
61-66: Prefer explicit typing or an emptiness matcher in comparisons.Tiny clarity nit for future readers.
- const firstStage2 = FirstStageResource(cores: [ - StageCoreResource(coreSerial: 'B1013'), - StageCoreResource(coreSerial: 'B1014'), - ]); + const firstStage2 = FirstStageResource(cores: <StageCoreResource>[ + StageCoreResource(coreSerial: 'B1013'), + StageCoreResource(coreSerial: 'B1014'), + ]);test/models/stage/first_stage_ext_test.dart (2)
37-66: Add a test for null cores on the network model.Current tests cover empty list; add a case where
coresis null to verify null-safe mapping after fixing the extension.group('FirstStageExt.toResource', () { @@ test('should handle null cores correctly', () { - final emptyModel = const NetworkFirstStageModel(cores: []); - final resource = emptyModel.toResource(); + const emptyModel = NetworkFirstStageModel(cores: []); + final resourceEmpty = emptyModel.toResource(); - expect(resource.cores, <dynamic>[]); + expect(resourceEmpty.cores, isEmpty); }); + + test('should handle null cores on network model', () { + const nullModel = NetworkFirstStageModel(cores: null); + final resourceNull = nullModel.toResource(); + expect(resourceNull.cores, isNull); + });
72-73: Use an emptiness matcher instead of a typed literal list.Improves readability and avoids dynamic.
- expect(resource.cores, <dynamic>[]); + expect(resource.cores, isEmpty);test/models/launch/rocket_resource_ext_test.dart (1)
41-43: List equality assertion is fine; consider an explicit length check.Optional: also assert length and order for clarity.
Apply:
expect(resource.firstStage?.cores, [ const StageCoreResource(coreSerial: 'B1013'), const StageCoreResource(coreSerial: 'B1014'), ]); + expect(resource.firstStage?.cores, hasLength(2));test/models/stage/stage_core_ext_test.dart (1)
8-20: Type the json map explicitly.Minor: make the literal type explicit to avoid inference to Map<String, Object?> on newer SDKs.
Apply:
- final json = { + final Map<String, dynamic> json = { 'core_serial': 'B1013', 'flight': 1, 'block': 1, 'gridfins': true, 'legs': true, 'reused': false, 'land_success': true, 'landing_intent': true, 'landing_type': 'Ocean', 'landing_vehicle': null };lib/data/network/model/stage/network_first_stage_model.g.dart (1)
18-22: ToJson writes cores directly.If you need nested toJson invoked explicitly, ensure @JsonSerializable(explicitToJson: true) is set on the parent model; otherwise this is fine for Freezed defaults.
test/data/network/model/stage/network_first_stage_model_test.dart (1)
67-79: Make thecoreslist literalconst
Make thecoreslist literalconstto avoid an extra allocation.- cores: [ + cores: const [ NetworkStageCoreModel( coreSerial: 'B9999', flight: 9, block: 9, gridfins: false, legs: false, reused: false, landSuccess: null, landingIntent: null, landingType: null, landingVehicle: null, ), ],lib/data/network/data_source/cores_network_data_source.dart (2)
33-36: Simplify error path; avoid redundant Future.value and type the catch
asyncmethods don’t needFuture.value. Also prefercatch (Object e)to avoid implicitdynamic.- return ApiResult.success(list); - } catch (e) { - return Future.value(ApiResult.error(e.toString())); + return ApiResult.success(list); + } catch (Object e) { + return ApiResult.error(e.toString()); }
44-46: Same simplification for getCore error pathMirror the change here for consistency.
- } catch (e) { - return Future.value(ApiResult.error(e.toString())); + } catch (Object e) { + return ApiResult.error(e.toString()); }lib/data/network/model/stage/network_first_stage_model.dart (3)
6-16: Ensure nested lists serialize cleanly to JSONAdd
@JsonSerializable(explicitToJson: true)socores(list of objects) are emitted as pure JSON maps without relying onjsonEncode's implicit toJson calls.Apply:
@freezed abstract class NetworkFirstStageModel with _$NetworkFirstStageModel { - const factory NetworkFirstStageModel({ + @JsonSerializable(explicitToJson: true) + const factory NetworkFirstStageModel({ List<NetworkStageCoreModel>? cores, }) = _NetworkFirstStageModel;
18-37: Trim @jsonkey noise with fieldRenameUsing
@JsonSerializable(fieldRename: FieldRename.snake)reduces boilerplate and keeps names consistent.Apply:
@freezed abstract class NetworkStageCoreModel with _$NetworkStageCoreModel { - const factory NetworkStageCoreModel({ - @JsonKey(name: 'core_serial') String? coreSerial, - int? flight, - int? block, - bool? gridfins, - bool? legs, - bool? reused, - @JsonKey(name: 'land_success') bool? landSuccess, - @JsonKey(name: 'landing_intent') bool? landingIntent, - @JsonKey(name: 'landing_type') String? landingType, - @JsonKey(name: 'landing_vehicle') String? landingVehicle, - }) = _NetworkStageCoreModel; + @JsonSerializable(fieldRename: FieldRename.snake) + const factory NetworkStageCoreModel({ + String? coreSerial, + int? flight, + int? block, + bool? gridfins, + bool? legs, + bool? reused, + bool? landSuccess, + bool? landingIntent, + String? landingType, + String? landingVehicle, + }) = _NetworkStageCoreModel;
18-37: Optional: add brief docs on booleansShort comments for fields like
gridfins,legs,reused,landSuccessincrease clarity on semantics (hardware presence vs. outcome flags).test/models/core/core_resource_test.dart (3)
23-37: DRY up duplicate core fixtures
core1andcore2are identical. Prefer a shared fixture/builder to reduce maintenance overhead.Example:
CoreResource buildCoreMerlin2C() => const CoreResource( coreSerial: 'Merlin2C', block: 1, status: 'lost', originalLaunch: '2008-09-28T23:15:00.000Z', originalLaunchUnix: 1222643700, missions: [MissionResource(name: 'RatSat', flight: 4)], reuseCount: 0, rtlsAttempts: 0, rtlsLandings: 0, asdsAttempts: 0, asdsLandings: 0, waterLanding: false, details: 'Some details', );Also applies to: 39-53
66-82: Props assertion is brittle to internal orderAsserting the full
propslist ties tests to internal ordering. Prefer focused assertions (e.g., contains/equals per field) to reduce churn.Example:
expect(core1.props.first, 'Merlin2C'); expect(core1.props, contains(const MissionResource(name: 'RatSat', flight: 4)));
84-89: Strengthen immutability checkThe current test only reads a field. Consider asserting that
missionsis unmodifiable at runtime.Apply:
- test('is immutable', () { - // trying to reassign a const field will cause compile-time error, - // so here we just verify object identity - const modified = CoreResource(coreSerial: 'Merlin2C'); - expect(modified.coreSerial, 'Merlin2C'); - }); + test('is immutable', () { + expect(() => core1.missions!.add(const MissionResource(name: 'X', flight: 1)), + throwsUnsupportedError); + });lib/data/network/model/core/network_core_model.g.dart (1)
28-43: Add explicitToJson to NetworkCoreModel annotation
Update the source in lib/data/network/model/core/network_core_model.dart to:@freezed class NetworkCoreModel with _$NetworkCoreModel { @JsonSerializable(explicitToJson: true, fieldRename: FieldRename.snake) const factory NetworkCoreModel({ … }) = _NetworkCoreModel; factory NetworkCoreModel.fromJson(Map<String, dynamic> json) => _$NetworkCoreModelFromJson(json); }Then rerun build_runner to regenerate the .g.dart so missions are serialized via toJson instead of relying on jsonEncode.
lib/data/network/service/cores/cores_service.g.dart (3)
24-35: Questionableidquery param default — likely unintended API call shape
iddefaults totrue, so every request adds?id=true. Unless the SpaceX v3 /cores endpoint actually supports this flag, this can cause filtering or be ignored unpredictably. Prefer not sending it unless explicitly requested.Because this file is generated, adjust the interface instead (then re-generate). For example in cores_service.dart:
- Future<List<NetworkCoreModel>> fetchCores({@Query('id') bool? hasId = true, @Query('limit') int? limit, @Query('offset') int? offset}); + // Default to null so the param is omitted unless explicitly provided. + Future<List<NetworkCoreModel>> fetchCores({@Query('id') bool? hasId, @Query('limit') int? limit, @Query('offset') int? offset});
47-58: Network errors aren’t logged via errorLogger; only parse errors areExceptions thrown by
_dio.fetchwon’t hit this try/catch. If you want uniform logging, add a Dio error interceptor at composition time rather than editing generated code.Example (configure where Dio is created):
_dio.interceptors.add(InterceptorsWrapper( onError: (e, h) { errorLogger?.logError(e, e.stackTrace, e.requestOptions); h.next(e); }, ));
63-77: Path param safetyIf
coreSerialcan contain reserved URL chars, consider ensuring encoding at the interface level (Retrofit by default URL-encodes path params; if you’ve setencoded: true, remove it). Double-check your annotation, regenerate if needed.lib/models/core/core_resource.dart (2)
22-35: Enforce immutability ofmissionsLists are mutable even when final. Wrap with
UnmodifiableListViewin the ctor or expose an unmodifiable view.+import 'dart:collection'; ... const CoreResource({ this.coreSerial, this.block, this.status, this.originalLaunch, this.originalLaunchUnix, - this.missions, + List<MissionResource>? missions, this.reuseCount, this.rtlsAttempts, this.rtlsLandings, this.asdsAttempts, this.asdsLandings, this.waterLanding, this.details, - }); + }) : missions = missions == null ? null : UnmodifiableListView(missions);
24-27: Consider DateTime fororiginalLaunchIf consumers parse this string, promote it to
DateTime?in the resource layer to avoid repeated parsing downstream.lib/data/network/model/core/network_core_model.freezed.dart (3)
20-38: Consider stronger types for status and date.
- status as enum (e.g., CoreStatus) instead of String.
- originalLaunch as DateTime? (with a converter) vs String? to avoid downstream parsing.
If you want, I can sketch the source (non-generated) changes.
59-77: Deep collection equality is fine; watch perf on large lists.DeepCollectionEquality on missions is OK; if missions grow large, consider an identity or id-based comparison in the domain model to reduce cost.
Also applies to: 86-96, 98-101
451-466: Defensive copy missions at construction (in source).The getter wraps with EqualUnmodifiableListView, but the backing list can still be externally mutated before read. In the source model, copy into an unmodifiable list at construction.
Example (in network_core_model.dart, not here):
factory NetworkCoreModel({ List<NetworkMission>? missions, ... }) = _NetworkCoreModel; const _NetworkCoreModel({ List<NetworkMission>? missions, ... }) : _missions = missions == null ? null : List<NetworkMission>.unmodifiable(missions), ... ```<!-- review_comment_end --> </blockquote></details> <details> <summary>lib/data/network/model/stage/network_first_stage_model.freezed.dart (2)</summary><blockquote> `242-256`: **Defensive copy cores at construction (in source).** Mirror the missions advice: create an unmodifiable copy for _cores in the source model to prevent external mutation. <!-- review_comment_end --> --- `329-399`: **landingType would benefit from an enum.** landingType as String? is flexible but brittle. Consider an enum with a JsonConverter to constrain values. <!-- review_comment_end --> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between bdae65af678945c84d6236ad0e2bb260273adaaf and cee965807244ce488516863ef76a97958ff66ce6. </details> <details> <summary>📒 Files selected for processing (30)</summary> * `lib/data/network/data_source/cores_network_data_source.dart` (1 hunks) * `lib/data/network/model/core/network_core_model.dart` (1 hunks) * `lib/data/network/model/core/network_core_model.freezed.dart` (12 hunks) * `lib/data/network/model/core/network_core_model.g.dart` (1 hunks) * `lib/data/network/model/stage/network_first_stage_model.dart` (1 hunks) * `lib/data/network/model/stage/network_first_stage_model.freezed.dart` (9 hunks) * `lib/data/network/model/stage/network_first_stage_model.g.dart` (2 hunks) * `lib/data/network/service/cores/cores_service.dart` (1 hunks) * `lib/data/network/service/cores/cores_service.g.dart` (1 hunks) * `lib/models/core/core_ext.dart` (1 hunks) * `lib/models/core/core_resource.dart` (1 hunks) * `lib/models/stage/first_stage_ext.dart` (1 hunks) * `lib/models/stage/first_stage_resource.dart` (1 hunks) * `lib/models/stage/stage_core_ext.dart` (1 hunks) * `lib/models/stage/stage_core_resource.dart` (1 hunks) * `test/data/network/fixtures/cores/core.json` (1 hunks) * `test/data/network/fixtures/cores/core2.json` (1 hunks) * `test/data/network/fixtures/cores/cores.json` (1 hunks) * `test/data/network/model/core/network_core_model_test.dart` (1 hunks) * `test/data/network/model/stage/network_first_stage_model_test.dart` (1 hunks) * `test/data/network/model/stage/network_stage_core_model_test.dart` (1 hunks) * `test/data/network/service/cores/cores_service_test.dart` (1 hunks) * `test/models/core/core_ext_test.dart` (1 hunks) * `test/models/core/core_resource_test.dart` (1 hunks) * `test/models/launch/rocket_resource_ext_test.dart` (3 hunks) * `test/models/launch/rocket_resource_test.dart` (2 hunks) * `test/models/stage/first_stage_ext_test.dart` (3 hunks) * `test/models/stage/first_stage_resource_test.dart` (2 hunks) * `test/models/stage/stage_core_ext_test.dart` (1 hunks) * `test/models/stage/stage_core_resource_test.dart` (1 hunks) </details> <details> <summary>⏰ 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)</summary> * GitHub Check: Codacy Static Code Analysis * GitHub Check: App * GitHub Check: Flutter Integration Tests (31) </details> <details> <summary>🔇 Additional comments (47)</summary><blockquote> <details> <summary>test/data/network/fixtures/cores/core.json (1)</summary><blockquote> `1-20`: **Fixture shape matches NetworkCoreModel. LGTM.** Keys and types align with the updated network model (snake_case, nested missions). </blockquote></details> <details> <summary>test/data/network/fixtures/cores/core2.json (1)</summary><blockquote> `1-20`: **LGTM.** Consistent with core.json and the model; good coverage for parsing. </blockquote></details> <details> <summary>lib/models/stage/stage_core_resource.dart (1)</summary><blockquote> `4-43`: **Solid immutable value object.** Looks consistent with Equatable usage elsewhere. </blockquote></details> <details> <summary>lib/data/network/model/core/network_core_model.dart (1)</summary><blockquote> `8-22`: **Model field overhaul looks correct.** Snake_case mappings, nested NetworkMission, and added counters align with fixtures. </blockquote></details> <details> <summary>lib/models/stage/first_stage_ext.dart (1)</summary><blockquote> `3-3`: **Import swap is correct.** Switching to stage_core_ext.dart aligns with the new stage-scoped core mapping. </blockquote></details> <details> <summary>lib/models/stage/first_stage_resource.dart (1)</summary><blockquote> `3-10`: **Type migration to StageCoreResource looks good.** Field type and import are consistent with the stage-core split. </blockquote></details> <details> <summary>test/models/stage/stage_core_resource_test.dart (1)</summary><blockquote> `1-73`: **Solid coverage for value semantics.** Equality, hashCode, and props order assertions are clear and thorough. </blockquote></details> <details> <summary>test/data/network/model/stage/network_stage_core_model_test.dart (1)</summary><blockquote> `6-39`: **JSON round-trip and field mapping look correct.** Snake_case to camelCase mapping validated; toJson round-trip matches input. </blockquote></details> <details> <summary>test/models/stage/first_stage_resource_test.dart (1)</summary><blockquote> `2-57`: **LGTM on type update and deep equality.** Tests correctly reflect StageCoreResource and cover null handling. </blockquote></details> <details> <summary>lib/models/stage/stage_core_ext.dart (1)</summary><blockquote> `4-19`: **1:1 field mapping looks correct.** Straightforward conversion from NetworkStageCoreModel to StageCoreResource. </blockquote></details> <details> <summary>test/models/launch/rocket_resource_test.dart (1)</summary><blockquote> `10-17`: **StageCoreResource substitution is consistent.** Nested equality remains valid with the new type. </blockquote></details> <details> <summary>test/models/launch/rocket_resource_ext_test.dart (2)</summary><blockquote> `9-9`: **Import switch to StageCoreResource looks right.** Aligns tests with the new stage-core split. --- `19-21`: **Using NetworkStageCoreModel for first-stage cores is consistent with the new model.** Nice targeted fixture updates. </blockquote></details> <details> <summary>test/data/network/service/cores/cores_service_test.dart (6)</summary><blockquote> `22-46`: **Good happy-path coverage for getCores.** Verifies delegation, type, and non-empty data. --- `47-60`: **Error path assertion looks good.** Covers exception propagation to ApiResult.Error. --- `61-80`: **Empty-list case covered.** Solid edge-case test. --- `82-107`: **fetchCore success path is correct.** Verifies call, wrapper type, and payload. --- `127-140`: **fetchCore error path covered.** Interaction verification and error wrapping look good. --- `16-20`: **registerFallbackValue(Uri()) is invalid and unnecessary here.** Uri has no default constructor; this line will fail to compile and isn’t used (no any<Uri>() in mocks). Remove it. Apply: ```diff setUp(() async { - registerFallbackValue(Uri()); service = MockCoresService(); dataSource = CoresNetworkDataSource(service); });Likely an incorrect or invalid review comment.
test/models/stage/stage_core_ext_test.dart (2)
21-42: Full-field mapping test reads well.Covers all properties and equality.
44-76: Null-field mapping test is thorough.Good guard against regressions.
lib/data/network/service/cores/cores_service.dart (2)
12-17: Confirm query params and naming against the backend.Is @query('id') (bool hasId) actually supported by the API? If not, drop or rename to a real filter; otherwise document expected semantics.
19-22: Endpoint shape looks correct.Signature and path params align with Retrofit conventions.
lib/data/network/model/stage/network_first_stage_model.g.dart (2)
13-16: Deserializing cores via NetworkStageCoreModel.fromJson is correct.Matches the new nested model.
24-52: Stage core JSON mapping LGTM.Snake_case to camelCase keys are handled consistently.
lib/models/core/core_ext.dart (1)
25-29: LGTM: mission mapping is clean and directStraight mapping of fields looks correct.
lib/data/network/data_source/cores_network_data_source.dart (1)
5-13: Interface and implementation shape look goodClear API, sensible params, and straightforward delegation to service.
Also applies to: 15-25
lib/data/network/model/stage/network_first_stage_model.dart (1)
9-10: No action required; all downstream consumers and mappings forNetworkFirstStageModel.coreshave been updated.test/models/core/core_resource_test.dart (2)
5-20: MissionResource tests look solidEquality and props checks are accurate and minimal.
60-64: Equality test LGTMCovers equal and non-equal cases cleanly.
lib/data/network/model/core/network_core_model.g.dart (2)
9-26: fromJson changes LGTMNew fields map correctly; numeric casts use
toInt()safely; nestedmissionsparse viaNetworkMission.fromJson.
45-55: NetworkMission serialization looks correctName/flight mapped as expected; helper functions generated.
lib/data/network/service/cores/cores_service.g.dart (2)
89-100: Response type handling looks correctStream type coercion matches Retrofit generator patterns.
102-114: Base URL resolution LGTMGracefully handles absolute/relative base URLs.
lib/models/core/core_resource.dart (2)
6-20: API surface change LGTMNew fields align with v3 core JSON (status, originalLaunch, missions, attempts/landings, etc.). Immutability via
constctor is good.
55-66: MissionResource is fineSimple value object; Equatable works well here.
test/data/network/model/core/network_core_model_test.dart (3)
13-28: Good, const model with nested missionsCovers nested parsing and equality/hash for the freezed model.
68-72: Round-trip JSON assertion LGTMValidates encoder/decoder symmetry with snake_case keys.
30-47: No action needed:blockis included in the generatedtoJsoneven when null.test/models/core/core_ext_test.dart (2)
94-111: Multi-mission mapping test looks solidAsserts element-wise mission mapping; deep list equality via
equalsmatcher will catch order/content issues.
9-27: The verification script has been queued. I’ll review the definition ofNetworkCoreModelto confirm whether its equality operator properly handles deep list comparison formissions.lib/data/network/model/core/network_core_model.freezed.dart (3)
485-491: Immutable view for missions: LGTM.EqualUnmodifiableListView exposure is correct.
20-38: Old core fields removed from NetworkCoreModel; no residual references found
Verified thatgridfins,legs,reused,landSuccess,landingIntent,landingTypeandlandingVehicleonly appear onNetworkStageCoreModeland the full‐launchNetworkCoretype, with no usages onNetworkCoreModel.
690-706: Missions JSON keys (“name”, “flight”) align with both fixtures and generated service models—no changes needed.lib/data/network/model/stage/network_first_stage_model.freezed.dart (3)
711-752: JSON key mapping for stage core fields: LGTM.Snake_case keys (land_success, landing_intent, landing_type, landing_vehicle) correctly mapped.
239-291: Tests and fixtures already coverland_success,landing_intent,landing_type, andlanding_vehicle.
17-17: NetworkStageCoreModel switch verified
Mappers, fixtures, and UI extensions all reference the new element type.
| status: status, | ||
| originalLaunch: originalLaunch, | ||
| originalLaunchUnix: originalLaunchUnix, | ||
| missions: missions?.map((m) => m.toResource()).toList(), |
There was a problem hiding this comment.
Fix null-aware chain: potential NPE on missions mapping
missions?.map(...).toList() calls toList() on null when missions is null. Use one of the safe patterns below based on CoreResource.missions nullability.
Option A (missions is nullable):
- missions: missions?.map((m) => m.toResource()).toList(),
+ missions: missions == null
+ ? null
+ : missions!.map((m) => m.toResource()).toList(),Option B (missions is non-nullable):
- missions: missions?.map((m) => m.toResource()).toList(),
+ missions: (missions?.map((m) => m.toResource()) ?? const <MissionResource>[])
+ .toList(),🤖 Prompt for AI Agents
In lib/models/core/core_ext.dart around line 12 the expression calls toList() on
the result of missions?.map(...), which can be null and will throw; if
CoreResource.missions is nullable, replace the expression with a conditional
that returns null when missions is null and otherwise maps then converts to a
list (i.e., check missions for null first, then call map().toList()); if
CoreResource.missions is non‑nullable, remove the null-aware operator and always
call map().toList(), or alternatively convert a null source into an empty list
by using the map result null-coalesced to an empty list.
lib/models/core/core_resource.dart
Outdated
| @override | ||
| List<Object?> get props => [ | ||
| coreSerial, | ||
| flight, | ||
| block, | ||
| gridfins, | ||
| legs, | ||
| reused, | ||
| landSuccess, | ||
| landingIntent, | ||
| landingType, | ||
| landingVehicle, | ||
| status, | ||
| originalLaunch, | ||
| originalLaunchUnix, | ||
| missions, | ||
| reuseCount, | ||
| rtlsAttempts, | ||
| rtlsLandings, | ||
| asdsAttempts, | ||
| asdsLandings, | ||
| waterLanding, | ||
| details, | ||
| ]; |
There was a problem hiding this comment.
Equatable won’t deep-compare missions list — equality is brittle
Including missions directly in props compares list identity, not contents. Tests that compare whole CoreResource instances (and any code relying on value equality) can fail unexpectedly.
Apply this minimal fix to ensure deep equality and stable hashing (keep Equatable for back-compat, but override equality):
class CoreResource extends Equatable {
const CoreResource({
this.coreSerial,
this.block,
this.status,
this.originalLaunch,
this.originalLaunchUnix,
this.missions,
this.reuseCount,
this.rtlsAttempts,
this.rtlsLandings,
this.asdsAttempts,
this.asdsLandings,
this.waterLanding,
this.details,
});
// ... fields ...
- @override
- List<Object?> get props => [
- coreSerial,
- block,
- status,
- originalLaunch,
- originalLaunchUnix,
- missions,
- reuseCount,
- rtlsAttempts,
- rtlsLandings,
- asdsAttempts,
- asdsLandings,
- waterLanding,
- details,
- ];
+ @override
+ bool operator ==(Object other) {
+ if (identical(this, other)) return true;
+ return other is CoreResource &&
+ other.coreSerial == coreSerial &&
+ other.block == block &&
+ other.status == status &&
+ other.originalLaunch == originalLaunch &&
+ other.originalLaunchUnix == originalLaunchUnix &&
+ listEquals(other.missions, missions) &&
+ other.reuseCount == reuseCount &&
+ other.rtlsAttempts == rtlsAttempts &&
+ other.rtlsLandings == rtlsLandings &&
+ other.asdsAttempts == asdsAttempts &&
+ other.asdsLandings == asdsLandings &&
+ other.waterLanding == waterLanding &&
+ other.details == details;
+ }
+
+ @override
+ int get hashCode => Object.hash(
+ coreSerial,
+ block,
+ status,
+ originalLaunch,
+ originalLaunchUnix,
+ // Hash list contents to keep hash stable across identical values.
+ Object.hashAll(missions ?? const []),
+ reuseCount,
+ rtlsAttempts,
+ rtlsLandings,
+ asdsAttempts,
+ asdsLandings,
+ waterLanding,
+ details,
+ );📝 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.
| @override | |
| List<Object?> get props => [ | |
| coreSerial, | |
| flight, | |
| block, | |
| gridfins, | |
| legs, | |
| reused, | |
| landSuccess, | |
| landingIntent, | |
| landingType, | |
| landingVehicle, | |
| status, | |
| originalLaunch, | |
| originalLaunchUnix, | |
| missions, | |
| reuseCount, | |
| rtlsAttempts, | |
| rtlsLandings, | |
| asdsAttempts, | |
| asdsLandings, | |
| waterLanding, | |
| details, | |
| ]; | |
| class CoreResource extends Equatable { | |
| const CoreResource({ | |
| this.coreSerial, | |
| this.block, | |
| this.status, | |
| this.originalLaunch, | |
| this.originalLaunchUnix, | |
| this.missions, | |
| this.reuseCount, | |
| this.rtlsAttempts, | |
| this.rtlsLandings, | |
| this.asdsAttempts, | |
| this.asdsLandings, | |
| this.waterLanding, | |
| this.details, | |
| }); | |
| // ... fields ... | |
| @override | |
| bool operator ==(Object other) { | |
| if (identical(this, other)) return true; | |
| return other is CoreResource && | |
| other.coreSerial == coreSerial && | |
| other.block == block && | |
| other.status == status && | |
| other.originalLaunch == originalLaunch && | |
| other.originalLaunchUnix == originalLaunchUnix && | |
| listEquals(other.missions, missions) && | |
| other.reuseCount == reuseCount && | |
| other.rtlsAttempts == rtlsAttempts && | |
| other.rtlsLandings == rtlsLandings && | |
| other.asdsAttempts == asdsAttempts && | |
| other.asdsLandings == asdsLandings && | |
| other.waterLanding == waterLanding && | |
| other.details == details; | |
| } | |
| @override | |
| int get hashCode => Object.hash( | |
| coreSerial, | |
| block, | |
| status, | |
| originalLaunch, | |
| originalLaunchUnix, | |
| // Hash list contents to keep hash stable across identical values. | |
| Object.hashAll(missions ?? const []), | |
| reuseCount, | |
| rtlsAttempts, | |
| rtlsLandings, | |
| asdsAttempts, | |
| asdsLandings, | |
| waterLanding, | |
| details, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In lib/models/core/core_resource.dart around lines 36 to 51, the current props
list leaves missions as a List and Equatable will compare its identity rather
than contents; add a custom equality override that keeps Equatable (back-compat)
but compares missions by value: import a list deep-equality helper (e.g., from
package:collection DeepCollectionEquality or flutter/foundation listEquals),
implement operator == to check identical/runtimeType and then compare scalar
fields normally while comparing missions with the deep-equality helper, and
implement hashCode to combine the other fields with a deep hash of missions
(using DeepCollectionEquality().hash(missions) or similar) so equality and
hashing are stable and based on list contents.
| @immutable | ||
| class StageCoreResource extends Equatable { | ||
| const StageCoreResource({ | ||
| this.coreSerial, | ||
| this.flight, | ||
| this.block, | ||
| this.gridfins, | ||
| this.legs, | ||
| this.reused, | ||
| this.landSuccess, | ||
| this.landingIntent, | ||
| this.landingType, | ||
| this.landingVehicle, | ||
| }); | ||
|
|
||
| final String? coreSerial; | ||
| final int? flight; | ||
| final int? block; | ||
| final bool? gridfins; | ||
| final bool? legs; | ||
| final bool? reused; | ||
| final bool? landSuccess; | ||
| final bool? landingIntent; | ||
| final String? landingType; | ||
| final String? landingVehicle; | ||
|
|
||
| @override | ||
| List<Object?> get props => [ | ||
| coreSerial, | ||
| flight, | ||
| block, | ||
| gridfins, | ||
| legs, | ||
| reused, | ||
| landSuccess, | ||
| landingIntent, | ||
| landingType, | ||
| landingVehicle, | ||
| ]; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify old CoreResource references are fully migrated.
Ensure no lingering usages remain after switching first-stage cores to StageCoreResource.
🏁 Script executed:
#!/bin/bash
# Find any remaining references to the old CoreResource type.
rg -nP --type=dart -C2 '\bCoreResource\b' -g '!**/build/**' -g '!**/.dart_tool/**'Length of output: 148
#!/bin/bash
# Search for any remaining references to the old CoreResource type across all Dart files
rg -nP '\bCoreResource\b' -g '**/*.dart' -g '!**/build/**' -g '!**/.dart_tool/**' -n🏁 Script executed:
#!/bin/bash
# Corrected search: find any remaining 'CoreResource' references in all Dart files
rg -n '\bCoreResource\b' -g '**/*.dart' -g '!**/build/**' -g '!**/.dart_tool/**'Length of output: 892
Migrate all remaining CoreResource references to StageCoreResource
The legacy CoreResource class and its uses still exist in:
- lib/models/core/core_resource.dart
- lib/models/core/core_ext.dart
- test/models/core/core_resource_test.dart
- test/models/core/core_ext_test.dart
Replace these with StageCoreResource and delete the old class/tests once migration is complete.
🤖 Prompt for AI Agents
In lib/models/stage/stage_core_resource.dart around lines 4 to 43, tests and
legacy model files still reference the old CoreResource class; replace those
references with StageCoreResource and remove the legacy files. Update import
paths in lib/models/core/core_ext.dart and any other places to import
lib/models/stage/stage_core_resource.dart, change all type names and
constructors from CoreResource to StageCoreResource (field names remain the same
so no field changes needed), update tests in
test/models/core/core_resource_test.dart and test/models/core/core_ext_test.dart
to reference StageCoreResource and adjust import paths, then delete
lib/models/core/core_resource.dart and the two legacy test files, run the test
suite and fix any resulting type/import errors.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (15)
test/repository/cores_repository_impl_test.dart (1)
31-36: Optional: verify parameter forwarding.Consider verifying the exact named args forwarded to the data source to lock the contract (hasId, limit, offset).
+ verify(() => mockDataSource.getCores( + hasId: any(named: 'hasId'), + limit: any(named: 'limit'), + offset: any(named: 'offset'), + )).called(1);lib/features/cores/bloc/cores_event.dart (2)
5-7:reloadis unused—either wire it or remove.If meant to force a refresh/bypass cache, handle it in the bloc; otherwise drop it to avoid dead params.
3-4: Prefer@freezed(lowercase) and concrete class style for consistency.Align with common Freezed usage across the codebase. Functionally equivalent.
-@Freezed() -abstract class CoresEvent with _$CoresEvent { +@freezed +class CoresEvent with _$CoresEvent {test/repository/cores_repository_test.dart (2)
33-39: Prefer Exception over Error in repository-facing tests.Domain/repository failures should surface as Exceptions; using Error suggests programming errors. This keeps tests aligned with intended failure modes.
- when(repository.getCores()).thenAnswer((_) => Future.error(Error())); + when(repository.getCores()).thenAnswer((_) => Future.error(Exception('boom'))); ... - throwsA(isA<Error>()), + throwsA(isA<Exception>()),
17-23: Nit: grammar in test name.“returns list of core” → “returns list of cores”.
- test('returns list of core', () { + test('returns list of cores', () {lib/features/cores/bloc/cores_bloc.dart (3)
12-29: DRY the duplicated load/refresh logic into a private method.Both handlers are identical. Extract to a single
_loadCores(emit)and call it from each handler. Fewer branches, easier to maintain.class CoresBloc extends Bloc<CoresEvent, CoresState> { CoresBloc(this._repository) : super(const CoresState.loading()) { - on<CoresLoadEvent>((event, emit) async { - emit(const CoresState.loading()); - try { - final cores = await _repository.getCores( - hasId: true, - limit: null, - offset: null, - ); - if (cores.isEmpty) { - emit(const CoresState.empty()); - } else { - emit(CoresState.success(cores: cores)); - } - } catch (e) { - emit(const CoresState.error()); - } - }); + on<CoresLoadEvent>((event, emit) async => _loadCores(emit)); - on<CoresRefreshEvent>((event, emit) async { - emit(const CoresState.loading()); - try { - final cores = await _repository.getCores( - hasId: true, - limit: null, - offset: null, - ); - if (cores.isEmpty) { - emit(const CoresState.empty()); - } else { - emit(CoresState.success(cores: cores)); - } - } catch (e) { - emit(const CoresState.error()); - } - }); + on<CoresRefreshEvent>((event, emit) async => _loadCores(emit)); } final CoresRepository _repository; + + Future<void> _loadCores(Emitter<CoresState> emit) async { + emit(const CoresState.loading()); + try { + final cores = await _repository.getCores(hasId: true); + emit(cores.isEmpty ? const CoresState.empty() : CoresState.success(cores: cores)); + } catch (_) { + emit(const CoresState.error()); + } + }Also applies to: 31-48
15-19: Drop explicit nulls for optional params.
limit: null, offset: nullare redundant.- final cores = await _repository.getCores( - hasId: true, - limit: null, - offset: null, - ); + final cores = await _repository.getCores(hasId: true);Also applies to: 34-38
12-12: Optional: throttle refresh/load bursts.If the UI can trigger rapid refreshes, consider a
droppable()orrestartable()transformer to prevent overlapping requests.lib/features/cores/bloc/cores_state.dart (1)
7-9: Use a typed constant empty list for the default.Ensures a compile-time constant and correct type inference in Freezed defaults.
- const factory CoresState.success({ - @Default([]) List<CoreResource> cores, - }) = CoresSuccessState; + const factory CoresState.success({ + @Default(<CoreResource>[]) List<CoreResource> cores, + }) = CoresSuccessState;test/features/cores/bloc/cores_bloc_test.dart (2)
7-7: Avoid importing another test file for fixtures.
import '../../../repository/cores_repository_test.dart';couples tests. Extract shared fixtures (e.g., mockCoreLost) into a small test-only fixture file and import that from both tests.
80-80: Nit: test name typo.Remove the leading underscore: “handles CoresRefreshEvent same as load”.
- 'handles _CoresRefreshEvent same as load', + 'handles CoresRefreshEvent same as load',lib/repository/cores_repository.dart (2)
21-37: Consider makinghasIdnon-nullable or defaulting to null.
bool? hasId = trueis awkward—eitherbool hasId = trueorbool? hasId(no default) to signal tri-state (true/false/unspecified). Current shape invites accidental nulls with an implicit default.- Future<List<CoreResource>> getCores( - {bool? hasId = true, int? limit, int? offset}) async { + Future<List<CoreResource>> getCores( + {bool hasId = true, int? limit, int? offset}) async {
30-36: Optional: introduce a typed repository exception.Throwing raw Exception strings makes error handling brittle. A
RepositoryException(wrapping message/cause) improves downstream handling and testability.Also applies to: 43-49
lib/features/cores/bloc/cores_bloc.freezed.dart (1)
553-562: Defensive immutability for lists (optional).Getter wraps with
EqualUnmodifiableListView, but the underlying_corescan still be mutated by external references passed to the constructor/copyWith. If this state is widely shared, consider defensively copying in the source (not here) before construction (e.g.,List.unmodifiable([...cores])).Also applies to: 607-620
test/mocks.mocks.dart (1)
6-16: Mockito mocks regenerate looks consistent; one nit on Navigator import.Generated import targets
package:flutter/src/widgets/navigator.dart. Prefer referencingNavigatorObserverfrompackage:flutter/widgets.dartin your annotations/imports so Mockito pulls the public API (avoids relying on implementation imports).If desired, I can adjust test/mocks.dart to import widgets.dart explicitly and re-run generation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
lib/features/cores/bloc/cores_bloc.dart(1 hunks)lib/features/cores/bloc/cores_bloc.freezed.dart(1 hunks)lib/features/cores/bloc/cores_event.dart(1 hunks)lib/features/cores/bloc/cores_state.dart(1 hunks)lib/repository/cores_repository.dart(1 hunks)test/features/cores/bloc/cores_bloc_test.dart(1 hunks)test/mocks.dart(2 hunks)test/mocks.mocks.dart(11 hunks)test/repository/cores_repository_impl_test.dart(1 hunks)test/repository/cores_repository_test.dart(1 hunks)
🧰 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/cores/bloc/cores_bloc.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 (7)
test/mocks.dart (1)
3-3: LGTM: MockCoresRepository generation wired upImporting CoresRepository and adding it to @GenerateMocks is correct. The generated MockCoresRepository will be available via mocks.mocks.dart for tests.
Also applies to: 13-16
lib/features/cores/bloc/cores_bloc.dart (1)
12-12: Verify initial dispatch aligns with project pattern.Per prior learnings, BlocProviders typically dispatch an initial load with cascade (
..add(const CoresLoadEvent())). Ensure the DI/provider follows this for CoresBloc.Would you like me to scan the repo for the provider and open a small PR if it’s missing?
lib/repository/cores_repository.dart (1)
30-36: Question: canloadingoccur here?A one-shot Future API shouldn’t surface a transient loading state. If
ApiResult.loadingis only for streams, consider removing this branch or converting the API to a stream if you need progressive states.lib/features/cores/bloc/cores_bloc.freezed.dart (2)
59-66: Pattern matching requires Dart 3+.Switch-case patterns like
case CoresLoadEvent() when ...require Dart ≥ 3. Ensure your SDK constraint and Flutter channel align.Also applies to: 174-181, 378-389
32-35: Sanity-check Freezed/Analyzer compatibility for CopyWith stubs.These
$...CopyWithdeclarations diverge from the usualabstract class ... { factory ... = _Impl; }style. If the analyzer flags them, bump Freezed/freezed_annotation, clean, and regenerate.Would you like me to pin compatible versions and list the exact pubspec updates?
Also applies to: 315-317
test/mocks.mocks.dart (2)
182-225: New MockCoresRepository aligns with repository API.Signatures for
getCores/getCorematch expectations and defaults. Good addition for bloc/repo tests.
296-311: NavigatorObserver.didChangeTop requires recent Flutter.This method exists on newer Flutter versions. Ensure your Flutter SDK in CI/dev matches that API to avoid missing-member compile errors.
| // GENERATED CODE - DO NOT MODIFY BY HAND | ||
| // coverage:ignore-file | ||
| // ignore_for_file: type=lint | ||
| // ignore_for_file: unused_element, deprecated_member_use, deprecated_member_use_from_same_package, use_function_type_syntax_for_parameters, unnecessary_const, avoid_init_to_null, invalid_override_different_default_values_named, prefer_expression_function_bodies, annotate_overrides, invalid_annotation_target, unnecessary_question_mark | ||
|
|
||
| part of 'cores_bloc.dart'; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Generated file: looks fine; ensure the source part directive exists.
Confirm that lib/features/cores/bloc/cores_bloc.dart includes part 'cores_bloc.freezed.dart'; and that build_runner is wired in CI.
Run:
🏁 Script executed:
#!/bin/bash
rg -n "^part 'cores_bloc\.freezed\.dart';" lib/features/cores/bloc/cores_bloc.dart
rg -nP '^environment:\n(\s*)sdk:\s*"\^?3' pubspec.yamlLength of output: 33
🏁 Script executed:
#!/bin/bash
rg -n "build_runner" .github/workflows
rg -n "build_runner" .Length of output: 304
Ensure CI runs Freezed code generation
lib/features/cores/bloc/cores_bloc.dart includes the part 'cores_bloc.freezed.dart'; directive (line 6); build_runner is declared in pubspec.yaml and used in the Makefile, but no GitHub Actions workflow invokes code generation—add a CI job in .github/workflows to run dart run build_runner build --delete-conflicting-outputs.
🤖 Prompt for AI Agents
In lib/features/cores/bloc/cores_bloc.freezed.dart lines 1-7: the generated
Freezed part is present but CI does not run build_runner; add a new GitHub
Actions job file under .github/workflows (or update existing workflow) that runs
on push/pull_request, checks out code, sets up the required Dart/Flutter SDK,
installs dependencies, and runs the command `dart run build_runner build
--delete-conflicting-outputs` (optionally with caching for pub) so the generated
cores_bloc.freezed.dart is produced during CI before tests/build steps.
| setUp(() { | ||
| mockRepository = MockCoresRepository(); | ||
| bloc = CoresBloc(mockRepository); | ||
| }); |
There was a problem hiding this comment.
Build a fresh bloc per test; don’t reuse a single instance.
bloc_test closes the bloc after each test. Reusing the same instance can cause “Bloc has been closed” or flakiness. Construct a new CoresBloc in each build: and avoid holding it in setUp.
- late MockCoresRepository mockRepository;
- late CoresBloc bloc;
+ late MockCoresRepository mockRepository;
...
- setUp(() {
- mockRepository = MockCoresRepository();
- bloc = CoresBloc(mockRepository);
- });
+ setUp(() {
+ mockRepository = MockCoresRepository();
+ });
...
- build: () {
- when(() => mockRepository.getCores(
+ build: () {
+ when(() => mockRepository.getCores(
hasId: any(named: 'hasId'),
limit: any(named: 'limit'),
offset: any(named: 'offset'),
)).thenAnswer((_) async => [mockCoreLost]);
- return bloc;
+ return CoresBloc(mockRepository);
},
...
- return bloc;
+ return CoresBloc(mockRepository);
},
...
- return bloc;
+ return CoresBloc(mockRepository);
},
...
- return bloc;
+ return CoresBloc(mockRepository);
},Additionally, for the “initial state is loading” test:
- test('initial state is loading', () {
- expect(bloc.state, const CoresState.loading());
- });
+ test('initial state is loading', () {
+ final bloc = CoresBloc(mockRepository);
+ expect(bloc.state, const CoresState.loading());
+ bloc.close();
+ });Also applies to: 27-35, 45-54, 62-71, 79-88
🤖 Prompt for AI Agents
In test/features/cores/bloc/cores_bloc_test.dart around lines 15 to 18 (and
similarly at lines 27-35, 45-54, 62-71, 79-88), the current setUp creates and
stores a single CoresBloc instance which is then reused across tests causing
"Bloc has been closed" flakiness; remove creation of the bloc from setUp and
instead construct a fresh CoresBloc inside each test's build: callback (e.g.,
build: () => CoresBloc(mockRepository)), keeping mockRepository in setUp if
desired, and update the “initial state is loading” test to use the same pattern
so each test gets its own new bloc instance.
| test('throws exception when error', () async { | ||
| when(() => mockDataSource.getCores( | ||
| hasId: any(named: 'hasId'), | ||
| limit: any(named: 'limit'), | ||
| offset: any(named: 'offset'), | ||
| )).thenAnswer((_) async => const ApiResult.error('Network error')); | ||
|
|
||
| expect(() => repository.getCores(), throwsA(isA<Exception>())); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix async error assertions (use Future-based expectations, not closures).
getCores()/getCore() return Futures. Wrap the Future directly (or use expectLater), otherwise the matcher observes a synchronous, non-throwing closure.
- expect(() => repository.getCores(), throwsA(isA<Exception>()));
+ expect(repository.getCores(), throwsA(isA<Exception>()));
...
- expect(() => repository.getCores(), throwsA(isA<Exception>()));
+ expect(repository.getCores(), throwsA(isA<Exception>()));
...
- expect(() => repository.getCore('B1049'), throwsA(isA<Exception>()));
+ expect(repository.getCore('B1049'), throwsA(isA<Exception>()));
...
- expect(() => repository.getCore('B1049'), throwsA(isA<Exception>()));
+ expect(repository.getCore('B1049'), throwsA(isA<Exception>()));Also applies to: 53-61, 82-87, 89-94
🤖 Prompt for AI Agents
In test/repository/cores_repository_impl_test.dart around lines 43-51 (and
similarly at 53-61, 82-87, 89-94), the tests assert exceptions by passing a
synchronous closure to expect, but repository.getCores()/getCore() return
Futures; change the assertions to await the Future-based expectation (e.g., use
expect(repository.getCores(), throwsA(...)) or await
expectLater(repository.getCores(), throwsA(...))) so the matcher observes the
asynchronous exception; update all mentioned test blocks accordingly.
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (3)
test/features/cores/bloc/cores_bloc_test.dart (1)
49-60: Build a fresh CoresBloc per test; don’t reuse a shared instance.Reusing a closed bloc causes flakiness (“Bloc has been closed”). Construct inside each build: and in standalone tests use a local instance. Also drop global bloc/tearDown. This mirrors prior feedback.
Apply these diffs (pattern—replicate for similar blocks):
- Setup/teardown:
late CoresRepository repository; - late CoresBloc bloc; setUp(() { repository = MockCoresRepository(); - bloc = CoresBloc(repository); }); - tearDown(() { - bloc.close(); - });
- blocTest build: closures (example 1):
)).thenAnswer((_) async => testCores); - return bloc; + return CoresBloc(repository);
- “stores cores in allCores”:
- bloc.add(const CoresLoadEvent()); + final bloc = CoresBloc(repository); + bloc.add(const CoresLoadEvent()); ... - expect(bloc.allCores, equals(testCores)); + expect(bloc.allCores, equals(testCores)); + await bloc.close();
- Filter tests that prefill allCores (example pattern):
- build: () { - bloc.allCores = testCores; - return bloc; - }, + build: () { + final b = CoresBloc(repository)..allCores = testCores; + return b; + },For the “empty allCores” case:
- bloc.allCores = []; - return bloc; + final b = CoresBloc(repository)..allCores = []; + return b;For the “null values” case:
- bloc.allCores = [ + final b = CoresBloc(repository)..allCores = [ const CoreResource( coreSerial: null, missions: null, status: null, ), const CoreResource( coreSerial: 'B1051', missions: [MissionResource(name: null, flight: 1)], status: 'active', ), ]; - return bloc; + return b;
- State management tests:
- test('initial state is loading', () { - expect(bloc.state, const CoresState.loading()); - }); + test('initial state is loading', () { + final bloc = CoresBloc(repository); + expect(bloc.state, const CoresState.loading()); + bloc.close(); + }); - test('allCores is initially empty', () { - expect(bloc.allCores, isEmpty); - }); + test('allCores is initially empty', () { + final bloc = CoresBloc(repository); + expect(bloc.allCores, isEmpty); + bloc.close(); + });Also applies to: 58-60, 66-73, 90-97, 107-114, 122-140, 146-153, 166-171, 185-190, 203-208, 225-230, 247-252, 269-274, 283-288, 302-307, 316-332, 348-355
lib/models/core/core_resource.dart (1)
68-84: Equatable props compare List by identity — deep equality for missions is brokenIncluding missions in props makes equality/hash depend on list identity, not contents, causing brittle comparisons and potential test flakiness.
Apply this diff to stop relying on props for equality:
- @override - List<Object?> get props => [ - coreSerial, - block, - status, - originalLaunch, - originalLaunchUnix, - missions, - reuseCount, - rtlsAttempts, - rtlsLandings, - asdsAttempts, - asdsLandings, - waterLanding, - details, - ]; + @override + List<Object?> get props => const []; // unused – custom ==/hashCode belowThen add custom equality and hashCode inside the class:
@override bool operator ==(Object other) { if (identical(this, other)) return true; return other is CoreResource && other.coreSerial == coreSerial && other.block == block && other.status == status && other.originalLaunch == originalLaunch && other.originalLaunchUnix == originalLaunchUnix && listEquals(other.missions, missions) && other.reuseCount == reuseCount && other.rtlsAttempts == rtlsAttempts && other.rtlsLandings == rtlsLandings && other.asdsAttempts == asdsAttempts && other.asdsLandings == asdsLandings && other.waterLanding == waterLanding && other.details == details; } @override int get hashCode => Object.hash( coreSerial, block, status, originalLaunch, originalLaunchUnix, Object.hashAll(missions ?? const []), reuseCount, rtlsAttempts, rtlsLandings, asdsAttempts, asdsLandings, waterLanding, details, );lib/features/cores/bloc/cores_bloc.freezed.dart (1)
1-7: Generated Freezed file: CI should build it, not humans.Looks fine. Echoing the earlier note: ensure CI runs
build_runnerso this part is regenerated on PRs.
🧹 Nitpick comments (36)
test/features/main/destinations_test.dart (1)
33-34: Assertion updates — LGTM; add a length guard to catch regressionsConsider asserting the count to ensure indexes remain stable.
final destinations = getDestinations(context); // Check labels + expect(destinations.length, 4); expect(destinations[0].label, context.launchesTitle); expect(destinations[1].label, context.rocketsTab); expect(destinations[2].label, context.coresLabel); expect(destinations[3].label, context.settingsTitle);lib/di/di_repository_module.dart (1)
25-27: CoresRepository factory — LGTMMatches the DI style used for Launches/Rocket repos. Minor: unrelated to this change, provideAccidentsRepository() naming looks off for ThemeRepository — consider renaming in a follow-up.
test/features/cores/utils/cores_ext_test.dart (1)
38-50: Confirm intent: should 'all'.toStatus() map to CoreFilterStatus.all?Parsing ‘all’ to unknown may be surprising given CoreFilterStatus.all exists. If ‘all’ is a valid input (e.g., from query params), consider mapping to CoreFilterStatus.all.
- expect('all'.toStatus(), CoreFilterStatus.unknown); + expect('all'.toStatus(), CoreFilterStatus.all);lib/features/main/navigation/destinations.dart (1)
23-29: New Cores destination — LGTM; minor key consistency nitKeys use 'launches', 'rockets', 'settings'; consider 'cores' (vs 'cores_screen') for consistency. Verify no external key dependency before changing.
- key: const Key('cores_screen'), + key: const Key('cores'),lib/features/cores/widget/core_loading_content.dart (1)
9-10: Sliver-only loading widget — LGTM; add intent doc to avoid misuseClarify it’s meant for sliver contexts to prevent RenderObject type errors if used in non-sliver trees.
class CoreLoadingContent extends StatelessWidget { const CoreLoadingContent({super.key}); @override Widget build(BuildContext context) { - return const SliverFillRemaining(child: LoadingContent()); + // Sliver-only: use inside CustomScrollView slivers. + return const SliverFillRemaining(child: LoadingContent()); } }lib/features/cores/widget/cores_error_widget.dart (2)
25-33: Center and wrap texts to avoid overflow on small screensAdd textAlign to both texts (title and error message) so long/localized strings don’t overflow.
- Text( - loc.errorLoadingCores, - style: Theme.of(context).textTheme.headlineSmall, - ), + Text( + loc.errorLoadingCores, + textAlign: TextAlign.center, + style: Theme.of(context).textTheme.headlineSmall, + ), @@ - Text( - errorMessage, - style: Theme.of(context).textTheme.bodyMedium, - ), + Text( + errorMessage, + textAlign: TextAlign.center, + style: Theme.of(context).textTheme.bodyMedium, + ),
35-41: Expose a testable hook for retryAdd a key for easier testing; optional semantics label improves a11y.
- FilledButton.icon( + FilledButton.icon( + key: const Key('cores_error_retry'), onPressed: () { context.read<CoresBloc>().add(const CoresRefreshEvent()); }, icon: const Icon(Icons.refresh), - label: Text(loc.retry), + label: Text(loc.retry, semanticsLabel: loc.retry), ),lib/features/cores/widget/cores_not_found_widget.dart (1)
16-25: Use theme-aware colors and center textPrefer colorScheme.outline over hard-coded grey; center text to prevent overflow.
- const Icon( - Icons.inbox_outlined, - size: 64, - color: Colors.grey, - ), + Icon( + Icons.inbox_outlined, + size: 64, + color: Theme.of(context).colorScheme.outline, + ), @@ - Text( - S.of(context).noCoresFound(searchQuery), - style: const TextStyle(fontSize: 18), - ), + Text( + S.of(context).noCoresFound(searchQuery), + textAlign: TextAlign.center, + style: Theme.of(context).textTheme.bodyLarge, + ),lib/features/cores/widget/cores_empty_widget.dart (1)
15-24: Theme-aware color and centered textAligns with Material 3 and handles long/localized strings better.
- const Icon( - Icons.inbox_outlined, - size: 64, - color: Colors.grey, - ), + Icon( + Icons.inbox_outlined, + size: 64, + color: Theme.of(context).colorScheme.outline, + ), @@ - Text( - loc.emptyList, - style: const TextStyle(fontSize: 18), - ), + Text( + loc.emptyList, + textAlign: TextAlign.center, + style: Theme.of(context).textTheme.bodyLarge, + ),test/features/cores/utils/core_utils_test.dart (1)
12-35: Make locale explicit for deterministic formattingSet locale to 'en' to avoid DateFormat differences across environments.
- MaterialApp( + MaterialApp( + locale: const Locale('en'), localizationsDelegates: appLocalizationsDelegates, supportedLocales: appSupportedLocales,Repeat the same for the other MaterialApp instances in this file.
test/features/cores/cores_screen_test.dart (1)
65-81: Assert error copy is shownRe-enable the assertion to verify the message is visible.
- expect(find.byType(CoresErrorWidget), findsOneWidget); - //expect(find.text(errorMessage), findsOneWidget); + expect(find.byType(CoresErrorWidget), findsOneWidget); + expect(find.text(errorMessage), findsOneWidget);lib/features/cores/bloc/cores_event.dart (1)
3-15: Freezed/style nits: prefer @freezed and drop 'final' in paramsMinor style alignment with common Freezed usage.
-@Freezed() +@freezed abstract class CoresEvent with _$CoresEvent { @@ const factory CoresEvent.filter({ required String searchQuery, - final CoreFilterStatus? statusFilter, + CoreFilterStatus? statusFilter, }) = CoresFilterEvent;lib/features/cores/widget/core_item_widget.dart (1)
14-24: Ensure ripple is clipped to the card radiusInkWell splash can bleed past the Card. Set shape and clipBehavior on Card.
- return Card( - elevation: 0, - color: Theme.of(context) + return Card( + elevation: 0, + clipBehavior: Clip.antiAlias, + shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(12)), + color: Theme.of(context) .colorScheme .surfaceContainerHighest - .withValues(alpha: 0.3), + .withOpacity(0.3),Also replace
.withValues(alpha: 0.2)at Line 77 with.withOpacity(0.2)for broader SDK compatibility.If you intend to keep
withValues, confirm the project targets Flutter 3.22+.Also applies to: 75-78
lib/features/cores/utils/core_utils.dart (1)
20-26: Harden date parsing and honor locale/timezone.Guard bad input, format with current locale, and show local time.
String formatFirstLaunch(BuildContext context, String? isoDate) { if (isoDate == null || isoDate.isEmpty) return ''; - final date = DateTime.parse(isoDate); - final loc = S.of(context); - final formatted = DateFormat.yMMMd().format(date); + final date = DateTime.tryParse(isoDate)?.toLocal(); + if (date == null) return ''; + final loc = S.of(context); + final locale = Localizations.localeOf(context).toString(); + final formatted = DateFormat.yMMMd(locale).format(date); return '${loc.firstLaunch}: $formatted'; }lib/features/cores/bloc/cores_state.dart (2)
3-4: Prefer@freezed(lowercase) for consistency with Freezed conventions.Minor style/readability tweak; both may work depending on imports, but the lowercase annotation is the common pattern.
-@Freezed() +@freezed abstract class CoresState with _$CoresState {
10-11: MakesearchQuerynon-nullable if defaulting to empty string.Avoid
String?with@Default(''). Use non-nullableStringto simplify consumers.- @Default('') String? searchQuery, + @Default('') String searchQuery,lib/features/cores/bloc/cores_bloc.dart (5)
35-39: Preserve active filters after load to keep UI and list consistentCurrently, a fresh load resets the filtered list even if the user has a search/status filter active. Re-apply the last filter when new data arrives.
- if (cores.isEmpty) { - emit(const CoresState.empty()); - } else { - emit(CoresState.success(cores: cores, filteredCores: cores)); - } + if (cores.isEmpty) { + emit(const CoresState.empty()); + } else { + final filtered = + _applyFilters(cores, _lastSearchQuery, _lastStatusFilter); + if (filtered.isEmpty && + (_lastSearchQuery.isNotEmpty || _lastStatusFilter != null)) { + emit(CoresState.notFound(searchQuery: _lastSearchQuery)); + } else { + emit(CoresState.success( + cores: cores, + filteredCores: filtered, + searchQuery: _lastSearchQuery, + statusFilter: _lastStatusFilter, + )); + } + }Additional code (outside the shown range):
// Fields String _lastSearchQuery = ''; CoreFilterStatus? _lastStatusFilter; // Helper List<CoreResource> _applyFilters( List<CoreResource> source, String query, CoreFilterStatus? status, ) { final q = query.trim().toLowerCase(); return source.where((core) { final matchesSearch = q.isEmpty || (core.coreSerial?.toLowerCase().contains(q) ?? false) || (core.missions?.any((m) => m.name?.toLowerCase().contains(q) ?? false) ?? false); final matchesStatus = status == null || status == CoreFilterStatus.all || core.status.toStatus() == status; return matchesSearch && matchesStatus; }).toList(); }
58-62: Refresh also drops filters; re-apply themRefreshing should keep the user’s current search and status filter.
- if (cores.isEmpty) { - emit(const CoresState.empty()); - } else { - emit(CoresState.success(cores: cores, filteredCores: cores)); - } + if (cores.isEmpty) { + emit(const CoresState.empty()); + } else { + final filtered = + _applyFilters(cores, _lastSearchQuery, _lastStatusFilter); + if (filtered.isEmpty && + (_lastSearchQuery.isNotEmpty || _lastStatusFilter != null)) { + emit(CoresState.notFound(searchQuery: _lastSearchQuery)); + } else { + emit(CoresState.success( + cores: cores, + filteredCores: filtered, + searchQuery: _lastSearchQuery, + statusFilter: _lastStatusFilter, + )); + } + }
68-76: Remember the current filter inputs in the BlocStore the last search and status to make refresh/load re-apply them.
void _onFilter( CoresFilterEvent event, Emitter<CoresState> emit, ) { + _lastSearchQuery = event.searchQuery; + _lastStatusFilter = event.statusFilter; if (allCores.isEmpty) { emit(const CoresState.empty()); return; }
77-89: Tiny perf/readability: compute lowercase onceAvoid repeated toLowerCase() calls and null-coalescing chains.
- final filtered = allCores.where((core) { - final matchesSearch = event.searchQuery.isEmpty || - (core.coreSerial - ?.toLowerCase() - .contains(event.searchQuery.toLowerCase()) ?? - false) || - (core.missions?.any((m) => - m.name - ?.toLowerCase() - .contains(event.searchQuery.toLowerCase()) ?? - false) ?? - false); + final q = event.searchQuery.trim().toLowerCase(); + final filtered = allCores.where((core) { + final matchesSearch = q.isEmpty || + (core.coreSerial?.toLowerCase().contains(q) ?? false) || + (core.missions?.any((m) => m.name?.toLowerCase().contains(q) ?? false) ?? false);
40-42: Don’t surface raw exception strings to the UIEmit a user-friendly message and log the error separately.
- } catch (e) { - emit(CoresState.error(e.toString())); + } catch (e, st) { + emit(const CoresState.error('Unable to load cores. Please try again.')); + _logLoadError(e, st); }Additional code (outside the shown range):
import 'dart:developer' as developer; void _logLoadError(Object error, StackTrace st) { developer.log('CoresBloc load/refresh failed', error: error, stackTrace: st); }Also applies to: 63-65
lib/features/cores/cores_screen.dart (5)
26-29: No-op onPressed handler
onPressed: () => printis a no-op. Either remove the action or usedebugPrintwith a message.- onPressed: () => print, + onPressed: () => debugPrint('Cores info tapped'),
84-98: Clear action should trigger UI rebuild immediatelyAfter clearing, rebuild so the trailing clear button hides and the list reflects the empty query.
- onPressed: () { - _searchController.clear(); - context.read<CoresBloc>().add( + onPressed: () { + _searchController.clear(); + setState(() {}); + context.read<CoresBloc>().add( CoresFilterEvent( searchQuery: '', statusFilter: _selectedStatus, ), ); },
39-51: Allow refresh in Empty/NotFound states tooUsers often want to retry in empty/not-found states. Show the FAB in those states.
- if (state is CoresSuccessState) { + if (state is CoresSuccessState || + state is CoresEmptyState || + state is CoresNotFoundState) {
151-174: Remove unreachable fallbackThe exhaustive
switchover union types makes the trailingSizedBox.shrink()unreachable.
183-199: Add stable keys for list itemsHelps with sliver diffing and avoids unnecessary rebuilds.
- return CoreItemWidget( - core: core, - ); + return CoreItemWidget( + key: ValueKey(core.coreSerial ?? 'core_$index'), + core: core, + );lib/l10n/intl_pt.arb (3)
224-226: Use ICU plurals for correct singular/plural formsCurrent strings render “1 missões”. Use ICU syntax.
- "missions": "{count} missões", - "reuses": "{count} reutilizações", + "missions": "{count, plural, one {# missão} other {# missões}}", + "reuses": "{count, plural, one {# reutilização} other {# reutilizações}}",Optionally add metadata (outside the shown range):
"@missions": { "description": "Mission count label", "placeholders": { "count": { "type": "int" } } }, "@reuses": { "description": "Reuse count label", "placeholders": { "count": { "type": "int" } } }
234-237: Add placeholder metadata for better toolingProvide
@metadata for placeholders to improve linting and IDE support.Example additions (adjacent to the keys):
"@noCoresFound": { "description": "Shown when no cores match a query", "placeholders": { "query": {} } }, "@blockLabel": { "description": "Label for Falcon block number", "placeholders": { "blockNumber": {} } }
226-228: Consolidate duplicate ARB keysnaandnotAvailable. Both entries inlib/l10n/intl_pt.arb(lines 177, 227) resolve to “N/D”. Pick one key, remove the other, then regenerate your localized APIs and verify that noS.of(context).naorS.of(context).notAvailableusages break.lib/l10n/app_localizations_de.dart (3)
339-346: Use ICU plurals for correct German grammarHard-coding "$count Missionen/Wiederverwendungen" is grammatically off for 1 and special cases. Prefer Intl pluralization in ARB so gen-l10n creates proper methods.
Example ARB (intl_de.arb):
"missions": "{count, plural, one {# Mission} other {# Missionen}}", "reuses": "{count, plural, one {# Wiederverwendung} other {# Wiederverwendungen}}"
351-353: Align 'na' with existing 'notAvailable' wordingYou have both notAvailable ('Nicht verfügbar') and na ('N/V'). Consider using the same wording to avoid UI inconsistency.
- String get na => 'N/V'; + String get na => 'Nicht verfügbar';
373-375: Minor: drop escaping around quotes or use typographic quotesEscaping " inside single-quoted strings is noisy. Either drop the escapes or use typographic quotes.
- return 'Keine Kerne für \"$query\" gefunden'; + return 'Keine Kerne für "$query" gefunden';lib/l10n/app_localizations_uk.dart (1)
341-348: Use ICU plurals for UkrainianNumbers require complex plural forms in Ukrainian. Move to Intl pluralization in ARB.
Example ARB (intl_uk.arb):
"missions": "{count, plural, one {# місія} few {# місії} many {# місій} other {# місії}}", "reuses": "{count, plural, one {# повтор} few {# повтори} many {# повторів} other {# повтори}}"lib/models/core/core_resource.dart (1)
36-66: Optional: enforce immutability for missions listThe class is @immutable but holds a mutable List. Consider copying and exposing an unmodifiable view in ctor/copyWith to prevent accidental mutation after construction.
If acceptable to drop const constructor:
- const CoreResource({ + CoreResource({ ... - this.missions, + List<MissionResource>? missions, ... }) : missions = missions == null ? null : List.unmodifiable(missions);And similarly wrap in copyWith.
lib/features/cores/bloc/cores_bloc.freezed.dart (2)
709-711: Unify searchQuery nullability across states.In Success it’s
String?(default ''), while NotFound uses non-nullableString. Consider making Success.searchQuery non-nullable to avoid tri-state handling.Apply in the source union (cores_bloc.dart), then regenerate:
-const factory CoresState.success({ - @Default([]) List<CoreResource> cores, - List<CoreResource>? filteredCores, - @Default('') String? searchQuery, - CoreFilterStatus? statusFilter, -}) = CoresSuccessState; +const factory CoresState.success({ + @Default([]) List<CoreResource> cores, + List<CoreResource>? filteredCores, + @Default('') String searchQuery, + CoreFilterStatus? statusFilter, +}) = CoresSuccessState;Also applies to: 886-891
725-741: Deep list equality can be costly on large pages.If cores lists can be large/frequent, consider comparing by identity or stable IDs in the source union to reduce rebuild triggers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
lib/generated/intl/messages_de.dartis excluded by!**/generated/**lib/generated/intl/messages_en.dartis excluded by!**/generated/**lib/generated/intl/messages_pt.dartis excluded by!**/generated/**lib/generated/intl/messages_uk.dartis excluded by!**/generated/**lib/generated/l10n.dartis excluded by!**/generated/**
📒 Files selected for processing (35)
lib/di/app_bloc_providers.dart(2 hunks)lib/di/app_repository_providers.dart(2 hunks)lib/di/di_initializer.config.dart(3 hunks)lib/di/di_network_module.dart(3 hunks)lib/di/di_repository_module.dart(2 hunks)lib/features/cores/bloc/cores_bloc.dart(1 hunks)lib/features/cores/bloc/cores_bloc.freezed.dart(1 hunks)lib/features/cores/bloc/cores_event.dart(1 hunks)lib/features/cores/bloc/cores_state.dart(1 hunks)lib/features/cores/cores_screen.dart(1 hunks)lib/features/cores/model/core_filter_status.dart(1 hunks)lib/features/cores/utils/core_utils.dart(1 hunks)lib/features/cores/utils/cores_ext.dart(1 hunks)lib/features/cores/widget/core_item_widget.dart(1 hunks)lib/features/cores/widget/core_loading_content.dart(1 hunks)lib/features/cores/widget/cores_empty_widget.dart(1 hunks)lib/features/cores/widget/cores_error_widget.dart(1 hunks)lib/features/cores/widget/cores_not_found_widget.dart(1 hunks)lib/features/main/navigation/destinations.dart(2 hunks)lib/l10n/app_localizations.dart(1 hunks)lib/l10n/app_localizations_de.dart(1 hunks)lib/l10n/app_localizations_en.dart(1 hunks)lib/l10n/app_localizations_pt.dart(1 hunks)lib/l10n/app_localizations_uk.dart(1 hunks)lib/l10n/intl_de.arb(1 hunks)lib/l10n/intl_en.arb(1 hunks)lib/l10n/intl_pt.arb(1 hunks)lib/l10n/intl_uk.arb(1 hunks)lib/models/core/core_resource.dart(1 hunks)lib/utils/string_resources.dart(1 hunks)test/features/cores/bloc/cores_bloc_test.dart(1 hunks)test/features/cores/cores_screen_test.dart(1 hunks)test/features/cores/utils/core_utils_test.dart(1 hunks)test/features/cores/utils/cores_ext_test.dart(1 hunks)test/features/main/destinations_test.dart(1 hunks)
🧰 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/di/app_bloc_providers.dartlib/di/app_repository_providers.dartlib/features/main/navigation/destinations.dartlib/features/cores/bloc/cores_bloc.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: Flutter Integration Tests (31)
- GitHub Check: App
🔇 Additional comments (14)
lib/features/cores/model/core_filter_status.dart (1)
1-7: Enum looks goodValues cover the expected filter set; naming is consistent and clear.
lib/l10n/app_localizations_en.dart (1)
318-387: Generated additions align with ARB keysGetters and builders match intl_en.arb. Once ICU plurals are adopted in ARB, re-run gen_l10n to regenerate corresponding plural-aware methods.
lib/l10n/app_localizations_pt.dart (1)
318-387: Generated PT additions look correctStrings match the intended semantics. After converting ARB to ICU plurals, regenerate to avoid “1 missões/1 reutilizações”.
lib/utils/string_resources.dart (1)
35-36: LGTM: exposes coresLabel through context extensionKeeps nav labeling consistent with other keys.
lib/di/app_bloc_providers.dart (2)
46-52: Good: eager-load CoresBloc with initial eventMatches the project’s preferred cascade pattern and keeps behavior consistent with other blocs.
5-9: Confirm CoresRepository is provided before Bloc initialization
RepositoryProvider forCoresRepositoryis declared inlib/di/app_repository_providers.dart(line 26) andAppBlocProviders.providers()is called after includingAppRepositoryProviders.providers(). DI correctly bindsCoresRepositorybeforeCoresBloccreation. No changes needed.lib/di/app_repository_providers.dart (1)
26-28: Approve: CoresRepository provider wiring and initial CoresBloc load verified. CoresBloc is instantiated withRepositoryProvider.of<CoresRepository>(context)and immediately dispatchesCoresEvent.load()on creation.lib/di/di_network_module.dart (1)
39-42: Cores DI bindings — LGTMService and DataSource factories align with existing modules.
Also applies to: 55-57
test/features/cores/utils/cores_ext_test.dart (1)
11-35: Localization title checks — LGTMCovers all statuses against S.of(context) keys.
lib/di/di_initializer.config.dart (1)
14-16: Generated DI for Cores — LGTMRegistrations for CoresService → CoresDataSource → CoresRepository are present. Keep this file generated-only.
Also applies to: 20-22, 32-34, 68-70, 74-76, 80-81
lib/l10n/intl_pt.arb (1)
221-223: Remove duplicate translation key
Confirm which identifier is actually used in the UI—likelytryAgainButton—then remove the unusedretryentry (and its counterparts in other arb files) to keep a single, consistent key.lib/l10n/app_localizations.dart (1)
681-805: Verify presence oftryAgainButtonto resolve potential duplication withretry
All new localization keys are implemented across locales. Confirm thattryAgainButtonis defined in the base interface and in everyapp_localizations_*.dartbefore deciding whether to keep both or consolidate:rg -n 'String get tryAgainButton' lib/l10n/app_localizations.dart lib/l10n/app_localizations_*.dartlib/features/cores/bloc/cores_bloc.freezed.dart (2)
693-707: Immutable list exposure: LGTM.Using EqualUnmodifiableListView for cores/filteredCores is correct and prevents accidental mutation.
683-691: Ignore “dual empty” warning
Success is only emitted inside theelsebranch after checkingcores.isEmpty—empty loads always yieldCoresEmptyState—and the filter handler emitsnotFoundwhen its results are empty rather thanSuccessStatewith an empty list.Likely an incorrect or invalid review comment.
| @JsonKey() | ||
| List<CoreResource> get cores { | ||
| if (_cores is EqualUnmodifiableListView) return _cores; | ||
| // ignore: implicit_dynamic_type | ||
| return EqualUnmodifiableListView(_cores); | ||
| } | ||
|
|
||
| final List<CoreResource>? _filteredCores; | ||
| List<CoreResource>? get filteredCores { | ||
| final value = _filteredCores; | ||
| if (value == null) return null; | ||
| if (_filteredCores is EqualUnmodifiableListView) return _filteredCores; | ||
| // ignore: implicit_dynamic_type | ||
| return EqualUnmodifiableListView(value); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify required imports exist in the source part.
This file uses EqualUnmodifiableListView/DeepCollectionEquality and JsonKey via freezed_annotation. Confirm cores_bloc.dart imports them.
Also applies to: 724-728
🏁 Script executed:
#!/bin/bash
rg -n "import 'package:collection/collection.dart';" lib/features/cores/bloc/cores_bloc.dart
rg -n "import 'package:freezed_annotation/freezed_annotation.dart';" lib/features/cores/bloc/cores_bloc.dart
rg -n "^part 'cores_bloc\\.freezed\\.dart';" lib/features/cores/bloc/cores_bloc.dartLength of output: 96
Add missing collection import
In lib/features/cores/bloc/cores_bloc.dart, insert
import 'package:collection/collection.dart';to enable usage of EqualUnmodifiableListView.
🤖 Prompt for AI Agents
In lib/features/cores/bloc/cores_bloc.dart (affects generated code around
lib/features/cores/bloc/cores_bloc.freezed.dart lines 693-707), the code uses
EqualUnmodifiableListView but the collection package isn't imported; add the
missing import line "import 'package:collection/collection.dart';" at the top of
lib/features/cores/bloc/cores_bloc.dart so the EqualUnmodifiableListView symbol
is resolved and regenerate/freezed if necessary.
| Color getStatusColor(BuildContext context, String? status) { | ||
| if (status == null) return Colors.blue; | ||
|
|
||
| final loc = S.of(context); | ||
|
|
||
| final statusMap = <String, Color>{ | ||
| loc.core_status_active.toLowerCase(): Colors.green, | ||
| loc.core_status_lost.toLowerCase(): Colors.red, | ||
| loc.core_status_inactive.toLowerCase(): Colors.orange, | ||
| loc.core_status_unknown.toLowerCase(): Colors.grey, | ||
| }; | ||
|
|
||
| return statusMap[status.toLowerCase()] ?? Colors.blue; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stop matching on localized strings; map by status enum to avoid i18n bugs.
Using translated labels as logic keys will break when the app language changes. Normalize to CoreFilterStatus (via toStatus) and map colors by enum; default unknown/all to grey. Also avoid building a map each call.
Apply this diff:
Color getStatusColor(BuildContext context, String? status) {
- if (status == null) return Colors.blue;
-
- final loc = S.of(context);
-
- final statusMap = <String, Color>{
- loc.core_status_active.toLowerCase(): Colors.green,
- loc.core_status_lost.toLowerCase(): Colors.red,
- loc.core_status_inactive.toLowerCase(): Colors.orange,
- loc.core_status_unknown.toLowerCase(): Colors.grey,
- };
-
- return statusMap[status.toLowerCase()] ?? Colors.blue;
+ if (status == null) return Colors.grey;
+ switch (status.toStatus()) {
+ case CoreFilterStatus.active:
+ return Colors.green;
+ case CoreFilterStatus.lost:
+ return Colors.red;
+ case CoreFilterStatus.inactive:
+ return Colors.orange;
+ case CoreFilterStatus.all:
+ case CoreFilterStatus.unknown:
+ default:
+ return Colors.grey;
+ }
}Add these imports outside the shown range:
import 'package:flutter_bloc_app_template/features/cores/model/core_filter_status.dart';
import 'package:flutter_bloc_app_template/features/cores/utils/cores_ext.dart';🤖 Prompt for AI Agents
In lib/features/cores/utils/core_utils.dart around lines 5 to 18, stop using
localized strings as keys: instead import the CoreFilterStatus enum and the
cores_ext extension (add the two imports indicated) and convert the incoming
status string to the enum via toStatus; map the enum to a Color (use a switch or
a single static const map defined once, not rebuilt each call) returning grey
for unknown/all and avoid using translated labels; ensure the function handles
null by returning blue if that behavior should remain or change to grey per
comment (use grey for unknown/all as requested).
| extension CoreFilterStatusStringX on String? { | ||
| CoreFilterStatus toStatus() { | ||
| switch (this?.toLowerCase()) { | ||
| case 'active': | ||
| return CoreFilterStatus.active; | ||
| case 'lost': | ||
| return CoreFilterStatus.lost; | ||
| case 'inactive': | ||
| return CoreFilterStatus.inactive; | ||
| case 'unknown': | ||
| return CoreFilterStatus.unknown; | ||
| case 'all': | ||
| case null: | ||
| return CoreFilterStatus.unknown; | ||
| default: | ||
| return CoreFilterStatus.unknown; | ||
| } | ||
| } |
There was a problem hiding this comment.
'all' should map to CoreFilterStatus.all (not unknown)
This affects filtering logic and UI labels.
CoreFilterStatus toStatus() {
switch (this?.toLowerCase()) {
@@
case 'unknown':
return CoreFilterStatus.unknown;
case 'all':
- case null:
- return CoreFilterStatus.unknown;
+ return CoreFilterStatus.all;
+ case null:
+ return CoreFilterStatus.unknown;
default:
return CoreFilterStatus.unknown;
}
}📝 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.
| extension CoreFilterStatusStringX on String? { | |
| CoreFilterStatus toStatus() { | |
| switch (this?.toLowerCase()) { | |
| case 'active': | |
| return CoreFilterStatus.active; | |
| case 'lost': | |
| return CoreFilterStatus.lost; | |
| case 'inactive': | |
| return CoreFilterStatus.inactive; | |
| case 'unknown': | |
| return CoreFilterStatus.unknown; | |
| case 'all': | |
| case null: | |
| return CoreFilterStatus.unknown; | |
| default: | |
| return CoreFilterStatus.unknown; | |
| } | |
| } | |
| extension CoreFilterStatusStringX on String? { | |
| CoreFilterStatus toStatus() { | |
| switch (this?.toLowerCase()) { | |
| case 'active': | |
| return CoreFilterStatus.active; | |
| case 'lost': | |
| return CoreFilterStatus.lost; | |
| case 'inactive': | |
| return CoreFilterStatus.inactive; | |
| case 'unknown': | |
| return CoreFilterStatus.unknown; | |
| case 'all': | |
| return CoreFilterStatus.all; | |
| case null: | |
| return CoreFilterStatus.unknown; | |
| default: | |
| return CoreFilterStatus.unknown; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In lib/features/cores/utils/cores_ext.dart around lines 22 to 39, the switch
currently maps the string 'all' to CoreFilterStatus.unknown; change the 'all'
case to return CoreFilterStatus.all instead of unknown, leaving the null case
(and default) mapping as appropriate (e.g., null -> CoreFilterStatus.unknown) so
filtering and UI labels correctly reflect the "all" state.
| children: core.missions?.take(2).map((mission) { | ||
| return Chip( | ||
| label: Text( | ||
| mission.name ?? loc.na, | ||
| style: const TextStyle(fontSize: 11), | ||
| ), | ||
| visualDensity: VisualDensity.compact, | ||
| ); | ||
| }).toList() ?? | ||
| [], |
There was a problem hiding this comment.
Null-safety bug: calling .map on a nullable iterable
If core.missions is null, core.missions?.take(2) yields null and .map throws before ?? [] applies.
- children: core.missions?.take(2).map((mission) {
- return Chip(
- label: Text(
- mission.name ?? loc.na,
- style: const TextStyle(fontSize: 11),
- ),
- visualDensity: VisualDensity.compact,
- );
- }).toList() ??
- [],
+ children: (core.missions ?? const <MissionResource>[])
+ .take(2)
+ .map(
+ (mission) => Chip(
+ label: Text(
+ mission.name ?? loc.na,
+ style: const TextStyle(fontSize: 11),
+ ),
+ visualDensity: VisualDensity.compact,
+ ),
+ )
+ .toList(),📝 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.
| children: core.missions?.take(2).map((mission) { | |
| return Chip( | |
| label: Text( | |
| mission.name ?? loc.na, | |
| style: const TextStyle(fontSize: 11), | |
| ), | |
| visualDensity: VisualDensity.compact, | |
| ); | |
| }).toList() ?? | |
| [], | |
| children: (core.missions ?? const <MissionResource>[]) | |
| .take(2) | |
| .map( | |
| (mission) => Chip( | |
| label: Text( | |
| mission.name ?? loc.na, | |
| style: const TextStyle(fontSize: 11), | |
| ), | |
| visualDensity: VisualDensity.compact, | |
| ), | |
| ) | |
| .toList(), |
| String get spaceXCoresTitle => 'Супутникові ядра Falcon від SpaceX'; | ||
|
|
There was a problem hiding this comment.
Fix mistranslation of SpaceX cores title
"Супутникові ядра" implies “satellite cores.” Suggested:
- String get spaceXCoresTitle => 'Супутникові ядра Falcon від SpaceX';
+ String get spaceXCoresTitle => 'Ядра Falcon від SpaceX';📝 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.
| String get spaceXCoresTitle => 'Супутникові ядра Falcon від SpaceX'; | |
| String get spaceXCoresTitle => 'Ядра Falcon від SpaceX'; |
🤖 Prompt for AI Agents
In lib/l10n/app_localizations_uk.dart around lines 385 to 386, the Ukrainian
string "Супутникові ядра Falcon від SpaceX" mistranslates "SpaceX cores" as
"satellite cores"; replace the value with a correct translation such as "Ядра
Falcon від SpaceX" (or "Ядра Falcon компанії SpaceX" if you want to include
"company") so the string accurately reflects rocket cores rather than
satellites.
lib/l10n/intl_de.arb
Outdated
| "core_status_active": "aktiv", | ||
| "core_status_lost": "verloren", | ||
| "core_status_inactive": "inaktiv", | ||
| "core_status_unknown": "unbekannt", | ||
| "errorLoadingCores": "Fehler beim Laden der Kerne", | ||
| "retry": "Erneut versuchen", | ||
| "firstLaunch": "Erststart", | ||
| "missions": "{count} Missionen", | ||
| "reuses": "{count} Wiederverwendungen", | ||
| "unknown": "Unbekannt", | ||
| "na": "N/V", | ||
| "core_filter_status_all": "Alle", | ||
| "core_filter_status_active": "Aktiv", | ||
| "core_filter_status_lost": "Verloren", | ||
| "core_filter_status_inactive": "Inaktiv", | ||
| "core_filter_status_unknown": "Unbekannt", | ||
| "core_filter_search_hint": "Suche nach Kernen oder Missionen...", | ||
| "noCoresFound": "Keine Kerne für \"{query}\" gefunden", | ||
| "blockLabel": "Block {blockNumber}", | ||
| "spaceXCoresTitle": "SpaceX Falcon-Kerne", | ||
| "coresLabel": "Kerne" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider ICU plurals for count-based strings
For better grammar and i18n, switch missions/reuses to ICU plurals. This avoids outputs like “1 Missionen”.
Apply in this ARB (and mirror in all locales):
- "missions": "{count} Missionen",
+ "missions": "{count, plural, one {# Mission} other {# Missionen}}",
- "reuses": "{count} Wiederverwendungen",
+ "reuses": "{count, plural, one {# Wiederverwendung} other {# Wiederverwendungen}}",📝 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.
| "core_status_active": "aktiv", | |
| "core_status_lost": "verloren", | |
| "core_status_inactive": "inaktiv", | |
| "core_status_unknown": "unbekannt", | |
| "errorLoadingCores": "Fehler beim Laden der Kerne", | |
| "retry": "Erneut versuchen", | |
| "firstLaunch": "Erststart", | |
| "missions": "{count} Missionen", | |
| "reuses": "{count} Wiederverwendungen", | |
| "unknown": "Unbekannt", | |
| "na": "N/V", | |
| "core_filter_status_all": "Alle", | |
| "core_filter_status_active": "Aktiv", | |
| "core_filter_status_lost": "Verloren", | |
| "core_filter_status_inactive": "Inaktiv", | |
| "core_filter_status_unknown": "Unbekannt", | |
| "core_filter_search_hint": "Suche nach Kernen oder Missionen...", | |
| "noCoresFound": "Keine Kerne für \"{query}\" gefunden", | |
| "blockLabel": "Block {blockNumber}", | |
| "spaceXCoresTitle": "SpaceX Falcon-Kerne", | |
| "coresLabel": "Kerne" | |
| "missions": "{count, plural, one {# Mission} other {# Missionen}}", | |
| "reuses": "{count, plural, one {# Wiederverwendung} other {# Wiederverwendungen}}", |
🤖 Prompt for AI Agents
In lib/l10n/intl_de.arb around lines 217 to 237, the keys "missions" and
"reuses" are simple "{count} X" strings which produce incorrect German forms for
singular; replace them with ICU plural syntax (e.g. {count, plural, one {#
Mission} other {# Missionen}} and similarly for Wiederverwendungen) so
singular/plural are correct, keep the {count} placeholder semantics and ensure
callers pass an integer count; apply the same ICU-plural change to the
corresponding keys in all other locale ARB files.
| "core_status_active": "active", | ||
| "core_status_lost": "lost", | ||
| "core_status_inactive": "inactive", | ||
| "core_status_unknown": "unknown", | ||
| "errorLoadingCores": "Error loading cores", | ||
| "retry": "Retry", | ||
| "firstLaunch": "First Launch", | ||
| "missions": "{count} missions", | ||
| "reuses": "{count} reuses", | ||
| "unknown": "Unknown", | ||
| "na": "N/A", | ||
| "core_filter_status_all": "All", | ||
| "core_filter_status_active": "Active", | ||
| "core_filter_status_lost": "Lost", | ||
| "core_filter_status_inactive": "Inactive", | ||
| "core_filter_status_unknown": "Unknown", | ||
| "core_filter_search_hint": "Search cores or missions...", | ||
| "noCoresFound": "No cores found for \"{query}\"", | ||
| "blockLabel": "Block {blockNumber}", | ||
| "spaceXCoresTitle": "SpaceX Falcon Cores", | ||
| "coresLabel": "Cores" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Use ICU plurals for count-based labels
Prevents “1 missions/1 reuses” in UI and enables correct pluralization across locales.
Apply:
- "missions": "{count} missions",
+ "missions": "{count, plural, one {# mission} other {# missions}}",
- "reuses": "{count} reuses",
+ "reuses": "{count, plural, one {# reuse} other {# reuses}}",Run to ensure all locales define the same keys/types and to spot missing ICU forms:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
root="lib/l10n"
en="$root/intl_en.arb"
# Gather non-metadata keys from each ARB
python - <<'PY'
import json, glob, os, sys
root="lib/l10n"
files=sorted(glob.glob(os.path.join(root,"intl_*.arb")))
arb = {}
for f in files:
with open(f, encoding="utf-8") as fh:
data=json.load(fh)
keys={k for k in data.keys() if not k.startswith("@")}
arb[os.path.basename(f)]=keys
all_keys=set.union(*arb.values())
base='intl_en.arb'
missing={}
for f, keys in arb.items():
miss=all_keys-keys
if miss: missing[f]=sorted(miss)
if missing:
print("Missing keys by file:")
for f, ks in missing.items():
print(f" - {f}: {', '.join(ks)}")
else:
print("All ARB files share the same keys.")
# Quick heuristic: print keys likely needing plurals
for f in files:
with open(f, encoding="utf-8") as fh:
data=json.load(fh)
for k,v in data.items():
if k in ("missions","reuses") and not k.startswith("@"):
if isinstance(v,str) and "plural" not in v:
print(f"[Plural check] {os.path.basename(f)} -> {k} lacks ICU plural")
PYLength of output: 539
Enforce ICU plurals and synchronize locale keys
- Replace
missionsandreusesentries with ICU plural syntax in everyintl_*.arbfile:- "missions": "{count} missions", + "missions": "{count, plural, one {# mission} other {# missions}}", - "reuses": "{count} reuses", + "reuses": "{count, plural, one {# reuse} other {# reuses}}",
- Add the missing
experimentalThemeTitlekey tointl_de.arbandintl_pt.arbso all locale files share the same set of keys.
📝 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.
| "core_status_active": "active", | |
| "core_status_lost": "lost", | |
| "core_status_inactive": "inactive", | |
| "core_status_unknown": "unknown", | |
| "errorLoadingCores": "Error loading cores", | |
| "retry": "Retry", | |
| "firstLaunch": "First Launch", | |
| "missions": "{count} missions", | |
| "reuses": "{count} reuses", | |
| "unknown": "Unknown", | |
| "na": "N/A", | |
| "core_filter_status_all": "All", | |
| "core_filter_status_active": "Active", | |
| "core_filter_status_lost": "Lost", | |
| "core_filter_status_inactive": "Inactive", | |
| "core_filter_status_unknown": "Unknown", | |
| "core_filter_search_hint": "Search cores or missions...", | |
| "noCoresFound": "No cores found for \"{query}\"", | |
| "blockLabel": "Block {blockNumber}", | |
| "spaceXCoresTitle": "SpaceX Falcon Cores", | |
| "coresLabel": "Cores" | |
| "core_status_active": "active", | |
| "core_status_lost": "lost", | |
| "core_status_inactive": "inactive", | |
| "core_status_unknown": "unknown", | |
| "errorLoadingCores": "Error loading cores", | |
| "retry": "Retry", | |
| "firstLaunch": "First Launch", | |
| "missions": "{count, plural, one {# mission} other {# missions}}", | |
| "reuses": "{count, plural, one {# reuse} other {# reuses}}", | |
| "unknown": "Unknown", | |
| "na": "N/A", | |
| "core_filter_status_all": "All", | |
| "core_filter_status_active": "Active", | |
| "core_filter_status_lost": "Lost", | |
| "core_filter_status_inactive": "Inactive", | |
| "core_filter_status_unknown": "Unknown", | |
| "core_filter_search_hint": "Search cores or missions...", | |
| "noCoresFound": "No cores found for \"{query}\"", | |
| "blockLabel": "Block {blockNumber}", | |
| "spaceXCoresTitle": "SpaceX Falcon Cores", | |
| "coresLabel": "Cores" |
🤖 Prompt for AI Agents
In lib/l10n/intl_en.arb around lines 354 to 374, replace the simple "missions"
and "reuses" entries with proper ICU plural syntax (e.g. a single key using the
plural ICU form with zero/one/other variants) and ensure the plural placeholders
match across locales; additionally add the missing "experimentalThemeTitle" key
to lib/l10n/intl_de.arb and lib/l10n/intl_pt.arb so all intl_*.arb files contain
the same set of keys, keeping key names identical and values
translated/placeholdered as needed.
| "core_status_active": "активний", | ||
| "core_status_lost": "втрачений", | ||
| "core_status_inactive": "неактивний", | ||
| "core_status_unknown": "невідомий", | ||
| "errorLoadingCores": "Помилка завантаження ядер", | ||
| "retry": "Повторити", | ||
| "firstLaunch": "Перший запуск", | ||
| "missions": "{count} місій", | ||
| "reuses": "{count} повторів", | ||
| "unknown": "Невідомо", | ||
| "na": "Н/Д", | ||
| "core_filter_status_all": "Усі", | ||
| "core_filter_status_active": "Активний", | ||
| "core_filter_status_lost": "Втрачений", | ||
| "core_filter_status_inactive": "Неактивний", | ||
| "core_filter_status_unknown": "Невідомо", | ||
| "core_filter_search_hint": "Пошук ядер або місій...", | ||
| "noCoresFound": "Ядра за запитом \"{query}\" не знайдено", | ||
| "blockLabel": "Блок {blockNumber}", | ||
| "spaceXCoresTitle": "Супутникові ядра Falcon від SpaceX", | ||
| "coresLabel": "Ядра" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix mistranslation and plural/grammar issues (user-facing text)
- “spaceXCoresTitle” should not say “Супутникові”.
- Use proper ICU plurals for
missions/reuses. - Genitive plural in “noCoresFound”.
Apply:
- "missions": "{count} місій",
+ "missions": "{count, plural, one {# місія} few {# місії} many {# місій} other {# місії}}",
- "reuses": "{count} повторів",
+ "reuses": "{count, plural, one {# повторне використання} few {# повторні використання} many {# повторних використань} other {# повторних використань}}",
- "noCoresFound": "Ядра за запитом \"{query}\" не знайдено",
+ "noCoresFound": "Ядер за запитом «{query}» не знайдено",
- "spaceXCoresTitle": "Супутникові ядра Falcon від SpaceX",
+ "spaceXCoresTitle": "Ядра Falcon від SpaceX",📝 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.
| "core_status_active": "активний", | |
| "core_status_lost": "втрачений", | |
| "core_status_inactive": "неактивний", | |
| "core_status_unknown": "невідомий", | |
| "errorLoadingCores": "Помилка завантаження ядер", | |
| "retry": "Повторити", | |
| "firstLaunch": "Перший запуск", | |
| "missions": "{count} місій", | |
| "reuses": "{count} повторів", | |
| "unknown": "Невідомо", | |
| "na": "Н/Д", | |
| "core_filter_status_all": "Усі", | |
| "core_filter_status_active": "Активний", | |
| "core_filter_status_lost": "Втрачений", | |
| "core_filter_status_inactive": "Неактивний", | |
| "core_filter_status_unknown": "Невідомо", | |
| "core_filter_search_hint": "Пошук ядер або місій...", | |
| "noCoresFound": "Ядра за запитом \"{query}\" не знайдено", | |
| "blockLabel": "Блок {blockNumber}", | |
| "spaceXCoresTitle": "Супутникові ядра Falcon від SpaceX", | |
| "coresLabel": "Ядра" | |
| "firstLaunch": "Перший запуск", | |
| "missions": "{count, plural, one {# місія} few {# місії} many {# місій} other {# місії}}", | |
| "reuses": "{count, plural, one {# повторне використання} few {# повторні використання} many {# повторних використань} other {# повторних використань}}", | |
| "unknown": "Невідомо", | |
| "na": "Н/Д", | |
| "noCoresFound": "Ядер за запитом «{query}» не знайдено", | |
| "blockLabel": "Блок {blockNumber}", | |
| "spaceXCoresTitle": "Ядра Falcon від SpaceX", | |
| "coresLabel": "Ядра" |
🤖 Prompt for AI Agents
lib/l10n/intl_uk.arb lines 350-370: update translations to correct wording,
grammar and use ICU plurals—change "spaceXCoresTitle" from "Супутникові ядра
Falcon від SpaceX" to a neutral "Ядра Falcon від SpaceX"; replace simple count
strings "missions" and "reuses" with ICU plural forms using Ukrainian plural
categories (one, few, many, other) and the correct stems (місія / місії / місій
and повтор / повтори / повторів or whichever project-preferred stem), and change
"noCoresFound" to use the genitive plural form "Ядер за запитом \"{query}\" не
знайдено" (or reorder to "За запитом \"{query}\" ядра не знайдено" if preferred)
so the noun is in genitive plural.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
test/features/cores/widget/core_item_widget_test.dart (6)
29-37: Stabilize locale to avoid i18n flakiness.Force a known locale so text-based assertions (e.g., “Block”) don’t depend on the runner’s environment.
- return MaterialApp( + return MaterialApp( + locale: const Locale('en'), localizationsDelegates: appLocalizationsDelegates, supportedLocales: appSupportedLocales,
39-46: Scope the “Block” assertion to the widget subtree.Prevents false positives from other widgets rendering “Block”.
- expect(find.text('B1049'), findsOneWidget); - expect(find.textContaining('Block'), findsOneWidget); + expect(find.text('B1049'), findsOneWidget); + expect( + find.descendant( + of: find.byType(CoreItemWidget), + matching: find.textContaining('Block'), + ), + findsOneWidget, + );
48-60: Assert the actual status color equals getStatusColor; avoid null-only check.The current check passes even if the color is wrong but non-null. Compare against the computed expected and read the effective style from RichText.
final statusFinder = find.text('active'); expect(statusFinder, findsOneWidget); - final textWidget = tester.widget<Text>(statusFinder); - final color = textWidget.style?.color; - // should match getStatusColor(context, 'active') - expect(color, isNotNull); + final context = tester.element(find.byType(CoreItemWidget)); + final expected = getStatusColor(context, 'active'); + final richText = tester.widget<RichText>( + find.descendant(of: statusFinder, matching: find.byType(RichText)), + ); + final effectiveColor = (richText.text as TextSpan).style?.color; + expect(effectiveColor, equals(expected));
72-81: Also assert reuse count is rendered.The test name mentions reuse count but it’s not verified. Add an assertion tied to how the widget displays it (text, chip, badge, etc.). If possible, expose a Key (e.g., Key('core_reuse_count')) on that UI element for a stable finder.
Do you expect a specific format (e.g., “2x”, “Reused 2”, or an icon + number)? If you share it, I can provide an exact matcher.
83-97: Limit Chip count to CoreItemWidget to reduce coupling.Avoid counting chips from unrelated widgets.
- expect(find.byType(Chip), findsNWidgets(2)); + expect( + find.descendant( + of: find.byType(CoreItemWidget), + matching: find.byType(Chip), + ), + findsNWidgets(2), + );
99-106: Strengthen the “no block” assertion and decouple from English strings.Scope the finder to the widget subtree and consider adding a Key to the block container in the widget for language-agnostic tests.
- expect(find.textContaining('Block'), findsNothing); + expect( + find.descendant( + of: find.byType(CoreItemWidget), + matching: find.textContaining('Block'), + ), + findsNothing, + );If you can tag the block UI with Key('core_block'), replace the text finder with
find.byKey(const Key('core_block')).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/features/cores/widget/core_item_widget_test.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Flutter Integration Tests (31)
- GitHub Check: App
🔇 Additional comments (1)
test/features/cores/widget/core_item_widget_test.dart (1)
62-70: LGTM: formatted first-launch assertion matches widget context.Good use of the same formatter and context to avoid locale/timezone drift.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/models/core/core_resource_test.dart (4)
66-76: Add an identity check to strengthen copyWith testAlso assert the original instance remains unchanged.
final copied = core.copyWith( coreSerial: 'B1051', block: 6, ); expect(copied.coreSerial, 'B1051'); expect(copied.block, 6); expect(copied.status, isNull); // other fields stay the same + expect(copied, isNot(same(core))); // new instance, not mutated + expect(core.coreSerial, 'B1049'); // original unchanged + expect(core.block, 5);
78-85: Remove redundant equality testThis duplicates the earlier “supports equality” test; trimming keeps the suite lean.
- test('should check equality', () { - final core1 = const CoreResource(coreSerial: 'B1049', block: 5); - final core2 = const CoreResource(coreSerial: 'B1049', block: 5); - final core3 = const CoreResource(coreSerial: 'B1051', block: 5); - - expect(core1, core2); - expect(core1 == core3, isFalse); - });
87-103: Props assertion is brittle; consider behavior-focused checksAsserting the entire props list tightly couples tests to internal ordering of Equatable props. Prefer testing equality semantics (already covered) and a few sentinel values if needed. Keep as-is if intentional to lock the contract.
105-110: Rewrite immutability test to actually verify const canonicalizationCurrent test doesn’t validate immutability. Use identity for two identical consts.
- test('is immutable', () { - // trying to reassign a const field will cause compile-time error, - // so here we just verify object identity - const modified = CoreResource(coreSerial: 'Merlin2C'); - expect(modified.coreSerial, 'Merlin2C'); - }); + test('is immutable (const canonicalization)', () { + const a = CoreResource(coreSerial: 'Merlin2C'); + const b = CoreResource(coreSerial: 'Merlin2C'); + expect(a, same(b)); // identical const instances + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/models/core/core_resource_test.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: App
- GitHub Check: Flutter Integration Tests (31)
🔇 Additional comments (4)
test/models/core/core_resource_test.dart (4)
5-20: MissionResource tests look solidEquality and props coverage are correct and concise.
23-37: CoreResource fixtures are well-shapedGood representative data including nested MissionResource and all numeric/boolean flags for parity checks.
Also applies to: 39-53
55-58: Minimal inequality fixture is appropriateUsing a sparse CoreResource for negative equality works well.
60-64: Equality assertions are correctThis covers value semantics and hashCode. No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
test/models/core/core_resource_test.dart (8)
23-53: Reduce test fixture duplication for core1/core2.Define a small factory to DRY up identical CoreResource instances and ease future changes.
Example helper to add near the top of the file:
CoreResource _coreFixture() => const CoreResource( coreSerial: 'Merlin2C', block: 1, status: 'lost', originalLaunch: '2008-09-28T23:15:00.000Z', originalLaunchUnix: 1222643700, missions: [MissionResource(name: 'RatSat', flight: 4)], reuseCount: 0, rtlsAttempts: 0, rtlsLandings: 0, asdsAttempts: 0, asdsLandings: 0, waterLanding: false, details: 'Some details', );
66-76: Also assert original remains unchanged after copyWith.This tightens immutability expectations.
Apply:
final copied = core.copyWith( coreSerial: 'B1051', block: 6, ); expect(copied.coreSerial, 'B1051'); expect(copied.block, 6); expect(copied.status, isNull); // other fields stay the same + // original unchanged + expect(core.coreSerial, 'B1049'); + expect(core.block, 5);
78-91: Avoid asserting on identity for copyWith() with no args.Returning
thisis a valid optimization and shouldn’t fail the test.Apply:
expect(copied, equals(core)); // Equatable should ensure equality - expect( - identical(copied, core), isFalse); // should not be the same reference + // Identity is unspecified; copyWith() may return the same instance if no changes.
92-99: Remove redundant equality test.This repeats the earlier equality scenario and adds little signal.
Apply:
- test('should check equality', () { - final core1 = const CoreResource(coreSerial: 'B1049', block: 5); - final core2 = const CoreResource(coreSerial: 'B1049', block: 5); - final core3 = const CoreResource(coreSerial: 'B1051', block: 5); - - expect(core1, core2); - expect(core1 == core3, isFalse); - });
101-118: Differentiate duplicate test names.Rename to clarify that this is the const-fixture variant.
Apply:
- test('props contains all fields', () { + test('props contains all fields (const)', () {
119-153: Make test name distinct and align date/unix for clarity.Also prefer an ISO 8601 timestamp matching the unix value to avoid confusion for readers.
Apply:
- test('props contains all fields', () { + test('props contains all fields (non-const)', () { @@ - originalLaunch: '2020-06-30', - originalLaunchUnix: 1593504000, + originalLaunch: '2020-06-30T00:00:00.000Z', + originalLaunchUnix: 1593475200,
155-170: Avoid asserting hashCode inequality.Hash collisions are valid; only equality must imply equal hashCodes.
Apply:
// core3 differs in coreSerial expect(core1, isNot(equals(core3))); - expect(core1.hashCode, isNot(equals(core3.hashCode))); + // Do not assert on hashCode inequality to avoid brittleness.
172-177: Drop the “is immutable” test or repurpose.As written, it only re-reads a field; immutability is better covered by copyWith tests.
Apply:
- test('is immutable', () { - // trying to reassign a const field will cause compile-time error, - // so here we just verify object identity - const modified = CoreResource(coreSerial: 'Merlin2C'); - expect(modified.coreSerial, 'Merlin2C'); - }); + // (Removed) Redundant immutability test; immutability is exercised via copyWith cases above.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/models/core/core_resource_test.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Flutter Integration Tests (31)
- GitHub Check: App
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
test/models/core/core_resource_test.dart (4)
5-14: Solid equality coverage for MissionResource.Good use of consts and hashCode checks.
16-19: Props assertion looks good.Directly locking the props order is fine here.
55-58: LGTM: minimal unequal comparator.A lean instance (different coreSerial) cleanly exercises inequality.
60-64: Equality/hashCode check is clear and sufficient.Covers the primary path well.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
test/features/cores/bloc/cores_bloc_test.dart (3)
49-61: Build a fresh CoresBloc per test; don’t reuse a shared instance (bloc_test auto-closes).Reusing a single bloc across tests causes flakiness and “Bloc has been closed” errors. Construct a new bloc in each build: and remove tearDown close.
void main() { late CoresRepository repository; - late CoresBloc bloc; setUp(() { repository = MockCoresRepository(); - bloc = CoresBloc(repository); }); - - tearDown(() { - bloc.close(); - });
66-74: Return a new bloc from every build:, and seed allCores via cascade where needed.Replace return bloc; with a new instance. For filter tests that prefill allCores, set it on the freshly built bloc.
@@ - return bloc; + return CoresBloc(repository); @@ - return bloc; + return CoresBloc(repository); @@ - return bloc; + return CoresBloc(repository); @@ - return bloc; + return CoresBloc(repository); @@ - return bloc; + return CoresBloc(repository); @@ - return bloc; + return CoresBloc(repository); @@ - bloc.allCores = testCores; - return bloc; + return CoresBloc(repository)..allCores = testCores; @@ - bloc.allCores = testCores; - return bloc; + return CoresBloc(repository)..allCores = testCores; @@ - bloc.allCores = testCores; - return bloc; + return CoresBloc(repository)..allCores = testCores; @@ - bloc.allCores = testCores; - return bloc; + return CoresBloc(repository)..allCores = testCores; @@ - bloc.allCores = testCores; - return bloc; + return CoresBloc(repository)..allCores = testCores; @@ - bloc.allCores = []; - return bloc; + return CoresBloc(repository)..allCores = []; @@ - bloc.allCores = testCores; - return bloc; + return CoresBloc(repository)..allCores = testCores; @@ - bloc.allCores = testCores; - return bloc; + return CoresBloc(repository)..allCores = testCores;Also applies to: 90-98, 106-114, 145-152, 166-172, 186-193, 203-207, 221-226, 240-245, 262-267, 284-289, 306-311, 321-325, 340-344
385-387: Instantiate a local bloc in these state-management tests.Prevents coupling to a global instance and double-closing.
- test('initial state is loading', () { - expect(bloc.state, const CoresState.loading()); - }); + test('initial state is loading', () { + final bloc = CoresBloc(repository); + expect(bloc.state, const CoresState.loading()); + bloc.close(); + }); @@ - test('allCores is initially empty', () { - expect(bloc.allCores, isEmpty); - }); + test('allCores is initially empty', () { + final bloc = CoresBloc(repository); + expect(bloc.allCores, isEmpty); + bloc.close(); + });Also applies to: 389-391
🧹 Nitpick comments (3)
codecov.yml (1)
6-6: Prefer keeping project target auto and enforce patch coverage instead of dropping to 50%.Lowering the overall bar to 50% risks silent regressions. Keep the long-term bar steady and gate new code via a stricter patch status.
Apply within this hunk:
- target: 50% + target: autoAdd outside this hunk to gate new/changed lines:
coverage: status: patch: default: target: 85% threshold: 0%test/features/cores/bloc/cores_bloc_test.dart (2)
14-47: Prefer const for test fixtures.Make testCores const for immutability and faster comparisons.
-final testCores = [ +const testCores = [
395-419: Avoid tying equality tests to concrete state classes.If states are Freezed unions or change class names, these tests become brittle. Prefer factory constructors or matchers.
Example alternative:
expect(const CoresState.loading(), const CoresState.loading()); expect(CoresState.success(cores: testCores), CoresState.success(cores: testCores)); expect(const CoresState.empty(), const CoresState.empty()); expect(const CoresState.error('Error message'), const CoresState.error('Error message'));Or use:
expect(const CoresState.loading(), isA<CoresState>());Can you confirm whether CoresLoadingState/CoresEmptyState/CoresErrorState are public API and stable?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
codecov.yml(1 hunks)test/features/cores/bloc/cores_bloc_test.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Flutter Integration Tests (31)
- GitHub Check: App
| test('stores cores in allCores when load succeeds', () async { | ||
| when(() => repository.getCores( | ||
| hasId: true, | ||
| limit: null, | ||
| offset: null, | ||
| )).thenAnswer((_) async => testCores); | ||
|
|
||
| bloc.add(const CoresLoadEvent()); | ||
| await expectLater( | ||
| bloc.stream, | ||
| emitsInOrder([ | ||
| const CoresState.loading(), | ||
| CoresState.success(cores: testCores, filteredCores: testCores), | ||
| ]), | ||
| ); | ||
|
|
||
| expect(bloc.allCores, equals(testCores)); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make this test self-contained: create/close a local bloc.
Removes reliance on the shared bloc and ensures cleanup.
test('stores cores in allCores when load succeeds', () async {
when(() => repository.getCores(
hasId: true,
limit: null,
offset: null,
)).thenAnswer((_) async => testCores);
- bloc.add(const CoresLoadEvent());
+ final bloc = CoresBloc(repository);
+ bloc.add(const CoresLoadEvent());
await expectLater(
bloc.stream,
emitsInOrder([
const CoresState.loading(),
CoresState.success(cores: testCores, filteredCores: testCores),
]),
);
expect(bloc.allCores, equals(testCores));
+ await bloc.close();
});📝 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.
| test('stores cores in allCores when load succeeds', () async { | |
| when(() => repository.getCores( | |
| hasId: true, | |
| limit: null, | |
| offset: null, | |
| )).thenAnswer((_) async => testCores); | |
| bloc.add(const CoresLoadEvent()); | |
| await expectLater( | |
| bloc.stream, | |
| emitsInOrder([ | |
| const CoresState.loading(), | |
| CoresState.success(cores: testCores, filteredCores: testCores), | |
| ]), | |
| ); | |
| expect(bloc.allCores, equals(testCores)); | |
| }); | |
| test('stores cores in allCores when load succeeds', () async { | |
| when(() => repository.getCores( | |
| hasId: true, | |
| limit: null, | |
| offset: null, | |
| )).thenAnswer((_) async => testCores); | |
| final bloc = CoresBloc(repository); | |
| bloc.add(const CoresLoadEvent()); | |
| await expectLater( | |
| bloc.stream, | |
| emitsInOrder([ | |
| const CoresState.loading(), | |
| CoresState.success(cores: testCores, filteredCores: testCores), | |
| ]), | |
| ); | |
| expect(bloc.allCores, equals(testCores)); | |
| await bloc.close(); | |
| }); |
🤖 Prompt for AI Agents
In test/features/cores/bloc/cores_bloc_test.dart around lines 122 to 139, the
test currently relies on a shared bloc; make it self-contained by creating a
local CoresBloc instance inside the test (using the same mocked repository), use
that local bloc for bloc.add and bloc.stream assertions, and ensure you close it
at the end of the test (await bloc.close()) so resources are cleaned up; replace
references to the shared bloc with the local variable and assert
localBloc.allCores equals testCores.
| blocTest<CoresBloc, CoresState>( | ||
| 'handles null values in core data gracefully', | ||
| build: () { | ||
| bloc.allCores = [ | ||
| const CoreResource( | ||
| coreSerial: null, | ||
| missions: null, | ||
| status: null, | ||
| ), | ||
| const CoreResource( | ||
| coreSerial: 'B1051', | ||
| missions: [MissionResource(name: null, flight: 1)], | ||
| status: 'active', | ||
| ), | ||
| ]; | ||
| return bloc; | ||
| }, | ||
| act: (bloc) => bloc.add( | ||
| const CoresFilterEvent(searchQuery: 'B1051'), | ||
| ), | ||
| expect: () => [ | ||
| CoresState.success( | ||
| cores: bloc.allCores, | ||
| filteredCores: [bloc.allCores[1]], | ||
| searchQuery: 'B1051', | ||
| statusFilter: null, | ||
| ), | ||
| ], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Decouple from the global bloc in this test; share a local seeded list across build/expect.
Avoid referencing bloc in expect. Define a local coresWithNulls and use it in both build and expect.
- blocTest<CoresBloc, CoresState>(
+ blocTest<CoresBloc, CoresState>(
'handles null values in core data gracefully',
- build: () {
- bloc.allCores = [
- const CoreResource(
- coreSerial: null,
- missions: null,
- status: null,
- ),
- const CoreResource(
- coreSerial: 'B1051',
- missions: [MissionResource(name: null, flight: 1)],
- status: 'active',
- ),
- ];
- return bloc;
- },
+ build: () {
+ final coresWithNulls = <CoreResource>[
+ const CoreResource(coreSerial: null, missions: null, status: null),
+ const CoreResource(
+ coreSerial: 'B1051',
+ missions: [MissionResource(name: null, flight: 1)],
+ status: 'active',
+ ),
+ ];
+ return CoresBloc(repository)..allCores = coresWithNulls;
+ },
act: (bloc) => bloc.add(
const CoresFilterEvent(searchQuery: 'B1051'),
),
expect: () => [
- CoresState.success(
- cores: bloc.allCores,
- filteredCores: [bloc.allCores[1]],
- searchQuery: 'B1051',
- statusFilter: null,
- ),
+ isA<CoresSuccessState>()
+ .having((s) => s.searchQuery, 'searchQuery', 'B1051')
+ .having((s) => s.statusFilter, 'statusFilter', null)
+ .having((s) => s.filteredCores.map((e) => e.coreSerial).toList(),
+ 'filtered coreSerials', ['B1051']),
],
);Note: Using a matcher avoids relying on list identity in Equatable props. If you’d rather keep value equality, ensure states compare list contents deeply.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/features/cores/bloc/cores_bloc_test.dart around lines 353-381, the test
currently references the shared bloc instance in the expect block which couples
the assertion to global state; instead, create a local variable (e.g.,
coresWithNulls) inside the test and seed bloc.allCores with that local list in
build, then reference the same local list in expect (or use a matcher like
contains/equals on contents) so the test does not read from the bloc instance
during assertion and avoids list identity issues—alternatively ensure the
CoresState equality compares list contents deeply if you must compare by value.
# Conflicts: # lib/generated/intl/messages_de.dart # lib/generated/intl/messages_en.dart # lib/generated/intl/messages_pt.dart # lib/generated/intl/messages_uk.dart # lib/generated/l10n.dart # lib/l10n/app_localizations.dart # lib/l10n/app_localizations_de.dart # lib/l10n/app_localizations_en.dart # lib/l10n/app_localizations_pt.dart # lib/l10n/app_localizations_uk.dart # lib/l10n/intl_de.arb # lib/l10n/intl_en.arb # lib/l10n/intl_pt.arb # lib/l10n/intl_uk.arb
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
lib/features/cores/cores_screen.dart (2)
81-84: Flutter SDK compatibility: WidgetStatePropertyAll vs MaterialStatePropertyAll
WidgetStatePropertyAllrequires newer SDKs; if your pubspec allows older (e.g., >=3.19), this will fail. PreferMaterialStatePropertyAllunless you bump the SDK.Apply:
- padding: const WidgetStatePropertyAll<EdgeInsets>( + padding: const MaterialStatePropertyAll<EdgeInsets>( EdgeInsets.symmetric(horizontal: 16.0), ),To confirm constraints and usage:
#!/bin/bash rg -nC2 '^environment:' pubspec.yaml rg -n 'WidgetStatePropertyAll|MaterialStatePropertyAll' -g '!**/build/**'
64-66: Dispose the TextEditingController and update trailing clear iconController isn’t disposed and the clear icon won’t update as text changes.
Apply:
class _CoresSearchFilterWidgetState extends State<CoresSearchFilterWidget> { final TextEditingController _searchController = TextEditingController(); CoreFilterStatus? _selectedStatus; + @override + void initState() { + super.initState(); + _searchController.addListener(() => setState(() {})); + } + + @override + void dispose() { + _searchController.dispose(); + super.dispose(); + }
🧹 Nitpick comments (11)
lib/l10n/app_localizations_pt.dart (3)
418-421: Tiny string clean-up: no need to escape double quotes inside single quotes.- return 'Nenhum núcleo encontrado para \"$query\"'; + return 'Nenhum núcleo encontrado para "$query"';
393-398: Remove duplicate translation key in Portuguese localization The gettersnaandnotAvailableinlib/l10n/app_localizations_pt.dartboth return'N/D'; drop the redundantnaproperty.
374-382: Remove unusedretrylocalization key
Theretryentry in app_localizations_pt.dart isn’t referenced anywhere (no wrapper or direct call) and duplicates thetryAgainButtonstring. Deleteretryand usetryAgainButtonuniformly to avoid redundancy and keep phrasing consistent..github/workflows/ci.yml (1)
139-140: LGTM: new cores integration tests wired into CIBoth integration tests are added consistently with existing ones and run under the same emulator profile/flavor.
Optionally exclude generated Freezed files from coverage to reduce noise:
- lcov remove: add 'lib/**/*.freezed.dart'
- very_good_coverage exclude: add **/*.freezed.dart
lib/l10n/app_localizations_en.dart (1)
378-379: Consolidate ‘retry’ and ‘tryAgainButton’ localization keys
No references toS.of(context).retryor.tryAgainButtonwere found; pick one getter and delete the redundant one for consistency.integration_test/cores_screen_integration_test.dart (2)
82-84: Assert the surfaced error text as wellStrengthen the test by verifying the error message, since the widget accepts it.
Apply:
- //expect(find.text(errorMessage), findsOneWidget); + expect(find.textContaining(errorMessage), findsOneWidget);
24-26: Close the mock bloc in tearDown to avoid stream leaksNot strictly required for a mock, but good hygiene for future changes where the widget tree might own the bloc lifecycle.
Add (outside this hunk):
tearDown(() { mockBloc.close(); });integration_test/cores_integration_live_test.dart (2)
17-23: Key reuse may conflate tab vs screen
Key('cores_screen')is used for both locating the tab and the screen. Consider distinct keys (e.g.,cores_tabfor the nav item,cores_screenfor the page) to reduce selector ambiguity.
73-75: Selector is brittle: prefer a stable key for search
find.byType(TextField)depends on SearchBar internals. Expose a dedicated Key on the SearchBar (e.g.,cores_search_field) and select by that.If you add the key in the widget, update here:
- final searchField = find.byType(TextField); + final searchField = find.byKey(const Key('cores_search_field'));lib/features/cores/cores_screen.dart (2)
77-107: Expose a stable Key on the SearchBar for testsThis enables robust selection in integration tests instead of relying on internal TextField.
Apply:
SearchBar( + key: const Key('cores_search_field'), controller: _searchController,
39-51: Optional: allow refresh from non-success statesLet users retry after errors/empty/not-found without additional navigation.
Example change:
floatingActionButton: BlocBuilder<CoresBloc, CoresState>( builder: (context, state) { final canRefresh = state is! CoresLoadingState; return canRefresh ? FloatingActionButton( onPressed: () => context.read<CoresBloc>().add(const CoresRefreshEvent()), child: const Icon(Icons.refresh), ) : const SizedBox.shrink(); }, ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
lib/generated/intl/messages_en.dartis excluded by!**/generated/**lib/generated/intl/messages_pt.dartis excluded by!**/generated/**lib/generated/intl/messages_uk.dartis excluded by!**/generated/**lib/generated/l10n.dartis excluded by!**/generated/**
📒 Files selected for processing (13)
.github/workflows/ci.yml(1 hunks)integration_test/cores_integration_live_test.dart(1 hunks)integration_test/cores_screen_integration_test.dart(1 hunks)lib/features/cores/cores_screen.dart(1 hunks)lib/features/main/navigation/destinations.dart(2 hunks)lib/l10n/app_localizations.dart(1 hunks)lib/l10n/app_localizations_de.dart(1 hunks)lib/l10n/app_localizations_en.dart(1 hunks)lib/l10n/app_localizations_pt.dart(1 hunks)lib/l10n/app_localizations_uk.dart(1 hunks)lib/l10n/intl_en.arb(1 hunks)lib/l10n/intl_pt.arb(1 hunks)lib/l10n/intl_uk.arb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- lib/features/main/navigation/destinations.dart
- lib/l10n/app_localizations_de.dart
- lib/l10n/app_localizations_uk.dart
- lib/l10n/intl_uk.arb
- lib/l10n/app_localizations.dart
- lib/l10n/intl_en.arb
- lib/l10n/intl_pt.arb
⏰ 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: Flutter Integration Tests (31)
- GitHub Check: App
🔇 Additional comments (6)
lib/l10n/app_localizations_pt.dart (5)
362-373: LGTM: Status labels read well in PT (lowercase usage seems intentional).
399-413: Verify intended casing difference between status values and filter chips.
core_status_* are lowercase; core_filter_status_* are capitalized. Confirm UI expectations.
415-416: LGTM: Clear, natural search hint.
423-425: LGTM: Block label reads naturally.
427-431: LGTM: Titles sound correct and brand usage (SpaceX/Falcon) is appropriate.lib/l10n/app_localizations_en.dart (1)
396-399: Confirm and consolidate duplicate N/A getters
BothnaandnotAvailablereturn “N/A” and no usages were found in the codebase—please manually verify across all localization calls, then remove one getter or have it delegate to the other to prevent future drift.
| await tester.enterText(searchField, 'flutter'); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| expect(find.text('No cores found for "flutter"'), findsOneWidget); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use localized string instead of hard-coded English
Avoid locale-coupled flakiness; assert using l10n.
Apply:
+import 'package:flutter_bloc_app_template/generated/l10n.dart';
@@
- expect(find.text('No cores found for "flutter"'), findsOneWidget);
+ final ctx = tester.element(find.byType(Scaffold).first);
+ final l10n = S.of(ctx);
+ expect(find.text(l10n.noCoresFound('flutter')), findsOneWidget);📝 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.
| expect(find.text('No cores found for "flutter"'), findsOneWidget); | |
| // At the top of integration_test/cores_integration_live_test.dart | |
| import 'package:flutter_bloc_app_template/generated/l10n.dart'; | |
| [...] | |
| // In your test, replace the hard-coded expect at line 79: | |
| final ctx = tester.element(find.byType(Scaffold).first); | |
| final l10n = S.of(ctx); | |
| expect(find.text(l10n.noCoresFound('flutter')), findsOneWidget); |
🤖 Prompt for AI Agents
In integration_test/cores_integration_live_test.dart around line 79, the test
asserts a hard-coded English message; replace that with the app's localized
string to avoid locale-dependent flakiness by retrieving the localization from
the test widget tree (e.g. obtain AppLocalizations via
AppLocalizations.of(tester.element(find.byType(<root app widget>))) or similar)
and use the localized formatter/getter for the "No cores found for {query}"
message with the query 'flutter' when calling expect.
| IconButton( | ||
| icon: const Icon(Icons.info_outline), | ||
| onPressed: () => print, | ||
| ), |
There was a problem hiding this comment.
Fix onPressed: it currently does nothing
onPressed: () => print, returns the print function instead of invoking it; effectively a no-op.
Apply:
- onPressed: () => print,
+ onPressed: () {},Or replace with debugPrint('Cores info tapped').
📝 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.
| IconButton( | |
| icon: const Icon(Icons.info_outline), | |
| onPressed: () => print, | |
| ), | |
| IconButton( | |
| icon: const Icon(Icons.info_outline), | |
| onPressed: () {}, | |
| ), |
🤖 Prompt for AI Agents
In lib/features/cores/cores_screen.dart around lines 26 to 29, the IconButton's
onPressed is set to "=> print" which returns the print function and does
nothing; replace it with a call that actually logs a message — e.g. change to an
inline call like "() => print('Cores info tapped')" or use Flutter's debugPrint:
"() { debugPrint('Cores info tapped'); }" so the button triggers a visible log
when pressed.
| String missions(Object count) { | ||
| return '$count missions'; | ||
| } | ||
|
|
||
| @override | ||
| String reuses(Object count) { | ||
| return '$count reuses'; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper pluralization for missions/reuses
Always appending “s” is grammatically incorrect for singular.
Apply:
- String missions(Object count) {
- return '$count missions';
- }
+ String missions(Object count) {
+ final c = int.tryParse('$count') ?? 0;
+ return intl.Intl.pluralLogic(
+ c,
+ locale: localeName,
+ one: '$c mission',
+ other: '$c missions',
+ );
+ }
- String reuses(Object count) {
- return '$count reuses';
- }
+ String reuses(Object count) {
+ final c = int.tryParse('$count') ?? 0;
+ return intl.Intl.pluralLogic(
+ c,
+ locale: localeName,
+ one: '$c reuse',
+ other: '$c reuses',
+ );
+ }This also makes the intl import meaningful (removes the unused_import need).
📝 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.
| String missions(Object count) { | |
| return '$count missions'; | |
| } | |
| @override | |
| String reuses(Object count) { | |
| return '$count reuses'; | |
| } | |
| String missions(Object count) { | |
| final c = int.tryParse('$count') ?? 0; | |
| return intl.Intl.pluralLogic( | |
| c, | |
| locale: localeName, | |
| one: '$c mission', | |
| other: '$c missions', | |
| ); | |
| } | |
| @override | |
| String reuses(Object count) { | |
| final c = int.tryParse('$count') ?? 0; | |
| return intl.Intl.pluralLogic( | |
| c, | |
| locale: localeName, | |
| one: '$c reuse', | |
| other: '$c reuses', | |
| ); | |
| } |
🤖 Prompt for AI Agents
In lib/l10n/app_localizations_en.dart around lines 384 to 391, the missions and
reuses methods now always append "s" and must use proper pluralization; change
each method to use Intl.plural (from package:intl/intl.dart) with the numeric
count (cast Object to int) and supply at least the zero, one, and other forms
(e.g., '0 missions', '1 mission', '{count} missions') so singular is correct;
ensure the file imports package:intl/intl.dart (which makes the import
meaningful and removes the unused_import warning).
| String noCoresFound(Object query) { | ||
| return 'No cores found for \"$query\"'; | ||
| } |
There was a problem hiding this comment.
Fix escaping: backslash shows up in UI
Escaping double quotes inside single-quoted Dart strings is unnecessary and renders a literal backslash.
Apply:
- return 'No cores found for \"$query\"';
+ return 'No cores found for "$query"';📝 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.
| String noCoresFound(Object query) { | |
| return 'No cores found for \"$query\"'; | |
| } | |
| String noCoresFound(Object query) { | |
| return 'No cores found for "$query"'; | |
| } |
🤖 Prompt for AI Agents
In lib/l10n/app_localizations_en.dart around lines 418 to 420, the string uses
an unnecessary backslash before the double quotes which causes a literal
backslash to render; remove the escape so the interpolated query is enclosed by
plain double quotes inside the single-quoted string (e.g. change to use " $query
" with no backslashes) and keep the interpolation intact.
| String missions(Object count) { | ||
| return '$count missões'; | ||
| } | ||
|
|
||
| @override | ||
| String reuses(Object count) { | ||
| return '$count reutilizações'; | ||
| } |
There was a problem hiding this comment.
Fix PT pluralization for missions/reuses using Intl.pluralLogic.
Currently shows “1 missões” / “1 reutilizações”. Use singular for one.
Apply:
@override
String missions(Object count) {
- return '$count missões';
+ final num c = count is num ? count : (num.tryParse('$count') ?? 0);
+ return intl.Intl.pluralLogic(
+ c,
+ locale: localeName,
+ one: '$count missão',
+ other: '$count missões',
+ );
}
@override
String reuses(Object count) {
- return '$count reutilizações';
+ final num c = count is num ? count : (num.tryParse('$count') ?? 0);
+ return intl.Intl.pluralLogic(
+ c,
+ locale: localeName,
+ one: '$count reutilização',
+ other: '$count reutilizações',
+ );
}🤖 Prompt for AI Agents
In lib/l10n/app_localizations_pt.dart around lines 384 to 391, the pluralization
for missions and reuses is incorrect ("1 missões"/"1 reutilizações"); change
both methods to use Intl.pluralLogic on the numeric count to return the correct
singular form for one and the plural form for other (and handle zero if
desired). Replace the current string interpolation with calls like
Intl.pluralLogic(count, one: '1 missão', other: '$count missões') and
Intl.pluralLogic(count, one: '1 reutilização', other: '$count reutilizações')
(ensuring count is converted to num/int as needed and import/usage matches
existing Intl conventions in the file).
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
test/features/cores/widget/cores_search_filter_widget_test.dart (2)
61-66: Use the descendant-based finder consistently when typing.Prevents breakage if SearchBar’s internal widgets change.
- final searchField = find.byType(TextField); + final searchField = find.descendant( + of: find.byType(SearchBar), + matching: find.byType(TextField), + );
85-101: Minor: Reuse the descendant finder for chips test for consistency.Keeps all interactions scoped under the SearchBar container.
No code change required, but consider defining a common
final searchField = find.descendant(...)at each test’s start for consistency.lib/features/cores/widget/cores_search_filter_widget.dart (2)
74-80: Simplify selection logic (handles “all” and toggle in one place).Cleaner and avoids double writes.
- setState(() { - _selectedStatus = selected ? status : null; - if (status == CoreFilterStatus.all) { - _selectedStatus = null; - } - }); + setState(() { + _selectedStatus = status == CoreFilterStatus.all + ? null + : (selected ? status : null); + });
43-51: Optional: Consider also emitting onChanged('') on clear.Some consumers expect a single stream of changes from one callback. If you adopt this, update tests accordingly.
IconButton( icon: const Icon(Icons.clear), onPressed: () { _searchController.clear(); widget.onClear.call(_selectedStatus); + widget.onChanged.call('', _selectedStatus); setState(() {}); }, ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/features/cores/cores_screen.dart(1 hunks)lib/features/cores/widget/cores_search_filter_widget.dart(1 hunks)test/features/cores/widget/cores_search_filter_widget_test.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/cores/cores_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). (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: App
- GitHub Check: Flutter Integration Tests (31)
🔇 Additional comments (1)
lib/features/cores/widget/cores_search_filter_widget.dart (1)
30-95: Nice sliver-friendly composition and testable keys.Good use of SliverToBoxAdapter, localized labels, and deterministic keys for chips.
| class _CoresSearchFilterWidgetState extends State<CoresSearchFilterWidget> { | ||
| final TextEditingController _searchController = TextEditingController(); | ||
| CoreFilterStatus? _selectedStatus; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Dispose the TextEditingController to avoid leaks.
Controllers must be disposed in State objects.
class _CoresSearchFilterWidgetState extends State<CoresSearchFilterWidget> {
final TextEditingController _searchController = TextEditingController();
CoreFilterStatus? _selectedStatus;
+ @override
+ void dispose() {
+ _searchController.dispose();
+ super.dispose();
+ }📝 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.
| class _CoresSearchFilterWidgetState extends State<CoresSearchFilterWidget> { | |
| final TextEditingController _searchController = TextEditingController(); | |
| CoreFilterStatus? _selectedStatus; | |
| class _CoresSearchFilterWidgetState extends State<CoresSearchFilterWidget> { | |
| final TextEditingController _searchController = TextEditingController(); | |
| CoreFilterStatus? _selectedStatus; | |
| @override | |
| void dispose() { | |
| _searchController.dispose(); | |
| super.dispose(); | |
| } | |
| // ...rest of your state implementation... | |
| } |
🤖 Prompt for AI Agents
In lib/features/cores/widget/cores_search_filter_widget.dart around lines 22 to
25, the TextEditingController _searchController is created but not disposed; add
an override of dispose() in _CoresSearchFilterWidgetState that calls
_searchController.dispose() and then super.dispose(); if you attached any
listeners to the controller remove them before disposing.
| expect(find.byType(TextField), findsOneWidget); | ||
| for (final status in CoreFilterStatus.values) { | ||
| expect( | ||
| find.byKey(Key('core_status_filter_${status.name.toLowerCase()}')), | ||
| findsOneWidget, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stabilize the TextField finder (avoid coupling to SearchBar internals).
SearchBar internals can change; scope the TextField as a descendant of SearchBar to keep tests robust.
- expect(find.byType(TextField), findsOneWidget);
+ final searchField = find.descendant(
+ of: find.byType(SearchBar),
+ matching: find.byType(TextField),
+ );
+ expect(searchField, findsOneWidget);📝 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.
| expect(find.byType(TextField), findsOneWidget); | |
| for (final status in CoreFilterStatus.values) { | |
| expect( | |
| find.byKey(Key('core_status_filter_${status.name.toLowerCase()}')), | |
| findsOneWidget, | |
| ); | |
| } | |
| final searchField = find.descendant( | |
| of: find.byType(SearchBar), | |
| matching: find.byType(TextField), | |
| ); | |
| expect(searchField, findsOneWidget); | |
| for (final status in CoreFilterStatus.values) { | |
| expect( | |
| find.byKey(Key('core_status_filter_${status.name.toLowerCase()}')), | |
| findsOneWidget, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In test/features/cores/widget/cores_search_filter_widget_test.dart around lines
49 to 55, the test currently asserts the TextField by type which couples it to
SearchBar internals; change the TextField assertion to look for a TextField
descendant of the SearchBar (use a descendant finder combining
find.byType(SearchBar) and find.byType(TextField)) so the test targets the
SearchBar's child without depending on internal structure, keeping the existing
loop that asserts the status filter keys unchanged.
| final searchField = find.byType(TextField); | ||
| await tester.enterText(searchField, 'merlin'); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| final clearButton = find.byIcon(Icons.clear); | ||
| expect(clearButton, findsOneWidget); | ||
|
|
||
| await tester.tap(clearButton); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| verify(() => mockOnClear.call(null)).called(1); | ||
| expect(find.text(''), findsOneWidget); | ||
| }); |
There was a problem hiding this comment.
Fix brittle assertion for cleared text.
find.text('') is unreliable. Inspect the TextField’s controller instead.
- final searchField = find.byType(TextField);
+ final searchField = find.descendant(
+ of: find.byType(SearchBar),
+ matching: find.byType(TextField),
+ );
@@
- verify(() => mockOnClear.call(null)).called(1);
- expect(find.text(''), findsOneWidget);
+ verify(() => mockOnClear.call(null)).called(1);
+ final textFieldWidget = tester.widget<TextField>(searchField);
+ expect(textFieldWidget.controller?.text ?? '', isEmpty);🤖 Prompt for AI Agents
In test/features/cores/widget/cores_search_filter_widget_test.dart around lines
71 to 83, the final assertion uses find.text('') which is brittle; instead
retrieve the TextField widget from the tester and assert its controller's text
is empty. Update the test to get the TextField via
tester.widget<TextField>(searchField), then assert that its controller?.text ==
'' (or use controller!.text if non-nullable), keeping the existing clear button
tap and mockOnClear verification unchanged.
# Conflicts: # .github/workflows/ci.yml # lib/data/network/model/core/network_core_model.freezed.dart # lib/data/network/model/stage/network_first_stage_model.freezed.dart # lib/di/app_bloc_providers.dart # lib/di/app_repository_providers.dart # lib/di/di_initializer.config.dart # lib/di/di_network_module.dart # lib/di/di_repository_module.dart # lib/generated/intl/messages_en.dart # lib/generated/intl/messages_pt.dart # lib/generated/intl/messages_uk.dart # lib/generated/l10n.dart # lib/l10n/app_localizations.dart # lib/l10n/app_localizations_de.dart # lib/l10n/app_localizations_en.dart # lib/l10n/app_localizations_pt.dart # lib/l10n/app_localizations_uk.dart # lib/l10n/intl_en.arb # lib/l10n/intl_pt.arb # lib/l10n/intl_uk.arb # test/mocks.dart # test/mocks.mocks.dart
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
lib/l10n/app_localizations_pt.dart (1)
455-462: Fix PT singular/plural for missions/reuses.Show “1 missão” / “1 reutilização” using Intl.pluralLogic.
@override String missions(Object count) { - return '$count missões'; + final num c = count is num ? count : (num.tryParse('$count') ?? 0); + return intl.Intl.pluralLogic( + c, + locale: localeName, + one: '$count missão', + other: '$count missões', + ); } @override String reuses(Object count) { - return '$count reutilizações'; + final num c = count is num ? count : (num.tryParse('$count') ?? 0); + return intl.Intl.pluralLogic( + c, + locale: localeName, + one: '$count reutilização', + other: '$count reutilizações', + ); }lib/l10n/app_localizations_en.dart (2)
455-462: Use proper pluralization for missions/reuses; leverage IntlSingular currently renders as “1 missions/1 reuses”.
- String missions(Object count) { - return '$count missions'; - } + String missions(Object count) { + final c = int.tryParse('$count') ?? 0; + return intl.Intl.pluralLogic( + c, + locale: localeName, + zero: '$c missions', + one: '$c mission', + other: '$c missions', + ); + } - String reuses(Object count) { - return '$count reuses'; - } + String reuses(Object count) { + final c = int.tryParse('$count') ?? 0; + return intl.Intl.pluralLogic( + c, + locale: localeName, + zero: '$c reuses', + one: '$c reuse', + other: '$c reuses', + ); + }
489-491: Fix escaping: backslash appears in UIRemove unnecessary escapes inside single quotes.
- return 'No cores found for \"\$query\"'; + return 'No cores found for "$query"';
🧹 Nitpick comments (5)
lib/l10n/app_localizations_pt.dart (2)
465-469: Avoid duplicate semantics with “N/D”.You already have notAvailable => 'N/D'. Consider removing/reusing one key to prevent drift.
489-491: Remove unnecessary escaping of quotes.The backslashes may render literally; not needed inside single quotes.
String noCoresFound(Object query) { - return 'Nenhum núcleo encontrado para \"$query\"'; + return 'Nenhum núcleo encontrado para "$query"'; }lib/l10n/app_localizations_en.dart (1)
1-1: Drop unused_import ignore once Intl is used aboveAfter adopting Intl in missions/reuses, this suppression becomes unnecessary.
-// ignore: unused_importtest/mocks.mocks.dart (2)
223-266: LGTM: New MockCoresRepository.Signatures match CoreResource API; defaults (empty list/fake) are sensible for tests. If tests need richer data, stub with thenAnswer on getCores/getCore.
I can generate common stubs/fixtures for cores tests on request.
6-21: Use public Flutter widgets API for mock generation and re-generate mocksEdit the mock source (test/mocks.dart) to import NavigatorObserver (and Route if needed) from package:flutter/widgets.dart (show) and include NavigatorObserver in @GenerateMocks, then run build_runner to regenerate. The generated file still imports internals at test/mocks.mocks.dart:8 — confirm removal with: rg -n 'package:flutter/src/widgets/navigator.dart' test/.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
lib/generated/colors.gen.dartis excluded by!**/generated/**lib/generated/intl/messages_en.dartis excluded by!**/generated/**lib/generated/intl/messages_pt.dartis excluded by!**/generated/**lib/generated/intl/messages_uk.dartis excluded by!**/generated/**lib/generated/l10n.dartis excluded by!**/generated/**
📒 Files selected for processing (15)
lib/di/app_bloc_providers.dart(2 hunks)lib/di/app_repository_providers.dart(2 hunks)lib/di/di_initializer.config.dart(3 hunks)lib/di/di_network_module.dart(3 hunks)lib/di/di_repository_module.dart(2 hunks)lib/l10n/app_localizations.dart(1 hunks)lib/l10n/app_localizations_de.dart(1 hunks)lib/l10n/app_localizations_en.dart(1 hunks)lib/l10n/app_localizations_pt.dart(1 hunks)lib/l10n/app_localizations_uk.dart(1 hunks)lib/l10n/intl_en.arb(1 hunks)lib/l10n/intl_pt.arb(1 hunks)lib/l10n/intl_uk.arb(1 hunks)test/mocks.dart(2 hunks)test/mocks.mocks.dart(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- lib/l10n/app_localizations_uk.dart
- lib/di/di_initializer.config.dart
- test/mocks.dart
- lib/l10n/app_localizations_de.dart
- lib/l10n/app_localizations.dart
- lib/di/di_repository_module.dart
- lib/di/app_repository_providers.dart
- lib/l10n/intl_pt.arb
- lib/l10n/intl_uk.arb
- lib/l10n/intl_en.arb
- lib/di/di_network_module.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/di/app_bloc_providers.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). (1)
- GitHub Check: Flutter Integration Tests (31)
🔇 Additional comments (20)
lib/l10n/app_localizations_pt.dart (5)
433-444: LGTM: core status translations look correct.Lowercase style is consistent for inline status chips.
446-453: LGTM: error/retry/first‑launch keys.Natural PT phrasing; matches existing style.
471-484: LGTM: filter status labels.Terminology consistent with the rest of the locale.
494-496: LGTM: block label.Correct placement/order for “Bloco {n}”.
499-502: LGTM: titles.Natural PT for the screen title and label.
lib/di/app_bloc_providers.dart (3)
5-5: LGTM: CoresBloc import added.Import path looks correct and consistent with existing feature bloc imports.
9-9: LGTM: CoresRepository import wired.Consistent with repository import pattern in this file.
55-61: LGTM — verification incomplete: confirm CoresRepository is provided and CoresBloc doesn't self-dispatchSearch returned no matches for RepositoryProvider or any cores_bloc.dart; unable to verify DI wiring or double-dispatch. Confirm both:
- CoresRepository is provided (RepositoryProvider or DI registration) in scope of this BlocProvider.
- CoresBloc constructor does not call add(const CoresEvent.load()).
lib/l10n/app_localizations_en.dart (5)
445-453: LGTM: error/loading/firstLaunch copyCopy is clear and consistent with existing localization tone.
471-487: LGTM: core filter labels and search hintLabels/readability look good; aligns with other sections.
494-496: LGTM: block labelMatches expected formatting used elsewhere.
499-503: LGTM: titlesNames are clear and consistent.
432-444: Status strings are programmatic keys — no user-facing change requiredcore_status_* are used programmatically (lib/features/cores/utils/core_utils.dart builds a statusMap with loc.core_status_.toLowerCase(), and tests reference them in test/features/cores/utils/core_utils_test.dart); l10n values are lowercase in lib/l10n/.dart and lib/generated/intl/*.dart. No direct UI label usage found — if these will be shown to users, provide Title Case display strings or separate display keys.
test/mocks.mocks.dart (7)
70-78: LGTM: _FakeCoreResource_3 added for Cores mocks.Fake aligns with CoreResource; no issues.
90-97: LGTM: Future alias updates for EmailListRepository.Return typing is consistent with dart:async aliasing and prior patterns.
135-150: LGTM: getLaunch mock returns a SmartFake LaunchFullResource.Matches repository contract; fine for tests.
155-195: LGTM: RocketRepository mock updates (interface and methods).Signatures and default return values align with other mocks.
201-221: LGTM: RoadsterRepository mock updates.Implements the correct interface and returns a Fake resource.
109-133: LaunchesRepository.getLaunches mock is correct — launchSuccess is int?Confirmed: LaunchesRepository.getLaunches and LaunchesDataSource.getLaunches use int? for launchSuccess and the generated mock matches; LaunchResource.launchSuccess remains a bool? (separate domain field).
271-377: didChangeTop added in Flutter 3.19 — OK with repo constraintdidChangeTop was introduced in Flutter 3.19 (stable); pubspec.yaml requires flutter >=3.19.2, so the mock is compatible — ensure CI/toolchain uses SDK >=3.19.2.
Summary by CodeRabbit