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. |
 
@@ -316,13 +316,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

.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/services/drink_sort_service.dart

@@ -1,4 +1,5 @@
 import '../../models/models.dart';
+import '../../utils/string_comparison_helper.dart';
 import '../models/models.dart' as domain;
 
 /// Service for sorting drinks based on different criteria
@@ -8,15 +9,28 @@ import '../models/models.dart' as domain;
 class DrinkSortService {
   /// Sort drinks based on the given sort option
   ///
-  /// Returns a new sorted list without modifying the original
+  /// Returns a new sorted list without modifying the original.
+  ///
+  /// Text sorts are case-insensitive (via
+  /// [StringComparisonHelper.compareCaseInsensitive]), matching how the style
+  /// facet and the My Festival list already order themselves. A raw
+  /// [String.compareTo] sorts every capitalised value ahead of every lowercase
+  /// one, which put a brewery like `d'Achouffe` at the bottom of the list
+  /// instead of alphabetically among the Cs and Ds.
   List<Drink> sortDrinks(List<Drink> drinks, domain.DrinkSort sortBy) {
     final sorted = List<Drink>.from(drinks);
     switch (sortBy) {
       case domain.DrinkSort.nameAsc:
-        sorted.sort((a, b) => a.name.compareTo(b.name));
+        sorted.sort(
+          (a, b) =>
+              StringComparisonHelper.compareCaseInsensitive(a.name, b.name),
+        );
         break;
       case domain.DrinkSort.nameDesc:
-        sorted.sort((a, b) => b.name.compareTo(a.name));
+        sorted.sort(
+          (a, b) =>
+              StringComparisonHelper.compareCaseInsensitive(b.name, a.name),
+        );
         break;
       case domain.DrinkSort.abvHigh:
         sorted.sort((a, b) => b.abv.compareTo(a.abv));
@@ -25,10 +39,20 @@ class DrinkSortService {
         sorted.sort((a, b) => a.abv.compareTo(b.abv));
         break;
       case domain.DrinkSort.brewery:
-        sorted.sort((a, b) => a.breweryName.compareTo(b.breweryName));
+        sorted.sort(
+          (a, b) => StringComparisonHelper.compareCaseInsensitive(
+            a.breweryName,
+            b.breweryName,
+          ),
+        );
         break;
       case domain.DrinkSort.style:
-        sorted.sort((a, b) => (a.style ?? '').compareTo(b.style ?? ''));
+        sorted.sort(
+          (a, b) => StringComparisonHelper.compareCaseInsensitive(
+            a.style ?? '',
+            b.style ?? '',
+          ),
+        );
         break;
     }
     return sorted;

lib/main.dart

@@ -135,7 +135,7 @@ class _ProviderInitializerState extends State<ProviderInitializer>
 
     // When app resumes to foreground, refresh data if stale
     if (state == AppLifecycleState.resumed) {
-      context.read<BeerProvider>().refreshIfStale();
+      unawaited(context.read<BeerProvider>().refreshIfStale());
     }
   }
 
@@ -144,13 +144,17 @@ class _ProviderInitializerState extends State<ProviderInitializer>
     super.didChangeDependencies();
     if (!_initialized) {
       _initialized = true;
-      // Initialize and load drinks for all routes
+      // Initialize and load drinks for all routes. initialize() never throws
+      // (a startup failure surfaces as provider.error), so the redirect below
+      // always runs and the app never strands on the loading screen.
       final provider = context.read<BeerProvider>();
-      provider.initialize().then((_) {
-        provider.loadDrinks();
-        // After initialization, trigger redirects that were deferred
-        _handlePostInitRedirect();
-      });
+      unawaited(
+        provider.initialize().then((_) {
+          unawaited(provider.loadDrinks());
+          // After initialization, trigger redirects that were deferred
+          _handlePostInitRedirect();
+        }),
+      );
     }
   }
 
@@ -236,10 +240,12 @@ class _ProviderInitializerState extends State<ProviderInitializer>
         // coverage:ignore-start
         // In production, log to crashlytics
         final provider = context.read<BeerProvider>();
-        provider.analyticsService.logError(
-          e,
-          stackTrace,
-          reason: 'Post-initialization redirect failed',
+        unawaited(
+          provider.analyticsService.logError(
+            e,
+            stackTrace,
+            reason: 'Post-initialization redirect failed',
+          ),
         );
         // coverage:ignore-end
       }