fix(models): make Drink user-state fields immutable with copyWith (#366)

* fix(models): make Drink user-state fields immutable with copyWith

isFavorite, rating, and isTasted are now final. BeerProvider replaces
list elements via copyWith rather than mutating shared objects, enabling
correct widget diffing and unblocking == / hashCode (see #323).

Fixes #322

* fix(provider): replace by stable id in _replaceDrink; await setRating in test

_replaceDrink now finds drinks by id+festivalId rather than object
identity, preventing missed updates when a caller holds a stale
reference after copyWith.

Also awaits provider.setRating() in drink_detail_screen_test to
eliminate a potential flaky assertion.

Addresses review comments on #366.

Author: richardthe3rd

lib/domain/repositories/api_drink_repository.dart

@@ -87,10 +87,13 @@ class ApiDrinkRepository implements DrinkRepository {
   /// Populate favorite status, ratings, and tasted status in a single pass.
   void _applyUserState(List<Drink> drinks, String festivalId) {
     final favorites = _favoritesService.getFavorites(festivalId);
-    for (final drink in drinks) {
-      drink.isFavorite = favorites.contains(drink.id);
-      drink.rating = _ratingsService.getRating(festivalId, drink.id);
-      drink.isTasted = _tastingLogService.hasTasted(festivalId, drink.id);
+    for (var i = 0; i < drinks.length; i++) {
+      final drink = drinks[i];
+      drinks[i] = drink.copyWith(
+        isFavorite: favorites.contains(drink.id),
+        rating: _ratingsService.getRating(festivalId, drink.id),
+        isTasted: _tastingLogService.hasTasted(festivalId, drink.id),
+      );
     }
   }
 

lib/models/drink.dart

@@ -223,9 +223,11 @@ class Drink {
   final Product product;
   final Producer producer;
   final String festivalId;
-  bool isFavorite;
-  int? rating;
-  bool isTasted;
+  final bool isFavorite;
+  final int? rating;
+  final bool isTasted;
+
+  static const _absent = Object();
 
   Drink({
     required this.product,
@@ -236,6 +238,17 @@ class Drink {
     this.isTasted = false,
   });
 
+  Drink copyWith({bool? isFavorite, Object? rating = _absent, bool? isTasted}) {
+    return Drink(
+      product: product,
+      producer: producer,
+      festivalId: festivalId,
+      isFavorite: isFavorite ?? this.isFavorite,
+      rating: identical(rating, _absent) ? this.rating : rating as int?,
+      isTasted: isTasted ?? this.isTasted,
+    );
+  }
+
   String get id => product.id;
   String get name => product.name;
   String get breweryName => producer.name;

lib/providers/beer_provider.dart

@@ -680,6 +680,20 @@ class BeerProvider extends ChangeNotifier {
     await prefs.setInt(PreferenceKeys.themeMode, mode.index);
   }
 
+  /// Replace a drink in [_allDrinks] with [updated] and recompute filters.
+  ///
+  /// Returns [updated] for convenience.
+  Drink _replaceDrink(Drink old, Drink updated) {
+    final idx = _allDrinks.indexWhere(
+      (d) => d.id == old.id && d.festivalId == old.festivalId,
+    );
+    if (idx != -1) {
+      _allDrinks[idx] = updated;
+    }
+    _filter.recompute();
+    return updated;
+  }
+
   /// Toggle favorite status for a drink
   Future<void> toggleFavorite(Drink drink) async {
     if (_drinkRepository == null) return;
@@ -688,11 +702,7 @@ class BeerProvider extends ChangeNotifier {
       currentFestival.id,
       drink.id,
     );
-    drink.isFavorite = newStatus;
-
-    if (_filter.showFavoritesOnly) {
-      _filter.recompute();
-    }
+    _replaceDrink(drink, drink.copyWith(isFavorite: newStatus));
 
     notifyListeners();
 
@@ -710,13 +720,12 @@ class BeerProvider extends ChangeNotifier {
 
     if (rating == null) {
       await _drinkRepository!.removeRating(currentFestival.id, drink.id);
-      drink.rating = null;
     } else {
       await _drinkRepository!.setRating(currentFestival.id, drink.id, rating);
-      drink.rating = rating;
       // Log analytics event for rating (fire and forget)
       unawaited(_analyticsService.logRatingGiven(drink, rating));
     }
+    _replaceDrink(drink, drink.copyWith(rating: rating));
     notifyListeners();
   }
 
@@ -728,13 +737,7 @@ class BeerProvider extends ChangeNotifier {
       currentFestival.id,
       drink.id,
     );
-    drink.isTasted = newStatus;
-
-    // Re-filter so the drink appears/disappears immediately when the
-    // not-tasted visibility filter is active.
-    if (_filter.visibilityFilters.contains(DrinkVisibilityFilter.notTasted)) {
-      _filter.recompute();
-    }
+    _replaceDrink(drink, drink.copyWith(isTasted: newStatus));
 
     notifyListeners();
 

test/beer_provider_test.dart

@@ -1034,7 +1034,7 @@ void main() {
         await provider.initialize();
 
         final sampleDrinks = createSampleDrinks();
-        sampleDrinks[0].isTasted = true;
+        sampleDrinks[0] = sampleDrinks[0].copyWith(isTasted: true);
         when(
           mockDrinkRepository.getDrinks(any),
         ).thenAnswer((_) async => sampleDrinks);
@@ -2337,15 +2337,16 @@ void main() {
           mockDrinkRepository.toggleTasted(any, any),
         ).thenAnswer((_) async => true);
         await provider.toggleTasted(drink);
-        expect(drink.isTasted, isTrue);
+        expect(provider.getDrinkById(drink.id)!.isTasted, isTrue);
         verify(mockAnalyticsService.logTastedAdded(drink)).called(1);
 
         when(
           mockDrinkRepository.toggleTasted(any, any),
         ).thenAnswer((_) async => false);
-        await provider.toggleTasted(drink);
-        expect(drink.isTasted, isFalse);
-        verify(mockAnalyticsService.logTastedRemoved(drink)).called(1);
+        final drink2 = provider.getDrinkById(drink.id)!;
+        await provider.toggleTasted(drink2);
+        expect(provider.getDrinkById(drink.id)!.isTasted, isFalse);
+        verify(mockAnalyticsService.logTastedRemoved(drink2)).called(1);
       });
 
       test(
@@ -2502,8 +2503,8 @@ void main() {
           ).thenAnswer((_) async => true);
           await provider.toggleFavorite(drink);
 
-          // The favourites-only list is refreshed in place by toggleFavorite.
-          expect(provider.drinks, contains(drink));
+          // The favourites-only list is refreshed by toggleFavorite.
+          expect(provider.drinks.any((d) => d.id == drink.id), isTrue);
         },
       );
 

test/brewery_screen_test.dart

@@ -200,7 +200,7 @@ void main() {
       await tester.pumpWidget(createTestWidget('brewery1'));
       await tester.pumpAndSettle();
 
-      expect(drink1.isFavorite, false);
+      expect(provider.getDrinkById(drink1.id)!.isFavorite, false);
 
       // Mock toggleFavorite to properly toggle state
       final favorites = <String>{};
@@ -225,7 +225,7 @@ void main() {
       await tester.tap(favoriteButton);
       await tester.pumpAndSettle();
 
-      expect(drink1.isFavorite, true);
+      expect(provider.getDrinkById(drink1.id)!.isFavorite, true);
     });
 
     testWidgets('displays correct count of drinks', (

test/domain/controllers/drink_filter_controller_test.dart

@@ -181,7 +181,7 @@ void main() {
     group('favorites filter', () {
       test('shows only favourites when enabled', () {
         final drinks = _sampleDrinks();
-        drinks[0].isFavorite = true;
+        drinks[0] = drinks[0].copyWith(isFavorite: true);
         controller.setSource(drinks);
 
         controller.setShowFavoritesOnly(true);
@@ -221,7 +221,7 @@ void main() {
 
       test('notTasted filter hides tasted drinks', () {
         final drinks = _sampleDrinks();
-        drinks[0].isTasted = true;
+        drinks[0] = drinks[0].copyWith(isTasted: true);
         controller.setSource(drinks);
         controller.setVisibilityFilter(DrinkVisibilityFilter.notTasted, true);
         expect(
@@ -327,18 +327,21 @@ void main() {
     });
 
     group('recompute', () {
-      test('reflects in-place favourite mutation when favourites-only', () {
-        final drinks = _sampleDrinks();
-        controller.setSource(drinks);
-        controller.setShowFavoritesOnly(true);
-        expect(controller.filteredDrinks, isEmpty);
-
-        // Simulate BeerProvider mutating the drink in place, then asking the
-        // controller to re-run the pipeline.
-        drinks[1].isFavorite = true;
-        controller.recompute();
-        expect(controller.filteredDrinks.map((d) => d.name), ['Beta Bitter']);
-      });
+      test(
+        'reflects favourite change via list replacement when favourites-only',
+        () {
+          final drinks = _sampleDrinks();
+          controller.setSource(drinks);
+          controller.setShowFavoritesOnly(true);
+          expect(controller.filteredDrinks, isEmpty);
+
+          // Simulate BeerProvider replacing a list element via copyWith, then
+          // asking the controller to re-run the pipeline.
+          drinks[1] = drinks[1].copyWith(isFavorite: true);
+          controller.setSource(drinks);
+          expect(controller.filteredDrinks.map((d) => d.name), ['Beta Bitter']);
+        },
+      );
     });
 
     group('clearCategoryStyleSearch', () {

test/domain/services/drink_filter_service_test.dart

@@ -137,16 +137,16 @@ void main() {
 
     group('filterByFavorites', () {
       test('filters to show only favorites', () {
-        testDrinks[0].isFavorite = true;
-        testDrinks[2].isFavorite = true;
+        testDrinks[0] = testDrinks[0].copyWith(isFavorite: true);
+        testDrinks[2] = testDrinks[2].copyWith(isFavorite: true);
 
         final result = service.filterByFavorites(testDrinks, true).toList();
         expect(result, hasLength(2));
         expect(result.every((d) => d.isFavorite), isTrue);
       });
 
       test('returns all drinks when favoritesOnly is false', () {
-        testDrinks[0].isFavorite = true;
+        testDrinks[0] = testDrinks[0].copyWith(isFavorite: true);
 
         final result = service.filterByFavorites(testDrinks, false).toList();
         expect(result, hasLength(5));
@@ -190,16 +190,16 @@ void main() {
 
     group('filterByNotTasted', () {
       test('hides drinks already tasted', () {
-        testDrinks[0].isTasted = true;
-        testDrinks[1].isTasted = true;
+        testDrinks[0] = testDrinks[0].copyWith(isTasted: true);
+        testDrinks[1] = testDrinks[1].copyWith(isTasted: true);
 
         final result = service.filterByNotTasted(testDrinks, true).toList();
         expect(result, hasLength(3));
         expect(result.every((d) => !d.isTasted), isTrue);
       });
 
       test('returns all drinks when notTastedOnly is false', () {
-        testDrinks[0].isTasted = true;
+        testDrinks[0] = testDrinks[0].copyWith(isTasted: true);
 
         final result = service.filterByNotTasted(testDrinks, false).toList();
         expect(result, hasLength(5));
@@ -390,7 +390,7 @@ void main() {
 
     group('filterDrinks', () {
       test('applies all filters in combination', () {
-        testDrinks[0].isFavorite = true; // Hoppy IPA
+        testDrinks[0] = testDrinks[0].copyWith(isFavorite: true); // Hoppy IPA
 
         final result = service.filterDrinks(
           testDrinks,

test/drink_card_test.dart

@@ -115,7 +115,7 @@ void main() {
     testWidgets('shows favorite icon as outlined when not favorite', (
       WidgetTester tester,
     ) async {
-      testDrink.isFavorite = false;
+      testDrink = testDrink.copyWith(isFavorite: false);
       await tester.pumpWidget(createTestWidget(drink: testDrink));
 
       expect(find.byIcon(Icons.favorite_border), findsOneWidget);
@@ -125,7 +125,7 @@ void main() {
     testWidgets('shows favorite icon as filled when favorite', (
       WidgetTester tester,
     ) async {
-      testDrink.isFavorite = true;
+      testDrink = testDrink.copyWith(isFavorite: true);
       await tester.pumpWidget(createTestWidget(drink: testDrink));
 
       expect(find.byIcon(Icons.favorite), findsOneWidget);

test/drink_detail_screen_test.dart

@@ -319,7 +319,7 @@ void main() {
       await tester.pumpWidget(createTestWidget('drink1'));
       await tester.pumpAndSettle();
 
-      expect(drink.isFavorite, false);
+      expect(provider.getDrinkById('drink1')!.isFavorite, false);
       expect(find.byIcon(Icons.favorite_border), findsOneWidget);
 
       // Mock toggleFavorite to properly toggle state
@@ -341,7 +341,7 @@ void main() {
       await tester.tap(find.byIcon(Icons.favorite_border));
       await tester.pumpAndSettle();
 
-      expect(drink.isFavorite, true);
+      expect(provider.getDrinkById('drink1')!.isFavorite, true);
       expect(find.byIcon(Icons.favorite), findsOneWidget);
     });
 
@@ -417,11 +417,12 @@ void main() {
     testWidgets('displays rating value when drink has rating', (
       WidgetTester tester,
     ) async {
-      when(mockDrinkRepository.getDrinks(any)).thenAnswer((_) async => [drink]);
+      final ratedDrink = drink.copyWith(rating: 4);
+      when(
+        mockDrinkRepository.getDrinks(any),
+      ).thenAnswer((_) async => [ratedDrink]);
       await provider.loadDrinks();
 
-      drink.rating = 4;
-
       await tester.pumpWidget(createTestWidget('drink1'));
       await tester.pumpAndSettle();
 
@@ -434,8 +435,6 @@ void main() {
       when(mockDrinkRepository.getDrinks(any)).thenAnswer((_) async => [drink]);
       await provider.loadDrinks();
 
-      drink.rating = null;
-
       await tester.pumpWidget(createTestWidget('drink1'));
       await tester.pumpAndSettle();
 
@@ -451,13 +450,16 @@ void main() {
       await tester.pumpWidget(createTestWidget('drink1'));
       await tester.pumpAndSettle();
 
-      expect(drink.rating, null);
+      expect(provider.getDrinkById('drink1')!.rating, null);
 
       // Simulate rating change through provider
-      provider.setRating(drink, 5);
+      when(
+        mockDrinkRepository.setRating(any, any, any),
+      ).thenAnswer((_) async {});
+      await provider.setRating(provider.getDrinkById('drink1')!, 5);
       await tester.pumpAndSettle();
 
-      expect(drink.rating, 5);
+      expect(provider.getDrinkById('drink1')!.rating, 5);
       expect(find.text('5/5'), findsOneWidget);
     });
 

test/models_test.dart

@@ -905,32 +905,36 @@ void main() {
       expect(drink.rating, isNull);
     });
 
-    test('can set isFavorite', () {
+    test('copyWith changes isFavorite', () {
       final drink = Drink(
         product: testProduct,
         producer: testProducer,
         festivalId: 'cbf2025',
         isFavorite: true,
       );
 
+      final updated = drink.copyWith(isFavorite: false);
+      expect(updated.isFavorite, isFalse);
+      // Original is unchanged
       expect(drink.isFavorite, isTrue);
-
-      drink.isFavorite = false;
-      expect(drink.isFavorite, isFalse);
     });
 
-    test('can set rating', () {
+    test('copyWith changes rating', () {
       final drink = Drink(
         product: testProduct,
         producer: testProducer,
         festivalId: 'cbf2025',
         rating: 4,
       );
 
+      final updated = drink.copyWith(rating: 5);
+      expect(updated.rating, 5);
+      // Original is unchanged
       expect(drink.rating, 4);
 
-      drink.rating = 5;
-      expect(drink.rating, 5);
+      // Clearing the rating to null
+      final cleared = drink.copyWith(rating: null);
+      expect(cleared.rating, isNull);
     });
 
     group('getShareMessage', () {