fix(filters): scope drink facets by all filters but their own - #505
Merged
Conversation
Category, style, and allergen facets each used a different, ad-hoc scoping rule, so selecting one filter didn't consistently narrow the others and could hide the very option a user had just selected. Replace all three with one rule, implemented once in _scopeFor(): a facet is derived from every structural filter except its own. A selected option is always kept in its facet list/count even when its scoped count drops to 0, so an active filter (especially an allergen exclusion, a safety control) never disappears from the UI. Also fix availableAllergens to only list allergens with a non-zero value in scope, since a value of 0 means "confirmed absent" — previously zeroed keys inflated the allergen list with permanent no-op toggles. Free-text search stays excluded from facet scoping, unchanged. Fixes #317
Contributor
There was a problem hiding this comment.
Pull request overview
This PR standardizes how DrinkFilterController derives facet options/counts (categories, styles, allergens) by introducing a single “apply all other structural filters, but never the facet’s own filter” scoping rule. It fixes cases like allergen facets showing options that don’t apply to the currently visible subset of drinks (e.g. after filtering to cider).
Changes:
- Replaced ad-hoc facet scoping with a single
_scopeFor(_Facet)helper that reusesDrinkFilterService.filterDrinks(excluding free-text search by design). - Updated category/style count maps and option lists to preserve the invariant that selected options remain visible even when their scoped count is 0.
- Expanded controller unit tests to cover per-facet scoping, “does not narrow itself” behavior, allergen presence semantics (non-zero only), and the deliberate “search does not affect facets” rule.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/domain/controllers/drink_filter_controller.dart | Implements the unified facet-scoping rule via _scopeFor, updates facet getters/counts, and documents the rule + invariants. |
| test/domain/controllers/drink_filter_controller_test.dart | Adds comprehensive tests for facet scoping, invariants, allergen presence rules, and confirms search does not scope facets. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
🚀 Cloudflare Pages PreviewYour preview deployment is ready! Preview URL: https://fix-317-facet-scoping.staging-cambeerfestival.pages.dev This preview will be automatically updated when you push new commits to this PR. |
richardthe3rd
pushed a commit
that referenced
this pull request
Jul 28, 2026
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
richardthe3rd
added a commit
that referenced
this pull request
Aug 3, 2026
… 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces three ad-hoc facet-scoping rules in
DrinkFilterControllerwith one stated rule.The problem
The five facet getters each used a different, undocumented scope:
availableCategories/categoryCountsMap_sourceavailableStyles/styleCountsMap_categoryScopedSource()— category onlyavailableAllergens_sourceFilter to cider and the allergen sheet still offered every allergen in the festival, none of which applies to any cider (#317).
The rule
A facet is derived from the source with every other structural filter applied — but never its own. Structural filters are category, styles, favourites-only, visibility filters, and excluded allergens. A facet must not narrow itself, or selecting one of its options would hide its siblings and the list would collapse under the user's finger.
One private
_scopeFor(_Facet)helper reusingDrinkFilterService.filterDrinksimplements it; all five getters call it and_categoryScopedSource()is gone. Counts are now honest — "Stout (12)" really yields 12.Two invariants
An active filter is never hidden. A selected option is always listed even when its scoped count is 0 (and appears in the count map as 0). Non-negotiable for allergens: they are a safety filter, and a user who ticked "gluten" then narrowed to cider must still see the row to untick it.
excludedAllergensis persisted, so this also covers exclusions saved before this change.availableAllergenslists only allergens actually present (non-zero) in scope. It previously useddrink.allergens.keys, which includes keys explicitly set to0— meaning the allergen is absent. This now matchesfilterByExcludedAllergens's own definition of presence.Deliberate deviation
Free-text search is excluded from facet scoping.
drinks_screen.dart:140deriveshasStyleFilterfromavailableStyles.isNotEmpty, so scoping by the search query would make the Style button appear and disappear from the filter bar as the user types. Pinned by a test so it isn't "fixed" later.Measured impact (live cbf2026 feed, 689 drinks)
Invariant 2 turns out to matter more than the issue suggested. Five beer products carry a fully-zeroed allergen declaration, so the sheet offered 14 allergen toggles of which only 5 are ever non-zero anywhere in the festival. Celery, soybeans, lupins, mustard, egg, sesame, molluscs, crustaceans and fish were permanent no-ops.
Combined with scoping:
Scope
Domain layer only — no UI, provider, or persistence changes. Behaviour visible in the existing sheets improves because they receive better data.
Multi-select categories (#319) and grouping styles by category (#318) are a follow-up PR stacked on this one.
Testing
./bin/mise run checkclean. Controller tests go from 30 to 55, adding per-facet scoping coverage, "does not narrow itself" tests per facet, both invariants, bool/num allergen-value variants, and assertions that ticking a scoped option yields exactly the stated count.One existing test changed:
availableAllergens aggregates keys across the sourceasserted the old behaviour of listing a zero-valued key, and was replaced by the allergen-presence group.Fixes #317
Generated by Claude Code