refactor: clear five low-priority backlog issues - #539
Merged
Conversation
…alue 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
…ecked 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
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
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
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses five low-priority, self-contained backlog issues by tightening model/value semantics, reducing duplication, and adopting more robust Flutter/Dart idioms. The only intentional user-visible behavior change is fixing Festival.formattedDates for festivals that span New Year so the end date carries the correct year.
Changes:
- Refactors
Festival.formattedDatesto useintlDateFormatand fixes the year on the end date for year-spanning ranges; adds tests for the New Year case. - Adds
Festivalequality/hashCode semantics keyed onid(with an empty-id identity fallback) and verifies behavior via tests. - Improves safety/consistency: converts
AvailabilityStatusswitches to exhaustiveness-checked switch expressions, extractsStringFormattingHelper.drinkCountLabel, and swapsMediaQuery.of(...).sizetoMediaQuery.sizeOf(...)in bottom sheets.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/string_formatting_helper_test.dart | Adds unit coverage for StringFormattingHelper.drinkCountLabel pluralization cases. |
| test/models_test.dart | Adds coverage for year-spanning formattedDates and Festival equality/hashCode semantics (including empty-id fallback). |
| lib/widgets/festival_menu_sheets.dart | Uses MediaQuery.sizeOf(context) to depend only on size when computing sheet max height. |
| lib/widgets/festival_header.dart | Routes drink-count label through StringFormattingHelper.drinkCountLabel (and imports utils barrel). |
| lib/widgets/drink_filter_sheets.dart | Uses MediaQuery.sizeOf(context) for sheet sizing and centralizes drink-count semantics labels via drinkCountLabel. |
| lib/widgets/drink_card.dart | Converts AvailabilityStatus handling to switch expressions without wildcard arms for compile-time exhaustiveness checking. |
| lib/utils/string_formatting_helper.dart | Introduces drinkCountLabel(int) helper to avoid repeated inline pluralization logic. |
| lib/screens/my_festival_screen.dart | Routes section-header semantics label through StringFormattingHelper.drinkCountLabel. |
| lib/models/festival.dart | Uses intl for date formatting, fixes New Year range year display, and adds ==/hashCode keyed on id. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
🚀 Cloudflare Pages PreviewYour preview deployment is ready! Preview URL: https://claude-open-issues-backlog-u.staging-cambeerfestival.pages.dev This preview will be automatically updated when you push new commits to this PR. |
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.
Five self-contained backlog issues, one commit each (except the two
festival.dartones, which share a file and a test file). No user-visible behaviour change except the New Year date fix noted below.Festival.formattedDatesusesintl'sDateFormatinstead of a hand-rolled month tableFestivalgets==/hashCodekeyed onid, matchingDrink/Product/ProducerAvailabilityStatusswitches indrink_card.dartbecome switch expressionsStringFormattingHelper.drinkCountLabelextracted; all four call sites routed through itMediaQuery.sizeOf(context).heightTwo things worth a reviewer's attention
#532 fixes a latent bug the issue raised as a decision. The cross-month branch printed
start.yearon both ends, so a festival spanning New Year reported the wrong year on its end date. The end date now carries its own year. Output is byte-identical for every existing case, including same-year cross-month (May 28 - Jun 2, 2025); the only changed output is the year-spanning case, which has a new test.#534's "done when" was verified, not assumed. Added a seventh
AvailabilityStatusvalue, confirmednon_exhaustive_switch_expressionfires at bothlib/widgets/drink_card.dart:165and:270, then reverted. Neither switch has a wildcard arm — that is the point of the change, so please don't add one to silence a future analyzer error.Notes
Produceruses. Its test builds instances viafromJsonrather thanconstliterals — aconstpair is canonicalised to the same instance and cannot distinguish identity equality from id equality.viewInsetsOffor keyboard-aware padding. That is a separate judgement and is not in this PR.Verification
./bin/mise run checkpasses on the committed state — 1320 tests, analyzer clean, formatters clean. No goldens changed (no visual change). Manual browser/device testing not performed.Fixes #529
Fixes #531
Fixes #532
Fixes #534
Fixes #513
Generated by Claude Code