fix: correctness fixes from a codebase review

.claude/skills/architecture-contract/SKILL.md

@@ -281,7 +281,7 @@ run on every launch.
 | Personal state (favourites/ratings/tastings) is catalogue-independent (#390) | `DrinkRepository.getPersonalEntries` doc (`drink_repository.dart:56-65`): "the caller can enumerate a user's favourites, ratings, and tasting history purely from the personal-data store, before (or without) the drink catalogue being fetched." Fixed the favourites-flash bug family (#310/#397) as a side effect, because the My Festival list stopped being `_allDrinks.where(...)` (whichever festival happened to be loaded) and became its own festival-scoped query. |
 | Stale-while-revalidate (SWR) with two independent per-type caches | `cache_service.dart:8-19` class doc: render last-good data instantly, refresh in background, keep cache on failure. Per-*type* (not per-festival) caching specifically so a flaky `cider.json` fetch can't wipe out a good `beer.json` cache — see invariant 11. |
 | The `/` route must have a `builder`, not just a `redirect` | `router.dart:76-83` comment, citing issue #386: a redirect-only route that stays put (because the provider hasn't initialized yet) leaves go_router with an empty `pages` list and no `onGenerateRoute`, which crashes with "Null check operator used on a null value" in **release** builds only. The minimal `CircularProgressIndicator` builder at `router.dart:84-85` is the fix; removing it reintroduces a release-only crash invisible in debug/tests. |
-| `navigateToRoute()` branches `context.go()` (web) vs `context.push()` (mobile) | `navigation_helpers.dart:228-240`: `push` from inside a `ShellRoute` doesn't update the browser URL bar on web, but `push` is preferred on mobile to preserve the native back-stack. Always use this helper for drill-down navigation (drink detail, brewery) rather than calling `context.go`/`context.push` directly — see AGENTS.md's Navigation pattern. |
+| `navigateToRoute()` pushes on every platform (no web/mobile branch since #470) | `navigation_helpers.dart:237-239` is now just `context.push(path)`. It used to branch to `context.go()` on web because `push` from inside a `ShellRoute` didn't update the browser URL bar; enabling `GoRouter.optionURLReflectsImperativeAPIs` (`router.dart`) fixed that, and `go` was disposing the calling screen and losing its scroll position (#470, PR #478). The one-line helper is kept deliberately: it is the only place that rationale is recorded, and the single seam if that flag ever has to come back off. Always use it for drill-down navigation (drink detail, brewery) rather than calling `context.go`/`context.push` directly. |
 | Analytics only fires in production | `AnalyticsService._isAnalyticsEnabled = isProduction()` (`analytics_service.dart:21`); `EnvironmentService.isProduction()`/`isProductionHost()` (`environment_service.dart`). Fixed issue #269: unknown hostnames used to default to "production", polluting real analytics with staging/preview traffic; now unknown → NOT production (under-count is the safe failure direction). `logError` is the one exception — it runs in **every** environment so Crashlytics still sees staging crashes. |
 | `DefaultFestivals` hard-coded fallback | `models/festival.dart:262+` — four literal `Festival` objects (`cbf2026` active, `cbf2025`, `cbfw2025`, `cbf2024`) used only when both the network *and* the festival cache are unavailable (`FestivalController.currentFestival` getter, `festival_controller.dart:41-47`, and `BeerProvider.loadDrinks`, `beer_provider.dart:391-398`). This is a last-resort constant, not a data source to keep in sync with `data/festivals.json` — do not add new festivals here expecting them to appear in the switcher; that's the registry's job. |
 
@@ -303,11 +303,13 @@ found" — they're tracked.
   (`docs/todos.md:57-68`).
 - **No way to navigate back from the `/about` deep link** — archived todo
   H6 (`docs/todos.md:136-147`).
-- **URL fragments are lost during the post-init redirect.** Explicitly
-  marked with a `TODO` in `test/router_test.dart:666`: "Fix this by
-  preserving `currentUri.fragment` in redirect URL construction." The test
-  at lines 655-664 documents the current (lossy) behaviour as a known
-  limitation, not a passing spec for correct behaviour.
+- **URL fragments in the *post-init* redirect** (`main.dart`'s
+  `_handlePostInitRedirect`) are still dropped — it rebuilds the path from
+  `segments` + query only. The router's own invalid-festival redirect no
+  longer loses them: `_redirectToCurrentFestival` (`router.dart`) carries
+  query and fragment across verbatim, and `test/router_test.dart`'s "URL
+  fragments survive the invalid-festival redirect" pins that. The old
+  lossy-behaviour test and its TODO are gone.
 - **Flutter web renders to `<canvas>`**, so Playwright (the only E2E tool
   in use, per ADR 0005) can never assert on rendered UI content — a route
   can return HTTP 200 and still be showing an error state underneath.
@@ -316,13 +318,13 @@ found" — they're tracked.
 - **AGENTS.md architecture doc drift** — see the callout at the top of this
   file. `FavoritesService`/`RatingsService`/`TastingLogService` are gone;
   `UserDataStore` is reality.
-- **The class is called `FavoritesScreen` but the file is
-  `lib/screens/my_festival_screen.dart`.** PR #448 renamed the underlying
+- **The screen class is `MyFestivalScreen` but its route is still
+  `/:festivalId/favorites`.** PR #448 renamed the underlying
   model (`FavoriteDrinkEntry` → `MyFestivalEntry`) and generalised
-  `favoriteDrinks` → `myFestivalEntries`, but the screen class name and its
-  URL path (`/:festivalId/favorites`) are unchanged **on purpose** — URLs
-  are a public contract (see skill `change-control`'s unwritten rule #1).
-  Don't be surprised the class name and file name disagree; don't rename
+  `favoriteDrinks` → `myFestivalEntries`, and the class has since been renamed
+  to match its file, but the URL path (`/:festivalId/favorites`) is unchanged
+  **on purpose** — URLs are a public contract (see skill `change-control`'s
+  unwritten rule #1). Don't be surprised the class and route disagree; don't rename
   the class without checking every import, and never rename the route.
 - **The `/v1alpha` catalogue API is contract-only** (issue #432/PR #433):
   proto + generated OpenAPI + a read-only worker endpoint exist, but there
@@ -440,8 +442,8 @@ grep "^version:" pubspec.yaml
 # Confirm known-weak-points are still open (re-check state, not just existence)
 gh issue view 432 --json state,title 2>/dev/null || echo "use mcp__github__issue_read method=get issue_number=432 instead"
 
-# Re-read the fragment-loss TODO to confirm it hasn't been fixed
-sed -n '650,667p' test/router_test.dart
+# Confirm the router redirect still preserves query + fragment
+grep -n "hasFragment\|hasQuery" lib/router.dart
 ```
 
 If any of these disagree with the text above, the code has moved on —

.claude/skills/my-festival-campaign/SKILL.md

@@ -112,7 +112,7 @@ Ground rules for the whole track:
 - **#412 is already largely landed.** `MyFestivalEntry`
   (`lib/models/my_festival_entry.dart`), `BeerProvider.myFestivalEntries` /
   `favoriteEntries`, and `lib/screens/my_festival_screen.dart` (which today
-  contains class `FavoritesScreen`) already exist. Don't re-create them; extend
+  contains class `MyFestivalScreen`) already exist. Don't re-create them; extend
   them. Note the issues say `FavoriteDrinkEntry` — the code already renamed it to
   `MyFestivalEntry`.
 
@@ -223,7 +223,7 @@ want-to-try / tasted badge` — Fixes #413.
 favourites body. Rename the nav tab.
 
 **Files:** `lib/screens/my_festival_screen.dart` (extend the existing
-`FavoritesScreen`), `lib/screens/screens.dart`, `lib/main.dart` (nav tab
+`MyFestivalScreen`), `lib/screens/screens.dart`, `lib/main.dart` (nav tab
 icon/label/semantics ~406–417), `lib/router.dart`,
 `test/screens/my_festival_screen_test.dart` (new).
 

.claude/skills/ui-and-accessibility/SKILL.md

@@ -52,7 +52,7 @@ that failure mode is expensive and it has already happened.
    skill `change-control` for the full unwritten-rules list. Concretely for UI
    work: the "My Festival" rename (issue #414) renames the **nav label and
    icon**, not the route. The route stays `/:festivalId/favorites`
-   (`lib/router.dart:111`, `FavoritesScreen` class in
+   (`lib/router.dart`, `MyFestivalScreen` class in
    `lib/screens/my_festival_screen.dart`) even though the tab text changes to
    "My Festival." A visual/branding rename is never a licence to touch
    `router.dart` path strings.
@@ -62,8 +62,7 @@ that failure mode is expensive and it has already happened.
    deliberately small shared-component set (the identity hero panels
    `DrinkHeroPanel`/`BreweryHeroPanel`/`StyleHeroPanel` with their shared
    `FactsStrip`/`FactCell`, `InfoChip`, `SectionHeader`,
-   `BottomActionBar`/`ActionButton`, `BreadcrumbBar`, `buildOverflowMenu`, the
-   filter sheets). A second widget that does 90% of
+   `buildOverflowMenu`, the filter sheets). A second widget that does 90% of
    what an existing one does is scope creep and a maintenance burden for a
    solo maintainer.
 
@@ -140,8 +139,8 @@ unless noted.
 | Divided strip of "key facts" (centred value over an uppercase label) inside a hero | `FactsStrip` + `FactCell` | `facts_strip.dart` | `FactsStrip` owns the top/bottom/left dividers; a `FactCell` becomes a navigation button when given `onTap` + `semanticLabel` |
 | Small metadata pill (style, dispense, bar location) | `InfoChip` | `info_chip.dart` | Optional `onTap` makes it a `Semantics(button: true)` link |
 | Section title with underline on a detail screen | `SectionHeader` | `section_header.dart` | `showSeparator` toggles the underline |
-| Sticky bottom row of actions (tasting log, rate, favourite, share) | `BottomActionBar` + `ActionButton` | `bottom_action_bar.dart` | `ActionButton.isActive` drives colour + `FontWeight`; `semanticLabel` overrides the visible label for screen readers |
-| Back-navigation header on a detail screen (drink/brewery/style) | `BreadcrumbBar` | `breadcrumb_bar.dart` | Only the `IconButton` gets `Semantics`, never the text row — see the "BAD" example in `docs/code/widget-standards.md`. 28px icon → 48×48 touch target. Text segments only become tappable/underlined when a callback is provided |
+| Sticky bottom row of actions (tasting log, rate, favourite, share) | `BottomActionBar` + `ActionButton` | `bottom_action_bar.dart` | **Currently unused** — no screen wires it up (the drink detail screen uses a FAB + `YourTakeCard` instead). Available, but check it still fits before adopting. `ActionButton.isActive` drives colour + `FontWeight`; `semanticLabel` overrides the visible label for screen readers |
+| Back-navigation header on a detail screen (drink/brewery/style) | `BreadcrumbBar` | `breadcrumb_bar.dart` | **Currently unused** — detail screens use `CollapsingDetailAppBar` + `buildHomeLeadingButton`. Only the `IconButton` gets `Semantics`, never the text row — see the "BAD" example in `docs/code/widget-standards.md`. 28px icon → 48×48 touch target. Text segments only become tappable/underlined when a callback is provided |
 | Three-dot menu for festival switch / settings / about | `buildOverflowMenu(context)` | `overflow_menu.dart` | A function, not a widget class — `docs/code/ui-components.md` documents where to include it (Drinks, My Festival screen) and where not to (detail screens, About, modals) |
 | Modal filter pickers (category, style, sort, visibility) | `showCategoryFilter` / `showStyleFilter` / `showSortOptions` / `showVisibilityFilter` | `drink_filter_sheets.dart` | All route through the private `_showSheet` helper (`isScrollControlled: true`) and share `_SheetHandle` — add a new filter type by adding a sheet class + show-function here, not a bespoke `showModalBottomSheet` call elsewhere |
 | Star rating display or picker | `StarRating` | `star_rating.dart` | `isEditable` toggles read-only vs tap-to-rate; semantic `value` is always `'$rating out of 5 stars'` |
@@ -276,7 +275,7 @@ citable bug. Breaking one silently reintroduces a fixed defect.
 
 ### 1. Festival-flash guard (REQUIRED in any festival-scoped screen)
 
-`lib/screens/my_festival_screen.dart:15` (inside `FavoritesScreen.build`):
+`lib/screens/my_festival_screen.dart` (first thing in `MyFestivalScreen.build`):
 ```dart
 final provider = context.watch<BeerProvider>();
 if (provider.currentFestival.id != festivalId) {
@@ -314,10 +313,11 @@ invent a fifth loading state or collapse two of these into one.
 
 ### 3. `navigateToRoute` + typed path builders
 
-`lib/utils/navigation_helpers.dart:234` — `navigateToRoute(context, path)`
-picks `context.go(path)` on web and `context.push(path)` on mobile, because
-`push` from inside a `ShellRoute` doesn't update the browser URL bar on web,
-while `push` is preferred on mobile to preserve the native back stack. Use it
+`lib/utils/navigation_helpers.dart:237` — `navigateToRoute(context, path)`
+calls `context.push(path)` on every platform. It used to branch to
+`context.go()` on web (the URL bar didn't follow a `push` from inside a
+`ShellRoute`); `GoRouter.optionURLReflectsImperativeAPIs` fixed that in #470,
+and `go` was disposing the calling screen and losing its scroll. Use it
 for any drill-down navigation to content (drink detail, brewery, style). For
 root/tab navigation that replaces the stack (bottom nav taps, "go home"), call
 `context.go()` directly instead — that's a deliberate exception, not an

AGENTS.md

@@ -206,7 +206,7 @@ Container(
 
 **Provider reads** — `context.watch<BeerProvider>()` in `build()` only (subscribes to rebuilds). `context.read<BeerProvider>()` in callbacks, `initState`, and post-frame callbacks (one-shot, no rebuild subscription). Analytics calls in `initState` must be deferred via `WidgetsBinding.instance.addPostFrameCallback()`.
 
-**Navigation** — for drill-down navigation to content (drink detail, brewery), use `navigateToRoute()` from `lib/utils/navigation_helpers.dart`; it selects `context.go()` (web) or `context.push()` (mobile) automatically. For root/tab navigation that replaces the route stack (bottom nav, home button), use `context.go()` directly. Build URL paths with the typed helpers (`buildFestivalPath()`, `buildDrinkDetailPath()`, etc.) — never interpolate raw strings.
+**Navigation** — for drill-down navigation to content (drink detail, brewery), use `navigateToRoute()` from `lib/utils/navigation_helpers.dart`; it pushes the route on every platform, so the calling screen keeps its scroll position (#470). For root/tab navigation that replaces the route stack (bottom nav, home button), use `context.go()` directly. Build URL paths with the typed helpers (`buildFestivalPath()`, `buildDrinkDetailPath()`, etc.) — never interpolate raw strings.
 
 **Loading/error states** — four mutually exclusive signals on `BeerProvider`:
 

lib/domain/controllers/drink_filter_controller.dart

@@ -62,7 +62,7 @@ class DrinkFilterController {
 
   /// Unique styles in the source drinks, narrowed to the selected category when
   /// one is active, sorted case-insensitively (via
-  /// [StringComparisonHelper.compareLocaleAware]) so styles order in a stable,
+  /// [StringComparisonHelper.compareCaseInsensitive]) so styles order in a stable,
   /// human-friendly way regardless of capitalisation. Presentation consumes
   /// this directly — no sorting in the UI.
   List<String> get availableStyles {
@@ -71,7 +71,7 @@ class DrinkFilterController {
         .map((d) => d.style!)
         .toSet()
         .toList()
-      ..sort(StringComparisonHelper.compareLocaleAware);
+      ..sort(StringComparisonHelper.compareCaseInsensitive);
   }
 
   /// Drink count per category across the full source.

lib/domain/repositories/api_drink_repository.dart

@@ -20,6 +20,10 @@ class ApiDrinkRepository implements DrinkRepository {
 
   static const Uuid _uuid = Uuid();
 
+  /// Availability phrases already reported to Crashlytics this session, so a
+  /// novel `status_text` is logged once rather than on every refresh.
+  final Set<String> _reportedUnknownStatuses = {};
+
   ApiDrinkRepository({
     required BeerApiService apiService,
     required UserDataStore userDataStore,
@@ -85,12 +89,19 @@ class ApiDrinkRepository implements DrinkRepository {
 
     _applyUserState(update.drinks, festival.id);
 
+    // Report only phrases this session hasn't already reported. getDrinks runs
+    // on every cold start, festival switch, staleness refresh and pull-to-
+    // refresh, so without this a single new phrase coined by the organisers
+    // would log an error from every user roughly hourly for the whole run of
+    // the festival — drowning real crashes in the Crashlytics dashboard.
     final unknownStatuses = update.drinks
         .where((d) => d.availabilityStatus == AvailabilityStatus.unknown)
         .map((d) => d.statusText)
         .whereType<String>()
-        .toSet();
+        .toSet()
+        .difference(_reportedUnknownStatuses);
     if (unknownStatuses.isNotEmpty) {
+      _reportedUnknownStatuses.addAll(unknownStatuses);
       final sample = unknownStatuses.take(5).join(', ');
       final count = unknownStatuses.length;
       unawaited(

lib/domain/services/drink_filter_service.dart

@@ -128,8 +128,14 @@ class DrinkFilterService {
   /// 5. Allergen exclusions
   /// 6. Search filter
   ///
-  /// Each filter is only applied if its criteria is active.
-  /// Uses Iterable chaining to avoid intermediate list allocations.
+  /// Each filter is only applied if its criteria is active (each `filterByX`
+  /// short-circuits on an inactive criterion).
+  ///
+  /// Composed from the single-purpose filters above rather than re-testing each
+  /// predicate inline: there is exactly one copy of every rule, so a fix to
+  /// `filterByAvailability` (say) cannot pass its own unit test while leaving
+  /// the list the app actually renders unchanged. Still lazy — the `Iterable`
+  /// chain materialises once at the end.
   List<Drink> filterDrinks(
     List<Drink> drinks, {
     String? category,
@@ -139,43 +145,27 @@ class DrinkFilterService {
     Set<String> excludedAllergens = const {},
     String searchQuery = '',
   }) {
-    Iterable<Drink> result = drinks;
-
-    if (category != null) {
-      result = result.where((d) => d.category == category);
-    }
-
-    if (styles != null && styles.isNotEmpty) {
-      result = result.where((d) => d.style != null && styles.contains(d.style));
-    }
-
-    if (favoritesOnly) {
-      result = result.where((d) => d.isFavorite);
-    }
-
-    if (visibilityFilters.contains(DrinkVisibilityFilter.availableOnly)) {
-      result = result.where(
-        (d) => d.availabilityStatus != AvailabilityStatus.out,
-      );
-    }
-    if (visibilityFilters.contains(DrinkVisibilityFilter.notTasted)) {
-      result = result.where((d) => !d.isTasted);
-    }
-    if (visibilityFilters.contains(DrinkVisibilityFilter.veganOnly)) {
-      result = result.where((d) => d.isVegan == true);
-    }
-
-    if (excludedAllergens.isNotEmpty) {
-      result = result.where(
-        (d) => excludedAllergens.every((a) => (d.allergens[a] ?? 0) == 0),
-      );
-    }
-
-    if (searchQuery.isNotEmpty) {
-      final lowerQuery = searchQuery.toLowerCase();
-      result = result.where((d) => _matchesSearch(d, lowerQuery));
-    }
-
+    Iterable<Drink> result = filterByCategory(drinks, category);
+    result = filterByStyles(result, styles ?? const {});
+    result = filterByFavorites(result, favoritesOnly: favoritesOnly);
+    result = filterByAvailability(
+      result,
+      hideUnavailable: visibilityFilters.contains(
+        DrinkVisibilityFilter.availableOnly,
+      ),
+    );
+    result = filterByNotTasted(
+      result,
+      notTastedOnly: visibilityFilters.contains(
+        DrinkVisibilityFilter.notTasted,
+      ),
+    );
+    result = filterByVegan(
+      result,
+      veganOnly: visibilityFilters.contains(DrinkVisibilityFilter.veganOnly),
+    );
+    result = filterByExcludedAllergens(result, excludedAllergens);
+    result = filterBySearch(result, searchQuery);
     return result.toList();
   }
 }