refactor: enforce the declared lint rules and break the main/router import cycle - #540
Merged
Merged
Conversation
`mise-tasks/analyze.sh` ran `flutter analyze --no-fatal-infos`, so only the seven rules promoted to `warning` in `analysis_options.yaml` could fail. Everything else — including `prefer_const_constructors` and `prefer_single_quotes`, which AGENTS.md advertised as enforced — was advisory, and 9 live violations had already accumulated. Drop the flag so every declared rule is fatal, and turn on the three strict analyzer modes plus four lint rules the code already satisfied, so that property is held rather than merely observed: - `strict-inference`, `strict-raw-types` (alongside existing `strict-casts`) - `avoid_dynamic_calls`, `unawaited_futures`, `use_super_parameters`, `prefer_relative_imports` Fixing the resulting 50 findings is mechanical and behaviour-preserving: explicit type arguments on collection literals and `PopupMenuButton`, `Future<void>.delayed`, null-aware map elements, `unawaited(...)` on fire-and-forget futures, and `const` where it was already implied. Note the issue measured only `lib/` — all 50 remaining findings were in `test/`, and `lib/` needed just the six one-line fixes it predicted. Fixes #524
Both workflows invoke `flutter analyze --no-fatal-infos` directly rather than going through `./bin/mise run analyze`, so without this the previous commit only tightens the local gate and CI stays advisory — which is the regression #524 is about. `.github/workflows/` is on the AGENTS.md Do-Not-Modify list, so this is kept as a separate commit: it can be dropped on its own if the maintainer would rather have the workflows call the mise task instead. Refs #524
router.dart imported main.dart for ProviderInitializer and BeerFestivalHome while main.dart imported router.dart for appRouter, so main.dart was not an entry point but a widget library that happened to contain main(), and neither file could be read or tested without the other. Move both widgets to lib/widgets/ and export them from the barrel. They are copied verbatim — the only edit is one comment pointer. globalRoutes moves from router.dart to utils/navigation_helpers.dart rather than staying put as the issue suggested: the redirect handler that reads it now lives under lib/widgets/, and router.dart imports the widgets barrel, so leaving it in router.dart would simply re-form the cycle one file over. navigation_helpers.dart has no project imports of its own and already owns the route-path builders, so it is the natural cycle-free home. main.dart is now main() + isTransientFontLoadError + BeerFestivalApp, 104 lines instead of 440. Nothing under lib/ imports it any more. Removing the moved code left six dead imports in main.dart, which the newly-fatal analyzer from #524 caught immediately. Follow-up not done here, to keep this a pure move: the BeerFestivalHome and ProviderInitializer tests still live in test/main_test.dart rather than mirroring the new lib/widgets/ layout. Fixes #527
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses two architecture/linting issues: it makes the analyzer enforce the lint rules declared in analysis_options.yaml (including strict inference/raw types), and it breaks the main.dart ↔ router.dart import cycle by moving app-shell widgets into lib/widgets/.
Changes:
- Tightens analysis in local tasks and CI by removing
--no-fatal-infosand enabling stricter analyzer/linter rules; applies mechanical fixes acrosslib/andtest/to comply. - Breaks the
main.dart/router.dartcycle by extractingProviderInitializerandBeerFestivalHomeinto dedicated widget files and updating exports/imports accordingly. - Centralizes “global (non-festival-scoped) routes” into
navigation_helpers.dartto keep redirects cycle-free.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
analysis_options.yaml |
Enables strict inference/raw types and additional lints (async correctness, type safety, relative imports). |
mise-tasks/analyze.sh |
Removes --no-fatal-infos so infos become build-failing (matches declared lint policy). |
.github/workflows/ci.yml |
Aligns CI analysis step with stricter analyzer behavior (drops --no-fatal-infos). |
.github/workflows/release-web.yml |
Aligns release workflow analysis step with stricter analyzer behavior (drops --no-fatal-infos). |
AGENTS.md |
Updates documentation to match the now-enforced analyzer/lint contract. |
.claude/skills/build-and-env/SKILL.md |
Updates tooling docs to reflect flutter analyze without --no-fatal-infos. |
.claude/skills/diagnostics-and-tooling/SKILL.md |
Updates diagnostics guidance for fatal infos behavior. |
.claude/skills/validation-and-qa/SKILL.md |
Updates QA gate documentation for analyzer behavior. |
lib/main.dart |
Removes app-shell widgets so main.dart is back to being primarily the entry point / app root. |
lib/router.dart |
Stops importing main.dart; uses widgets barrel instead and updates redirect comment pointer. |
lib/widgets/widgets.dart |
Exports the newly extracted BeerFestivalHome and ProviderInitializer. |
lib/widgets/provider_initializer.dart |
New home for provider initialization + post-init redirect behavior (cycle break). |
lib/widgets/beer_festival_home.dart |
New home for the shell scaffold/navigation + exit-confirm behavior (cycle break). |
lib/utils/navigation_helpers.dart |
Hosts globalRoutes constant alongside path builder helpers to avoid reintroducing cycles. |
lib/screens/about_screen.dart |
Adds explicit showModalBottomSheet<void> for strict inference compliance. |
lib/screens/drink_detail_screen.dart |
Wraps fire-and-forget animation future in unawaited(...) for async lint compliance. |
lib/widgets/drink_filter_sheets.dart |
Adds explicit showModalBottomSheet<void> for strict inference compliance. |
lib/widgets/festival_menu_sheets.dart |
Adds explicit showModalBottomSheet<void> for strict inference compliance. |
test/main_test.dart |
Updates imports to reference widgets moved out of main.dart. |
test/router_test.dart |
Marks a non-awaited navigation future as unawaited(...) per async lint. |
test/widgets/overflow_menu_test.dart |
Adds explicit generic type to PopupMenuButton under strict-raw-types. |
test/widgets/your_take_card_test.dart |
Updates deprecated semantics API usage (hasFlag → flagsCollection). |
test/widgets/festival_menu_sheets_test.dart |
Mechanical const/unawaited changes to satisfy newly-enforced lints. |
test/widgets/drink_card_test.dart |
Updates collection literal types for strict inference (and touches optional JSON field building). |
test/models_test.dart |
Adds explicit map/list literal type args for strict inference. |
test/drinks_screen_style_filter_test.dart |
Uses unawaited(...) for intentionally fire-and-forget provider load. |
test/drinks_screen_scroll_position_test.dart |
Adds explicit list literal type args for strict inference. |
test/domain/services/search_match_service_test.dart |
Makes a long test string const per const lint. |
test/domain/services/drink_sort_service_test.dart |
Adds explicit list literal type args for strict inference. |
test/domain/services/drink_filter_service_test.dart |
Adds explicit map/list literal type args for strict inference. |
test/domain/controllers/user_drink_state_controller_test.dart |
Adds explicit list literal type args for strict inference. |
test/domain/controllers/drink_filter_controller_test.dart |
Adds explicit list literal type args for strict inference (and touches optional JSON field building). |
test/beer_provider_test.dart |
Adds explicit map/list literal type args and Future<void>.delayed for strict inference. |
test/beer_api_service_test.dart |
Adds explicit list literal type args in JSON test data for strict inference. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
670
to
674
| 'style': 'Mild', | ||
| 'dispense': 'cask', | ||
| 'abv': '3.4', | ||
| if (description != null) 'notes': description, | ||
| 'notes': ?description, | ||
| }), |
Owner
Author
There was a problem hiding this comment.
Skipping — this compiles. 'notes': ?description is a null-aware map entry, stable Dart syntax since 3.9; this package is on sdk: '>=3.10.0 <4.0.0'. key: ?value omits the entry when the value is null, which is exactly the previous if (description != null) 'notes': description behaviour.
CI is ground truth here and it's green on this commit — analyze and all 1320 tests pass.
Reverting would also break the build rather than fix it: this change was required by the use_null_aware_elements lint, which flagged the old collection-if form. Since this PR drops --no-fatal-infos, that lint is now fatal, so restoring the conditional entry would fail the analyze job.
Generated by Claude Code
Comment on lines
38
to
+42
| 'category': category, | ||
| if (style != null) 'style': style, | ||
| 'style': ?style, | ||
| 'dispense': 'cask', | ||
| 'abv': abv, | ||
| if (notes != null) 'notes': notes, | ||
| 'notes': ?notes, |
Owner
Author
There was a problem hiding this comment.
Skipping, same reason as the equivalent comment on drink_card_test.dart: 'style': ?style is a null-aware map entry, valid since Dart 3.9 (this package requires >=3.10.0), and it is what the now-fatal use_null_aware_elements lint asked for. analyze and the full test suite are green on this commit.
Generated by Claude Code
Comment on lines
+95
to
+99
| // Check if we're on root path - redirect to festival home | ||
| if (currentPath == '/') { | ||
| router.go('/${provider.currentFestival.id}'); | ||
| return; | ||
| } |
Comment on lines
+134
to
+138
| final queryString = currentUri.query.isNotEmpty | ||
| ? '?${currentUri.query}' | ||
| : ''; | ||
| router.go('/${provider.currentFestival.id}$restOfPath$queryString'); | ||
| } |
Comment on lines
+8
to
10
| flutter analyze "$@" 2>&1 | | ||
| grep -v -E "Woah! You appear|superuser privileges" | | ||
| tee "$ANALYZE_LOG" |
Three of the five review comments were correct:
- `provider_initializer.dart` built festival-root paths by interpolating
`'/${provider.currentFestival.id}'` directly. AGENTS.md requires the typed
helpers, so both sites now go through `buildFestivalHome()`. Output is
identical; the helper adds a non-empty assert.
- The `analyze` grep hint still suggested `error\|warning`, which now hides
the info-severity findings that fail the task. Widened to
`grep -nE 'info|warning|error'`.
This means the moved widgets are no longer byte-identical to the originals
as claimed in the PR description; the two `router.go` call sites are the
only difference.
The other two comments claimed `'key': ?value` is invalid Dart. It is a
null-aware map entry, stable since 3.9 (this package requires >=3.10.0), and
is what the now-fatal `use_null_aware_elements` lint asked for — reverting
would fail `analyze`. Refuted on the threads.
Refs #524, #527
Contributor
🚀 Cloudflare Pages PreviewYour preview deployment is ready! Preview URL: https://claude-issue-selection-d1tzs.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.
Two independent issues from the 2026-08-09 architecture-review batch, both chosen for having concrete file:line specs and objective verification.
Fixes #524
Fixes #527
#524 — Analyzer does not enforce the lint rules the project claims to enforce
mise-tasks/analyze.shranflutter analyze --no-fatal-infos, so only the seven rules promoted towarningin theerrors:block could ever fail.prefer_const_constructors,prefer_single_quotes,prefer_final_localsand friends were advertised in AGENTS.md as enforced but were advisory, and 9 violations had already drifted in.--no-fatal-infosso every declared rule is fatal at any severity.strict-inferenceandstrict-raw-typesalongside the existingstrict-casts.avoid_dynamic_calls,unawaited_futures,use_super_parameters,prefer_relative_imports— all four were already satisfied, so this holds a property the code has rather than asking for new work.Correction to the issue's estimate
The issue measured by running
analyze lib/only, and predicted 6 findings.lib/did need exactly those 6 — buttest/contributed 44 more that the measurement never saw. 50 total, all mechanical and behaviour-preserving:<Map<String, dynamic>>[],<String, int>{})unawaited(...)on fire-and-forget futuresFuture<void>.delayedconstremovedPopupMenuButton→PopupMenuButton<String>Plus the
lib/six: fiveshowModalBottomSheet<void>and oneunawaited(_pulseController.forward(...)).CI gate — separate commit, easy to drop
Both
ci.ymlandrelease-web.ymlinvokeflutter analyze --no-fatal-infosdirectly rather than going through./bin/mise run analyze, so without touching them this only tightens the local gate and CI stays advisory — which is the regression the issue is about..github/workflows/is on the AGENTS.md Do-Not-Modify list, so that change is isolated in its own commit (ci: drop --no-fatal-infos from the analyze steps). Drop it on its own if you would rather have the workflows call the mise task instead.analysis_options.yamlis likewise marked as needing a deliberate maintainer decision. I treated the issue — which specifies the exact rules to add — as that decision. Happy to route it through an ADR instead.#527 — main.dart and router.dart import each other
router.dartimportedmain.dartforProviderInitializerandBeerFestivalHome;main.dartimportedrouter.dartforappRouter. The effect was thatmain.dartwas not an entry point but a widget library that happened to containmain().ProviderInitializer→lib/widgets/provider_initializer.dartBeerFestivalHome(and the exit-confirmation state) →lib/widgets/beer_festival_home.dartlib/widgets/widgets.dart;router.dartimports the barrel.main.dartis nowmain()+isTransientFontLoadError+BeerFestivalApp, 104 lines instead of 440, and nothing underlib/imports it any more.The move was verified with
diffagainst the originals.BeerFestivalHomeis byte-identical.ProviderInitializerdiffers in exactly three lines: one comment pointer, and the tworouter.gocall sites changed in review (see below).One deviation from the issue's plan
The issue says
globalRoutes"is already shared in the correct direction and stays put". It cannot stay put: its only consumer (_handlePostInitRedirect) has moved underlib/widgets/, androuter.dartimports the widgets barrel — so leaving the constant inrouter.dartre-forms the same cycle one file over.It moved to
lib/utils/navigation_helpers.dart, which has no project imports of its own and already owns the route-path builders, making it a cycle-free home for route-path data.Removing the moved code left six dead imports in
main.dart— caught immediately by the newly-fatal analyzer from #524.Follow-up deliberately not done
The
BeerFestivalHomeandProviderInitializertests still live intest/main_test.dartrather than mirroring the newlib/widgets/layout. Splitting that file would have turned a pure move into a real diff; happy to do it separately.Review round 1
Three of the five Copilot comments were correct and are fixed in
8902acb:provider_initializer.dartnow build their path withbuildFestivalHome()instead of interpolating'/${provider.currentFestival.id}', per the AGENTS.md rule about typed path helpers. Same output; the helper adds a non-empty assert.analyzegrep hint suggestederror\|warning, which now hides the info-severity findings that fail the task. Widened togrep -nE 'info|warning|error'.Two comments claimed
'key': ?valueis invalid Dart that "will fail to compile". It is a null-aware map entry, stable since Dart 3.9 (this package requires>=3.10.0), and it is what the now-fataluse_null_aware_elementslint asked for — reverting would failanalyze. Refuted on the threads rather than actioned.Verification
./bin/mise run checkgreen: format clean,flutter analyzereports no issues with the stricter config and no--no-fatal-infos, all 1320 tests pass. CI agreed on the first push — notablyanalyze, which is the real test of #524.Festival freeze checked — cbf2026 ended 2026-05-23 and nothing is upcoming, so no freeze applies.
Not verifiable by an agent: no manual browser or device pass on the
main.dartsplit. It is a near-verbatim widget move with tests covering both widgets, but the app was not launched.