Modernize Flutter app: state management, new features, CI

.agents/tasks/task-modernize-flutter-app/2025-01-15-120000-review.md

@@ -0,0 +1,126 @@
+# Provider refactor, favorites/history/map features, and CI pipeline
+
+This change modernizes a monolithic Flutter app by extracting state management into Provider-based ChangeNotifiers, then adds three new user-facing features (favorites bookmarking, search history with replay, 2D map visualization) and a CI workflow. The original 552-line main.dart collapses to 49 lines of provider wiring; the dynamic cast and listener-removal anti-patterns from the old code are eliminated. The approach is conservative: existing widget APIs are preserved (SearchTab still receives props, not providers directly), limiting blast radius while establishing the new pattern.
+
+**Watch for:** SearchState is instantiated locally in OreFinderScreen yet also injected into the widget tree via `ChangeNotifierProvider.value` -- this dual-ownership pattern risks dispose-order issues if the widget tree accesses the provider during teardown (confirmed). The `flutter-ci.yml` workflow uses `--fatal-warnings` but the project has 2 pre-existing warnings, so CI will fail on every run (confirmed). Non-English l10n ARB files use English placeholder text for all 10 new keys (confirmed).
+
+## High-level view
+
+The state management extraction splits concerns cleanly: AppSettings owns theme/locale, SearchState owns form fields and search execution, FavoritesProvider and SearchHistoryProvider each own their persistence domain. The listener-removal fix uses named method references rather than anonymous closures. However, SearchState is owned by a StatefulWidget that also manually provides it to the tree, creating a lifecycle coupling that wouldn't exist if it were created at the MultiProvider level in main.dart.
+
+Favorites and search history both persist to SharedPreferences with JSON serialization, cap their storage (100 and 20 entries respectively), and expose unmodifiable lists. The map visualization uses CustomPainter with InteractiveViewer for zoom/pan, but tap-target detection doesn't account for the InteractiveViewer transform matrix, so tooltips break after zoom/pan.
+
+The CI workflow is correctly scoped to flutter_app paths but will fail immediately because `flutter analyze --fatal-warnings` treats the 2 pre-existing lint warnings as fatal. The test suite adds 54 focused tests for providers and models, though the widget tests exercise rendering without verifying behavioral interactions (tap-to-replay, swipe-to-delete).
+
+<details>
+<summary>Issues (6)</summary>
+
+1. **SearchState dual ownership** — SearchState is created in `_OreFinderScreenState.initState()` and manually disposed there, but also provided to the tree via `ChangeNotifierProvider.value`. Move SearchState creation into the MultiProvider in main.dart to avoid dispose-order races.
+2. **CI will always fail** — `flutter analyze --fatal-warnings` exits non-zero due to 2 pre-existing warnings. Either fix the warnings (unused `_triangularFactor` and `iterations`) or drop `--fatal-warnings` in favor of `--fatal-infos` or bare `flutter analyze`.
+3. **L10n keys not translated** — All 10 new ARB keys in de/es/ja/fr use English text. Users on non-English locales will see English for favorites/history/map labels.
+4. **FavoriteLocation.x/y/z force-unwrap nullable fields** — The getters (`oreLocation!.x`) will throw if the model is constructed or deserialized with a mismatched type/null location. Add a guard or make the fields non-nullable per type.
+5. **Tooltip position doesn't account for InteractiveViewer transform** — `_handleTap` uses `details.localPosition` from the pre-transform coordinate space, but clamps the tooltip to fixed (0, 200) bounds. After zooming/panning, tap targets and tooltip placement will misalign.
+6. **Locale-reactivity test fragility** — The test now expects "Favorites" in all locales because the ARB files weren't translated. This passes today but will break as soon as proper translations are added if the test expectations aren't updated to match.
+
+</details>
+
+<details>
+<summary>Details</summary>
+
+## SearchState ownership and lifecycle
+
+OreFinderScreen creates `_searchState = SearchState()` in `initState()`, then wraps its subtree with `ChangeNotifierProvider<SearchState>.value(value: _searchState, ...)`. This means the provider in the widget tree is backed by a value that's disposed in `_OreFinderScreenState.dispose()`. If any descendant widget holds a reference to the provider (via `context.read<SearchState>()`) and accesses it during the dispose phase -- for example, a `Dismissible.onDismissed` callback firing as the tree is torn down -- it hits a disposed ChangeNotifier.
+
+The pattern also means SearchState lives outside the MultiProvider in main.dart, unlike FavoritesProvider and SearchHistoryProvider. This asymmetry makes the provider graph harder to reason about: three providers are app-scoped, one is screen-scoped, and a consumer widget that depends on both SearchState and FavoritesProvider (like the bookmark buttons) relies on two different lifecycle boundaries.
+
+Moving SearchState creation into the MultiProvider (or into its own `ChangeNotifierProvider(create: ...)` wrapping OreFinderScreen) would unify the pattern and let Provider handle disposal.
+
+## CI workflow will fail on existing warnings
+
+```yaml
+- name: Analyze code
+  working-directory: flutter_app
+  run: flutter analyze --fatal-warnings
+```
+
+The project baseline documents 2 warnings: an unused `_triangularFactor` in `noise.dart` and an unused `iterations` variable in a test file. Both exist on `origin/main`. The `--fatal-warnings` flag converts these into exit-code-1, so the CI job will fail on every push/PR regardless of the new code's quality.
+
+## Non-English localization placeholders
+
+The ARB files for de, es, fr, ja received the 10 new keys but with English text as values:
+
+```json
+"favoritesTab": "Favorites",
+"favoritesEmpty": "No favorites yet",
+```
+
+The locale-reactivity test now asserts "Favorites" appears in every locale, which will break when actual translations are provided unless the test expectations are updated simultaneously.
+
+## FavoriteLocation model force-unwraps
+
+```dart
+int get x => type == FavoriteType.ore ? oreLocation!.x : structureLocation!.x;
+```
+
+The type check guards the unwrap at runtime, but nothing prevents constructing a `FavoriteLocation(type: FavoriteType.ore, oreLocation: null, ...)`. If `fromJson` receives `"type": "ore"` with a null or malformed `oreLocation` field, the type check passes but `oreLocation!` throws. A factory that validates invariants (or sealed-class pattern with separate OreBookmark / StructureBookmark subtypes) would eliminate this class of defect.
+
+## Map tooltip and tap targeting under zoom
+
+The `_handleTap` method recalculates dot positions from raw coordinates and compares against `details.localPosition`. The GestureDetector is a child of InteractiveViewer, which applies a transform matrix during zoom/pan. When the user zooms in and then taps, `localPosition` is in the transformed coordinate space, but the dot-position calculation uses the untransformed canvas coordinates. Tap detection will only work correctly at 1x zoom with no pan offset.
+
+The tooltip is positioned via:
+```dart
+left: _tooltipPosition!.dx.clamp(0, 200),
+top: _tooltipPosition!.dy.clamp(0, 200),
+```
+
+The clamping to 200 pixels is arbitrary and doesn't relate to the widget's actual size, so tooltips cluster in the top-left corner on larger screens.
+
+## Test coverage gaps
+
+What's not tested:
+
+- The `findOres` method on SearchState (the core search orchestration with validation, loading state transitions, and error paths)
+- SearchHistoryWidget's replay behavior (tapping "Replay" populates SearchState)
+- FavoritesTab's swipe-to-delete interaction
+- Bookmark toggle in ResultsTab
+- Map tooltip appearance on tap
+- The integration between OreFinderScreen._findOres and SearchHistoryProvider.addEntry
+
+</details>
+
+<details>
+<summary>File map</summary>
+
+| File | Change |
+|------|--------|
+| `flutter_app/lib/main.dart` | Collapsed from 552 to 49 lines; MultiProvider with AppSettings, FavoritesProvider, SearchHistoryProvider |
+| `flutter_app/lib/providers/search_state.dart` | New ChangeNotifier holding all search form state, controllers, and search execution logic |
+| `flutter_app/lib/providers/app_settings.dart` | New ChangeNotifier for theme toggle and locale with persistence |
+| `flutter_app/lib/providers/favorites_provider.dart` | New ChangeNotifier managing bookmarked locations with SharedPreferences persistence |
+| `flutter_app/lib/providers/search_history_provider.dart` | New ChangeNotifier recording last 20 searches |
+| `flutter_app/lib/models/favorite_location.dart` | New model wrapping ore/structure with metadata, JSON serialization |
+| `flutter_app/lib/models/search_history_entry.dart` | New model for past search parameters, JSON serialization |
+| `flutter_app/lib/widgets/ore_finder_screen.dart` | Extracted from main.dart; owns SearchState and TabController |
+| `flutter_app/lib/widgets/favorites_tab.dart` | New tab showing bookmarked locations grouped by seed |
+| `flutter_app/lib/widgets/search_history_widget.dart` | New collapsible section with replay buttons |
+| `flutter_app/lib/widgets/results_map_view.dart` | New 2D map with CustomPainter, InteractiveViewer, tap tooltips |
+| `flutter_app/lib/widgets/results_tab.dart` | Added map toggle, bookmark buttons for ore and structure cards |
+| `flutter_app/lib/widgets/search_tab.dart` | Converted to StatelessWidget, added SearchHistoryWidget |
+| `flutter_app/lib/widgets/world_settings_card.dart` | Converted to StatelessWidget, removed dynamic cast chain |
+| `flutter_app/lib/widgets/recent_seeds_widget.dart` | Listens to SearchState.recentSeedsRefreshNotifier instead of relying on dynamic cast |
+| `flutter_app/lib/l10n/app_*.arb` | Added 10 new l10n keys (English-only translations for non-en locales) |
+| `flutter_app/lib/l10n/app_localizations*.dart` | Generated localization classes with new getters |
+| `flutter_app/pubspec.yaml` | Added `provider: ^6.1.1` dependency |
+| `.github/workflows/flutter-ci.yml` | New CI workflow: analyze + test on push/PR to main |
+| `flutter_app/test/providers/search_state_test.dart` | 10 tests for SearchState setters and initial values |
+| `flutter_app/test/providers/favorites_provider_test.dart` | 13 tests for add/remove/limit/persistence |
+| `flutter_app/test/providers/search_history_provider_test.dart` | 9 tests for add/limit/clear/persistence |
+| `flutter_app/test/models/favorite_location_test.dart` | 9 tests for JSON roundtrip and accessors |
+| `flutter_app/test/models/search_history_entry_test.dart` | 6 tests for JSON roundtrip and edge cases |
+| `flutter_app/test/widgets/results_map_view_test.dart` | 7 widget tests for map rendering and legend |
+| `flutter_app/test/ui_locale_reactivity_test.dart` | Updated tab label expectations for new tab order |
+
+[Full diff: `git diff origin/main`]
+
+</details>

.agents/tasks/task-modernize-flutter-app/context.json

@@ -0,0 +1,44 @@
+{
+  "project_type": "Flutter mobile/web app (Minecraft Ore & Structure Finder) with AWS CDK infrastructure",
+  "language": "Dart (Flutter) + TypeScript (CDK/CLI)",
+  "build_system": "Flutter CLI for app, npm + TypeScript for infrastructure. Monorepo structure.",
+  "test_framework": "flutter_test with widget tests, property tests, integration tests. Jest for infrastructure.",
+  "build_command": "cd flutter_app && flutter pub get && flutter analyze",
+  "test_command": "cd flutter_app && flutter test",
+  "verification_instructions": "1. Run flutter pub get in flutter_app/. 2. Run flutter analyze --fatal-warnings. 3. Run flutter test. 4. Verify no regressions in existing tests.",
+  "snapshot_or_generated_files": "flutter_app/lib/l10n/app_localizations*.dart are generated from ARB files (flutter gen-l10n). Golden screenshots exist in test/ but are not checked in.",
+  "setup_instructions": "1. cd flutter_app && flutter pub get. 2. flutter analyze. 3. flutter test.",
+  "environment_constraints": "Full internet access available. Flutter SDK needed. No Docker daemon required for Flutter tests.",
+  "contribution_requirements": "The project uses flutter_lints for analysis. CI runs flutter analyze --fatal-warnings and flutter test. All widgets pass isDarkMode for theme-aware rendering.",
+  "key_patterns": "1. All state in StatefulWidgets (no state management library). 2. GamerCard/GamerSectionHeader reusable themed widgets. 3. PreferencesService uses static methods with SharedPreferences. 4. Listener removal anti-pattern in dispose() (creates new closures). 5. Dynamic casting for cross-widget communication. 6. Models use factory constructors with fromJson/toJson. 7. All UI strings use AppLocalizations (l10n). 8. Edition-aware RNG via GameRandom abstract class with Java/Bedrock implementations.",
+  "relevant_files": [
+    "flutter_app/lib/main.dart",
+    "flutter_app/lib/models/ore_finder.dart",
+    "flutter_app/lib/models/game_random.dart",
+    "flutter_app/lib/models/java_random.dart",
+    "flutter_app/lib/models/bedrock_random.dart",
+    "flutter_app/lib/models/structure_finder.dart",
+    "flutter_app/lib/models/ore_location.dart",
+    "flutter_app/lib/models/structure_location.dart",
+    "flutter_app/lib/models/search_result.dart",
+    "flutter_app/lib/models/noise.dart",
+    "flutter_app/lib/models/legacy_density_function.dart",
+    "flutter_app/lib/theme/gamer_theme.dart",
+    "flutter_app/lib/utils/preferences_service.dart",
+    "flutter_app/lib/utils/ore_utils.dart",
+    "flutter_app/lib/utils/structure_utils.dart",
+    "flutter_app/lib/widgets/search_tab.dart",
+    "flutter_app/lib/widgets/results_tab.dart",
+    "flutter_app/lib/widgets/world_settings_card.dart",
+    "flutter_app/lib/widgets/recent_seeds_widget.dart",
+    "flutter_app/lib/widgets/search_buttons.dart",
+    "flutter_app/lib/widgets/edition_version_card.dart",
+    "flutter_app/lib/widgets/guide_tab.dart",
+    "flutter_app/lib/widgets/bedwars_guide_tab.dart",
+    "flutter_app/pubspec.yaml",
+    "flutter_app/analysis_options.yaml",
+    "flutter_app/test/widget_test.dart",
+    "flutter_app/test/preferences_service_test.dart"
+  ],
+  "directory_structure": "flutter_app/lib/main.dart (entry + main screen state), flutter_app/lib/models/ (business logic: ore_finder, structure_finder, game_random, noise, density functions), flutter_app/lib/widgets/ (UI components: search_tab, results_tab, guide_tab, etc.), flutter_app/lib/theme/ (gamer_theme.dart), flutter_app/lib/utils/ (preferences_service, ore_utils, structure_utils), flutter_app/lib/l10n/ (generated localizations), flutter_app/test/ (22 test files), infrastructure/ (AWS CDK TypeScript)"
+}

.agents/tasks/task-modernize-flutter-app/features/FEAT-001.json

@@ -0,0 +1,22 @@
+{
+  "id": "FEAT-001",
+  "type": "chore",
+  "description": "Environment setup and baseline verification - Install Flutter dependencies, run analysis, and confirm all existing tests pass before making changes.",
+  "status": "completed",
+  "steps": [
+    "Run 'cd flutter_app && flutter pub get' to install dependencies",
+    "Run 'flutter analyze' in flutter_app/ to check current lint status",
+    "Run 'flutter test' in flutter_app/ to confirm existing tests pass",
+    "Document any existing failures or warnings as a baseline"
+  ],
+  "acceptance_criteria": [
+    "flutter pub get completes successfully",
+    "flutter analyze output documented (may have warnings but should not have errors)",
+    "flutter test passes or existing failures are documented as pre-existing"
+  ],
+  "verification": [
+    "Run 'cd /projects/sandbox/minecraft-finder/flutter_app && flutter pub get && flutter analyze && flutter test'"
+  ],
+  "blocked_reason": null,
+  "findings": "Environment: Flutter 3.44.2 (stable), Dart 3.12.2. Flutter SDK installed at /opt/flutter/bin. flutter pub get: completed successfully (22 packages have newer versions available but constraints are satisfied). flutter analyze: 2 warnings (no errors) - (1) unused_element: '_triangularFactor' in lib/models/noise.dart:103:8, (2) unused_local_variable: 'iterations' in test/ore_finder_legacy_range_property_test.dart:21:15. flutter test: All 209 tests passed (0 failures). Baseline is clean with only 2 pre-existing lint warnings."
+}