refactor(quality): code analysis cleanup - #322
Merged
Merged
Conversation
Addresses every finding in docs/code-analysis/2026-07-24-analysis.md. The duplication went first: one `sheetStepper` replaced ten private copies across the sheets, a `_PersistedFlag` base replaced ten hand-rolled one-key SharedPreferences notifiers, and one `_pickBytes` replaced five copies of the FilePicker preamble whose error handling had already drifted apart. All 46 storage keys were diffed against HEAD to prove none moved. The substantive work was the sheet rolls. Eleven sheets called a bare `Random()` in the widget layer with the pass/fail banding written inline, so none of it could be asserted — an Argosa roll-under or a DCC spell check could have been wrong in either direction and no test would have noticed. The banding now lives in a pure `sheet_rolls.dart` and the dice come from an injectable `diceProvider`, which buys 34 tests where there were none. Collapsing the HP ladder found a live bug the report had not: `encounter_screen` carried its own copy of it, missing Embark and DCC, so a character on either sheet showed no HP at all in an encounter while the roster showed it fine. Both now read the one `characterHpPool`, and each sheet owns its own clamp through a `HasHpPool` contract. Two deliberate limits. `Character` keeps its sixteen nullable sheet fields — removing them touches hundreds of `c.dnd`-style call sites for nothing the player can see — so instead the failure mode the finding actually named is now guarded: `character_sheet_slots_test.dart` enumerates all fifteen slots for JSON round-trip, and was mutation-tested by deleting a `toJson` branch to confirm it fails with the intended "silently dropped on campaign export" message. And of the stricter lints, only the seven already at zero violations were enabled; `avoid_redundant_argument_values` (169) and `directives_ordering` (142) are churn, not quality, and analysis_options.yaml records why with counts so nobody re-litigates them. `models.dart` is split into ten `part` files behind a 33-line header — verified lossless by an identical 144-symbol set — which left the design-system test reading it as a single file; that now scans the parts. 2292 tests pass, 86.4% coverage, analyze clean at the tightened level. Co-authored-by: John Taylor <jttaylor63@gmail.com> 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
2 participants
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.
Summary
Addresses every finding in
docs/code-analysis/2026-07-24-analysis.md.sheetStepperreplaces 10 private copies; a_PersistedFlagbase replaces 10 one-key prefs notifiers; one_pickBytesreplaces 5 copies of the FilePicker preamble (whose error handling had already drifted). All 46 storage keys diffed against HEAD to prove none moved.Random()in the widget layer with banding inline, so none of it could be asserted. Banding moved to a pureengine/sheet_rolls.dart, dice come from an injectablediceProvider. +34 tests where there were zero.encounter_screencarried its own drifted copy of the HP ladder missing Embark and DCC, so those characters showed no HP in an encounter while the roster showed it fine. Both now use the sharedcharacterHpPool; each sheet owns its clamp via aHasHpPoolcontract.models.dart5,663 → 33 LOC + 10partfiles (one scope, so none of the ~90 import sites changed). Verified lossless: identical 144-symbol set..metadatarestored (it had lost its android/ios/web entries to a tool run).Two deliberate limits
Characterkeeps its 16 nullable sheet fields. Removing them touches hundreds ofc.dnd-style call sites for nothing the player can see. Instead the failure mode the finding named is now guarded:character_sheet_slots_test.dartenumerates all 15 slots for JSON round-trip. Mutation-tested — deleting atoJsonbranch fails it with the intended "silently dropped on campaign export" message.avoid_redundant_argument_values(169) anddirectives_ordering(142) are churn, not quality;analysis_options.yamlrecords the reasons with measured counts.Test plan
flutter analyzeclean (at the tightened lint level)flutter test— 2292 tests pass (was 2192)build()extraction🤖 Generated with Claude Code