Conversation
WalkthroughReplaces in-memory text state with a Firestore-backed TextList provider (streaming snapshots and CRUD), adds Firestore-aware JSON (de)serialization to TextModel, introduces Firestore collection/field constants, gates sheet membership filtering on Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as UI / Widgets
participant Provider as TextListProvider (Riverpod)
participant Firestore as Firestore (Fake/Real)
UI->>Provider: subscribe/read text list
Provider->>Firestore: collection(...).snapshots()
Firestore-->>Provider: QuerySnapshot stream
Provider->>Provider: map docs -> TextModel list
Provider-->>UI: emit updated list
UI->>Provider: addText(TextModel)
Provider->>Firestore: collection.doc(id).set(text.toJson())
Firestore-->>Provider: snapshot with new doc
Provider-->>UI: emit updated list
UI->>Provider: updateTextTitle(id, title)
Provider->>Firestore: doc(id).update({title, updatedAt: FieldValue.serverTimestamp()})
Firestore-->>Provider: snapshot with updated doc
Provider-->>UI: emit updated item
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
test/features/text/screens/text_block_details_screen_test.dart (2)
37-39: UseFirestoreCollections.textsconstant instead of hardcoded string.The collection name
'texts'is hardcoded here, but a constantFirestoreCollections.textswas added in this PR specifically for this purpose. Using the constant ensures consistency and reduces the risk of typos.+import 'package:zoe/constants/firestore_collection_constants.dart';for (final text in textList) { - await fakeFirestore.collection('texts').doc(text.id).set(text.toJson()); + await fakeFirestore.collection(FirestoreCollections.texts).doc(text.id).set(text.toJson()); }
41-47: Add assertion after retry loop to fail fast if data is not ready.The retry loop silently exits after 20 attempts without verifying the data was actually populated. If data propagation takes longer than expected, subsequent tests may fail with confusing errors. Consider adding an assertion or throwing an exception.
int retries = 0; while (container.read(textListProvider).length < textList.length && retries < 20) { await Future.delayed(const Duration(milliseconds: 50)); retries++; } + + expect( + container.read(textListProvider).length, + greaterThanOrEqualTo(textList.length), + reason: 'Firestore data did not propagate within timeout', + ); });test/features/text/actions/text_actions_test.dart (2)
18-45: MissingtearDownto dispose theProviderContainer.The
ProviderContainershould be disposed after each test to clean up resources and cancel subscriptions. This prevents potential memory leaks and test interference.Add a
tearDownblock:setUp(() async { // ... existing setup code ... }); + + tearDown(() { + container.dispose(); + });
90-93: HelperverifySnackbarFunctionalityprovides no meaningful verification.This helper only checks that
showSnackBaris a function (which is always true). It doesn't verify that a snackbar was actually shown. Consider removing these assertions or implementing proper snackbar verification with a mock.test/features/text/providers/text_providers_test.dart (2)
13-13: Shadowed variabletestUseris redundant.
testUseris already imported frommock_fakefirestore_text.dart. This local declaration shadows the import and creates potential confusion.Remove the local declaration:
- final testUser = 'test-user';
15-39: MissingtearDownto dispose theProviderContainer.Same as the actions test file, add a
tearDownto properly clean up resources.Add disposal:
setUp(() async { // ... existing setup code ... }); + + tearDown(() { + container.dispose(); + });lib/features/text/providers/text_providers.dart (2)
28-34: Missing error handling for Firestore stream.If the Firestore stream encounters an error (network issues, permission denied), it will be unhandled and could crash the app. Consider adding error handling to the stream listener.
Add error handling:
_subscription = query.snapshots().listen((snapshot) { final items = snapshot.docs .map((doc) => TextModel.fromJson(doc.data())) .whereType<TextModel>() .toList(); state = items; - }); + }, onError: (error) { + // Log error or set error state + // Consider: state = []; or expose error state + });
43-45: Consider usingserverTimestamp()forcreatedAton new documents.Unlike the update methods which set
updatedAtviaserverTimestamp(),addTextrelies on the caller-providedcreatedAt. For consistency and server-side accuracy, consider settingcreatedAtserver-side.Future<void> addText(TextModel text) async { - await collection.doc(text.id).set(text.toJson()); + final data = text.toJson(); + data[FirestoreFieldConstants.createdAt] = FieldValue.serverTimestamp(); + await collection.doc(text.id).set(data); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
lib/common/widgets/choose_color_bottom_sheet.dart(0 hunks)lib/constants/firestore_collection_constants.dart(1 hunks)lib/constants/firestore_field_constants.dart(1 hunks)lib/features/sheet/providers/sheet_providers.dart(0 hunks)lib/features/sheet/providers/sheet_providers.g.dart(1 hunks)lib/features/text/models/text_model.dart(2 hunks)lib/features/text/providers/text_providers.dart(2 hunks)lib/features/text/providers/text_providers.g.dart(1 hunks)test/features/text/actions/text_actions_test.dart(18 hunks)test/features/text/providers/text_providers_test.dart(1 hunks)test/features/text/screens/text_block_details_screen_test.dart(2 hunks)test/features/text/utils/mock_fakefirestore_text.dart(1 hunks)test/features/text/utils/text_utils.dart(1 hunks)
💤 Files with no reviewable changes (2)
- lib/common/widgets/choose_color_bottom_sheet.dart
- lib/features/sheet/providers/sheet_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). (8)
- GitHub Check: Test zoe_native plugin
- GitHub Check: format
- GitHub Check: macos
- GitHub Check: android
- GitHub Check: linux
- GitHub Check: test
- GitHub Check: ios
- GitHub Check: windows
🔇 Additional comments (11)
test/features/text/utils/text_utils.dart (1)
6-13: LGTM!The function logic is correct with proper bounds checking. The only change is adding a trailing newline, which is a minor formatting improvement.
lib/features/sheet/providers/sheet_providers.g.dart (1)
44-44: LGTM!Generated code with updated hash reflecting changes in the source provider. No manual modifications needed.
lib/features/text/providers/text_providers.g.dart (1)
44-44: LGTM!Generated code with updated hash reflecting changes in the source text provider implementation. No manual modifications needed.
lib/constants/firestore_collection_constants.dart (1)
1-4: LGTM!The new
textscollection constant follows the established pattern and provides a centralized reference for Firestore text collection access.lib/constants/firestore_field_constants.dart (1)
9-10: LGTM!The new field constants
emojiandorderIndexfollow the established pattern and support the TextModel's Firestore field operations.test/features/text/screens/text_block_details_screen_test.dart (1)
28-35: Test setup correctly configures Firestore-backed providers.The setUp properly initializes FakeFirebaseFirestore, overrides both firestoreProvider and loggedInUserProvider, and seeds test data. This provides a solid foundation for Firestore-backed integration tests.
test/features/text/utils/mock_fakefirestore_text.dart (1)
1-32: LGTM!Mock data is well-structured with clear identifiers and realistic test values. The mocks appropriately test both scenarios (with and without emoji).
test/features/text/actions/text_actions_test.dart (1)
95-166: Well-structured tests with good edge case coverage.The tests cover important scenarios including empty descriptions, null content, and various text configurations. The helper functions for clipboard and share content construction are clear and maintainable.
test/features/text/providers/text_providers_test.dart (1)
50-186: Comprehensive test coverage for Firestore-backed providers.Tests thoroughly cover CRUD operations, filtering, searching, and sorting. The helper methods improve readability, and the async handling with short delays is appropriate for stream-based updates.
lib/features/text/providers/text_providers.dart (2)
47-49:deleteTextand update methods may throw if document doesn't exist.Firestore's
update()throws if the document doesn't exist. Whiledelete()is idempotent, callers should be aware that update operations can fail. Consider adding try-catch or documenting this behavior.Verify that callers handle potential Firestore exceptions appropriately.
83-114: Derived providers are well-implemented.The providers for single text lookup, parent filtering, search, and sorting follow Riverpod best practices by watching
textListProviderand deriving state. UsingfirstOrNullsafely handles missing items.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/features/text/models/text_model.dart (1)
85-86: Handle null timestamps to prevent runtime crashes.
Timestamp.fromDate()expects a non-nullDateTime, butcreatedAtandupdatedAtare nullable fields. If aTextModelis constructed with null timestamps (e.g., via the constructor rather thanfromJson), callingtoJson()will throw a runtime error.Apply this diff to handle null values:
- 'createdAt': Timestamp.fromDate(createdAt), - 'updatedAt': Timestamp.fromDate(updatedAt), + 'createdAt': createdAt != null ? Timestamp.fromDate(createdAt!) : null, + 'updatedAt': updatedAt != null ? Timestamp.fromDate(updatedAt!) : null,Alternatively, if timestamps should never be null in Firestore, consider making them required fields in the constructor and ensuring
fromJsonthrows on missing values rather than defaulting toDateTime.now().
🧹 Nitpick comments (2)
lib/features/text/models/text_model.dart (2)
80-83: Consider omitting null description fields.When
descriptionis null, the map still includes the field with null values for bothplainTextandhtmlText. While this works, you could optimize by omitting the field entirely when null to reduce payload size.Apply this diff if you want to omit null descriptions:
- 'description': { - 'plainText': description?.plainText, - 'htmlText': description?.htmlText, - }, + if (description != null) + 'description': { + 'plainText': description!.plainText, + 'htmlText': description!.htmlText, + },
63-68: This timestamp fallback pattern is consistent across models; clarify if it's intentional design or requires rethinking.
DateTime.now()as a fallback for missingcreatedAtandupdatedAtfields is used consistently throughout the codebase (also inSheetModel.fromJsonat lines 169-174, and in constructors). While this pattern is deliberate and not isolated to this model, consider whether missing timestamps should throw an error or use a different fallback, as the current approach could mask data integrity issues. If this is intentional fallback behavior, document it; otherwise, align all models to handle missing timestamps more explicitly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/features/text/models/text_model.dart(2 hunks)
🔇 Additional comments (3)
lib/features/text/models/text_model.dart (3)
1-1: LGTM!The cloud_firestore import is necessary for Timestamp handling in the new serialization methods.
43-43: LGTM!The
copyWithmethod now correctly preserves the originalupdatedAtvalue when none is provided, addressing the previous concern.
55-60: LGTM!The null check for the
descriptionfield correctly handles missing data, addressing the previous critical issue.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/features/text/models/text_model.dart (1)
63-68: Add type safety for Timestamp casts infromJson.The direct cast
(json['createdAt'] as Timestamp)will throw if the value is not null but also not a Timestamp (e.g., from malformed data or non-Firestore sources). Consider adding type checking for robustness.Apply this diff to add defensive type checking:
createdAt: json['createdAt'] == null ? DateTime.now() - : (json['createdAt'] as Timestamp).toDate(), + : json['createdAt'] is Timestamp + ? (json['createdAt'] as Timestamp).toDate() + : DateTime.now(), updatedAt: json['updatedAt'] == null ? DateTime.now() - : (json['updatedAt'] as Timestamp).toDate(), + : json['updatedAt'] is Timestamp + ? (json['updatedAt'] as Timestamp).toDate() + : DateTime.now(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/features/text/models/text_model.dart(2 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). (8)
- GitHub Check: test
- GitHub Check: windows
- GitHub Check: Test zoe_native plugin
- GitHub Check: linux
- GitHub Check: ios
- GitHub Check: format
- GitHub Check: android
- GitHub Check: macos
🔇 Additional comments (2)
lib/features/text/models/text_model.dart (2)
85-86: Verify nullability ofcreatedAtandupdatedAtfields.
Timestamp.fromDate()will throw ifcreatedAtorupdatedAtare null. Verify that these fields are guaranteed to be non-null in the ContentModel superclass.
43-43: Non-standardcopyWithbehavior auto-updates timestamp on any field modification.The current implementation sets
updatedAt: updatedAt ?? DateTime.now(), which updates the timestamp whenevercopyWith()is called without explicitly providingupdatedAt. This differs from the standard pattern (updatedAt: updatedAt ?? this.updatedAt), which would preserve the original value.Clarify whether auto-updating the timestamp on any modification is intentional. If not, change to preserve the original timestamp when not explicitly provided.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/features/text/providers/text_providers.dart (1)
62-70: Consider using Description.toJson() for consistency.The description update manually constructs a map with 'plainText' and 'htmlText' fields. If the Description class has a toJson() method, using it would improve maintainability and reduce the risk of inconsistencies if the model serialization changes.
If Description has toJson(), apply this diff:
Future<void> updateTextDescription(String textId, Description desc) async { await collection.doc(textId).update({ - FirestoreFieldConstants.description: { - 'plainText': desc.plainText, - 'htmlText': desc.htmlText, - }, + FirestoreFieldConstants.description: desc.toJson(), FirestoreFieldConstants.updatedAt: FieldValue.serverTimestamp(), }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/features/sheet/providers/sheet_providers.g.dart(1 hunks)lib/features/text/providers/text_providers.dart(2 hunks)lib/features/text/providers/text_providers.g.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/features/sheet/providers/sheet_providers.g.dart
- lib/features/text/providers/text_providers.g.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). (7)
- GitHub Check: linux
- GitHub Check: Test zoe_native plugin
- GitHub Check: test
- GitHub Check: android
- GitHub Check: windows
- GitHub Check: macos
- GitHub Check: ios
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/features/text/actions/text_actions_test.dart (1)
91-94:verifySnackbarFunctionalitydoesn't verify snackbar behavior.This only checks that
CommonUtils.showSnackBaris aFunction, which is always true. It doesn't verify the snackbar was actually called with correct parameters. Either remove this assertion or use a mock to verify invocation.- // Helper method to verify snackbar functionality - void verifySnackbarFunctionality() { - expect(CommonUtils.showSnackBar, isA<Function>()); - } + // TODO: Mock CommonUtils.showSnackBar to verify it's called with expected message
🧹 Nitpick comments (5)
test/features/text/actions/text_actions_test.dart (5)
37-46: Polling wait loop has timeout but missing assertion failure handling.The retry loop waits up to 1 second (20 × 50ms) for data to load, with an assertion at the end. This is reasonable, but consider extracting this pattern into a reusable helper since it's duplicated across test files.
+// Consider extracting to a shared test utility +Future<void> waitForProviderData<T>( + ProviderContainer container, + ProviderListenable<List<T>> provider, + int expectedCount, { + int maxRetries = 20, + Duration delay = const Duration(milliseconds: 50), +}) async { + int retries = 0; + while (container.read(provider).length < expectedCount && retries < maxRetries) { + await Future.delayed(delay); + retries++; + } +}
49-66: Test helper duplicates production logic — tests become tautological.
buildClipboardContentreplicates the clipboard formatting logic from production code. If production logic changes, tests pass even when broken because both implementations change together. Instead, assert against expected literal strings or extract the production helper and test it directly.
68-89: Same concern:buildShareMessageduplicates production logic.This helper mirrors the share message construction. Consider testing against known expected outputs for specific inputs rather than re-implementing the formatting logic.
132-157: Test adds data but doesn't clean up — may affect other tests.The test adds
textWithEmptyDescto Firestore but doesn't remove it afterward. If tests run in a shared order, this could cause flakiness. Consider using unique IDs per test or cleaning up in tearDown.
356-379: Widget instantiation tests provide limited value.These tests only verify constructor parameters are stored correctly. Consider adding widget tests with
WidgetTesterto verify actual rendering and behavior, or remove these if the widget is tested elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
test/features/text/actions/text_actions_test.dart(18 hunks)test/features/text/providers/text_providers_test.dart(1 hunks)test/features/text/screens/text_block_details_screen_test.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/features/text/screens/text_block_details_screen_test.dart
🔇 Additional comments (6)
test/features/text/actions/text_actions_test.dart (2)
97-112: LGTM — test properly arranges, acts, and asserts.The test correctly fetches text by index, builds clipboard content, and verifies expected substrings.
207-217: Delete test looks correct but relies on pre-existing data.The test properly awaits deletion and verifies the item is removed. Good async handling.
test/features/text/providers/text_providers_test.dart (4)
42-49: Good helper abstraction for provider reads.These helper methods improve test readability by encapsulating provider access patterns.
59-74: Async test properly awaits mutations and stream propagation.The test correctly awaits
addTextand includes a delay for stream updates. The assertions verify both list length and item presence.
76-83: Delete test correctly handles async flow.Good pattern: await deletion, delay for stream, then verify removal.
180-187: Sorted texts test assumes alphabetical ordering — verify sort implementation.The comment notes
'Understanding Sheets' comes before 'Welcome to Zoe!'alphabetically. EnsuresortedTextsProvideractually sorts by title and the mock data titles match this expectation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/features/text/actions/text_actions_test.dart (1)
21-47: MissingtearDownto dispose theProviderContainer.
ProviderContainershould be disposed intearDownto prevent resource leaks and ensure test isolation.expect( container.read(textListProvider).length, greaterThanOrEqualTo(2), reason: 'Provider failed to load test data within timeout', ); }); + + tearDown(() { + container.dispose(); + });
🧹 Nitpick comments (3)
test/features/text/screens/text_block_details_screen_test.dart (1)
30-35: Consider usingProviderContainer.test()for consistency.The second test file (
text_actions_test.dart) usesProviderContainer.test()while this file usesProviderContainer(). For consistency and to leverage any test-specific behavior, consider using the.test()constructor here as well.- container = ProviderContainer( + container = ProviderContainer.test( overrides: [ firestoreProvider.overrideWithValue(fakeFirestore), loggedInUserProvider.overrideWithValue(AsyncValue.data(testUser)), ], );test/features/text/actions/text_actions_test.dart (2)
37-46: Polling loop for async data loading is reasonable but could be improved.The retry-based approach to wait for provider data is pragmatic. However, consider extracting this into a reusable test utility function, especially if similar patterns appear in other test files. The 1-second timeout (20 × 50ms) should be sufficient for fake Firestore operations.
91-94: Weak assertion - verifySnackbarFunctionality doesn't verify actual behavior.This helper only checks that
CommonUtils.showSnackBaris a function, not that it was called or with what parameters. Consider either removing these calls (since they don't add test value) or using a mock to verify actual invocations.- // Helper method to verify snackbar functionality - void verifySnackbarFunctionality() { - expect(CommonUtils.showSnackBar, isA<Function>()); - }If you need to verify snackbar behavior, consider mocking
CommonUtils.showSnackBarand verifying invocations with expected parameters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/features/text/models/text_model.dart(2 hunks)test/features/text/actions/text_actions_test.dart(18 hunks)test/features/text/screens/text_block_details_screen_test.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/text/models/text_model.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). (8)
- GitHub Check: test
- GitHub Check: windows
- GitHub Check: Test zoe_native plugin
- GitHub Check: linux
- GitHub Check: ios
- GitHub Check: macos
- GitHub Check: android
- GitHub Check: format
🔇 Additional comments (8)
test/features/text/screens/text_block_details_screen_test.dart (3)
45-58: LGTM!The test correctly verifies the empty state behavior when a non-existent text block ID is provided. The shared container setup with Firestore seeding is appropriate for this scenario.
60-78: LGTM!Comprehensive test validating that valid text block data renders all expected UI components correctly.
104-122: LGTM!Good coverage of the editing state toggle functionality, verifying both entering and exiting edit mode.
test/features/text/actions/text_actions_test.dart (5)
49-66: LGTM!The helper methods
buildClipboardContentandbuildShareMessageare well-structured and clearly document the expected format of clipboard/share content for assertions.Also applies to: 68-89
132-158: Good async test pattern for adding data before assertions.The test correctly awaits the
addTextoperation before reading and verifying the data.
207-219: LGTM!Good test for the delete operation with proper async handling.
260-292: LGTM!Comprehensive edge case coverage for share message generation with empty descriptions, emoji-only titles, and null descriptions.
Also applies to: 294-327, 329-357
360-384: LGTM!Simple instantiation tests for
ShareItemsBottomSheetcorrectly verify widget properties.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
lib/features/text/providers/text_providers.dart (3)
18-19: User-based access control still missing.The past review comment about adding user-based access control remains unaddressed. The collection reference does not filter by user, and line 28 calls
collection.snapshots()without user-scoped filtering.
48-70: Handle null return from runFirestoreOperation.Similar to
addText, these methods don't check the return value fromrunFirestoreOperation. The same fix should be applied to propagate success/failure status.
85-93: Handle null return from runFirestoreOperation.Same issue as the other CRUD methods - no null handling.
🧹 Nitpick comments (3)
lib/common/services/snackbar_service.dart (1)
7-9: Consider adding customization options for SnackBar.The
showmethod currently only accepts a message string. For better UX flexibility, consider adding optional parameters for duration, action buttons, or background color.Example enhancement:
void show( String message, { Duration duration = const Duration(seconds: 4), SnackBarAction? action, Color? backgroundColor, }) { messengerKey.currentState?.showSnackBar( SnackBar( content: Text(message), duration: duration, action: action, backgroundColor: backgroundColor, ), ); }lib/features/text/providers/text_providers.dart (1)
72-83: Use Description.toJson() for consistency.The description object is manually constructed as a map, which is inconsistent with how
TextModelusestoJson(). IfDescriptionhas atoJson()method, use it to ensure consistency and maintainability.Future<void> updateTextDescription(String textId, Description desc) async { await runFirestoreOperation( ref, () => collection.doc(textId).update({ - FirestoreFieldConstants.description: { - 'plainText': desc.plainText, - 'htmlText': desc.htmlText, - }, + FirestoreFieldConstants.description: desc.toJson(), FirestoreFieldConstants.updatedAt: FieldValue.serverTimestamp(), }), ); }lib/common/utils/firestore_error_handler.dart (1)
9-45: Extend error code coverage for better UX.The error handler only maps three Firestore error codes (
permission-denied,not-found,unavailable). Consider adding more common codes likeaborted,already-exists,cancelled,data-loss,deadline-exceeded,failed-precondition,resource-exhausted, andunauthenticatedfor more specific user feedback.Example additions:
switch (e.code) { case 'permission-denied': snackbar.show('You do not have permission.'); break; case 'not-found': snackbar.show('Document not found.'); break; case 'unavailable': snackbar.show('Network unavailable.'); break; + case 'unauthenticated': + snackbar.show('Please sign in to continue.'); + break; + case 'already-exists': + snackbar.show('This item already exists.'); + break; + case 'deadline-exceeded': + snackbar.show('Request timed out. Please try again.'); + break; default: snackbar.show('Unexpected Firestore error.'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/common/providers/common_providers.dart(2 hunks)lib/common/services/snackbar_service.dart(1 hunks)lib/common/utils/firestore_error_handler.dart(1 hunks)lib/features/text/providers/text_providers.dart(2 hunks)lib/main.dart(2 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). (8)
- GitHub Check: linux
- GitHub Check: test
- GitHub Check: format
- GitHub Check: ios
- GitHub Check: Test zoe_native plugin
- GitHub Check: windows
- GitHub Check: macos
- GitHub Check: android
🔇 Additional comments (3)
lib/features/text/providers/text_providers.dart (1)
96-127: LGTM!The query and filtering providers are well-structured and provide good data access patterns. The sorting and filtering logic is clear and correct.
lib/main.dart (1)
11-11: LGTM!The snackbar service integration is correctly wired into the MaterialApp. The scaffoldMessengerKey will enable global snackbar functionality across the app.
Also applies to: 62-62
lib/common/providers/common_providers.dart (1)
6-6: LGTM!The snackbar service provider is correctly implemented. Using
Provider(non-disposing) ensures the service remains available throughout the app lifecycle, which is appropriate for a global service.Also applies to: 25-28
| List<TextModel> build() { | ||
| _subscription?.cancel(); | ||
| _subscription = null; | ||
|
|
||
| _subscription = collection.snapshots().listen((snapshot) { | ||
| state = snapshot.docs | ||
| .map((doc) => TextModel.fromJson(doc.data())) | ||
| .toList(); | ||
| }); | ||
|
|
||
| void addText(TextModel text) { | ||
| state = [...state, text]; | ||
| ref.onDispose(() { | ||
| _subscription?.cancel(); | ||
| }); | ||
|
|
||
| return []; | ||
| } |
There was a problem hiding this comment.
Review subscription lifecycle management.
The build() method recreates the Firestore subscription every time it's called, which can happen during hot-reload, provider refresh, or dependency changes. While lines 25-26 cancel the previous subscription, this reactive approach may still cause brief periods with multiple active listeners or missed events.
Additionally, ref.onDispose() is registered on every build() call, which could accumulate multiple identical disposal callbacks.
Consider restructuring to ensure subscription is created once:
@override
List<TextModel> build() {
- _subscription?.cancel();
- _subscription = null;
-
- _subscription = collection.snapshots().listen((snapshot) {
- state = snapshot.docs
- .map((doc) => TextModel.fromJson(doc.data()))
- .toList();
- });
-
- ref.onDispose(() {
+ if (_subscription == null) {
+ _subscription = collection.snapshots().listen((snapshot) {
+ state = snapshot.docs
+ .map((doc) => TextModel.fromJson(doc.data()))
+ .toList();
+ });
+
+ ref.onDispose(() {
+ _subscription?.cancel();
+ });
+ }
- _subscription?.cancel();
- });
-
return [];
}📝 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.
| List<TextModel> build() { | |
| _subscription?.cancel(); | |
| _subscription = null; | |
| _subscription = collection.snapshots().listen((snapshot) { | |
| state = snapshot.docs | |
| .map((doc) => TextModel.fromJson(doc.data())) | |
| .toList(); | |
| }); | |
| void addText(TextModel text) { | |
| state = [...state, text]; | |
| ref.onDispose(() { | |
| _subscription?.cancel(); | |
| }); | |
| return []; | |
| } | |
| List<TextModel> build() { | |
| if (_subscription == null) { | |
| _subscription = collection.snapshots().listen((snapshot) { | |
| state = snapshot.docs | |
| .map((doc) => TextModel.fromJson(doc.data())) | |
| .toList(); | |
| }); | |
| ref.onDispose(() { | |
| _subscription?.cancel(); | |
| }); | |
| } | |
| return []; | |
| } |
🤖 Prompt for AI Agents
In lib/features/text/providers/text_providers.dart around lines 24 to 39, the
build() method is recreating the Firestore subscription and re-registering
ref.onDispose() on every call; change it so the subscription and the onDispose
callback are created only once: guard creation with a check like if
(_subscription == null) { _subscription = collection.snapshots().listen(...);
ref.onDispose(() { _subscription?.cancel(); _subscription = null; }); } and
remove the unconditional cancel/reset at the top of build so you don't briefly
create multiple listeners or accumulate disposal callbacks.
| Future<void> addText(TextModel text) async { | ||
| await runFirestoreOperation( | ||
| ref, | ||
| () => collection.doc(text.id).set(text.toJson()), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Handle null return from runFirestoreOperation.
runFirestoreOperation returns null on error, but this method doesn't check the return value. Callers of addText may assume success when the operation actually failed.
Consider propagating errors or returning a boolean success indicator:
-Future<void> addText(TextModel text) async {
- await runFirestoreOperation(
+Future<bool> addText(TextModel text) async {
+ final result = await runFirestoreOperation(
ref,
() => collection.doc(text.id).set(text.toJson()),
);
+ return result != null;
}📝 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.
| Future<void> addText(TextModel text) async { | |
| await runFirestoreOperation( | |
| ref, | |
| () => collection.doc(text.id).set(text.toJson()), | |
| ); | |
| } | |
| Future<bool> addText(TextModel text) async { | |
| final result = await runFirestoreOperation( | |
| ref, | |
| () => collection.doc(text.id).set(text.toJson()), | |
| ); | |
| return result != null; | |
| } |
🤖 Prompt for AI Agents
In lib/features/text/providers/text_providers.dart around lines 41–46, addText
calls runFirestoreOperation but ignores its return which is null on error;
change addText to handle that by capturing the result, and either propagate the
failure (throw a descriptive exception) or return a boolean success indicator:
update the method signature to Future<bool> (or keep Future<void> and rethrow),
await and store the runFirestoreOperation result, if result == null then return
false (or throw a specific exception), otherwise return true (or complete
normally), and update any callers to handle the new return/exception behavior.
2 participants
Summary by CodeRabbit
New Features
Improvements
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.