diff --git a/.claude/skills/architecture-contract/SKILL.md b/.claude/skills/architecture-contract/SKILL.md index 85ab327e..8fe1ba6f 100644 --- a/.claude/skills/architecture-contract/SKILL.md +++ b/.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 ``**, 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 — diff --git a/.claude/skills/my-festival-campaign/SKILL.md b/.claude/skills/my-festival-campaign/SKILL.md index 9454e8a1..b23fd06a 100644 --- a/.claude/skills/my-festival-campaign/SKILL.md +++ b/.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). diff --git a/.claude/skills/ui-and-accessibility/SKILL.md b/.claude/skills/ui-and-accessibility/SKILL.md index 7912d287..1cc638ac 100644 --- a/.claude/skills/ui-and-accessibility/SKILL.md +++ b/.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(); 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 diff --git a/AGENTS.md b/AGENTS.md index 9bdf348f..bb227b19 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -206,7 +206,7 @@ Container( **Provider reads** — `context.watch()` in `build()` only (subscribes to rebuilds). `context.read()` 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`: diff --git a/lib/domain/controllers/drink_filter_controller.dart b/lib/domain/controllers/drink_filter_controller.dart index ee7d0af7..2a9d7677 100644 --- a/lib/domain/controllers/drink_filter_controller.dart +++ b/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 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. diff --git a/lib/domain/repositories/api_drink_repository.dart b/lib/domain/repositories/api_drink_repository.dart index d9d8a179..2c4eaf2f 100644 --- a/lib/domain/repositories/api_drink_repository.dart +++ b/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 _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() - .toSet(); + .toSet() + .difference(_reportedUnknownStatuses); if (unknownStatuses.isNotEmpty) { + _reportedUnknownStatuses.addAll(unknownStatuses); final sample = unknownStatuses.take(5).join(', '); final count = unknownStatuses.length; unawaited( diff --git a/lib/domain/services/drink_filter_service.dart b/lib/domain/services/drink_filter_service.dart index 94d2a67e..21f143da 100644 --- a/lib/domain/services/drink_filter_service.dart +++ b/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 filterDrinks( List drinks, { String? category, @@ -139,43 +145,27 @@ class DrinkFilterService { Set excludedAllergens = const {}, String searchQuery = '', }) { - Iterable 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 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(); } } diff --git a/lib/domain/services/drink_sort_service.dart b/lib/domain/services/drink_sort_service.dart index a090240d..6b395b93 100644 --- a/lib/domain/services/drink_sort_service.dart +++ b/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 sortDrinks(List drinks, domain.DrinkSort sortBy) { final sorted = List.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; diff --git a/lib/main.dart b/lib/main.dart index c8895119..9c0a2b7d 100644 --- a/lib/main.dart +++ b/lib/main.dart @@ -135,7 +135,7 @@ class _ProviderInitializerState extends State // When app resumes to foreground, refresh data if stale if (state == AppLifecycleState.resumed) { - context.read().refreshIfStale(); + unawaited(context.read().refreshIfStale()); } } @@ -144,13 +144,17 @@ class _ProviderInitializerState extends State 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(); - 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 // coverage:ignore-start // In production, log to crashlytics final provider = context.read(); - provider.analyticsService.logError( - e, - stackTrace, - reason: 'Post-initialization redirect failed', + unawaited( + provider.analyticsService.logError( + e, + stackTrace, + reason: 'Post-initialization redirect failed', + ), ); // coverage:ignore-end } diff --git a/lib/providers/beer_provider.dart b/lib/providers/beer_provider.dart index f29f286a..57fd3879 100644 --- a/lib/providers/beer_provider.dart +++ b/lib/providers/beer_provider.dart @@ -203,7 +203,7 @@ class BeerProvider extends ChangeNotifier { // Unloaded placeholders (drink == null) sort after all named entries. if (a.drink == null && b.drink != null) return 1; if (a.drink != null && b.drink == null) return -1; - final byName = StringComparisonHelper.compareLocaleAware( + final byName = StringComparisonHelper.compareCaseInsensitive( a.drink?.name ?? a.drinkId, b.drink?.name ?? b.drinkId, ); @@ -249,8 +249,43 @@ class BeerProvider extends ChangeNotifier { /// Check if festivals data is stale and should be refreshed bool get isFestivalsDataStale => _festivalController.isFestivalsDataStale; - /// Initialize with SharedPreferences and load festivals + /// Initialize with SharedPreferences and load festivals. + /// + /// Never throws. [_restoreState] awaits SharedPreferences, two one-time + /// migrations, the festival cache and the saved selection — any of which can + /// fail on a corrupt store or a bad platform channel. If that failure escaped, + /// [_isInitialized] would never be set, the router's `/` redirect would keep + /// returning null, and the app would sit on the startup spinner forever with + /// no way out. Instead the failure is logged, surfaced as [error] so the + /// drinks screen offers a Retry, and startup completes. Future initialize() async { + var hadCachedFestivals = false; + try { + hadCachedFestivals = await _restoreState(); + } catch (e, stackTrace) { + _error = _getUserFriendlyErrorMessage(e); + unawaited( + _analyticsService.logError( + e, + stackTrace, + reason: 'Provider initialization failed', + ), + ); + } finally { + _isInitialized = true; + notifyListeners(); + } + + // When cached festivals were shown, refresh the registry in the background + // so drinks loading is never blocked on the network. + if (hadCachedFestivals) { + unawaited(loadFestivals()); + } + } + + /// Restores persisted state at startup. Returns whether cached festivals were + /// used (so the caller knows to kick a background registry refresh). + Future _restoreState() async { final prefs = await SharedPreferences.getInstance(); // Create repositories if not provided. These blocks run only in production @@ -330,18 +365,22 @@ class BeerProvider extends ChangeNotifier { ); } - _isInitialized = true; - notifyListeners(); - - // When cached festivals were shown, refresh the registry in the background - // so drinks loading is never blocked on the network. - if (cachedFestivals != null) { - unawaited(loadFestivals()); - } + return cachedFestivals != null; } /// Load festivals from the API Future loadFestivals() async { + if (_festivalRepository == null) { + // initialize() failed before the repositories were built. The catch below + // would already convert the null dereference into a festivalsError, but + // relying on a TypeError for control flow hides the intent and yields a + // generic message; say what actually happened instead. + _festivalsError = 'Could not load festivals. Please try again.'; + _isFestivalsLoading = false; + _festivalController.recordAttempt(); + notifyListeners(); + return; + } _isFestivalsLoading = true; _festivalsError = null; notifyListeners(); @@ -386,6 +425,17 @@ class BeerProvider extends ChangeNotifier { /// background. A failed refresh keeps any cached data on screen rather than /// blanking to an error. Future loadDrinks() async { + if (_drinkRepository == null) { + // initialize() failed before the repositories were built (e.g. the + // preferences store was unavailable). Surface the failure rather than + // dereferencing a null repository — keep initialize()'s more specific + // message if it set one. + _error ??= 'Something went wrong. Please try again.'; + _isLoading = false; + _isRefreshing = false; + notifyListeners(); + return; + } if (!_festivalController.hasFestivals) { // Wait for festivals to be loaded first await loadFestivals(); diff --git a/lib/router.dart b/lib/router.dart index 63230bd7..306f3c8b 100644 --- a/lib/router.dart +++ b/lib/router.dart @@ -1,41 +1,65 @@ +import 'dart:async'; + import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; import 'package:go_router/go_router.dart'; import 'package:provider/provider.dart'; import 'providers/beer_provider.dart'; import 'screens/screens.dart'; -import 'utils/navigation_helpers.dart'; import 'main.dart'; /// Global routes that exist outside festival scope /// IMPORTANT: Keep in sync with _handlePostInitRedirect in main.dart const List globalRoutes = ['/about']; -/// Shared redirect logic for festival-scoped routes. +/// Rebuilds [state]'s location with the leading festival segment replaced by +/// [currentFestivalId], preserving everything else about the URL. +/// +/// Every path segment is re-encoded on the way out because go_router hands +/// back **decoded** path parameters. Interpolating those decoded values +/// straight into a path (as each route used to do for itself) silently loses +/// the encoding: a `/` in a style name splits into two segments so the route +/// stops matching, a `?` starts a query, a `#` starts a fragment. /// -/// Returns null when uninitialized (loading screen is shown) or when the -/// festival ID is invalid without a custom handler. When the URL festival -/// differs from the provider's current festival, schedules a switch via -/// [WidgetsBinding.addPostFrameCallback] so the router can complete -/// navigation first. +/// The query string and fragment are carried across verbatim — `Uri.query` and +/// `Uri.fragment` both return the raw (still-encoded) form, so appending them +/// round-trips exactly and re-encoding would double-escape. The query was +/// previously preserved only on `/:festivalId` and dropped by the five nested +/// routes; the fragment was dropped by all six. +String _redirectToCurrentFestival( + GoRouterState state, + String currentFestivalId, +) { + final uri = state.uri; + final rest = uri.pathSegments.skip(1).map(Uri.encodeComponent).join('/'); + final buffer = StringBuffer('/$currentFestivalId'); + if (rest.isNotEmpty) buffer.write('/$rest'); + if (uri.hasQuery) buffer.write('?${uri.query}'); + if (uri.hasFragment) buffer.write('#${uri.fragment}'); + return buffer.toString(); +} + +/// Shared redirect logic for festival-scoped routes. /// -/// [onInvalidFestival] — called with the current festival ID when the URL -/// festival is invalid; return a redirect path or null to stay put. -String? _festivalScopeRedirect( - BuildContext context, - GoRouterState state, { - String? Function(String currentFestivalId)? onInvalidFestival, -}) { +/// Returns null when uninitialized (loading screen is shown). When the URL +/// festival is invalid, redirects to the same location under the provider's +/// current festival (see [_redirectToCurrentFestival]) — every festival-scoped +/// route wants exactly that, so it is done here rather than by six per-route +/// callbacks that each rebuilt their own path. When the URL festival is valid +/// but differs from the provider's current festival, schedules a switch via +/// [WidgetsBinding.addPostFrameCallback] so the router can complete navigation +/// first. +String? _festivalScopeRedirect(BuildContext context, GoRouterState state) { final festivalId = state.pathParameters['festivalId']; final provider = context.read(); if (!provider.isInitialized) return null; if (!provider.isValidFestivalId(festivalId)) { - return onInvalidFestival?.call(provider.currentFestival.id); + return _redirectToCurrentFestival(state, provider.currentFestival.id); } final festival = provider.getFestivalById(festivalId!); if (festival != null && provider.currentFestival.id != festivalId) { WidgetsBinding.instance.addPostFrameCallback((_) { - provider.setFestival(festival, persist: false); + unawaited(provider.setFestival(festival, persist: false)); }); } return null; @@ -108,16 +132,7 @@ GoRouter _buildRouter() { routes: [ GoRoute( path: '/:festivalId', - redirect: (context, state) => _festivalScopeRedirect( - context, - state, - onInvalidFestival: (currentId) { - final queryString = state.uri.query.isNotEmpty - ? '?${state.uri.query}' - : ''; - return '/$currentId$queryString'; - }, - ), + redirect: _festivalScopeRedirect, pageBuilder: (context, state) { final festivalId = state.pathParameters['festivalId']!; return NoTransitionPage( @@ -127,11 +142,7 @@ GoRouter _buildRouter() { ), GoRoute( path: '/:festivalId/favorites', - redirect: (context, state) => _festivalScopeRedirect( - context, - state, - onInvalidFestival: (currentId) => '/$currentId/favorites', - ), + redirect: _festivalScopeRedirect, pageBuilder: (context, state) { final festivalId = state.pathParameters['festivalId']!; return NoTransitionPage( @@ -144,12 +155,7 @@ GoRouter _buildRouter() { // Detail routes - Provider initialized, but no navigation bar GoRoute( path: '/:festivalId/drink/:category/:id', - redirect: (context, state) => _festivalScopeRedirect( - context, - state, - onInvalidFestival: (currentId) => - '/$currentId/drink/${state.pathParameters['category']}/${state.pathParameters['id']}', - ), + redirect: _festivalScopeRedirect, builder: (context, state) { final festivalId = state.pathParameters['festivalId']!; final id = state.pathParameters['id']!; @@ -166,12 +172,7 @@ GoRouter _buildRouter() { ), GoRoute( path: '/:festivalId/brewery/:id', - redirect: (context, state) => _festivalScopeRedirect( - context, - state, - onInvalidFestival: (currentId) => - '/$currentId/brewery/${state.pathParameters['id']}', - ), + redirect: _festivalScopeRedirect, builder: (context, state) { final festivalId = state.pathParameters['festivalId']!; final id = state.pathParameters['id']!; @@ -180,28 +181,19 @@ GoRouter _buildRouter() { ), GoRoute( path: '/:festivalId/style/:name', - redirect: (context, state) => _festivalScopeRedirect( - context, - state, - onInvalidFestival: (currentId) => - '/$currentId/style/${state.pathParameters['name']}', - ), + redirect: _festivalScopeRedirect, builder: (context, state) { final festivalId = state.pathParameters['festivalId']!; final name = state.pathParameters['name']!; - return StyleScreen( - festivalId: festivalId, - style: safeDecodeComponent(name), - ); + // `name` is already decoded — go_router percent-decodes path + // parameters before the builder sees them. Decoding again would + // mangle a style whose name literally contains a percent escape. + return StyleScreen(festivalId: festivalId, style: name); }, ), GoRoute( path: '/:festivalId/info', - redirect: (context, state) => _festivalScopeRedirect( - context, - state, - onInvalidFestival: (currentId) => '/$currentId/info', - ), + redirect: _festivalScopeRedirect, builder: (context, state) { final festivalId = state.pathParameters['festivalId']!; return FestivalInfoScreen(festivalId: festivalId); diff --git a/lib/screens/about_screen.dart b/lib/screens/about_screen.dart index d1670f07..0e9ebf0f 100644 --- a/lib/screens/about_screen.dart +++ b/lib/screens/about_screen.dart @@ -461,87 +461,7 @@ class _AboutScreenState extends State { void _showThemeSelector(BuildContext context, BeerProvider provider) { showModalBottomSheet( context: context, - builder: (context) => _ThemeSelectorSheet(provider: provider), - ); - } -} - -/// Theme selector bottom sheet -class _ThemeSelectorSheet extends StatelessWidget { - final BeerProvider provider; - - const _ThemeSelectorSheet({required this.provider}); - - @override - Widget build(BuildContext context) { - final theme = Theme.of(context); - - return Container( - padding: const EdgeInsets.all(16), - child: Column( - mainAxisSize: MainAxisSize.min, - crossAxisAlignment: CrossAxisAlignment.start, - children: [ - Center( - child: Container( - width: 32, - height: 4, - decoration: BoxDecoration( - color: theme.colorScheme.onSurfaceVariant, - borderRadius: BorderRadius.circular(2), - ), - ), - ), - const SizedBox(height: 16), - Text('Theme', style: theme.textTheme.titleLarge), - const SizedBox(height: 16), - RadioGroup( - groupValue: provider.themeMode, - onChanged: (value) { - if (value != null) { - provider.setThemeMode(value); - Navigator.pop(context); - } - }, - child: Column( - mainAxisSize: MainAxisSize.min, - children: [ - ListTile( - leading: const Radio(value: ThemeMode.system), - title: const Text('System'), - subtitle: const Text('Follow device settings'), - trailing: const Icon(Icons.brightness_auto), - onTap: () { - provider.setThemeMode(ThemeMode.system); - Navigator.pop(context); - }, - ), - ListTile( - leading: const Radio(value: ThemeMode.light), - title: const Text('Light'), - subtitle: const Text('Always use light theme'), - trailing: const Icon(Icons.light_mode), - onTap: () { - provider.setThemeMode(ThemeMode.light); - Navigator.pop(context); - }, - ), - ListTile( - leading: const Radio(value: ThemeMode.dark), - title: const Text('Dark'), - subtitle: const Text('Always use dark theme'), - trailing: const Icon(Icons.dark_mode), - onTap: () { - provider.setThemeMode(ThemeMode.dark); - Navigator.pop(context); - }, - ), - ], - ), - ), - const SizedBox(height: 16), - ], - ), + builder: (context) => ThemeSelectorSheet(provider: provider), ); } } diff --git a/lib/screens/brewery_screen.dart b/lib/screens/brewery_screen.dart index 7b6c6e31..66ff5740 100644 --- a/lib/screens/brewery_screen.dart +++ b/lib/screens/brewery_screen.dart @@ -51,6 +51,14 @@ class _BreweryScreenState extends State { Widget build(BuildContext context) { final provider = context.watch(); + // Festival-flash guard: on a URL-driven festival change the provider + // switches in a post-frame callback, so without this the screen would + // resolve its content against the PREVIOUS festival's catalogue for a + // frame — showing the wrong entity or a spurious "not found" (#397). + if (provider.currentFestival.id != widget.festivalId) { + return buildLoadingScaffold(); + } + // Show loading state while drinks are being fetched if (provider.isLoading) { return buildLoadingScaffold(); diff --git a/lib/screens/drink_detail_screen.dart b/lib/screens/drink_detail_screen.dart index 7aaaabdf..892d5054 100644 --- a/lib/screens/drink_detail_screen.dart +++ b/lib/screens/drink_detail_screen.dart @@ -155,6 +155,14 @@ class _DrinkDetailScreenState extends State Widget build(BuildContext context) { final provider = context.watch(); + // Festival-flash guard: on a URL-driven festival change the provider + // switches in a post-frame callback, so without this the screen would + // resolve its content against the PREVIOUS festival's catalogue for a + // frame — showing the wrong entity or a spurious "not found" (#397). + if (provider.currentFestival.id != widget.festivalId) { + return buildLoadingScaffold(); + } + // Show loading state while drinks are being fetched if (provider.isLoading) { return buildLoadingScaffold(); diff --git a/lib/screens/drinks_screen.dart b/lib/screens/drinks_screen.dart index 13231443..785446f3 100644 --- a/lib/screens/drinks_screen.dart +++ b/lib/screens/drinks_screen.dart @@ -40,6 +40,15 @@ class _DrinksScreenState extends State { @override Widget build(BuildContext context) { final provider = context.watch(); + // Festival-flash guard: the router schedules setFestival in a post-frame + // callback, so a URL-driven festival change (cross-festival deep link on a + // warm app, browser back/forward, the post-init redirect in main.dart) + // would otherwise render one frame of the previous festival's name and + // drinks before the provider catches up (issue #397). Keep it first in + // build(), as in MyFestivalScreen. + if (provider.currentFestival.id != widget.festivalId) { + return buildLoadingScaffold(); + } return PageTitle( pageTitle: provider.currentFestival.name, @@ -285,7 +294,11 @@ class _DrinksScreenState extends State { child: Column( mainAxisAlignment: MainAxisAlignment.center, children: [ - const Icon(Icons.error_outline, size: 64, color: Colors.red), + Icon( + Icons.error_outline, + size: 64, + color: Theme.of(context).colorScheme.error, + ), const SizedBox(height: 16), Text( 'Error loading drinks', diff --git a/lib/screens/my_festival_screen.dart b/lib/screens/my_festival_screen.dart index 4589285e..17ead4a8 100644 --- a/lib/screens/my_festival_screen.dart +++ b/lib/screens/my_festival_screen.dart @@ -136,10 +136,10 @@ class _MyFestivalScreenState extends State { child: Column( mainAxisAlignment: MainAxisAlignment.center, children: [ - const Icon( + Icon( Icons.local_bar_outlined, size: 64, - color: Colors.grey, + color: theme.colorScheme.onSurfaceVariant, ), const SizedBox(height: 16), Text( @@ -168,7 +168,10 @@ class _MyFestivalScreenState extends State { return [ _buildSectionHeader(context, 'Want to Try', wantToTry.length), if (wantToTry.isEmpty) - _buildSectionEmptyHint('No drinks in your want-to-try list yet.') + _buildSectionEmptyHint( + context, + 'No drinks in your want-to-try list yet.', + ) else for (final entry in wantToTry) _buildWantToTryRow(context, festivalId, entry), @@ -185,6 +188,7 @@ class _MyFestivalScreenState extends State { return [ _buildSectionHeader(context, 'Tasted', 0), _buildSectionEmptyHint( + context, 'Nothing tasted yet — mark a drink as tasted to start your log.', ), ]; @@ -214,10 +218,16 @@ class _MyFestivalScreenState extends State { ); } - Widget _buildSectionEmptyHint(String message) { + Widget _buildSectionEmptyHint(BuildContext context, String message) { + // onSurfaceVariant rather than Colors.grey: the fixed grey is 2.7:1 against + // a light surface, below the 4.5:1 WCAG AA needs for body text (it only + // passes in dark mode). The theme colour adapts to both. return Padding( padding: const EdgeInsets.fromLTRB(16, 0, 16, 8), - child: Text(message, style: const TextStyle(color: Colors.grey)), + child: Text( + message, + style: TextStyle(color: Theme.of(context).colorScheme.onSurfaceVariant), + ), ); } diff --git a/lib/screens/style_screen.dart b/lib/screens/style_screen.dart index 914cc831..4e6c43c4 100644 --- a/lib/screens/style_screen.dart +++ b/lib/screens/style_screen.dart @@ -41,6 +41,14 @@ class _StyleScreenState extends State { Widget build(BuildContext context) { final provider = context.watch(); + // Festival-flash guard: on a URL-driven festival change the provider + // switches in a post-frame callback, so without this the screen would + // resolve its content against the PREVIOUS festival's catalogue for a + // frame — showing the wrong entity or a spurious "not found" (#397). + if (provider.currentFestival.id != widget.festivalId) { + return buildLoadingScaffold(); + } + // Show loading state while drinks are being fetched if (provider.isLoading) { return buildLoadingScaffold(); diff --git a/lib/utils/navigation_helpers.dart b/lib/utils/navigation_helpers.dart index b6bc3dc0..d2d15865 100644 --- a/lib/utils/navigation_helpers.dart +++ b/lib/utils/navigation_helpers.dart @@ -276,25 +276,3 @@ bool _isFestivalListLocation(GoRouter router, String festivalId) { return path == buildFestivalHome(festivalId) || path == buildFavoritesPath(festivalId); } - -/// Decodes a percent-encoded URI component, returning the raw value if it -/// contains an illegal percent-encoding sequence. -/// -/// [Uri.decodeComponent] throws an [ArgumentError] when a `%` is not followed -/// by two hex digits (e.g. a stray `%` in an old bookmark or shared link). -/// This wrapper catches that case so callers get a usable string instead of a -/// crash. -/// -/// Example: -/// ```dart -/// safeDecodeComponent('IPA%20American') // Returns: 'IPA American' -/// safeDecodeComponent('50%') // Returns: '50%' (malformed — fallback) -/// safeDecodeComponent('normal') // Returns: 'normal' -/// ``` -String safeDecodeComponent(String value) { - try { - return Uri.decodeComponent(value); - } on ArgumentError { - return value; - } -} diff --git a/lib/utils/string_comparison_helper.dart b/lib/utils/string_comparison_helper.dart index b9e277a5..98bcb423 100644 --- a/lib/utils/string_comparison_helper.dart +++ b/lib/utils/string_comparison_helper.dart @@ -1,34 +1,29 @@ -/// Helper class for locale-aware string comparisons +/// Helper class for case-insensitive string comparisons. /// -/// Provides methods to properly sort and compare strings containing -/// non-ASCII characters (e.g., "rosé", "café") in a human-friendly way. +/// Used to order user-facing lists (drink names, breweries, styles) so that +/// capitalisation doesn't decide the order. class StringComparisonHelper { // Private constructor to prevent instantiation StringComparisonHelper._(); - /// Locale-aware case-insensitive string comparison + /// Case-insensitive string comparison. /// - /// This ensures that strings with accented characters (é, ñ, ü, etc.) - /// are sorted in a reasonable alphabetical order. While not perfect for - /// all locales, this approach handles common European accented characters - /// properly for beer/wine/cider style names. + /// Lower-cases both operands and compares them with [String.compareTo], so + /// "IPA", "Ipa", and "ipa" are treated as equal. /// - /// The comparison is case-insensitive, so "IPA", "Ipa", and "ipa" are - /// treated as equal. - /// - /// Examples: - /// - "Café" comes right after "Cafe" - /// - "Rosé" comes right after "Rose" - /// - "IPA" and "ipa" are treated as equal + /// **This is not collation.** [String.compareTo] compares UTF-16 code units, + /// so any character outside ASCII sorts after every ASCII letter regardless + /// of what it looks like: "Rosé" sorts after "Rosz", not next to "Rose", and + /// a brewery like "Ārpus" sorts below "Zötler". Sorting accented text the way + /// a reader would expect needs a real locale-aware collator (or a + /// diacritic-folded sort key), which would change the visible order of the + /// style filter and the My Festival list and should be its own change. /// /// For sorting lists: /// ```dart - /// styles.sort(StringComparisonHelper.compareLocaleAware); + /// styles.sort(StringComparisonHelper.compareCaseInsensitive); /// ``` - static int compareLocaleAware(String a, String b) { - // Use case-insensitive comparison - // This handles accented characters reasonably well for European languages - // by comparing the lowercase versions + static int compareCaseInsensitive(String a, String b) { return a.toLowerCase().compareTo(b.toLowerCase()); } } diff --git a/lib/widgets/festival_menu_sheets.dart b/lib/widgets/festival_menu_sheets.dart index 5fbef8c9..6c52f885 100644 --- a/lib/widgets/festival_menu_sheets.dart +++ b/lib/widgets/festival_menu_sheets.dart @@ -59,6 +59,23 @@ class FestivalSelectorSheet extends StatelessWidget { @override Widget build(BuildContext context) { + // Unlike every other sheet in this file, two of this one's controls — + // Retry and Refresh — deliberately do NOT pop the sheet, so their result + // has to land while it is still open. A captured provider reference does + // not subscribe to anything, and a modal route is not rebuilt by its + // opener, so without this the buttons were dead: loadFestivals() ran and + // nothing on screen changed. + // + // ListenableBuilder rather than Consumer: the provider is injected through + // the constructor, so listening to that instance directly keeps the widget + // usable without an ancestor Provider. + return ListenableBuilder( + listenable: provider, + builder: (context, _) => _buildSheet(context), + ); + } + + Widget _buildSheet(BuildContext context) { final theme = Theme.of(context); // Use dynamically loaded festivals (sorted) final festivals = provider.sortedFestivals; diff --git a/test/domain/repositories/api_drink_repository_test.dart b/test/domain/repositories/api_drink_repository_test.dart index 77c686a4..fbacf7f3 100644 --- a/test/domain/repositories/api_drink_repository_test.dart +++ b/test/domain/repositories/api_drink_repository_test.dart @@ -240,6 +240,53 @@ void main() { ).called(1); }); + test('logs each unknown status text only once per session', () async { + // getDrinks runs on every cold start, festival switch, staleness + // refresh and pull-to-refresh. Without dedup a single new phrase from + // the organisers would log an error from every user roughly hourly for + // the whole festival, burying real crashes. + when(apiService.fetchDrinksByType(festival)).thenAnswer( + (_) async => ok([makeDrinkWithStatus('d1', 'Not yet available')]), + ); + + await repository.getDrinks(festival); + await repository.getDrinks(festival); + await repository.getDrinks(festival); + + verify( + analyticsService.logError( + any, + any, + reason: argThat(contains('Not yet available'), named: 'reason'), + ), + ).called(1); + }); + + test('logs a newly appearing unknown status text', () async { + when(apiService.fetchDrinksByType(festival)).thenAnswer( + (_) async => ok([makeDrinkWithStatus('d1', 'Not yet available')]), + ); + await repository.getDrinks(festival); + + // A second, different phrase must still be reported — dedup is + // per-phrase, not "report once then go quiet". + when(apiService.fetchDrinksByType(festival)).thenAnswer( + (_) async => ok([ + makeDrinkWithStatus('d1', 'Not yet available'), + makeDrinkWithStatus('d2', 'Cellar delay'), + ]), + ); + await repository.getDrinks(festival); + + verify( + analyticsService.logError( + any, + any, + reason: argThat(contains('Cellar delay'), named: 'reason'), + ), + ).called(1); + }); + test( 'does not log analytics when all status texts are known vocabulary', () async { diff --git a/test/domain/services/drink_sort_service_test.dart b/test/domain/services/drink_sort_service_test.dart index 3b4ae7de..bcef0f72 100644 --- a/test/domain/services/drink_sort_service_test.dart +++ b/test/domain/services/drink_sort_service_test.dart @@ -255,5 +255,97 @@ void main() { } }); }); + + group('case-insensitive ordering', () { + // A raw String.compareTo puts every capitalised name before every + // lowercase one, so a brewery like "d'Achouffe" (real, in the cbf2026 + // feed) lands at the bottom of the list instead of between "Cydefx" and + // "Daleside". The style facet already sorts case-insensitively via + // StringComparisonHelper, so the two lists disagreed. + Drink drinkFrom({ + required String id, + required String name, + required String breweryName, + String? style, + }) => Drink( + product: Product( + id: id, + name: name, + category: 'beer', + style: style, + dispense: 'cask', + abv: 5, + ), + producer: Producer( + id: 'producer-$id', + name: breweryName, + location: 'Cambridge', + products: const [], + ), + festivalId: 'cbf2026', + ); + + test('sorts breweries case-insensitively', () { + final drinks = [ + drinkFrom(id: 'a', name: 'A', breweryName: 'Zotler'), + drinkFrom(id: 'b', name: 'B', breweryName: "d'Achouffe"), + drinkFrom(id: 'c', name: 'C', breweryName: 'Cydefx'), + drinkFrom(id: 'd', name: 'D', breweryName: 'Daleside'), + ]; + + final result = service.sortDrinks(drinks, DrinkSort.brewery); + + expect( + result.map((d) => d.breweryName).toList(), + ['Cydefx', "d'Achouffe", 'Daleside', 'Zotler'], + reason: + "d'Achouffe belongs between Cydefx and Daleside, not after Zotler", + ); + }); + + test('sorts names case-insensitively in both directions', () { + final drinks = [ + drinkFrom(id: 'a', name: 'Zebra Stout', breweryName: 'X'), + drinkFrom(id: 'b', name: 'abbot Ale', breweryName: 'Y'), + drinkFrom(id: 'c', name: 'Bishop Bitter', breweryName: 'Z'), + ]; + + expect( + service + .sortDrinks(drinks, DrinkSort.nameAsc) + .map((d) => d.name) + .toList(), + ['abbot Ale', 'Bishop Bitter', 'Zebra Stout'], + ); + expect( + service + .sortDrinks(drinks, DrinkSort.nameDesc) + .map((d) => d.name) + .toList(), + ['Zebra Stout', 'Bishop Bitter', 'abbot Ale'], + ); + }); + + test('sorts styles case-insensitively, nulls first', () { + final drinks = [ + drinkFrom(id: 'a', name: 'A', breweryName: 'X', style: 'Stout'), + drinkFrom( + id: 'b', + name: 'B', + breweryName: 'Y', + style: 'american ipa', + ), + drinkFrom(id: 'c', name: 'C', breweryName: 'Z'), + ]; + + expect( + service + .sortDrinks(drinks, DrinkSort.style) + .map((d) => d.style) + .toList(), + [null, 'american ipa', 'Stout'], + ); + }); + }); }); } diff --git a/test/drinks_screen_refresh_status_test.dart b/test/drinks_screen_refresh_status_test.dart index 127c5ec4..3a3bf64d 100644 --- a/test/drinks_screen_refresh_status_test.dart +++ b/test/drinks_screen_refresh_status_test.dart @@ -82,6 +82,48 @@ void main() { ); } + testWidgets('shows the full error view with Retry when nothing loaded', ( + tester, + ) async { + // The fourth of the four loading/error signals: a blocking failure with + // no data at all. Distinct from the refresh notice below, which keeps + // cached drinks on screen. + final failingDrinkRepo = MockDrinkRepository(); + when( + failingDrinkRepo.getDrinks(any), + ).thenThrow(BeerApiException('boom', 500)); + when(failingDrinkRepo.getCachedDrinks(any)).thenAnswer((_) async => null); + + final errorProvider = BeerProvider( + drinkRepository: failingDrinkRepo, + festivalRepository: mockFestivalRepository, + analyticsService: mockAnalyticsService, + ); + addTearDown(errorProvider.dispose); + await errorProvider.initialize(); + await errorProvider.loadDrinks(); + + expect(errorProvider.error, isNotNull); + expect(errorProvider.allDrinks, isEmpty); + + await tester.pumpWidget( + ChangeNotifierProvider.value( + value: errorProvider, + child: const MaterialApp(home: DrinksScreen(festivalId: 'cbf2025')), + ), + ); + await tester.pumpAndSettle(); + + expect(find.text('Error loading drinks'), findsOneWidget); + expect( + find.text('Server error. Please try again later.'), + findsOneWidget, + ); + expect(find.widgetWithText(ElevatedButton, 'Retry'), findsOneWidget); + // Not the cached-data banner — the two are mutually exclusive. + expect(find.textContaining('saved data'), findsNothing); + }); + testWidgets('shows a dismissible notice when a refresh fails with cache', ( tester, ) async { diff --git a/test/provider_test.dart b/test/provider_test.dart index 2798beba..35635fbd 100644 --- a/test/provider_test.dart +++ b/test/provider_test.dart @@ -43,6 +43,112 @@ void main() { ).thenAnswer((_) async => null); }); + group('initialize failure', () { + // initialize() awaits SharedPreferences, two migrations, the festival + // cache and the saved selection. If any of them throws and the failure + // escapes, _isInitialized is never set, so the router's '/' redirect + // keeps returning null and the app sits on the startup spinner forever. + test( + 'still completes initialization when a repository call throws', + () async { + when( + mockFestivalRepository.getCachedFestivals(), + ).thenThrow(Exception('corrupt cache')); + + final provider = BeerProvider( + drinkRepository: mockDrinkRepository, + festivalRepository: mockFestivalRepository, + analyticsService: mockAnalyticsService, + ); + addTearDown(provider.dispose); + + await expectLater(provider.initialize(), completes); + + expect( + provider.isInitialized, + isTrue, + reason: 'Startup must finish so the router can leave the spinner', + ); + expect( + provider.error, + isNotNull, + reason: 'The user needs a visible failure with a Retry, not a hang', + ); + verify( + mockAnalyticsService.logError(any, any, reason: anyNamed('reason')), + ).called(1); + }, + ); + + test( + 'loadDrinks without a repository reports an error, not a crash', + () async { + // The state left behind when initialize() fails before the + // repositories are built (e.g. SharedPreferences unavailable): + // loadDrinks must not dereference a null repository. + final provider = BeerProvider(analyticsService: mockAnalyticsService); + addTearDown(provider.dispose); + + await expectLater(provider.loadDrinks(), completes); + + expect(provider.error, isNotNull); + expect(provider.isLoading, isFalse); + expect(provider.isRefreshing, isFalse); + }, + ); + + test( + 'loadFestivals without a repository surfaces festivalsError', + () async { + final provider = BeerProvider(analyticsService: mockAnalyticsService); + addTearDown(provider.dispose); + + await expectLater(provider.loadFestivals(), completes); + + expect(provider.festivalsError, isNotNull); + expect(provider.isFestivalsLoading, isFalse); + expect(provider.hasFestivals, isFalse); + }, + ); + + test('refreshIfStale without repositories does not throw', () async { + // Called from didChangeAppLifecycleState on every resume, so a startup + // failure must not turn every foreground into an unhandled error. + final provider = BeerProvider(analyticsService: mockAnalyticsService); + addTearDown(provider.dispose); + + await expectLater(provider.refreshIfStale(), completes); + }); + + test('a later loadDrinks recovers from a failed initialize', () async { + when( + mockFestivalRepository.getCachedFestivals(), + ).thenThrow(Exception('corrupt cache')); + when(mockDrinkRepository.getDrinks(any)).thenAnswer((_) async => []); + + final provider = BeerProvider( + drinkRepository: mockDrinkRepository, + festivalRepository: mockFestivalRepository, + analyticsService: mockAnalyticsService, + ); + addTearDown(provider.dispose); + + await provider.initialize(); + expect(provider.error, isNotNull); + + // The startup failure must not be terminal — the Retry button on + // the error view calls loadDrinks, and that has to work. + await expectLater(provider.loadDrinks(), completes); + + expect( + provider.error, + isNull, + reason: 'A successful load clears the startup error', + ); + expect(provider.isLoading, isFalse); + }); + }); + group('loadDrinks error messages', () { test('shows user-friendly message for 404 error', () async { final provider = BeerProvider( diff --git a/test/router_test.dart b/test/router_test.dart index 68733ed3..56a7d2d5 100644 --- a/test/router_test.dart +++ b/test/router_test.dart @@ -632,10 +632,11 @@ void main() { }, ); // Edge cases and limitations - testWidgets('URL fragments are lost during redirect (KNOWN LIMITATION)', ( + testWidgets('URL fragments survive the invalid-festival redirect', ( tester, ) async { - // This documents the current limitation mentioned in lib/main.dart + // Previously a known limitation: the redirect rebuilt the path and + // dropped the fragment. _redirectToCurrentFestival now carries it over. await tester.pumpWidget( ChangeNotifierProvider.value( value: provider, @@ -643,7 +644,6 @@ void main() { ), ); - // Navigate to invalid festival with fragment appRouter.go('/invalid-fest#section'); await tester.pump(); await tester.pumpAndSettle(); @@ -652,7 +652,6 @@ void main() { appRouter.routerDelegate.currentConfiguration.uri.toString(), ); - // Currently fragments are lost during redirect expect( currentUri.pathSegments.first, testFestivalId, @@ -660,10 +659,9 @@ void main() { ); expect( currentUri.fragment, - isEmpty, - reason: 'Fragment is lost (KNOWN LIMITATION - see lib/main.dart)', + 'section', + reason: 'Fragment must survive the redirect', ); - // TODO: Fix this by preserving currentUri.fragment in redirect URL construction }); testWidgets('URL-encoded festival IDs are handled correctly', ( @@ -1087,6 +1085,150 @@ void main() { ); }, ); + + // go_router hands back *decoded* path parameters (match.dart's + // Uri.decodeComponent), so a redirect that rebuilds the path by string + // interpolation silently loses the encoding: a '/' reappears as a path + // separator, a '?' starts a query, a '#' starts a fragment. Every + // festival-scoped route rebuilds its path on an invalid festival id, so + // this has to hold for all of them. + testWidgets( + 'invalid-festival redirect preserves percent-encoded path parameters', + (tester) async { + await provider.initialize(); + + await tester.pumpWidget( + ChangeNotifierProvider.value( + value: provider, + child: MaterialApp.router(routerConfig: appRouter), + ), + ); + await tester.pumpAndSettle(); + + Uri currentUri() => appRouter.routerDelegate.currentConfiguration.uri; + + // A brewery id containing '?' must survive as one path segment. + appRouter.go('/$invalidFestivalId/brewery/what%3Fnow'); + await tester.pumpAndSettle(); + expect( + currentUri().pathSegments, + [testFestivalId, 'brewery', 'what?now'], + reason: 'A ? in a brewery id must not become a query string', + ); + expect(currentUri().hasQuery, isFalse); + + // A style containing '/' (e.g. "Porter/Stout", encoded by + // buildStylePath) must stay a single segment, not split the route. + appRouter.go('/$invalidFestivalId/style/porter%2Fstout'); + await tester.pumpAndSettle(); + expect( + currentUri().pathSegments, + [testFestivalId, 'style', 'porter/stout'], + reason: 'A / in a style name must not split into two segments', + ); + + // A '#' must not become a fragment. + appRouter.go('/$invalidFestivalId/brewery/hash%23tag'); + await tester.pumpAndSettle(); + expect(currentUri().pathSegments, [ + testFestivalId, + 'brewery', + 'hash#tag', + ]); + expect(currentUri().hasFragment, isFalse); + }, + ); + + // go_router decodes path parameters before handing them to the builder, so + // decoding again turns a style whose name literally contains a percent + // escape into a different string. + testWidgets('style route does not double-decode its path parameter', ( + tester, + ) async { + await provider.initialize(); + + await tester.pumpWidget( + ChangeNotifierProvider.value( + value: provider, + child: MaterialApp.router(routerConfig: appRouter), + ), + ); + await tester.pumpAndSettle(); + + // '%2520' decodes once to the literal text '%20'. + appRouter.go('/$testFestivalId/style/a%2520b'); + await tester.pumpAndSettle(); + + final screen = tester.widget(find.byType(StyleScreen)); + expect( + screen.style, + 'a%20b', + reason: 'Decoding a second time would yield "a b"', + ); + }); + + // Only the /:festivalId route used to preserve the query string; the five + // nested routes dropped it. Nothing pinned that difference, so it was + // drift rather than a decision. + testWidgets( + 'invalid-festival redirect preserves the query string on nested routes', + (tester) async { + await provider.initialize(); + + await tester.pumpWidget( + ChangeNotifierProvider.value( + value: provider, + child: MaterialApp.router(routerConfig: appRouter), + ), + ); + await tester.pumpAndSettle(); + + for (final path in [ + 'drink/beer/$testDrinkId', + 'brewery/$testBreweryId', + 'style/ipa', + 'info', + 'favorites', + ]) { + appRouter.go('/$invalidFestivalId/$path?utm=email&ref=friend'); + await tester.pumpAndSettle(); + + final uri = appRouter.routerDelegate.currentConfiguration.uri; + expect(uri.pathSegments.first, testFestivalId); + expect( + uri.queryParameters, + {'utm': 'email', 'ref': 'friend'}, + reason: 'Query params must survive the redirect on /$path', + ); + } + }, + ); + + testWidgets('invalid-festival redirect preserves the URL fragment', ( + tester, + ) async { + await provider.initialize(); + + await tester.pumpWidget( + ChangeNotifierProvider.value( + value: provider, + child: MaterialApp.router(routerConfig: appRouter), + ), + ); + await tester.pumpAndSettle(); + + appRouter.go('/$invalidFestivalId/info?a=1#sec%20tion'); + await tester.pumpAndSettle(); + + final uri = appRouter.routerDelegate.currentConfiguration.uri; + expect(uri.pathSegments, [testFestivalId, 'info']); + expect(uri.query, 'a=1'); + expect( + uri.fragment, + 'sec%20tion', + reason: 'Uri.fragment is the raw form; it must round-trip unchanged', + ); + }); }); group('Router Navigation Paths (Phase 1 - Festival-scoped)', () { diff --git a/test/screens/festival_flash_guard_test.dart b/test/screens/festival_flash_guard_test.dart new file mode 100644 index 00000000..a4f94e8a --- /dev/null +++ b/test/screens/festival_flash_guard_test.dart @@ -0,0 +1,156 @@ +import 'package:cambridge_beer_festival/models/models.dart'; +import 'package:cambridge_beer_festival/providers/beer_provider.dart'; +import 'package:cambridge_beer_festival/screens/screens.dart'; +import 'package:cambridge_beer_festival/services/services.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:mockito/mockito.dart'; +import 'package:provider/provider.dart'; +import 'package:shared_preferences/shared_preferences.dart'; + +import '../provider_test.mocks.dart'; + +/// Every festival-scoped screen must refuse to render content belonging to a +/// festival other than the one in its route. +/// +/// The router schedules `setFestival` in a post-frame callback +/// (`router.dart`'s `_festivalScopeRedirect`), so on a URL-driven festival +/// change — a cross-festival deep link on a warm app, browser back/forward, or +/// the post-init redirect in `main.dart` — the screen builds once *before* the +/// provider has switched. Without a guard that frame shows the previous +/// festival's data under the new festival's URL (issue #397). +/// +/// Kept as one file rather than five, so adding a festival-scoped screen has an +/// obvious place to be added to the list. +void main() { + late MockDrinkRepository mockDrinkRepository; + late MockFestivalRepository mockFestivalRepository; + late MockAnalyticsService mockAnalyticsService; + late BeerProvider provider; + + const loadedFestivalId = 'cbf2025'; + const otherFestivalId = 'cbf2024'; + + final drink = Drink( + product: const Product( + id: 'drink1', + name: 'Stale Ale', + abv: 4.5, + category: 'beer', + style: 'Bitter', + dispense: 'cask', + ), + producer: const Producer( + id: 'brewery1', + name: 'Stale Brewery', + location: 'Cambridge', + products: [], + ), + festivalId: loadedFestivalId, + ); + + setUp(() async { + SharedPreferences.setMockInitialValues({}); + mockDrinkRepository = MockDrinkRepository(); + mockFestivalRepository = MockFestivalRepository(); + mockAnalyticsService = MockAnalyticsService(); + + when(mockFestivalRepository.getFestivals()).thenAnswer( + (_) async => FestivalsResponse( + festivals: [ + const Festival( + id: loadedFestivalId, + name: 'Cambridge Beer Festival 2025', + dataBaseUrl: 'https://example.com', + ), + ], + defaultFestivalId: loadedFestivalId, + version: '1.0', + baseUrl: 'https://example.com', + ), + ); + when( + mockFestivalRepository.getSelectedFestivalId(), + ).thenAnswer((_) async => null); + when(mockDrinkRepository.getDrinks(any)).thenAnswer((_) async => [drink]); + + provider = BeerProvider( + drinkRepository: mockDrinkRepository, + festivalRepository: mockFestivalRepository, + analyticsService: mockAnalyticsService, + ); + await provider.initialize(); + await provider.loadDrinks(); + }); + + tearDown(() => provider.dispose()); + + Future pump(WidgetTester tester, Widget screen) => tester.pumpWidget( + ChangeNotifierProvider.value( + value: provider, + child: MaterialApp(home: screen), + ), + ); + + // Each entry renders a screen for a festival the provider is NOT on. + final mismatchedScreens = { + 'DrinksScreen': const DrinksScreen(festivalId: otherFestivalId), + 'MyFestivalScreen': const MyFestivalScreen(festivalId: otherFestivalId), + 'DrinkDetailScreen': const DrinkDetailScreen( + festivalId: otherFestivalId, + drinkId: 'drink1', + ), + 'BreweryScreen': const BreweryScreen( + festivalId: otherFestivalId, + breweryId: 'brewery1', + ), + 'StyleScreen': const StyleScreen( + festivalId: otherFestivalId, + style: 'Bitter', + ), + }; + + for (final entry in mismatchedScreens.entries) { + final name = entry.key; + final screen = entry.value; + testWidgets('$name shows loading, not the other festival\'s data', ( + tester, + ) async { + expect(provider.currentFestival.id, loadedFestivalId); + + await pump(tester, screen); + + expect( + find.byType(CircularProgressIndicator), + findsOneWidget, + reason: '$name must hold a loading state until the provider catches up', + ); + expect( + find.text('Stale Ale'), + findsNothing, + reason: + '$name must not render $loadedFestivalId data under a ' + '$otherFestivalId route', + ); + expect( + find.text('Stale Brewery'), + findsNothing, + reason: '$name must not render the previous festival\'s brewery', + ); + }); + } + + testWidgets('screens render normally once the festival matches', ( + tester, + ) async { + await pump(tester, const DrinksScreen(festivalId: loadedFestivalId)); + await tester.pumpAndSettle(); + + expect(find.text('Stale Ale'), findsOneWidget); + expect( + find.byType(CircularProgressIndicator), + findsNothing, + reason: 'The guard must not fire when the route and provider agree', + ); + }); +} diff --git a/test/string_comparison_helper_test.dart b/test/string_comparison_helper_test.dart index 17eaca2f..94cf7b41 100644 --- a/test/string_comparison_helper_test.dart +++ b/test/string_comparison_helper_test.dart @@ -6,7 +6,7 @@ void main() { test('sorts case-insensitively', () { final unsorted = ['ipa', 'IPA', 'bitter', 'BITTER', 'Stout', 'STOUT']; final sorted = List.from(unsorted) - ..sort(StringComparisonHelper.compareLocaleAware); + ..sort(StringComparisonHelper.compareCaseInsensitive); // All case variations of the same word should be grouped together expect(sorted[0].toLowerCase(), 'bitter'); @@ -17,31 +17,44 @@ void main() { expect(sorted[5].toLowerCase(), 'stout'); }); - test('sorts accented characters after their base characters', () { - // With case-insensitive comparison, accented versions should come - // after their non-accented counterparts in most cases + test('sorts an accented word directly after its exact base word', () { + // True only because 'rose' is a prefix of 'rosé' — see the test below + // for what actually happens once any other letter is in play. final unsorted = ['Rosé', 'Rose', 'Café', 'Cafe']; final sorted = List.from(unsorted) - ..sort(StringComparisonHelper.compareLocaleAware); + ..sort(StringComparisonHelper.compareCaseInsensitive); - // Verify Cafe comes before Café, and Rose comes before Rosé - final cafeIndex = sorted.indexWhere((s) => s == 'Cafe'); - final cafeAccentIndex = sorted.indexWhere((s) => s == 'Café'); expect( - cafeIndex, - lessThan(cafeAccentIndex), - reason: 'Cafe should come before Café', + sorted.indexWhere((s) => s == 'Cafe'), + lessThan(sorted.indexWhere((s) => s == 'Café')), ); - - final roseIndex = sorted.indexWhere((s) => s == 'Rose'); - final roseAccentIndex = sorted.indexWhere((s) => s == 'Rosé'); expect( - roseIndex, - lessThan(roseAccentIndex), - reason: 'Rose should come before Rosé', + sorted.indexWhere((s) => s == 'Rose'), + lessThan(sorted.indexWhere((s) => s == 'Rosé')), ); }); + test( + 'pins the known limitation: non-ASCII sorts after every ASCII letter', + () { + // compareCaseInsensitive is NOT collation — String.compareTo compares + // UTF-16 code units, so 'é' (U+00E9) is greater than every ASCII letter. + // This is the behaviour the app ships today; it is pinned so that + // introducing a real collator is a deliberate decision with a visible + // diff here, not an accidental reordering of the style filter. + final sorted = ['Rosé', 'Rosa', 'Rose', 'Rosz', 'Rosy'] + ..sort(StringComparisonHelper.compareCaseInsensitive); + + expect( + sorted, + ['Rosa', 'Rose', 'Rosy', 'Rosz', 'Rosé'], + reason: + 'Rosé sorts last, after Rosz — a locale-aware collator would put ' + 'it next to Rose', + ); + }, + ); + test('maintains consistent alphabetical ordering', () { final unsorted = [ 'Rosé', @@ -54,7 +67,7 @@ void main() { 'Stout', ]; final sorted = List.from(unsorted) - ..sort(StringComparisonHelper.compareLocaleAware); + ..sort(StringComparisonHelper.compareCaseInsensitive); // Verify basic alphabetical order (B < C < I < P < R < S) final bIndex = sorted.indexWhere((s) => s.toLowerCase().startsWith('b')); @@ -82,7 +95,7 @@ void main() { 'Nino', ]; final sorted = List.from(unsorted) - ..sort(StringComparisonHelper.compareLocaleAware); + ..sort(StringComparisonHelper.compareCaseInsensitive); // Verify basic alphabetical grouping works // All K's should come before M's, M's before N's @@ -110,7 +123,7 @@ void main() { const original = 'Rosé Cider'; const copy = 'Rosé Cider'; - StringComparisonHelper.compareLocaleAware(original, copy); + StringComparisonHelper.compareCaseInsensitive(original, copy); expect( original, @@ -121,10 +134,13 @@ void main() { }); test('handles empty strings', () { - expect(StringComparisonHelper.compareLocaleAware('', ''), 0); - expect(StringComparisonHelper.compareLocaleAware('', 'a'), lessThan(0)); + expect(StringComparisonHelper.compareCaseInsensitive('', ''), 0); + expect( + StringComparisonHelper.compareCaseInsensitive('', 'a'), + lessThan(0), + ); expect( - StringComparisonHelper.compareLocaleAware('a', ''), + StringComparisonHelper.compareCaseInsensitive('a', ''), greaterThan(0), ); }); @@ -135,9 +151,9 @@ void main() { const b = 'Café'; const c = 'IPA'; - final ab = StringComparisonHelper.compareLocaleAware(a, b); - final bc = StringComparisonHelper.compareLocaleAware(b, c); - final ac = StringComparisonHelper.compareLocaleAware(a, c); + final ab = StringComparisonHelper.compareCaseInsensitive(a, b); + final bc = StringComparisonHelper.compareCaseInsensitive(b, c); + final ac = StringComparisonHelper.compareCaseInsensitive(a, c); if (ab < 0 && bc < 0) { expect( @@ -162,7 +178,7 @@ void main() { ]; final sorted = List.from(styles) - ..sort(StringComparisonHelper.compareLocaleAware); + ..sort(StringComparisonHelper.compareCaseInsensitive); // Verify it's in a reasonable alphabetical order // B comes before I, I before K, K before M, etc. @@ -180,7 +196,7 @@ void main() { // This test verifies that the strings with accented characters // maintain their correct form after comparison final styles = ['Rosé', 'Café', 'Märzen'] - ..sort(StringComparisonHelper.compareLocaleAware); + ..sort(StringComparisonHelper.compareCaseInsensitive); // Verify the accented characters are preserved correctly expect( diff --git a/test/utils/navigation_helpers_test.dart b/test/utils/navigation_helpers_test.dart index 8de950d2..0d11c3bc 100644 --- a/test/utils/navigation_helpers_test.dart +++ b/test/utils/navigation_helpers_test.dart @@ -480,45 +480,5 @@ void main() { expect(find.text('Can pop: false'), findsOneWidget); }); }); - - group('safeDecodeComponent', () { - test('decodes a valid percent-encoded string', () { - expect(safeDecodeComponent('IPA%20American'), equals('IPA American')); - }); - - test('decodes unicode percent-encoding', () { - expect( - safeDecodeComponent('Bi%C3%A8re%20de%20Garde'), - equals('Bière de Garde'), - ); - }); - - test('returns unmodified string with no encoding', () { - expect(safeDecodeComponent('IPA'), equals('IPA')); - }); - - test('returns raw value for stray percent (illegal encoding)', () { - expect(safeDecodeComponent('50%'), equals('50%')); - }); - - test('returns raw value for truncated percent sequence', () { - expect(safeDecodeComponent('foo%2'), equals('foo%2')); - }); - - test('returns raw value for percent followed by non-hex', () { - expect(safeDecodeComponent('foo%ZZ'), equals('foo%ZZ')); - }); - - test('handles empty string', () { - expect(safeDecodeComponent(''), equals('')); - }); - - test('handles string with multiple valid encodings', () { - expect( - safeDecodeComponent('IPA%20-%20American%20Pale'), - equals('IPA - American Pale'), - ); - }); - }); }); } diff --git a/test/widgets/festival_menu_sheets_test.dart b/test/widgets/festival_menu_sheets_test.dart index 2b0ce1bc..ffd6350a 100644 --- a/test/widgets/festival_menu_sheets_test.dart +++ b/test/widgets/festival_menu_sheets_test.dart @@ -194,9 +194,12 @@ void main() { baseUrl: 'https://data.cambeerfestival.app', ), ); - // Don't pumpAndSettle — FestivalSelectorSheet is a StatelessWidget and - // won't rebuild, so CircularProgressIndicator keeps animating indefinitely. - await tester.pump(); + await tester.pumpAndSettle(); + + // The sheet listens to the provider, so finishing the load clears the + // spinner without reopening the sheet. + expect(find.byType(CircularProgressIndicator), findsNothing); + expect(find.text('No festivals available'), findsOneWidget); }); testWidgets('shows error state when festival loading fails', ( @@ -272,6 +275,16 @@ void main() { await tester.pumpAndSettle(); expect(retryProvider.festivalsError, isNull); + // Retry is one of the two controls in this sheet that does NOT pop it + // (Refresh is the other), so the result has to land on screen while the + // sheet is still open — otherwise the button looks dead. + expect( + find.text('Failed to load festivals'), + findsNothing, + reason: 'Retry must clear the error state from the open sheet', + ); + expect(find.byIcon(Icons.error_outline), findsNothing); + expect(find.text('No festivals available'), findsOneWidget); }); testWidgets('shows empty state when no festivals are available', (