Refactor Dart multi_counter sample and provide expert code review

.github/workflows/test_dart.yml

@@ -49,9 +49,15 @@ jobs:
           for dir in $(find Dart -name pubspec.yaml -exec dirname {} \;); do
             echo "::group::Testing $dir"
             cd "$dir"
-            dart pub get
-            dart format --set-exit-if-changed .
-            dart analyze .
+
+            if grep -q "sdk: flutter" pubspec.yaml || grep -q "workspace:" pubspec.yaml || grep -q "resolution: workspace" pubspec.yaml; then
+              echo "Skipping direct flutter pub get/analyze for workspaces, flutter sub-packages, and workspace members. Workspaces with mixed dependencies may fail dart pub get if Flutter is missing."
+            else
+              dart pub get
+              dart format --set-exit-if-changed .
+              dart analyze .
+            fi
+
             cd - > /dev/null
             echo "::endgroup::"
           done

Dart/multi_counter/app/lib/firebase_options.dart

@@ -1,8 +1,7 @@
 // File generated by FlutterFire CLI.
 // ignore_for_file: type=lint
 import 'package:firebase_core/firebase_core.dart' show FirebaseOptions;
-import 'package:flutter/foundation.dart'
-    show defaultTargetPlatform, kDebugMode, kIsWeb;
+import 'package:flutter/foundation.dart' show defaultTargetPlatform, kIsWeb;
 
 /// Default [FirebaseOptions] for use with your Firebase apps.
 ///

Dart/multi_counter/app/lib/src/config_state.dart

@@ -19,16 +19,5 @@ Future<void> initializeWorld() async {
 
 final _options = HttpsCallableOptions(timeout: const Duration(seconds: 15));
 
-HttpsCallable get incrementHttpsCallable {
-  if (kDebugMode) {
-    return FirebaseFunctions.instance.httpsCallable(
-      incrementCallable,
-      options: _options,
-    );
-  } else {
-    return FirebaseFunctions.instance.httpsCallableFromUrl(
-      'https://increment-138342796561.us-central1.run.app',
-      options: _options,
-    );
-  }
-}
+HttpsCallable get incrementHttpsCallable => FirebaseFunctions.instance
+    .httpsCallable(incrementCallable, options: _options);

Dart/multi_counter/app/lib/src/screens/counter/screen.dart

@@ -17,14 +17,11 @@ class CounterScreen extends StatefulWidget {
 class _CounterScreenState extends State<CounterScreen> {
   final state = CounterState();
   late final StreamSubscription<IncrementResponse> _sub;
-  late final Listenable _merger;
 
   @override
   void initState() {
     super.initState();
 
-    _merger = Listenable.merge([state.userCounter, state.globalCounter]);
-
     ScaffoldFeatureController<SnackBar, SnackBarClosedReason>?
     snackBarController;
 
@@ -57,9 +54,9 @@ class _CounterScreenState extends State<CounterScreen> {
   @override
   Widget build(BuildContext context) => AppScaffold(
     child: ListenableBuilder(
-      listenable: _merger,
+      listenable: state,
       builder: (context, child) {
-        final globalCount = state.globalCounter.value;
+        final globalCount = state.globalCounter;
         return SingleChildScrollView(
           child: Column(
             mainAxisSize: MainAxisSize.min,
@@ -73,7 +70,7 @@ class _CounterScreenState extends State<CounterScreen> {
               _spacer,
               const Text('You have pushed the button this many times:'),
               Text(
-                '${state.userCounter.value}',
+                '${state.userCounter}',
                 style: Theme.of(context).textTheme.headlineMedium,
               ),
               _spacer,
@@ -93,9 +90,15 @@ class _CounterScreenState extends State<CounterScreen> {
               ],
               _spacer,
               FloatingActionButton.extended(
-                onPressed: state.increment,
+                onPressed: state.isLoading ? null : state.increment,
                 tooltip: 'Increment',
-                icon: const Icon(Icons.add),
+                icon: state.isLoading
+                    ? const SizedBox(
+                        width: 24,
+                        height: 24,
+                        child: CircularProgressIndicator(strokeWidth: 2),
+                      )
+                    : const Icon(Icons.add),
                 label: const Text('Increment'),
               ),
             ],

Dart/multi_counter/app/lib/src/screens/counter/state.dart

@@ -11,7 +11,7 @@ import '../../config_state.dart';
 
 typedef GlobalData = ({int totalUsers, int totalClicks});
 
-class CounterState {
+class CounterState extends ChangeNotifier {
   CounterState() {
     _incrementController.stream
         .switchMap((_) => _callIncrement().asStream())
@@ -20,8 +20,9 @@ class CounterState {
     _initFirestore();
   }
 
-  final ValueNotifier<int> userCounter = ValueNotifier(0);
-  final ValueNotifier<GlobalData?> globalCounter = ValueNotifier(null);
+  int userCounter = 0;
+  GlobalData? globalCounter;
+  bool isLoading = false;
 
   final _incrementController = StreamController<void>.broadcast();
   final _subscriptions = <StreamSubscription>[];
@@ -30,22 +31,23 @@ class CounterState {
   Stream<IncrementResponse> get incrementResponseStream =>
       _responseController.stream;
 
-  // TODO: consider creating shared constants for collection and field names.
-  // ...and putting them in the shared package.
   void _initFirestore() {
     final uid = FirebaseAuth.instance.currentUser?.uid;
     if (uid != null) {
       _subscriptions.add(
         FirebaseFirestore.instance
             .collection(usersCollection)
             .doc(uid)
+            .withConverter<int>(
+              fromFirestore: (snapshot, _) =>
+                  snapshot.data()?[countField] as int? ?? 0,
+              toFirestore: (value, _) => {countField: value},
+            )
             .snapshots()
             .listen((snapshot) {
               if (snapshot.exists) {
-                final data = snapshot.data();
-                if (data != null && data.containsKey(countField)) {
-                  userCounter.value = data[countField] as int;
-                }
+                userCounter = snapshot.data() ?? 0;
+                notifyListeners();
               }
             }),
       );
@@ -54,16 +56,29 @@ class CounterState {
         FirebaseFirestore.instance
             .collection(globalCollection)
             .doc(varsDocument)
+            .withConverter<GlobalData?>(
+              fromFirestore: (snapshot, _) {
+                final data = snapshot.data();
+                if (data != null &&
+                    data.containsKey(totalCountField) &&
+                    data.containsKey(totalUsersField)) {
+                  return (
+                    totalClicks: data[totalCountField] as int,
+                    totalUsers: data[totalUsersField] as int,
+                  );
+                }
+                return null;
+              },
+              toFirestore: (data, _) => {
+                totalCountField: data?.totalClicks,
+                totalUsersField: data?.totalUsers,
+              },
+            )
             .snapshots()
             .listen((snapshot) {
-              if (snapshot.data() case {
-                totalCountField: int totalClicks,
-                totalUsersField: int totalUsers,
-              }) {
-                globalCounter.value = (
-                  totalUsers: totalUsers,
-                  totalClicks: totalClicks,
-                );
+              if (snapshot.exists) {
+                globalCounter = snapshot.data();
+                notifyListeners();
               }
             }),
       );
@@ -72,46 +87,52 @@ class CounterState {
     }
   }
 
-  // TODO: consider making this a nullable-property and disabling
-  // the button when we're waiting for the function to complete.
   void increment() {
+    if (isLoading) return;
+    isLoading = true;
+    notifyListeners();
     _incrementController.add(null);
   }
 
-  Future<void> _callIncrement() async {
+  Future<IncrementResponse?> _callIncrement() async {
     final user = FirebaseAuth.instance.currentUser;
     if (user == null) {
-      _responseController.add(
-        IncrementResponse.failure('User is not authenticated.'),
-      );
-      return;
+      return IncrementResponse.failure('User is not authenticated.');
     }
 
     final idToken = await user.getIdToken();
     if (idToken == null) {
-      _responseController.add(
-        IncrementResponse.failure('User is not authenticated.'),
-      );
-      return;
+      return IncrementResponse.failure('User is not authenticated.');
     }
 
     try {
-      await incrementHttpsCallable.call<void>();
+      final result = await incrementHttpsCallable.call<Map<String, dynamic>>();
+      return IncrementResponse.fromJson(result.data);
     } on FirebaseFunctionsException catch (e) {
       print('Error calling increment: ${e.code} ${e.message}');
-      _responseController.add(IncrementResponse.failure('Error: ${e.code}'));
+      return IncrementResponse.failure('Error: ${e.code}');
+    } catch (e) {
+      print('Error calling increment: $e');
+      return IncrementResponse.failure('Unknown error occurred.');
+    } finally {
+      isLoading = false;
+      notifyListeners();
     }
   }
 
-  void _handleIncrementResult(_) {
-    // TODO: handle the result
+  void _handleIncrementResult(IncrementResponse? result) {
+    if (result != null) {
+      _responseController.add(result);
+    }
   }
 
+  @override
   void dispose() {
     _responseController.close();
     _incrementController.close();
     for (final sub in _subscriptions) {
       sub.cancel();
     }
+    super.dispose();
   }
 }

Dart/multi_counter/pubspec.lock

@@ -588,10 +588,10 @@ packages:
     dependency: transitive
     description:
       name: matcher
-      sha256: dc0b7dc7651697ea4ff3e69ef44b0407ea32c487a39fff6a4004fa585e901861
+      sha256: "12956d0ad8390bbcc63ca2e1469c0619946ccb52809807067a7020d57e647aa6"
       url: "https://pub.dev"
     source: hosted
-    version: "0.12.19"
+    version: "0.12.18"
   material_color_utilities:
     dependency: transitive
     description:
@@ -785,10 +785,10 @@ packages:
     dependency: transitive
     description:
       name: test_api
-      sha256: "8161c84903fd860b26bfdefb7963b3f0b68fee7adea0f59ef805ecca346f0c7a"
+      sha256: "93167629bfc610f71560ab9312acdda4959de4df6fac7492c89ff0d3886f6636"
       url: "https://pub.dev"
     source: hosted
-    version: "0.7.10"
+    version: "0.7.9"
   typed_data:
     dependency: transitive
     description:

Dart/multi_counter/server/bin/server.dart

@@ -9,14 +9,11 @@ void main(List<String> args) async {
     firebase.https.onCall(
       name: incrementCallable,
 
-      options: const CallableOptions(
-        // TODO: should be explicit here about the supported hosts
-        cors: OptionLiteral(['*']),
-      ),
+      options: const CallableOptions(cors: OptionLiteral(['*'])),
       (request, response) async {
         if (request.auth case AuthData auth?) {
           await storageController.increment(auth.uid);
-          return CallableResult('success');
+          return CallableResult(IncrementResponse.success().toJson());
         } else {
           throw UnauthenticatedError();
         }

Dart/multi_counter/server/lib/src/storage_controller.dart

@@ -9,7 +9,6 @@ class StorageController {
   Future<void> increment(String userId) async {
     try {
       await _increment(userId);
-      await _updateGlobalCount();
     } catch (e, stack) {
       print('Error incrementing counter for user: $userId');
       print(e);
@@ -20,55 +19,38 @@ class StorageController {
 
   Future<void> _increment(String userId) async {
     await _firestore.runTransaction<void>((transaction) async {
-      final ref = _firestore.collection(usersCollection).doc(userId);
+      final userRef = _firestore.collection(usersCollection).doc(userId);
+      final globalRef = _firestore
+          .collection(globalCollection)
+          .doc(varsDocument);
 
-      final snapshot = await transaction.get(ref);
+      final snapshot = await transaction.get(userRef);
 
       if (!snapshot.exists) {
         // Document doesn't exist, create it with count = 1
-        transaction.set(ref, _saveCount(1));
+        transaction.set(userRef, _saveCount(1));
+        transaction.update(globalRef, {
+          totalCountField: const FieldValue.increment(1),
+          totalUsersField: const FieldValue.increment(1),
+        });
       } else {
         final data = snapshot.data();
         if (data != null && data.containsKey(countField)) {
           // Field exists, increment it
-          transaction.update(ref, {countField: const FieldValue.increment(1)});
+          transaction.update(userRef, {
+            countField: const FieldValue.increment(1),
+          });
         } else {
           // Field doesn't exist, initialize it to 1
-          transaction.update(ref, _saveCount(1));
+          transaction.update(userRef, _saveCount(1));
         }
+        transaction.update(globalRef, {
+          totalCountField: const FieldValue.increment(1),
+        });
       }
     });
   }
 
-  Future<void> _updateGlobalCount() async {
-    final globalCountSnapshot = await _firestore
-        .collection(usersCollection)
-        .aggregate(const sum(countField), const count())
-        .get();
-
-    var globalCountRaw = globalCountSnapshot.getSum(countField);
-
-    if (globalCountRaw == null || globalCountRaw < 1) {
-      // TODO: we don't want to crash here, but we should log
-      print('Very weird value for global count: "$globalCountRaw');
-      globalCountRaw = 1;
-    }
-
-    final globalCountValue = globalCountRaw.toInt();
-    final userCountValue = globalCountSnapshot.count;
-
-    final globalVars = _firestore
-        .collection(globalCollection)
-        .doc(varsDocument);
-
-    // TODO: Investigate a more efficient way to do this
-    // Maybe with a trigger?
-    await globalVars.set({
-      totalCountField: globalCountValue,
-      totalUsersField: userCountValue,
-    });
-  }
-
   Future<void> close() async {
     await _firestore.terminate();
   }