Conversation
Co-authored-by: kevmoo <kevmoo@google.com> Co-authored-by: Rody Davis <rody.davis.jr@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a multi-counter application using Flutter, Firebase, and Dart, organized within a workspace containing app, server, and shared packages. Key features include Google authentication, real-time Firestore synchronization for user and global statistics, and a Dart-based Cloud Function for incrementing counters. Feedback focuses on optimizing the server-side counter logic to use atomic increments instead of
| Future<void> increment(String userId) async { | ||
| try { | ||
| await _increment(userId); | ||
| await _updateGlobalCount(); | ||
| } catch (e, stack) { | ||
| print('Error incrementing counter for user: $userId'); | ||
| print(e); | ||
| print(stack); | ||
| rethrow; | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation updates the global count by performing a full aggregation over the users collection on every increment. This is an
Instead, you should update the global counters within the same transaction as the user's increment using FieldValue.increment. This ensures atomicity and provides _increment and _updateGlobalCount can be removed.
Future<void> increment(String userId) async {
try {
await _firestore.runTransaction<void>((transaction) async {
final userRef = _firestore.collection(usersCollection).doc(userId);
final globalRef = _firestore.collection(globalCollection).doc(varsDocument);
final userSnapshot = await transaction.get(userRef);
if (!userSnapshot.exists) {
transaction.set(userRef, {countField: 1});
transaction.update(globalRef, {
totalCountField: const FieldValue.increment(1),
totalUsersField: const FieldValue.increment(1),
});
} else {
transaction.update(userRef, {countField: const FieldValue.increment(1)});
transaction.update(globalRef, {
totalCountField: const FieldValue.increment(1),
});
}
});
} catch (e, stack) {
print('Error incrementing counter for user: $userId');
print(e);
print(stack);
rethrow;
}
}There was a problem hiding this comment.
I see what she's saying - but we lose the ability to guarantee we stay in sync!
| final user = FirebaseAuth.instance.currentUser; | ||
| if (user == null) { | ||
| _responseController.add( | ||
| IncrementResponse.failure('User is not authenticated.'), | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| final idToken = await user.getIdToken(); | ||
| if (idToken == null) { | ||
| _responseController.add( | ||
| IncrementResponse.failure('User is not authenticated.'), | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The call to user.getIdToken() is redundant. The Firebase Functions HttpsCallable automatically handles authentication tokens when a user is signed in. Additionally, the retrieved idToken is not used in the subsequent call.
| final user = FirebaseAuth.instance.currentUser; | |
| if (user == null) { | |
| _responseController.add( | |
| IncrementResponse.failure('User is not authenticated.'), | |
| ); | |
| return; | |
| } | |
| final idToken = await user.getIdToken(); | |
| if (idToken == null) { | |
| _responseController.add( | |
| IncrementResponse.failure('User is not authenticated.'), | |
| ); | |
| return; | |
| } | |
| final user = FirebaseAuth.instance.currentUser; | |
| if (user == null) { | |
| _responseController.add( | |
| IncrementResponse.failure('User is not authenticated.'), | |
| ); | |
| return; | |
| } |
There was a problem hiding this comment.
not a bad idea, although very unlikely, given the auth setup
| } | ||
|
|
||
| try { | ||
| await incrementHttpsCallable.call<void>(); |
There was a problem hiding this comment.
The _callIncrement method should notify the _responseController when the operation succeeds so the UI can provide feedback to the user (e.g., clearing a loading state or showing a success message).
| await incrementHttpsCallable.call<void>(); | |
| await incrementHttpsCallable.call<void>(); | |
| _responseController.add(IncrementResponse.success()); |
| } | ||
|
|
||
| void _handleIncrementResult(_) { | ||
| // TODO: handle the result |
There was a problem hiding this comment.
not sure what to do here. This method no longer does...anything. but it's the target of the stream.
|
|
||
| options: const CallableOptions( | ||
| // TODO: should be explicit here about the supported hosts | ||
| cors: OptionLiteral(['*']), |
| ], | ||
| redirect: (context, state) async { | ||
| // Ensure we capture the redirect result before checking the auth state. | ||
| await FirebaseAuth.instance.getRedirectResult(); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Maybe? I haven't tried it...
I think I put it here when fighting the other auth bits... Need to investigate.
* docs: add README for Dart multi_counter sample Adds a README to the Dart/multi_counter directory highlighting the sample was presented at Cloud Next 2026. Includes a link to the session. Also updates the root README.md to reference this sample. Co-authored-by: jhuleatt <3759507+jhuleatt@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --------- Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…endencies (#1275) * ci: setup flutter to build pure dart or flutter workspace projects Co-authored-by: jhuleatt <3759507+jhuleatt@users.noreply.github.com> * ci: fix unused import in firebase_options.dart causing dart analyze failure Co-authored-by: jhuleatt <3759507+jhuleatt@users.noreply.github.com> * ci: fix unused import in firebase_options.dart causing dart analyze failure Co-authored-by: jhuleatt <3759507+jhuleatt@users.noreply.github.com> * ci: fix unused import in firebase_options.dart causing dart analyze failure Co-authored-by: jhuleatt <3759507+jhuleatt@users.noreply.github.com> * ci: fix dart format exit code on lib/firebase_options.dart Co-authored-by: jhuleatt <3759507+jhuleatt@users.noreply.github.com> --------- Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
| Stream<IncrementResponse> get incrementResponseStream => | ||
| _responseController.stream; | ||
|
|
||
| // TODO: consider creating shared constants for collection and field names. |
| _responseController.stream; | ||
|
|
||
| // TODO: consider creating shared constants for collection and field names. | ||
| // ...and putting them in the shared package. |
| } | ||
| } | ||
|
|
||
| // TODO: consider making this a nullable-property and disabling |
| final user = FirebaseAuth.instance.currentUser; | ||
| if (user == null) { | ||
| _responseController.add( | ||
| IncrementResponse.failure('User is not authenticated.'), | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| final idToken = await user.getIdToken(); | ||
| if (idToken == null) { | ||
| _responseController.add( | ||
| IncrementResponse.failure('User is not authenticated.'), | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
not a bad idea, although very unlikely, given the auth setup
| } | ||
|
|
||
| void _handleIncrementResult(_) { | ||
| // TODO: handle the result |
There was a problem hiding this comment.
not sure what to do here. This method no longer does...anything. but it's the target of the stream.
| ); | ||
| } else { | ||
| return FirebaseFunctions.instance.httpsCallableFromUrl( | ||
| 'https://increment-138342796561.us-central1.run.app', |
There was a problem hiding this comment.
should we just put example.com here or something?
or do we put a throw in here with a TODO to populate it?
| const double doubleSpaceSize = spaceSize * 2; | ||
|
|
||
| // TODO: Update this to the final example URI | ||
| const githubDisplayUrl = 'github.com/kevmoo/next26_demo'; |
There was a problem hiding this comment.
Should likely ditch this and the bits that depend on it for the sample!
| ], | ||
| redirect: (context, state) async { | ||
| // Ensure we capture the redirect result before checking the auth state. | ||
| await FirebaseAuth.instance.getRedirectResult(); |
There was a problem hiding this comment.
Maybe? I haven't tried it...
I think I put it here when fighting the other auth bits... Need to investigate.
| name: incrementCallable, | ||
|
|
||
| options: const CallableOptions( | ||
| // TODO: should be explicit here about the supported hosts |
There was a problem hiding this comment.
The TODO is REAL – hrm... we could update it for the user to update when they know where they are deploying?
| Future<void> increment(String userId) async { | ||
| try { | ||
| await _increment(userId); | ||
| await _updateGlobalCount(); | ||
| } catch (e, stack) { | ||
| print('Error incrementing counter for user: $userId'); | ||
| print(e); | ||
| print(stack); | ||
| rethrow; | ||
| } | ||
| } |
There was a problem hiding this comment.
I see what she's saying - but we lose the ability to guarantee we stay in sync!
No description provided.