Conversation
|
Warning Rate limit exceeded@anisha-e10 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 6 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds Firestore persistence: new collection and field constants, JSON (de)serialization for EventModel and TextModel, Firestore-backed providers with real-time snapshot subscriptions and CRUD methods, small UI label change for RSVP display, updated generated provider hashes and adjusted tests. Changes
Sequence DiagramsequenceDiagram
participant App as Flutter UI
participant Provider as Event/Text Provider
participant Firestore as Firestore
participant Listener as Snapshot Listener
App->>Provider: initialize / build
Provider->>Firestore: collection.snapshots() (subscribe)
Firestore->>Listener: emit document snapshots
Listener->>Provider: deliver snapshots
Provider->>App: notify listeners / update state
App->>Provider: addEvent/addText(data)
Provider->>Firestore: set document (toJson)
Firestore-->>Listener: emit updated snapshot
Listener->>Provider: update local state
Provider->>App: notify observers
App->>Provider: updateEvent*/updateText*(fields)
Provider->>Firestore: update fields (+ updatedAt Timestamp)
Firestore-->>Listener: emit updated snapshot
Listener->>Provider: update local state
Provider->>App: notify observers
App->>Provider: dispose
Provider->>Listener: cancel subscription
Listener--xFirestore: stop streaming
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 6
🧹 Nitpick comments (14)
lib/common/services/snackbar_service.dart (1)
3-10: LGTM! Consider future enhancements for snackbar customization.The implementation is clean and straightforward. The nullable
currentStatesafely handles cases where the messenger key is not yet attached.For future enhancements, consider adding optional parameters to the
showmethod for customization: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/common/utils/firestore_error_handler.dart (1)
9-44: Clean centralized error handling utility.The implementation provides a solid foundation for consistent Firestore error handling across the app. The error code mapping and snackbar integration are well-structured.
Consider expanding the error code coverage for additional common Firestore scenarios:
case 'unavailable': snackbar.show('Network unavailable.'); break; + case 'cancelled': + snackbar.show('Operation cancelled.'); + break; + case 'deadline-exceeded': + snackbar.show('Operation timed out.'); + break; + case 'already-exists': + snackbar.show('Document already exists.'); + break; default: snackbar.show('Unexpected Firestore error.'); }test/features/text/screens/text_block_details_screen_test.dart (2)
24-43: Add tearDown to dispose the ProviderContainer.The
ProviderContainershould be disposed in atearDownto prevent resource leaks and ensure clean state between tests.setUp(() async { fakeFirestore = FakeFirebaseFirestore(); container = ProviderContainer( overrides: [ firestoreProvider.overrideWithValue(fakeFirestore), loggedInUserProvider.overrideWithValue(AsyncValue.data(testUser)), ], ); for (final text in textList) { await fakeFirestore .collection(FirestoreCollections.texts) .doc(text.id) .set(text.toJson()); } }); + tearDown(() { + container.dispose(); + });Also, consider using
constfortestUsersince it's a compile-time constant:- final testUser = 'test-user'; + const testUser = 'test-user';
45-58: Consider using pumpAndSettle for async provider resolution.The tests use a single
pump()afterpumpMaterialWidgetWithProviderScope, which may not be sufficient for async providers (like Firestore streams) to fully resolve. This could lead to flaky tests.await tester.pumpMaterialWidgetWithProviderScope( child: const TextBlockDetailsScreen(textBlockId: 'non-existent-id'), container: container, ); - await tester.pump(); + await tester.pumpAndSettle();If
pumpAndSettle()times out due to animations, consider multiplepump()calls orpump(Duration)with a reasonable timeout.lib/features/events/models/events_model.dart (1)
118-142: Consider using FirestoreFieldConstants for field keys.The
toJson()method uses hardcoded string keys. For consistency with the rest of the codebase and to leverage the newly addedFirestoreFieldConstants, consider using the constants:+ import 'package:zoe/constants/firestore_field_constants.dart'; Map<String, dynamic> toJson() { return { - 'startDate': Timestamp.fromDate(startDate), - 'endDate': Timestamp.fromDate(endDate), - 'rsvpResponses': rsvpResponses.map( + FirestoreFieldConstants.startDate: Timestamp.fromDate(startDate), + FirestoreFieldConstants.endDate: Timestamp.fromDate(endDate), + FirestoreFieldConstants.rsvpResponses: rsvpResponses.map( (key, value) => MapEntry(key, value.name), ), // ... similar for other fields }; }This also applies to
fromJson().test/features/text/providers/text_providers_test.dart (2)
16-40: Add tearDown and consider a more robust wait mechanism.
- Missing tearDown: The
ProviderContainershould be disposed to prevent resource leaks:setUp(() async { // ... existing setup }); + tearDown(() { + container.dispose(); + });
Retry loop could be fragile: The polling mechanism with hardcoded retries and delays could lead to flaky tests on slower CI environments. Consider extracting this to a helper with configurable timeout, or using a completer-based approach if the provider exposes a way to await readiness.
Use const for testUser:
- final testUser = 'test-user'; + const testUser = 'test-user';
59-74: Hardcoded delays may cause flaky tests.The 50ms
Future.delayedcalls after mutations assume the Firestore stream will propagate within that window. This can lead to intermittent failures on slower environments.Consider:
- Increasing the delay as a buffer
- Using a similar retry/poll pattern as in setUp
- Creating a helper that waits for a specific condition
Future<void> waitForCondition( bool Function() condition, { Duration timeout = const Duration(seconds: 2), Duration interval = const Duration(milliseconds: 50), }) async { final deadline = DateTime.now().add(timeout); while (!condition() && DateTime.now().isBefore(deadline)) { await Future.delayed(interval); } }test/features/text/utils/mock_fakefirestore_text.dart (1)
3-32: Consider making these constantsconstfor better performance and immutability.Since these values are known at compile time and never change, declaring them as
constrather thanfinalwould improve performance and enforce immutability in tests.Apply this diff if
TextModelsupports const constructors:-final textContent1 = 'text-content-1'; -final textContent2 = 'text-content-2'; -final testUser = 'test-user'; +const textContent1 = 'text-content-1'; +const textContent2 = 'text-content-2'; +const testUser = 'test-user'; -final mockText1 = TextModel( +const mockText1 = TextModel( id: textContent1, sheetId: 'sheet-1', parentId: 'sheet-1', title: 'Welcome to Zoe!', description: ( plainText: 'Welcome to Zoe - your intelligent personal workspace!', htmlText: '<p>Welcome to Zoe - your intelligent personal workspace!</p>', ), orderIndex: 1, createdBy: testUser, ); -final mockText2 = TextModel( +const mockText2 = TextModel( id: textContent2, sheetId: 'sheet-1', parentId: 'sheet-1', title: 'Understanding Sheets', emoji: '📋', description: ( plainText: 'Sheets are your main workspaces in Zoe', htmlText: '<p>Sheets are your main workspaces in Zoe</p>', ), orderIndex: 2, createdBy: testUser, );test/features/text/actions/text_actions_test.dart (2)
50-89: Helper methods duplicate production logic.The
buildClipboardContentandbuildShareMessagehelpers reimplement formatting logic that should exist in production code. Tests should verify the actual production implementation rather than duplicating it, otherwise format changes in production code won't be caught by tests.Consider:
- Moving this formatting logic to production classes (e.g.,
TextActions.getClipboardContent(TextModel))- Testing the actual production methods instead of test-specific reimplementations
- This ensures tests verify real behavior and catch regressions
92-94: Snackbar verification only checks type existence.
verifySnackbarFunctionality()only verifies thatshowSnackBaris a function, not that it was called or called with the correct message. This provides minimal test value.Consider using mocktail to verify snackbar interactions:
// In setUp, create a mock final mockSnackbarService = MockSnackbarService(); // Override provider snackbarServiceProvider.overrideWithValue(mockSnackbarService) // In tests, verify calls verify(() => mockSnackbarService.showSnackBar( message: any(named: 'message'), type: SnackbarType.success, )).called(1);lib/features/text/providers/text_providers.dart (2)
72-82: Use constants for nested description field keys.The hardcoded strings
'plainText'and'htmlText'should be defined inFirestoreFieldConstantsto prevent typos and ensure consistency across the codebase.Add to
FirestoreFieldConstants:static const String descriptionPlainText = 'plainText'; static const String descriptionHtmlText = 'htmlText';Then update the code:
() => collection.doc(textId).update({ FirestoreFieldConstants.description: { - 'plainText': desc.plainText, - 'htmlText': desc.htmlText, + FirestoreFieldConstants.descriptionPlainText: desc.plainText, + FirestoreFieldConstants.descriptionHtmlText: desc.htmlText, }, FirestoreFieldConstants.updatedAt: FieldValue.serverTimestamp(), }),
41-93: Consider adding optimistic updates for better UX.All mutations wait for Firestore round-trips before updating local state. While this ensures consistency, it can make the UI feel sluggish, especially on slow connections. The real-time listener will eventually sync the state, but there's a noticeable delay.
Consider implementing optimistic updates:
Future<void> updateTextTitle(String textId, String title) async { // Optimistic update state = state.map((text) { if (text.id == textId) { return text.copyWith(title: title); } return text; }).toList(); // Persist to Firestore (listener will sync on success/failure) await runFirestoreOperation( ref, () => collection.doc(textId).update({ FirestoreFieldConstants.title: title, FirestoreFieldConstants.updatedAt: FieldValue.serverTimestamp(), }), ); }lib/features/events/providers/event_providers.dart (2)
64-77: Use constants for nested description field keys.The hardcoded strings
'plainText'and'htmlText'should be defined inFirestoreFieldConstantsto prevent typos and ensure consistency across the codebase.Add to
FirestoreFieldConstants:static const String descriptionPlainText = 'plainText'; static const String descriptionHtmlText = 'htmlText';Then update the code:
() => collection.doc(eventId).update({ FirestoreFieldConstants.description: { - 'plainText': description.plainText, - 'htmlText': description.htmlText, + FirestoreFieldConstants.descriptionPlainText: description.plainText, + FirestoreFieldConstants.descriptionHtmlText: description.htmlText, }, FirestoreFieldConstants.updatedAt: FieldValue.serverTimestamp(), }),
142-155: Consider optimizing membership filtering.The current implementation reads
sheetProviderfor each event in a loop, which could cause performance issues with many events (N+1 pattern). Each sheet lookup may trigger additional provider reads.Consider:
- Batch-fetch all relevant sheets upfront
- Use a Firestore query with a membership filter if possible
- Cache sheet membership data
Example optimization:
@riverpod List<EventModel> eventsList(Ref ref) { final allEvents = ref.watch(eventListProvider); final currentUserId = ref.watch(loggedInUserProvider).value; if (currentUserId == null || currentUserId.isEmpty) return []; // Get all sheets at once to avoid N+1 final allSheets = ref.watch(sheetListProvider); final memberSheetIds = allSheets .where((sheet) => sheet.users.contains(currentUserId)) .map((sheet) => sheet.id) .toSet(); return allEvents .where((e) => memberSheetIds.contains(e.sheetId)) .toList(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
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/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/events/models/events_model.dart(3 hunks)lib/features/events/providers/event_providers.dart(4 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)lib/main.dart(2 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: windows
- GitHub Check: Test zoe_native plugin
- GitHub Check: ios
- GitHub Check: linux
- GitHub Check: macos
- GitHub Check: android
- GitHub Check: test
- GitHub Check: format
🔇 Additional comments (16)
test/features/text/utils/text_utils.dart (1)
13-13: LGTM!The addition of a trailing newline is a standard formatting practice and has no functional impact.
lib/features/sheet/providers/sheet_providers.g.dart (1)
44-44: LGTM!The hash update in this generated file is expected and reflects upstream changes to the provider implementation.
lib/main.dart (2)
11-11: LGTM!The import enables access to the snackbar service provider.
62-62: LGTM!The scaffoldMessengerKey wiring correctly connects the app-wide messenger to the snackbar service, enabling centralized snackbar display across the application.
lib/constants/firestore_collection_constants.dart (1)
3-4: LGTM!The new collection constants follow the established pattern and provide clear, descriptive names for the Firestore collections.
lib/features/text/providers/text_providers.g.dart (1)
44-44: LGTM!The hash update in this generated file reflects upstream changes to the text provider implementation.
lib/common/providers/common_providers.dart (2)
6-7: LGTM!The import enables the provider to create and expose the SnackbarService.
24-28: LGTM!The provider correctly exposes a singleton-like SnackbarService instance that Riverpod will cache, enabling centralized snackbar management across the app.
lib/features/text/models/text_model.dart (1)
1-1: LGTM!The Cloud Firestore import is necessary for Timestamp conversions in the JSON serialization methods.
lib/constants/firestore_field_constants.dart (1)
9-14: LGTM!The new field constants are well-named and consistent with the existing pattern. They properly support the new event model serialization and other Firestore operations.
lib/features/events/models/events_model.dart (1)
10-14: ThedisplayNamegetter is unused in serialization.The
displayNamegetter provides human-readable values ("Yes", "No", "Maybe"), buttoJson()on line 139 uses.nameinstead (which yields "yes", "no", "maybe"). IfdisplayNameis intended for user-facing display only, this is fine. However, if it was meant for serialization, the implementation is inconsistent.Verify the intended usage of
displayName- is it for UI display or Firestore storage?test/features/text/providers/text_providers_test.dart (1)
148-159: No action needed — mock data has explicit orderIndex values.The
mockText1andmockText2inmock_fakefirestore_text.darthave explicitorderIndexvalues (1 and 2 respectively), which guarantee the expected ordering. The test is properly supported by the mock data.test/features/text/actions/text_actions_test.dart (1)
132-442: LGTM! Thorough edge case coverage.The async test operations are properly awaited, and the test suite comprehensively covers edge cases including empty descriptions, null descriptions, and emoji-only scenarios. The AAA (Arrange-Act-Assert) pattern is consistently applied.
lib/features/text/providers/text_providers.dart (1)
96-127: LGTM! Clean derived provider implementations.The derived providers (
text,textByParent,textListSearch,sortedTexts) follow standard Riverpod patterns and correctly filter/transform the basetextListProviderstate. The use offirstOrNullsafely handles missing items.lib/features/events/providers/event_providers.dart (2)
126-138: LGTM! Correct RSVP update pattern.The use of string interpolation for the nested field path (
'${FirestoreFieldConstants.rsvpResponses}.$userId') is the correct approach for updating specific keys in a Firestore map field. The use ofstatus.nameproperly serializes the enum value.
157-278: LGTM! Well-structured event filtering and RSVP logic.The date-based filtering (today's, upcoming, past events) uses appropriate
DateTimecomparisons and sorting. The RSVP-related providers correctly count and filter responses. The code follows consistent patterns with the text providers.
| factory TextModel.fromJson(Map<String, dynamic> json) { | ||
| return TextModel( | ||
| /// ContentModel properties | ||
| id: json['id'], | ||
| sheetId: json['sheetId'] ?? '', | ||
| parentId: json['parentId'] ?? '', | ||
| title: json['title'] ?? '', | ||
| description: json['description'] != null | ||
| ? ( | ||
| plainText: json['description']['plainText'] ?? '', | ||
| htmlText: json['description']['htmlText'] ?? '', | ||
| ) | ||
| : null, | ||
| emoji: json['emoji'], | ||
| createdBy: json['createdBy'] ?? '', | ||
| createdAt: json['createdAt'] == null | ||
| ? DateTime.now() | ||
| : (json['createdAt'] as Timestamp).toDate(), | ||
| updatedAt: json['updatedAt'] == null | ||
| ? DateTime.now() | ||
| : (json['updatedAt'] as Timestamp).toDate(), | ||
| orderIndex: json['orderIndex'], | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the TextModel class definition
fd -e dart -type f | xargs grep -l "class TextModel" | head -5Repository: acterglobal/zoe
Length of output: 229
🏁 Script executed:
# Search for TextModel instantiation patterns as suggested in the original comment
rg -n "TextModel\(" --type dart -A 3 -B 2 | head -100Repository: acterglobal/zoe
Length of output: 7148
🏁 Script executed:
# Check if there are any tests that reveal expected behavior with null timestamps
fd -e dart -type f -path "*/test/*" | xargs grep -l "TextModel" | head -5Repository: acterglobal/zoe
Length of output: 229
🏁 Script executed:
cat -n lib/features/text/models/text_model.dartRepository: acterglobal/zoe
Length of output: 3299
🏁 Script executed:
# Search for fromJson calls in production code (non-test files)
rg -n "fromJson" lib/features/text --type dart -B 2 -A 2 | head -80Repository: acterglobal/zoe
Length of output: 831
🏁 Script executed:
# Check the ContentModel parent class to understand field requirements
rg -n "class ContentModel" lib --type dart -A 20Repository: acterglobal/zoe
Length of output: 1612
🏁 Script executed:
# Search for any Firestore rules or schema validation
fd -name "*.rules" -o -name "*.json" | xargs grep -l "createdAt\|updatedAt" 2>/dev/null | head -10Repository: acterglobal/zoe
Length of output: 289
🏁 Script executed:
# Check for any data validation or cleanup code related to timestamps
rg -n "DateTime.now()" lib/features --type dart -B 2 -A 2Repository: acterglobal/zoe
Length of output: 50371
🏁 Script executed:
# Check git history to see if this DateTime.now() pattern was intentional
git log --oneline -n 30 -- lib/features/text/models/text_model.dart 2>/dev/null | head -20Repository: acterglobal/zoe
Length of output: 103
🏁 Script executed:
# Look for any server-side timestamp handling or Firestore server timestamp patterns
rg -n "serverTimestamp\|FieldValue" lib --type dartRepository: acterglobal/zoe
Length of output: 41
Address timestamp and required field handling in fromJson.
The fromJson implementation has two issues that warrant fixing:
-
DateTime.now() fallback corrupts historical data (lines 63-68): When
createdAtorupdatedAtare null in the JSON, usingDateTime.now()replaces the original timestamp with the current time. SinceContentModelexpects non-nullable DateTime fields, either ensure Firestore documents always contain these fields, validate them before deserialization, or throw an error for missing audit timestamps. -
Empty string defaults mask missing required fields (lines 52-54, 62): Fields like
sheetId,parentId,title, andcreatedByare marked as required in the constructor but receive empty string defaults infromJsonwhen missing. This creates a contract violation—the model claims these fields are required but accepts empty values. Either validate and reject incomplete data, or make these fields nullable inContentModel.
This pattern appears across multiple content models (SheetModel, EventsModel, etc.) and should be addressed consistently.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/events/widgets/event_rsvp_widget.dart (1)
18-31: Prevent RSVP writes when user ID is missing
currentUsercomes from an async provider, but when it’s still loading or null you passcurrentUser.value ?? ''into_buildRsvpButtonsand then intoupdateRsvpResponse. That can record an RSVP under a blank user ID (or fail, depending on the backend). Consider disabling the buttons untilcurrentUserhas a non-empty ID, or guard inonTapto early-return / prompt login instead of writing.Also applies to: 116-124
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
lib/features/bullets/providers/bullet_providers.g.dart(1 hunks)lib/features/documents/providers/document_providers.g.dart(1 hunks)lib/features/events/models/events_model.dart(3 hunks)lib/features/events/providers/event_providers.dart(4 hunks)lib/features/events/providers/event_providers.g.dart(1 hunks)lib/features/events/widgets/event_rsvp_widget.dart(4 hunks)lib/features/task/providers/task_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)
✅ Files skipped from review due to trivial changes (3)
- lib/features/documents/providers/document_providers.g.dart
- lib/features/events/providers/event_providers.g.dart
- lib/features/task/providers/task_providers.g.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/features/text/models/text_model.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). (6)
- GitHub Check: test
- GitHub Check: Test zoe_native plugin
- GitHub Check: windows
- GitHub Check: linux
- GitHub Check: android
- GitHub Check: ios
🔇 Additional comments (3)
lib/features/bullets/providers/bullet_providers.g.dart (1)
47-47: LGTM! Expected hash change in generated file.The hash update is expected after regenerating the provider code. This indicates the underlying
BulletListprovider was modified, which aligns with the PR's goal of adding Firebase/Firestore support.lib/features/text/providers/text_providers.dart (1)
18-43: Stream-backed TextList lifecycle and write helpers look solidSubscription is correctly reset on rebuild, cancelled in
onDispose, errors are routed viarunFirestoreOperation, and all CRUD helpers use field constants and server timestamps consistently. No issues from my side here.Also applies to: 46-97
lib/features/events/providers/event_providers.dart (1)
20-46: EventList real-time subscription and CRUD helpers look consistentThe list provider correctly resets/cancels its subscription across rebuilds, disposes it via
ref.onDispose, and routes all add/delete/update operations throughrunFirestoreOperationwith the shared field constants and server timestamps. This matches the text-provider pattern and looks good.Also applies to: 48-128
| factory EventModel.fromJson(Map<String, dynamic> json) { | ||
| final rsvpResponses = <String, RsvpStatus>{}; | ||
|
|
||
| final raw = json['rsvpResponses']; | ||
|
|
||
| if (raw is Map<String, dynamic>) { | ||
| raw.forEach((key, value) { | ||
| final match = RsvpStatus.values.firstWhere((e) => e.name == value); | ||
| rsvpResponses[key] = match; | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Make RSVP deserialization resilient to unexpected values
firstWhere((e) => e.name == value) will throw if Firestore ever contains an unknown/legacy string for rsvpResponses (e.g., manual edits or older data), crashing deserialization. Safer to skip unknown values or fall back to a default:
if (raw is Map<String, dynamic>) {
raw.forEach((key, value) {
final status = RsvpStatus.values.where((e) => e.name == value).cast<RsvpStatus?>().firstWhere(
(s) => s != null,
orElse: () => null,
);
if (status != null) {
rsvpResponses[key] = status;
}
});
}(or an equivalent small helper to parse with a nullable result).
🤖 Prompt for AI Agents
In lib/features/events/models/events_model.dart around lines 77 to 88, the
current deserialization uses RsvpStatus.values.firstWhere(...) which will throw
if the stored string isn't a known enum name; change the parsing to a safe
lookup that returns null (or a default) for unknown values and only insert into
rsvpResponses when a valid RsvpStatus is found. Implement this by replacing
firstWhere with a version that supplies an orElse returning null (or a default
enum) and guarding the map insertion, or by using a small helper that maps the
string to a nullable RsvpStatus and skips unknown entries.
| Future<void> updateRsvpResponse( | ||
| String eventId, | ||
| String userId, | ||
| RsvpStatus status, | ||
| ) async { | ||
| await runFirestoreOperation( | ||
| ref, | ||
| () => collection.doc(eventId).update({ | ||
| '${FirestoreFieldConstants.rsvpResponses}.$userId': status.name, | ||
| FirestoreFieldConstants.updatedAt: FieldValue.serverTimestamp(), | ||
| }), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "event_providers.dart" -type fRepository: acterglobal/zoe
Length of output: 111
🏁 Script executed:
wc -l lib/features/events/providers/event_providers.dartRepository: acterglobal/zoe
Length of output: 113
🏁 Script executed:
cat -n lib/features/events/providers/event_providers.dart | sed -n '100,160p'Repository: acterglobal/zoe
Length of output: 2334
🏁 Script executed:
cat -n lib/features/events/providers/event_providers.dart | sed -n '1,50p'Repository: acterglobal/zoe
Length of output: 2036
🏁 Script executed:
rg "updateRsvpResponse" -A 3 -B 3Repository: acterglobal/zoe
Length of output: 1860
🏁 Script executed:
rg "loggedInUserProvider" -A 5 -B 2 | head -60Repository: acterglobal/zoe
Length of output: 5280
🏁 Script executed:
fd "\.dart$" | xargs rg -l "updateRsvpResponse" | grep -E "(widget|page)" | head -5Repository: acterglobal/zoe
Length of output: 109
🏁 Script executed:
rg "updateRsvpResponse" --files-with-matches | head -10Repository: acterglobal/zoe
Length of output: 217
🏁 Script executed:
cat -n lib/features/events/providers/event_providers.dart | sed -n '120,170p'Repository: acterglobal/zoe
Length of output: 2067
🏁 Script executed:
rg "userId" lib/features/events/providers/event_providers.dart -A 2 -B 2Repository: acterglobal/zoe
Length of output: 382
🏁 Script executed:
fd "\.dart$" -path "*/events/*" | xargs rg -l "updateRsvpResponse"Repository: acterglobal/zoe
Length of output: 403
🏁 Script executed:
find lib/features/events -name "*.dart" | xargs rg -l "updateRsvpResponse"Repository: acterglobal/zoe
Length of output: 160
🏁 Script executed:
find lib/features/events -type f -name "*.dart" | head -20Repository: acterglobal/zoe
Length of output: 823
🏁 Script executed:
rg -r "$0" --files-with-matches "updateRsvpResponse" lib/features/events/Repository: acterglobal/zoe
Length of output: 160
🏁 Script executed:
git ls-files lib/features/events/ | grep -E "\.(dart)$"Repository: acterglobal/zoe
Length of output: 823
🏁 Script executed:
rg "updateRsvpResponse" --type dart -lRepository: acterglobal/zoe
Length of output: 217
🏁 Script executed:
find lib/features/events -type f -name "*.dart"Repository: acterglobal/zoe
Length of output: 823
🏁 Script executed:
cat -n lib/features/events/providers/event_providers.dartRepository: acterglobal/zoe
Length of output: 11068
🏁 Script executed:
rg "class EventProvider\|class.*Event.*Provider" lib/features/events/providers/Repository: acterglobal/zoe
Length of output: 41
🏁 Script executed:
find lib/features/events -name "*view*" -o -name "*page*" | xargs grep -l "updateRsvpResponse" 2>/dev/null || echo "No matches found"Repository: acterglobal/zoe
Length of output: 75
🏁 Script executed:
rg "updateRsvpResponse" --type dartRepository: acterglobal/zoe
Length of output: 385
🏁 Script executed:
rg "FieldPath\|\.path\|Firestore.*path" lib/features/events/ -B 2 -A 2 | head -50Repository: acterglobal/zoe
Length of output: 41
🏁 Script executed:
rg "final.*loggedInUserProvider|loggedInUserProvider\s*=" lib/ -A 5 -B 2Repository: acterglobal/zoe
Length of output: 19231
🏁 Script executed:
find lib -name "*auth*provider*" -o -name "*user*provider*" | head -10Repository: acterglobal/zoe
Length of output: 256
🏁 Script executed:
find lib -path "*/providers*" -name "*.dart" | head -20Repository: acterglobal/zoe
Length of output: 1102
🏁 Script executed:
rg "import.*event_providers|from.*event_providers" --type dartRepository: acterglobal/zoe
Length of output: 3802
🏁 Script executed:
find lib/features/events -type f -name "*.dart" -exec basename {} \;Repository: acterglobal/zoe
Length of output: 405
🏁 Script executed:
ls -la lib/features/events/Repository: acterglobal/zoe
Length of output: 564
🏁 Script executed:
find lib/features/events -type dRepository: acterglobal/zoe
Length of output: 270
🏁 Script executed:
find lib/features/events -type fRepository: acterglobal/zoe
Length of output: 823
🏁 Script executed:
find lib -name "*event*view*" -o -name "*event*page*"Repository: acterglobal/zoe
Length of output: 41
🏁 Script executed:
rg "RSVP\|rsvp" lib/features/events/ --type dart -lRepository: acterglobal/zoe
Length of output: 41
🏁 Script executed:
cat -n lib/features/events/widgets/event_actions_button.dart 2>/dev/null || echo "File not found"Repository: acterglobal/zoe
Length of output: 73
🏁 Script executed:
ls lib/features/events/widgets/Repository: acterglobal/zoe
Length of output: 211
🏁 Script executed:
cat -n lib/features/events/widgets/event_actions_button.dartRepository: acterglobal/zoe
Length of output: 144
🏁 Script executed:
rg "loggedInUserProvider\|userId" lib/features/events/widgets/event_actions_button.dart -B 3 -A 3Repository: acterglobal/zoe
Length of output: 152
🏁 Script executed:
find lib -path "*/providers*" -name "*.dart" -exec grep -l "loggedInUserProvider" {} \;Repository: acterglobal/zoe
Length of output: 514
🏁 Script executed:
rg "final.*loggedInUserProvider\s*=" lib/ -B 2 -A 8Repository: acterglobal/zoe
Length of output: 41
🏁 Script executed:
ls lib/features/auth/providers/ 2>/dev/null || ls lib/features/*/providers/ | head -20Repository: acterglobal/zoe
Length of output: 100
🏁 Script executed:
rg "loggedInUserProvider" lib/ --type dart | head -20Repository: acterglobal/zoe
Length of output: 2276
🏁 Script executed:
find lib -name "*user*provider*"Repository: acterglobal/zoe
Length of output: 158
🏁 Script executed:
ls lib/shared/providers/ 2>/dev/null || ls lib/providers/ 2>/dev/null || echo "No standard providers dir"Repository: acterglobal/zoe
Length of output: 84
🏁 Script executed:
rg "loggedInUserProvider\s*=" lib/ --type dartRepository: acterglobal/zoe
Length of output: 164
🏁 Script executed:
cat -n lib/features/events/widgets/event_actions_button.dart | sed -n '1,50p'Repository: acterglobal/zoe
Length of output: 144
🏁 Script executed:
wc -l lib/features/events/widgets/event_actions_button.dartRepository: acterglobal/zoe
Length of output: 143
🏁 Script executed:
cat -n lib/features/events/widgets/event_actions_button.dart | sed -n '50,150p'Repository: acterglobal/zoe
Length of output: 144
🏁 Script executed:
rg "updateRsvpResponse" lib/features/events/widgets/ -B 10 -A 5Repository: acterglobal/zoe
Length of output: 1472
🏁 Script executed:
tail -n 100 lib/features/events/widgets/event_actions_button.dart | head -50Repository: acterglobal/zoe
Length of output: 171
🏁 Script executed:
rg "await.*updateRsvpResponse|updateRsvpResponse\(" lib/ --type dart -B 5 -A 3Repository: acterglobal/zoe
Length of output: 1446
🏁 Script executed:
cat lib/features/events/widgets/event_actions_button.dart | grep -n "updateRsvpResponse" -B 20 -A 5Repository: acterglobal/zoe
Length of output: 144
🏁 Script executed:
rg "\.update\(\{" lib/ --type dart -A 3 | head -40Repository: acterglobal/zoe
Length of output: 2990
🏁 Script executed:
rg "FieldPath" lib/ --type dartRepository: acterglobal/zoe
Length of output: 41
🏁 Script executed:
rg "assert.*userId|userId.*empty\|userId.*isEmpty\|userId != null" lib/features/events/ --type dartRepository: acterglobal/zoe
Length of output: 41
🏁 Script executed:
rg "runFirestoreOperation" lib/ --type dart -B 2 -A 10 | head -80Repository: acterglobal/zoe
Length of output: 5898
🏁 Script executed:
find lib -name "*user*model*" -o -name "*user*\.dart" | head -10Repository: acterglobal/zoe
Length of output: 538
🏁 Script executed:
rg "class User\|class.*User\s*\{" lib/ --type dart -A 5 | head -50Repository: acterglobal/zoe
Length of output: 41
🏁 Script executed:
rg "loggedInUserProvider" lib/ --type dart -A 2 | head -30Repository: acterglobal/zoe
Length of output: 2085
🏁 Script executed:
find lib/features -name "*auth*" -type dRepository: acterglobal/zoe
Length of output: 76
🏁 Script executed:
ls lib/features/auth/providers/Repository: acterglobal/zoe
Length of output: 100
🏁 Script executed:
cat lib/features/auth/providers/user_provider.dartRepository: acterglobal/zoe
Length of output: 137
🏁 Script executed:
cat lib/features/events/widgets/event_actions_button.dart | grep -n "loggedInUserProvider\|updateRsvpResponse" -B 10 -A 10Repository: acterglobal/zoe
Length of output: 144
🏁 Script executed:
cat -n lib/features/events/widgets/event_actions_button.dart | sed -n '80,180p'Repository: acterglobal/zoe
Length of output: 144
🏁 Script executed:
rg "rsvpResponses\|RSVP" lib/ --type dart | head -20Repository: acterglobal/zoe
Length of output: 41
🏁 Script executed:
rg "FirestoreFieldConstants" lib/ --type dart -A 1 -B 1 | head -60Repository: acterglobal/zoe
Length of output: 4425
🏁 Script executed:
find lib -name "*firestore*constant*" -o -name "*field*constant*"Repository: acterglobal/zoe
Length of output: 153
🏁 Script executed:
rg "class FirestoreFieldConstants" lib/ --type dart -A 30Repository: acterglobal/zoe
Length of output: 1368
🏁 Script executed:
web_search
Firestore field path special characters dots userId validationRepository: acterglobal/zoe
Length of output: 157
🏁 Script executed:
web_search
Firestore update field path dots special characters behaviorRepository: acterglobal/zoe
Length of output: 157
🏁 Script executed:
rg "class User" lib/features/auth/ --type dart -A 20 | head -50Repository: acterglobal/zoe
Length of output: 41
🏁 Script executed:
find lib/features/auth -name "*.dart" | xargs grep "class User"Repository: acterglobal/zoe
Length of output: 43
🏁 Script executed:
ls lib/features/auth/models/Repository: acterglobal/zoe
Length of output: 79
🏁 Script executed:
cat lib/features/auth/models/user_model.dartRepository: acterglobal/zoe
Length of output: 131
🏁 Script executed:
cat -n lib/features/events/widgets/event_actions_button.dart | sed -n '1,200p'Repository: acterglobal/zoe
Length of output: 144
🏁 Script executed:
grep -n "updateRsvpResponse" lib/features/events/widgets/event_actions_button.dartRepository: acterglobal/zoe
Length of output: 145
🏁 Script executed:
cat -n lib/features/events/widgets/event_actions_button.dart | sed -n '100,150p'Repository: acterglobal/zoe
Length of output: 144
🏁 Script executed:
cat -n lib/features/events/widgets/event_actions_button.dart | sed -n '110,140p'Repository: acterglobal/zoe
Length of output: 144
🏁 Script executed:
find lib -name "*.dart" -exec grep -l "runFirestoreOperation" {} \;Repository: acterglobal/zoe
Length of output: 203
🏁 Script executed:
rg "def runFirestoreOperation|runFirestoreOperation\s*\(" lib/ --type dart -B 2 -A 15 | head -80Repository: acterglobal/zoe
Length of output: 5981
🏁 Script executed:
rg "runFirestoreOperation" lib/ --type dart | head -5Repository: acterglobal/zoe
Length of output: 529
🏁 Script executed:
grep -n "runFirestoreOperation" lib/features/events/providers/event_providers.dartRepository: acterglobal/zoe
Length of output: 488
🏁 Script executed:
head -30 lib/features/events/providers/event_providers.dartRepository: acterglobal/zoe
Length of output: 1260
🏁 Script executed:
find lib -name "*firestore*extension*" -o -name "*firestore*" | grep -E "\.dart$"Repository: acterglobal/zoe
Length of output: 199
🏁 Script executed:
rg "runFirestoreOperation" lib/shared/ --type dart -A 15Repository: acterglobal/zoe
Length of output: 110
🏁 Script executed:
find lib/shared -name "*.dart" -type fRepository: acterglobal/zoe
Length of output: 104
🏁 Script executed:
cat lib/shared/extensions/firestore_extensions.dart | head -60Repository: acterglobal/zoe
Length of output: 138
🏁 Script executed:
rg "userId" lib/features/events/providers/event_providers.dart -B 2 -A 2Repository: acterglobal/zoe
Length of output: 382
🏁 Script executed:
web_search
Firestore FieldPath dotted path alternative userIdRepository: acterglobal/zoe
Length of output: 157
🏁 Script executed:
cat lib/features/events/widgets/event_rsvp_widget.dartRepository: acterglobal/zoe
Length of output: 5252
🏁 Script executed:
cat lib/features/users/providers/user_providers.dartRepository: acterglobal/zoe
Length of output: 3628
🌐 Web query:
Firestore dotted field path dots special characters userId
💡 Result:
Short answer: If a Firestore field name (e.g. a userId) contains dots or other special characters, you must treat the whole name as a single field name (not a dotted path). Use FieldPath (or the SDK equivalent) to refer to that single field-name segment. The REST/administration docs also show that field paths with special characters can be quoted/backticked when needed. [1][2]
Examples
-
JavaScript (web / Node):
- To access a field literally named "user.example@host" or "address.city" (a single key that contains a dot):
- const fp = new firebase.firestore.FieldPath('address.city'); // single segment containing a dot
- doc.get(fp) or doc.update({ [fp]: value }) (use SDK methods that accept FieldPath).
- To access nested map field address → city (two segments): new firebase.firestore.FieldPath('address','city'). [3][9]
- To access a field literally named "user.example@host" or "address.city" (a single key that contains a dot):
-
Java / Android:
- FieldPath.of("address.city") — treats "address.city" as one field name; FieldPath.of("address","city") — treats it as nested path segments. [6]
-
REST / Admin / index configuration:
- Field paths that contain special chars (.,
, [, ], *, etc.) may be quoted with backticks; inside quoted field paths only backtick must be escaped. Example:`address.city`` represents a field named address.city (not address.mapKey). [4][7]
- Field paths that contain special chars (.,
References
- Cloud Firestore REST field-path quoting and special-character rules. [4]
- Cloud Firestore REST docs (fields API) showing quoting/escaping examples. [7]
- Firebase JS FieldPath docs (constructor usage / segments). [9]
- Firebase REST reference page describing quoted fieldPath escaping. [3]
- Firebase Android FieldPath.of() reference (segments). [6]
If you want, tell me which SDK/language you’re using and I’ll give the exact code snippet.
🏁 Script executed:
rg "sanitize|validate|isEmpty" lib/features/events/ --type dart -A 2 -B 2Repository: acterglobal/zoe
Length of output: 2249
🏁 Script executed:
cat test/features/events/providers/event_providers_test.dart | grep -A 20 -B 5 "updateRsvpResponse"Repository: acterglobal/zoe
Length of output: 974
Validate/sanitize userId before building RSVP update path
updateRsvpResponse interpolates userId directly into a dotted field path ('${FirestoreFieldConstants.rsvpResponses}.$userId'). The button passes currentUser.value ?? '', which can be an empty string. Additionally, Firestore field paths with special characters (including dots) should use FieldPath to avoid unintended nested structures.
Add explicit validation (e.g., assert(userId.isNotEmpty)) or early-return to align with the validation pattern used elsewhere in the codebase (eventsList, currentUserRsvp providers).
🤖 Prompt for AI Agents
In lib/features/events/providers/event_providers.dart around lines 131-142, the
code interpolates userId directly into a dotted map update key which allows
empty strings and special characters (like dots) to produce invalid or nested
field paths; validate that userId is non-empty (assert or early-return)
consistent with other providers, and when applying the update use a
FieldPath-style key for map entries instead of string interpolation (e.g.,
construct the update using a FieldPath composed of the rsvpResponses map and the
sanitized userId) so special characters don’t create unintended nested fields.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/features/events/providers/event_providers.dart (1)
116-128: Add validation for emptyuserIdbefore Firestore update.The
updateRsvpResponsemethod interpolatesuserIddirectly into the field path without validation. If an empty string is passed, this creates an invalid field path likersvpResponses.which could cause unexpected Firestore behavior. This aligns with the validation pattern used ineventsListandcurrentUserRsvpproviders (lines 138, 224).Future<void> updateRsvpResponse( String eventId, String userId, RsvpStatus status, ) async { + if (userId.isEmpty) return; + await runFirestoreOperation( ref, () => collection.doc(eventId).update({ '${FirestoreFieldConstants.rsvpResponses}.$userId': status.name, FirestoreFieldConstants.updatedAt: FieldValue.serverTimestamp(), }), ); }
🧹 Nitpick comments (1)
lib/features/events/providers/event_providers.dart (1)
69-83: Use constants for description field keys.The nested description object uses hardcoded string keys
'plainText'and'htmlText'while other fields useFirestoreFieldConstants. This inconsistency could lead to maintenance issues if these keys need to change.() => collection.doc(eventId).update({ FirestoreFieldConstants.description: { - 'plainText': description.plainText, - 'htmlText': description.htmlText, + FirestoreFieldConstants.plainText: description.plainText, + FirestoreFieldConstants.htmlText: description.htmlText, }, FirestoreFieldConstants.updatedAt: FieldValue.serverTimestamp(), }),Ensure that
plainTextandhtmlTextconstants are added toFirestoreFieldConstantsif they don't already exist.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/features/events/providers/event_providers.dart(4 hunks)test/features/events/providers/event_providers_test.dart(3 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 zoe_native plugin
- GitHub Check: android
- GitHub Check: linux
- GitHub Check: ios
- GitHub Check: format
- GitHub Check: windows
- GitHub Check: macos
- GitHub Check: test
🔇 Additional comments (10)
test/features/events/providers/event_providers_test.dart (3)
26-34: LGTM!The setUp formatting changes improve readability. The test container is properly recreated with the
loggedInUserProvideroverride.
167-177: LGTM!The multiline formatting for the assertion block improves readability without changing the test logic. The conditional assertion correctly validates that visible matching events appear in filtered results.
324-334: LGTM!Consistent with the formatting applied in the earlier search test. The assertion logic remains correct.
lib/features/events/providers/event_providers.dart (7)
1-15: LGTM!The imports are well-organized and include all necessary dependencies for Firestore operations, error handling, and constants.
20-46: LGTM!The Firestore subscription pattern is correctly implemented:
- Subscription is cancelled on rebuild and dispose to prevent memory leaks
- Error handler is properly attached to the listener
- Initial empty state is returned while waiting for Firestore data
48-67: LGTM!The
addEvent,deleteEvent, andupdateEventTitlemethods correctly wrap Firestore operations withrunFirestoreOperationfor consistent error handling. Usingsetfor add anddelete/updatefor mutations is appropriate.
85-113: LGTM!The date and order index update methods correctly convert
DateTimetoTimestampfor Firestore storage and consistently update theupdatedAtfield.
131-145: LGTM!The membership filtering logic correctly guards against null/empty user IDs and properly filters events based on sheet membership.
147-199: LGTM!The derived event list providers correctly categorize events by date (today, upcoming, past) and apply appropriate sorting. The search provider properly handles empty search values.
201-268: LGTM!The event lookup and RSVP-related providers are well-implemented with proper null-safety checks and consistent use of
firstOrNullfor safe lookups.
…irebase # Conflicts: # lib/constants/firestore_field_constants.dart
1 participant
Summary by CodeRabbit
New Features
User-facing improvements
Documentation / Tests
✏️ Tip: You can customize this high-level summary in your review settings.