feat(drinks): multi-select categories and grouped style filter - #506
Conversation
Category filtering was single-select (String?), so users could not view beer + international-beer + low-no together even though those three categories make up 76% of a typical festival. Switch DrinkFilterController/Service and BeerProvider to a Set<String>-based category filter (OR logic across categories, matching the existing style-filter pattern) and convert CategoryFilterSheet from single-select radios to multi-select checkboxes that stay open across taps. Toggling a category now prunes (rather than clears) the selected styles, keeping only styles still present under the new category scope, since an unconditional clear was too destructive once several categories can be active at once. Analytics keeps logCategoryFilter's existing String? signature; the provider now passes a canonical sorted, comma-joined value (or null when cleared) so the logged value is independent of selection order. Fixes #319
With no category selected, the style list was one flat alphabetical run mixing styles from unrelated categories (e.g. wine and perry styles interleaved with beer styles). Add DrinkFilterController.stylesByCategory, a grouped view of the same style facet scope the facet-scoping rule already defines, and render a category header above each group in StyleFilterSheet. A lone group (the common case once a single category is selected) renders flat with no header. Headers reuse the existing "Allergen-free" label treatment and carry Semantics(header: true) for screen-reader section navigation. A selected style outside the current scope is still grouped, under its category in the full source, preserving the facet-scoping invariant that an active filter is never hidden. Fixes #318
The category filter button renders formatted names ('International
Beer') but its semanticLabel joined the raw category ids, so a screen
reader announced 'international-beer'. Sighted and screen-reader users
now get the same names, sorted so the announcement is deterministic
(a Set has no defined order).
Adds semantic tests pinning the label in all three states.
44d381d to
012c232
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
🚀 Cloudflare Pages PreviewYour preview deployment is ready! Preview URL: https://fix-319-multi-category.staging-cambeerfestival.pages.dev This preview will be automatically updated when you push new commits to this PR. |
The empty-state button is the only way out of a filter combination that matches nothing without reopening the category sheet, and this branch changed its callback from setCategory(null) to clearCategories() with no test exercising it. Asserts the button appears when a category selection yields no drinks, clears the selection when tapped, and disappears afterwards.
There was a problem hiding this comment.
Pull request overview
Implements multi-select drink categories and improves the style-filter UX by grouping styles under category headers, with accompanying analytics canonicalisation and semantics-focused tests. This builds on the facet-scoping work from #505 and addresses #319/#318 without changing URL contracts or persistence.
Changes:
- Replace single-select category filtering with multi-select (
Set<String>) across controller/service/provider and update UI from radios to checkboxes. - Add
stylesByCategoryto group style options by category in the style filter sheet (headers only when multiple groups are in scope). - Fix/extend accessibility semantics (notably: filter button semantic labels now use formatted category names in deterministic order) and add tests to pin the semantics.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/domain/controllers/drink_filter_controller.dart | Switch category selection to Set<String>, add pruning logic, and introduce stylesByCategory derived from the existing facet-scoping rule. |
| lib/domain/services/drink_filter_service.dart | Replace filterByCategory with filterByCategories and update filterDrinks to accept categories. |
| lib/providers/beer_provider.dart | Expose selectedCategories/stylesByCategory, implement toggleCategory + clearCategories, and log canonical analytics values. |
| lib/screens/drinks_screen.dart | Update category filter chip label/semanticLabel for multi-select, and wire empty-state “Clear Filters” to clearCategories(). |
| lib/widgets/drink_filter_sheets.dart | Convert category sheet to multi-select checkboxes, add Clear button, and group styles by category with semantics headers. |
| test/beer_provider_test.dart | Update provider tests for multi-select categories and style-pruning behavior. |
| test/domain/controllers/drink_filter_controller_test.dart | Update controller tests for new category semantics, OR logic, pruning, and stylesByCategory. |
| test/domain/services/drink_filter_service_test.dart | Update service tests for filterByCategories and filterDrinks(categories: …) behavior. |
| test/drinks_screen_style_filter_test.dart | Add semantics tests ensuring formatted, sorted category names are announced (no raw IDs). |
| test/provider_test.dart | Update analytics tests to assert canonical joined category values and null when cleared. |
| test/widgets/drink_filter_sheets_test.dart | Update sheet rendering/interaction tests for checkbox-based category selection and style headers. |
Comments suppressed due to low confidence (1)
lib/widgets/drink_filter_sheets.dart:331
- The style row Semantics label uses "$count drinks", which will announce "1 drinks" for single matches. Since these strings are consumed by screen readers, pluralise the label so it uses the singular form for 1.
final count = styleCounts[style] ?? 0;
final isSelected = selectedStyles.contains(style);
return Semantics(
label: 'Filter by $style, $count drinks',
value: isSelected ? 'Selected' : 'Not selected',
🚀 Cloudflare Pages PreviewYour preview deployment is ready! Preview URL: https://fix-319-multi-category.staging-cambeerfestival.pages.dev This preview will be automatically updated when you push new commits to this PR. |
Two screen-reader wording fixes surfaced in review: Category and style filter rows announced '1 drinks'. They now use the count == 1 ? 'drink' : 'drinks' idiom already used in festival_header and my_festival_screen. The empty-state button clears every selected category since this branch made categories multi-select, but announced 'Clear category filter' (singular) and hinted 'show all drinks' — a promise it cannot keep when a search or style filter is also active.
🚀 Cloudflare Pages PreviewYour preview deployment is ready! Preview URL: https://fix-319-multi-category.staging-cambeerfestival.pages.dev This preview will be automatically updated when you push new commits to this PR. |
The headers were the only children narrower than the sheet in a Column that defaults to centre alignment, so they rendered centred instead of sitting above their group. Every other Column in this file already sets CrossAxisAlignment.start. Found by driving a local release web build in a headless browser; the widget tests passed because none of them asserted horizontal position. The header test now pins it.
🚀 Cloudflare Pages PreviewYour preview deployment is ready! Preview URL: https://fix-319-multi-category.staging-cambeerfestival.pages.dev This preview will be automatically updated when you push new commits to this PR. |
Every symbol the filter sections named was removed when filter state moved into DrinkFilterController, and the drift widened with #505/#506. The worked example did not compile against the current code. - applyAllFilters -> filterDrinks, with its real named parameters - BeerProvider._applyFiltersAndSort() -> DrinkFilterController.recompute() as the single recompute path - category: String? -> categories: Set; hideUnavailable: bool -> visibilityFilters: Set - add the missing facet-scoping rule and its two invariants (an active filter is never hidden; allergens must be non-zero-present), plus the deliberate exclusion of free-text search from facet scoping Also correct references to FavoritesService/RatingsService, removed in #391/#395 in favour of UserDataStore, and add the controllers layer to the architecture diagram. Leaving those would have shipped a doc fix that still pointed at deleted classes. Fixes #511
… doc refresh (#514) * fix(mise): keep proto tools off the non-proto critical path api-linter sat in mise.dev.toml's [tools], so it was provisioned for every dev-env task. Its install fails behind a proxy, which aborted format, analyze, test and check — none of which need proto tooling. On Claude Code Web .miserc.toml auto-selects the dev env, so this broke plain `./bin/mise run check` outright. Declare buf and api-linter as task-scoped tools on the proto:* tasks that invoke them. Note what this does NOT fix, since the issue proposes it: pinning. Both tools were already pinned and locked — mise.dev.lock carried buf 1.70.0 and api-linter 2.3.1 with checksums and asset URLs, and the failure happened anyway. Verified on mise 2026.5.8 that a lockfile entry does not suppress api-linter's SLSA provenance call: download and checksum both succeed, only the provenance lookup 403s, and MISE_SLSA_VERIFY=0 does not disable it. mise had recorded github_attestations = "unavailable" and still attempted verification. What scoping buys is blast radius, which is the actual complaint in #510. Also set lockfile = true so mise maintains mise.lock/mise.dev.lock on install rather than only when regenerated by hand (#454). Known trade-off: task-scoped tools are not in a config's [tools] set, so `mise lock` drops them from mise.dev.lock and they re-resolve on a fresh worktree. Moving them to a MISE_ENV=dev,proto env file would keep both properties, at the cost of changing the proto:* invocation. Verified: `./bin/mise run check` completes (1313 tests pass) where it previously died during tool provisioning. Fixes #510 * fix(mise): exclude nested .mise and worktrees from shell file discovery The find in shell/check.sh and shell/format.sh excluded "./.mise/*", anchored at the repo root, so it did not match a *nested* .mise inside an agent worktree. Each worktree under .claude/worktrees/ carries its own extracted Flutter SDK — thousands of vendored .sh files, some of which shfmt rejects — so check aborted before analyze or test ran, pointing at vendored SDK code rather than anything the developer changed. This broke the pre-commit gate for the repo's own documented parallel-agent workflow, whose whole mechanism is agent worktrees. Make the exclusions unanchored and skip worktrees explicitly. Verified against a synthetic worktree tree: the old pattern picks up its scripts, the new one excludes them, and shell:check passes with it present. Fixes #509 * docs: rewrite domain-architecture filter sections against current code Every symbol the filter sections named was removed when filter state moved into DrinkFilterController, and the drift widened with #505/#506. The worked example did not compile against the current code. - applyAllFilters -> filterDrinks, with its real named parameters - BeerProvider._applyFiltersAndSort() -> DrinkFilterController.recompute() as the single recompute path - category: String? -> categories: Set; hideUnavailable: bool -> visibilityFilters: Set - add the missing facet-scoping rule and its two invariants (an active filter is never hidden; allergens must be non-zero-present), plus the deliberate exclusion of free-text search from facet scoping Also correct references to FavoritesService/RatingsService, removed in #391/#395 in favour of UserDataStore, and add the controllers layer to the architecture diagram. Leaving those would have shipped a doc fix that still pointed at deleted classes. Fixes #511 * docs(mise): clarify that scoping contains the failure, not fixes it The comment said a task-scoped tool that fails to provision degrades to a warning and "the task body still runs", which reads as though the task succeeds. It doesn't: proto:* will run its body and fail on `buf: command not found` unless the binary is already on PATH. Spell out that the win is check/test no longer being collateral damage, not that proto linting works without buf. Raised in review on #514. --------- Co-authored-by: Claude <noreply@anthropic.com>
* refactor(models): format festival dates with intl and give Festival value equality formattedDates carried its own month-name table alongside three range branches, while intl was already a dependency used by three screens. DateFormat replaces the table; output is byte-identical for every existing case. The cross-month branch printed the start year on both ends, so a festival spanning New Year reported the wrong year on its end date. The end date now carries its own year. Festival also had no == or hashCode while Drink, Product and Producer all implement identity-by-id, so two instances of the same festival parsed from cache and from the network compared unequal in any collection operation. Adds id-based equality with the same empty-id identity fallback Producer uses. Fixes #532 Fixes #529 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ER7MeRfgMqxnaKBGDaSRWy * refactor(widgets): make AvailabilityStatus switches exhaustiveness-checked Both switches over AvailabilityStatus in drink_card.dart were C-style switch statements, which are not exhaustiveness-checked. The semantic label switch had no safety net at all: a seventh enum value would fall through and silently omit availability from the screen-reader label. #349 established that the festival status_text vocabulary is not stable across festivals, so a new status value is a realistic event. Both are now switch expressions with no wildcard arm, so a new enum value is a compile error. Verified by adding a seventh value and confirming non_exhaustive_switch_expression fires at both sites. Output is unchanged. Fixes #534 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ER7MeRfgMqxnaKBGDaSRWy * refactor(ui): extract a shared drink-count pluralisation helper The 'count == 1 ? drink : drinks' ternary was inlined at four call sites, past the extract-a-helper threshold in AGENTS.md. The failure mode is silent and accessibility-facing: a new label that forgets the ternary announces "1 drinks" to a screen reader and nothing fails. Two of the four sites had exactly that bug until review caught it in #506. Adds StringFormattingHelper.drinkCountLabel and routes all four sites through it. Labels are unchanged. Fixes #513 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ER7MeRfgMqxnaKBGDaSRWy * refactor(widgets): read sheet height with MediaQuery.sizeOf Five bottom-sheet builders capped their height with MediaQuery.of(context).size.height, which subscribes the sheet to every MediaQuery change — keyboard insets, text scale, brightness, padding — not just size. For a bottom sheet the keyboard case is the one that actually fires. MediaQuery.sizeOf establishes a dependency on size alone. The value read is identical, so there is no behaviour change. Fixes #531 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ER7MeRfgMqxnaKBGDaSRWy --------- Co-authored-by: Claude <noreply@anthropic.com>
Follow-up to #505 (merged), rebased onto
main. Commits are deliberately not squashed so each is independently reviewable and revertible (the UI-change discipline inui-and-accessibilityasks for one widget per change).1. Multi-select categories —
Fixes #319_selectedCategory: String?→Set<String> _selectedCategories,setCategory→toggleCategory/clearCategories, radios → checkboxes.Why this is the highest-value of the three issues: in the live cbf2026 feed you currently cannot view beer (242) + international-beer (251) + low-no (31) together — 76% of the festival is split into three buckets you must browse one at a time. cider (102) + perry (23) and mead (14) + wine (26) are the other natural pairings.
DrinkFilterService.filterByCategory(String?)→filterByCategories(Set<String>), mirroring the existingfilterByStyles(empty set = no filter, OR logic). This and thesetCategory→toggleCategory/clearCategorieschange are breaking renames on public APIs; no in-tree caller outside this diff uses them.setCategoryused to wipe the entire style selection on any category change. Under multi-select that's too destructive — adding perry to cider would discard a cider style you just picked.toggleCategorynow prunes to styles still in scope. It deliberately recomputes the scope rather than readingavailableStyles, since that getter re-includes already-selected styles under invariant 1 and would make pruning a no-op.CategoryFilterSheetfollowsStyleFilterSheet's established pattern:Consumer-based, stays open while toggling, header "Clear" button, keyed rows, and the repo's filter-chipSemantics(label/value/selected).Category/ a single formatted name /N categories.Not included, deliberately: no URL/deep-link representation (filter state isn't in the URL today — the
:categorysegment inrouter.dartis the drink-detail route — and URLs are a public contract), and no persistence (category/style selections aren't persisted today, so noPreferenceKeysentry).AnalyticsServiceis unchanged;logCategoryFilterreceivesnullwhen empty, otherwise the sorted comma-joined selection so the value is order-independent.2. Group styles by category —
Fixes #318New
stylesByCategoryview on the controller, rendered as headed sections.The issue's premise needed correcting first: it says "Medium Dry (a cider style)", but no cider carries a
stylefield at all — 61% of drinks (all cider, all 251 international beers, all mead, all low-no) have no style, there are only 19 distinct styles, and zero occur in more than one category. The real muddle is the 5 wine and 2 perry styles interleaved alphabetically with 12 beer styles, soRed,WhiteandMediumsit betweenPorterandStout.Implements the issue's option 2. Options 1 and 3 are rejected: disabling the style filter when no category is selected removes the legitimate "all Stouts" case, and deriving styles from the fully-filtered set doesn't address the complaint at all — with nothing selected, the styles present in everything are all styles.
_scopeFor(_Facet.style)scope, not a second scoping rule. Ordering stays in the controller.labelMedium+onSurfaceVariant), wrapped inSemantics(header: true)for section navigation. No new typography, no restyling of the rest of the sheet.styleCountsMap(scope-wide per style name) so the number shown equals what ticking actually yields.3. Accessibility and correctness follow-ups
semanticLabel. It renderedInternational Beerbut announced the rawinternational-beer. Sighted and screen-reader users now get the same names, sorted so the announcement is deterministic (aSethas no defined order).count == 1 ? 'drink' : 'drinks'idiom already used infestival_header.dart:21andmy_festival_screen.dart:210. (Raised in review.)setCategory(null)toclearCategories()with no test touching it, even though it is the only escape from a filter combination matching nothing without reopening the sheet.Columnthat defaults to centre alignment, so they rendered centred instead of above their group. Every otherColumnin that file already setsCrossAxisAlignment.start.Testing
./bin/mise run checkgreen — 1313 tests pass, analyzer reports only 9 pre-existing info-level lints and no warnings. No goldens exist for the drinks screen or filter sheets, so none were regenerated.The header-alignment bug above was not caught by the widget tests, because none of them asserted horizontal position — it was found by driving a local release web build in headless Chromium. The header test now pins the header's left edge, and that assertion was confirmed to fail without the fix.
Verified in that browser run against the real 689-drink cbf2026 feed: checkboxes replace radios; "All (689)" reflects an empty selection; per-category counts match the feed exactly; the sheet stays open while toggling and the list updates live behind it; "Clear" appears once a selection exists; Beer + Wine yields 268 drinks (242 + 26) with a "2 categories" bar label; and the style sheet groups under Beer and Wine headers.
Two honest limits on that run: every webfont is served from a host this environment blocks, so a substitute font was injected and the typography is therefore not representative; and only the light theme at a single 430px viewport was checked.
Fixes #319
Fixes #318