fix: correctness fixes from a codebase review by richardthe3rd · Pull Request #498 · richardthe3rd/cambridge-beer-festival-app

richardthe3rd · 2026-07-24T21:40:43Z

Findings from a general code review of the current codebase. Each was verified against the actual code (and, where it mattered, against the live cbf2026 feeds and the real appRouter) before being fixed. Every fix is TDD — a failing test first, then the change.

Fixes

385e443 — path parameters corrupted on cross-festival redirect

go_router hands back percent-decoded path parameters, so the six per-route onInvalidFestival closures — each rebuilding its own path by string interpolation — silently lost the encoding. Verified against the real router:

URL	Before	After
`/badfest/brewery/what%3Fnow`	`/cbf2025/brewery/what?now` (id truncated, rest became a query)	id preserved
`/badfest/drink/beer/a%2Fb`	`/cbf2025/drink/beer/a/b` (extra segment, route stops matching)	single segment
`/badfest/brewery/hash%23tag`	`/cbf2025/brewery/hash#tag` (rest became a fragment)	id preserved

A style like Porter/Stout (encoded %2F by buildStylePath) reached via a stale link to a retired festival was enough to trigger it. All six closures did the same segment-0 swap, so it now happens once in _festivalScopeRedirect, from state.uri, with each segment re-encoded. That also carries the query string across — previously only /:festivalId did, and the five nested routes dropped it.

c053bae — DrinkSort.brewery mis-sorts live data

DrinkSortService used a raw String.compareTo, which orders every capitalised value ahead of every lowercase one. In the live cbf2026 feed that puts d'Achouffe at index 277 of 279 — below Zötler — instead of between Cydefx and Daleside. The style facet and My Festival list already sorted case-insensitively, so this was the outlier. Name and style sorts changed too, since they shared the comparator.

1d96d32 — Retry and Refresh were dead buttons

FestivalSelectorSheet renders provider state from a context.read reference, which subscribes to nothing, and a modal route isn't rebuilt by its opener. Every other sheet gets away with this because its controls pop before the notification lands — but Retry and Refresh deliberately stay open, so loadFestivals() ran and nothing on screen changed. Now wrapped in a ListenableBuilder.

a30e5ba — app could strand on the startup spinner

initialize() awaits SharedPreferences, two migrations, the festival cache and the saved selection with no error handling. A throw meant _isInitialized was never set, the / redirect kept returning null, and the app sat on the spinner permanently — Crashlytics saw it, the user had no way out. Failures are now logged and surfaced as error so the drinks screen offers a Retry.

324cc46 — festival-flash guard on the remaining four screens

MyFestivalScreen was the only screen carrying the #397 guard. The router switches festival in a post-frame callback, so on a URL-driven change the other screens built once against the previous festival's catalogue. The in-app switcher was already safe (it calls setFestival before router.go), which is why this never showed up in normal use.

dd88edf — compareLocaleAware renamed to compareCaseInsensitive

It lower-cases and calls String.compareTo, which compares UTF-16 code units — not collation. Rosé sorts after Rosz; Ārpus sorts below Zötler. Behaviour unchanged; the name and docstring now match, and the limitation is pinned by a test so adding a real collator later is a deliberate, visible change.

338ee00 — doc drift

navigateToRoute stopped branching web/mobile in #470, but AGENTS.md and two skill files still described the branch in the present tense with drifted line citations. Also corrects FavoritesScreen → MyFestivalScreen and marks three unused widgets in the reuse table as such.

Testing

./bin/mise run check green: 1310 tests, analyzer at its 9 pre-existing infos with none added.

Each fix went red before green. The flash-guard tests were checked for vacuity — with the guards stashed, 4 of the 5 fail (MyFestivalScreen passes, as it already had one).

Not done here

router.dart's safeDecodeComponent double-decodes an already-decoded parameter. A no-op for every real style name; same bug family, but it deserves its own change.
DrinkSortService still has no tiebreak, so equal-ABV drinks are in unspecified (non-alphabetical) order.
The initialize() change alters startup behaviour on a path no test previously covered. An agent can't verify this on a real device — worth a manual check with a deliberately corrupted preferences store before release.

Generated by Claude Code

go_router hands back percent-decoded path parameters, so the six per-route onInvalidFestival closures — each rebuilding its own path by string interpolation — silently lost the encoding. A style containing a slash ("Porter/Stout", encoded %2F by buildStylePath) split into two segments and stopped matching its route; a "?" became a query string and a "#" became a fragment, truncating the id. Reaching a stale link to a retired festival was enough to trigger it. All six closures did the same thing: swap segment 0 for the current festival. Doing that once in _festivalScopeRedirect, from state.uri with each segment re-encoded, fixes the corruption and collapses the duplication. It also carries the query string across, which previously only /:festivalId did and the five nested routes dropped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MoEMyawRmvhA2zuSDBmLdr

DrinkSortService used a raw String.compareTo, which orders every capitalised value ahead of every lowercase one. In the live cbf2026 feed that puts the brewery "d'Achouffe" at index 277 of 279 — below "Zotler" — instead of alphabetically between "Cydefx" and "Daleside". The style facet and the My Festival list already sort case-insensitively via StringComparisonHelper, so DrinkSortService was the outlier. Use the same comparator for all three text sorts. Tie order is still unspecified (equal-ABV drinks are not alphabetical); adding a secondary tiebreak is left as a separate change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MoEMyawRmvhA2zuSDBmLdr

FestivalSelectorSheet renders provider state from a reference captured via context.read, which subscribes to nothing, and a modal route is not rebuilt by its opener. Every other sheet in this file gets away with that because its controls pop the sheet before the notification lands — but Retry and Refresh deliberately stay open, so loadFestivals() ran and nothing on screen changed. Both were dead buttons. Wrap the body in a ListenableBuilder on the injected provider. Consumer would work too, but listening to the instance directly keeps the widget usable without an ancestor Provider, which is how its tests build it. The two tests covering this asserted provider state only; one carried a comment explaining that the sheet would not rebuild. Both now assert the rendered UI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MoEMyawRmvhA2zuSDBmLdr

The function lower-cases both operands and calls String.compareTo, which compares UTF-16 code units. That is not collation: 'é' is greater than every ASCII letter, so "Rosé" sorts after "Rosz" rather than next to "Rose", and the brewery "Ārpus" sorts below "Zötler". The old name and docstring promised locale-aware ordering the code never delivered, and the docstring's examples ("Café comes right after Cafe") were true only because the base word is a prefix of the accented one. Behaviour is unchanged — this renames the function and rewrites the docstring to describe what it does, including the limitation. The test that asserted the coincidental accent ordering now also pins the real behaviour, so introducing a collator later is a deliberate change with a visible diff rather than a silent reordering of the style filter. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MoEMyawRmvhA2zuSDBmLdr

BeerProvider.initialize() awaits SharedPreferences, two one-time migrations, the festival cache and the saved selection, with no error handling. If any of them threw, _isInitialized was never set, so the router's '/' redirect kept returning null and the app sat on the CircularProgressIndicator forever. Crashlytics recorded it as fatal, but the user had no way out short of reinstalling. Move the restore sequence into _restoreState() and wrap it: a failure is logged, surfaced as provider.error so the drinks screen offers a Retry, and startup completes either way. loadDrinks() now also refuses to dereference a null repository, which is the state left behind when initialize() fails before the repositories are built. Also wraps three fire-and-forget futures in unawaited() while in these files — including the logError call in _handlePostInitRedirect, the only analytics call in lib/ that was not wrapped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MoEMyawRmvhA2zuSDBmLdr

MyFestivalScreen was the only screen holding the festival-flash guard from #397. The router schedules setFestival in a post-frame callback, so on a URL-driven festival change — a cross-festival deep link on a warm app, browser back/forward, or the post-init redirect — the other four screens built once against the previous festival's catalogue. The drinks list showed the old festival's name and drinks; the detail screens resolved the wrong entity or a spurious "not found". The in-app switcher was already safe (it calls setFestival before router.go), which is why this never showed up in normal use. Adds the guard to DrinksScreen, DrinkDetailScreen, BreweryScreen and StyleScreen, and gives the invariant one test file covering all five so a new festival-scoped screen has an obvious place to be added. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MoEMyawRmvhA2zuSDBmLdr

navigateToRoute stopped branching on platform in #470 (PR #478) — it is now context.push on every platform — but AGENTS.md and two skill files still described the web/mobile branch in the present tense, with line citations that had also drifted. Skills steer future agents, so this sent them looking for code that no longer exists. Also corrects references to the FavoritesScreen class (renamed to MyFestivalScreen; the route stays /:festivalId/favorites, and that distinction is now stated as such), and marks BottomActionBar, ActionButton and BreadcrumbBar in the reuse table as currently unused — no screen wires them up, so the table was recommending widgets that are not in fact an established pattern here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MoEMyawRmvhA2zuSDBmLdr

codecov · 2026-07-24T21:44:03Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copilot

Pull request overview

Correctness-focused fixes across routing, sorting, provider initialization, and festival-scoped UI rendering, with tests added/updated to pin the regressions described in the PR metadata.

Changes:

Centralize invalid-festival redirects to preserve percent-encoded path segments and query strings across all festival-scoped routes.
Make text-based drink sorting case-insensitive (brewery/name/style), and rename the string comparator helper to reflect its real behavior (non-collation).
Prevent “festival flash” on URL-driven festival switches by adding a guard to the remaining festival-scoped screens; fix FestivalSelectorSheet to rebuild while staying open.

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`lib/router.dart`	Centralizes invalid-festival redirect behavior and adds `unawaited` for deferred festival switches.
`lib/providers/beer_provider.dart`	Makes `initialize()` non-throwing and adds a defensive guard in `loadDrinks()` for failed init.
`lib/domain/services/drink_sort_service.dart`	Switches text sorts to the shared case-insensitive comparator.
`lib/utils/string_comparison_helper.dart`	Renames comparator and documents/pins the known non-collation limitation.
`lib/domain/controllers/drink_filter_controller.dart`	Updates style sorting to use the renamed comparator.
`lib/widgets/festival_menu_sheets.dart`	Ensures FestivalSelectorSheet rebuilds while open via `ListenableBuilder`.
`lib/screens/drinks_screen.dart`	Adds festival-flash guard to prevent one-frame stale festival content.
`lib/screens/drink_detail_screen.dart`	Adds festival-flash guard to prevent stale festival entity resolution.
`lib/screens/brewery_screen.dart`	Adds festival-flash guard to prevent stale festival entity resolution.
`lib/screens/style_screen.dart`	Adds festival-flash guard to prevent stale festival entity resolution.
`lib/main.dart`	Uses `unawaited` for lifecycle refresh and startup init/load sequence.
`test/router_test.dart`	Adds redirect tests for encoded path parameters + query preservation on nested routes.
`test/provider_test.dart`	Adds tests ensuring init failures don’t strand the app and that later loads can recover.
`test/domain/services/drink_sort_service_test.dart`	Pins case-insensitive ordering behavior for name/brewery/style sorts.
`test/string_comparison_helper_test.dart`	Updates comparator name and adds a pinned test for non-ASCII ordering limitation.
`test/widgets/festival_menu_sheets_test.dart`	Updates sheet tests to assert live rebuild while sheet remains open.
`test/screens/festival_flash_guard_test.dart`	New tests ensuring mismatched festival routes show loading, not stale content.
`AGENTS.md`	Updates navigateToRoute documentation to reflect current router behavior.
`.claude/skills/ui-and-accessibility/SKILL.md`	Updates references for MyFestivalScreen + navigateToRoute docs; reuse table edits.
`.claude/skills/my-festival-campaign/SKILL.md`	Updates stale naming references (FavoritesScreen → MyFestivalScreen).
`.claude/skills/architecture-contract/SKILL.md`	Updates navigateToRoute invariant and corrects drift about MyFestival naming.

richardthe3rd · 2026-07-25T11:05:44Z

+      );
+    } finally {
+      _isInitialized = true;
+      notifyListeners();
+    }


Guard added in cd91843 — but correcting the premise, because the distinction matters for anyone reading this later.

It did not crash. _festivalRepository! is dereferenced inside loadFestivals's try, so the TypeError was caught by the existing catch (e) and converted to a festivalsError via _getUserFriendlyErrorMessage. The fallback line beneath it already uses ?., so that was null-safe too. I verified rather than assumed — two tests added, both of which pass against the unmodified code:

loadFestivals without a repository surfaces festivalsError

refreshIfStale without repositories does not throw

So there was no hard lock or crash path, and the user-visible festivalsError you suggested as the remedy was already what happened.

That said, the underlying instinct is right: catching a TypeError as control flow hides the intent and yields the generic "Something went wrong" message rather than one about festivals. loadFestivals now has an explicit null guard with its own message, and the two tests stay to pin the behaviour either way.

Generated by Claude Code

github-actions · 2026-07-24T21:47:26Z

🚀 Cloudflare Pages Preview

Your preview deployment is ready!

Preview URL: https://claude-codebase-review-0wpcq.staging-cambeerfestival.pages.dev

This preview will be automatically updated when you push new commits to this PR.

Router: - Preserve the URL fragment across the invalid-festival redirect. The docstring claimed "everything else about the URL" but dropped it. Uri.query and Uri.fragment both return the raw, still-encoded form, so appending round-trips exactly (verified — re-encoding double-escapes). This retires a documented known limitation: the test that asserted the lossy behaviour, and its TODO, are replaced by one pinning the fix. - Stop double-decoding the style path parameter. go_router already percent-decodes path parameters, so safeDecodeComponent turned a style literally containing "%20" into a space. The helper existed only for that call site and is removed with it. Provider: - Guard loadFestivals() against a null repository explicitly. This was not a crash — the null dereference was already caught and surfaced as festivalsError — but relying on a TypeError for control flow hid the intent and produced a generic message. Tests pin loadFestivals and refreshIfStale completing without repositories. - Cover the loadDrinks null-repository guard, which Codecov correctly flagged as the untested part of the previous commit. Repository: - Report each unknown status_text once per session. getDrinks runs on every cold start, festival switch, staleness refresh and pull-to- refresh, so one new phrase from the organisers logged a Crashlytics error from every user roughly hourly for the whole festival. UI: - Replace Colors.grey / Colors.red with theme colours. The fixed grey is 2.7:1 against a light surface, below the 4.5:1 WCAG AA requires for body text (it only passed in dark mode). Docs: - Fix a broken markdown table row introduced in the previous commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MoEMyawRmvhA2zuSDBmLdr

github-actions · 2026-07-25T11:11:57Z

🚀 Cloudflare Pages Preview

Your preview deployment is ready!

Preview URL: https://claude-codebase-review-0wpcq.staging-cambeerfestival.pages.dev

This preview will be automatically updated when you push new commits to this PR.

DrinkFilterService defined eight single-purpose filters and then re-implemented every one of those predicates inline in filterDrinks, so each rule existed twice: the copy with unit tests had no production caller, and the copy the app actually ran had none of its own. A fix to filterByAvailability could pass its test while changing nothing on screen. filterDrinks now composes them, leaving one copy of each rule. The service's 67 existing tests pass unchanged, which is the evidence the composition is behaviour-identical. about_screen carried a 77-line verbatim copy of ThemeSelectorSheet; it now uses the shared widget. Also adds a test for the drinks screen's full error view — one of the four documented loading/error signals, previously asserted only as "not showing" and never rendered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MoEMyawRmvhA2zuSDBmLdr

github-actions · 2026-07-25T11:19:07Z

🚀 Cloudflare Pages Preview

Your preview deployment is ready!

Preview URL: https://claude-codebase-review-0wpcq.staging-cambeerfestival.pages.dev

This preview will be automatically updated when you push new commits to this PR.

Copilot

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.

claude added 7 commits July 24, 2026 21:19

Copilot AI review requested due to automatic review settings July 24, 2026 21:40

Copilot started reviewing on behalf of richardthe3rd July 24, 2026 21:41 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

This was referenced Jul 25, 2026

Festival status badge is implemented twice with divergent labels #499

Closed

Dead widgets and helpers kept alive only by their own tests #500

Closed

richardthe3rd requested a review from Copilot July 25, 2026 21:46

Copilot started reviewing on behalf of richardthe3rd July 25, 2026 21:47 View session

Copilot AI reviewed Jul 25, 2026

View reviewed changes

richardthe3rd merged commit faba3e1 into main Jul 25, 2026
15 checks passed

richardthe3rd deleted the claude/codebase-review-0wpcql branch July 25, 2026 21:55

github-actions Bot mentioned this pull request Jul 25, 2026

Release 2026.8.0 #495

Open

Navigation Menu

fix: correctness fixes from a codebase review - #498

fix: correctness fixes from a codebase review#498
richardthe3rd merged 9 commits into
mainfrom
claude/codebase-review-0wpcql

richardthe3rd commented Jul 24, 2026

Uh oh!

codecov Bot commented Jul 24, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

richardthe3rd Jul 25, 2026

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jul 24, 2026

Uh oh!

github-actions Bot commented Jul 25, 2026

Uh oh!

github-actions Bot commented Jul 25, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

richardthe3rd commented Jul 24, 2026

Fixes

Testing

Not done here

Uh oh!

codecov Bot commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

richardthe3rd Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jul 24, 2026

🚀 Cloudflare Pages Preview

Uh oh!

github-actions Bot commented Jul 25, 2026

🚀 Cloudflare Pages Preview

Uh oh!

github-actions Bot commented Jul 25, 2026

🚀 Cloudflare Pages Preview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Jul 24, 2026 •

edited

Loading