diff --git a/.firebaserc b/.firebaserc new file mode 100644 index 0000000..c999c89 --- /dev/null +++ b/.firebaserc @@ -0,0 +1,5 @@ +{ + "projects": { + "default": "circle-60af7" + } +} diff --git a/docs/codebase_audit.md b/docs/codebase_audit.md new file mode 100644 index 0000000..bd08286 --- /dev/null +++ b/docs/codebase_audit.md @@ -0,0 +1,218 @@ +# Circle — Codebase Audit & Architecture Review + +**Date:** 2026-06-24 +**Reviewed at branch:** `code-audit` +**App version:** `0.1.5+12` +**Flutter:** 3.44.3 (stable) · Dart SDK constraint `^3.7.0-183.0.dev` +**Scope:** ~22,100 lines of hand-written Dart across 6 product features (`auth`, `meds`, `water`, `home`, `emergency`, `profile`) plus `health_connect`. + +--- + +## 1. Executive Summary + +Circle is a Flutter + Firebase + ObjectBox app for Sickle Cell disease management. The codebase shows a **strong, deliberate architectural intent** — a clean feature-first layout, a layered `Screen → Notifier → Repository → Service` flow, functional error handling with `Either`, and consistent documentation. Whoever set this up understood the patterns they were reaching for. + +However, the **execution diverges from the intent** in ways that matter. The dependency-injection story is undermined by global mutable singletons that defeat the very testability the layering was designed to enable; there is **effectively no automated test coverage** (a single 30-line stub) for an app that stores and processes health data; there is a large volume of dead/commented code and TODOs; and a few real correctness bugs exist in core model logic. For a health-domain application, the privacy posture (verbose logging of state, always-on Crashlytics, no Firestore security rules in the repo) needs attention before this is production-grade. + +### Overall Rating: **5.5 / 10** — "Promising architecture, gaps in execution and rigor" + +| Dimension | Score | One-line verdict | +|---|---|---| +| Architecture & layering | 7.0 | Sound, idiomatic structure; let down by DI execution | +| State management (Riverpod) | 5.0 | Works, but anti-patterns negate Riverpod's strengths | +| Error handling | 7.0 | Genuine strength; one broken mapper | +| Code quality & consistency | 5.0 | Heavy boilerplate, dead code, a few real bugs | +| Testing | 1.0 | Essentially absent — the biggest risk | +| Tooling & CI/CD | 6.5 | Good release automation; no test gate | +| Security & privacy | 4.0 | Health data + verbose logs + no rules in repo | +| Dependencies | 7.0 | Current and reasonable; a few pre-1.0 bets | +| Documentation | 7.0 | Strong inline docs and CLAUDE.md | + +The numbers are weighted toward the dimensions that matter most for a shipping health app (testing, state management, security). A codebase with this architecture and *real* tests would comfortably sit at 7.5–8. + +--- + +## 2. What's Good + +These are real strengths worth preserving through any refactor. + +### 2.1 Clean feature-first structure +The `lib/features//{models,services,repositories,providers,screens}` convention is applied consistently across every feature. A new engineer can open any feature and immediately know where things live. The barrel files (`auth.dart`, `meds.dart`, etc.) keep imports tidy. `core/` and `components/` separation is correct. + +### 2.2 Disciplined layering and separation of concerns +The `Screen → Notifier → Repository → Service` flow is followed almost everywhere: +- **Services** do raw I/O only (ObjectBox boxes, Firebase calls) and throw typed `AppException`s. +- **Repositories** wrap services, orchestrate local/remote, and convert exceptions into `Failure` via `futureHandler`. +- **Notifiers** hold `AsyncValue` state and expose intent-level methods to the UI. + +This is the right shape. The local-first design (ObjectBox as source of truth, Firebase as remote sync via `updateRemote`/`getFromRemote` flags) is a sensible choice for an app that must work offline. + +### 2.3 Functional error handling +`core/utils/method_handler.dart` (`futureHandler`/`methodHandler`) is the best-engineered part of the codebase. It centralizes try/catch, maps `FirebaseException` and `AppException` to typed `Failure`s, logs, and reports to Crashlytics — all in one place. Combined with `FutureEither = Future>` (fpdart), repositories stay clean and exceptions don't leak uncontrolled into the UI. This is a genuine asset. + +### 2.4 Error reporting & crash handling +`CrashlyticsService` correctly wires `FlutterError.onError` and `PlatformDispatcher.instance.onError`, and distinguishes fatal vs non-fatal. The plumbing through `futureHandler` means most caught errors are reported with context. + +### 2.5 Documentation culture +Models and key utilities carry thorough dartdoc with usage examples (`Medication`, `AuthRepository`, `futureHandler`). `CLAUDE.md` accurately describes the architecture. This is well above average for an app of this size. + +### 2.6 Release automation +The CI/CD (`.github/workflows/deploy.yml`) auto-increments the build number, tags, builds AAB/APK, and deploys to the Play internal track on `release/*` PRs. The "merge-not-rebase / don't touch the build number" conventions are documented. That's mature for a solo/small-team project. + +### 2.7 Theming discipline +A centralized Material 3 theme (`AppColors`, `AppTextStyles`, `AppConstants`) with a custom font and spring-based theme animation. The intent to avoid hardcoded colors/spacing is stated and largely followed. + +--- + +## 3. What Needs Improvement + +Ordered roughly by severity. + +### 3.1 🔴 Critical — Almost no test coverage +- `test/` contains **one file** (`widget_test.dart`, 30 lines) — the default Flutter counter stub, which doesn't even match this app. +- There are **zero** unit tests for repositories, notifiers, models, the `futureHandler` error mapper, or the ObjectBox type converters. +- `mocktail` is in `dev_dependencies` and the architecture was explicitly built for injectable mocks — but the capability is unused. + +**Why it matters:** This is a health app. Medication scheduling, dose tracking, streak calculation, and date-window queries are exactly the kind of logic that breaks silently. Without tests, every refactor (including the Riverpod 3 upgrade you're considering) is high-risk. This single gap is the largest drag on the score. + +### 3.2 🔴 High — Dependency injection is defeated by global singletons +Every notifier file declares **module-level mutable globals** for its service/repository graph and a hand-built provider instance, e.g. in `med_notifier.dart`: + +```dart +final MedService medService = MedService(); +final MedLocalService medLocalService = MedLocalService(store: database.store); +final MedRepository medRepository = MedRepositoryImpl(...); +final MedNotifierProvider medNotifierProviderImpl = + MedNotifierProvider(medRepository: medRepository); +``` + +The notifier then takes the repository as a `build({required MedRepository medRepository})` family parameter, and the UI watches the **pre-constructed global** `medNotifierProviderImpl`. This pattern appears in `auth`, `user`, `meds`, and both `water` notifiers. + +Problems: +- **It negates Riverpod's core value.** Provider overrides (`ProviderScope(overrides: ...)`) — the standard way to inject mocks in tests/previews — can't cleanly replace a graph that's already wired through top-level `final`s reading the global `database.store`. +- **`database.store` is touched at file load time** (`MedLocalService(store: database.store)`), creating a hidden initialization-order dependency on `main()` having run. This makes the notifiers essentially un-testable in isolation and fragile to import ordering. +- **The family parameter is misused as a DI seam** when it should be a normal Riverpod dependency (`ref.watch(repositoryProvider)`). Passing a repository as a family key also means Riverpod uses it in equality/caching, which is not the intent. + +**Fix direction:** introduce provider-based DI — `@riverpod MedRepository medRepository(ref) => ...` — and have notifiers read `ref.watch(medRepositoryProvider)` instead of a build parameter. Then tests/previews override the repository provider. This also removes the global singletons entirely. (This pairs naturally with the Riverpod 3 migration — see the companion plan.) + +### 3.3 🟠 Medium — Repetitive notifier boilerplate; no `AsyncValue.guard` +Every notifier method follows the same 20-line shape: `state = const AsyncValue.loading();` → `await repo.x()` → `response.fold((failure) { state = AsyncValue.error(...); log(...) }, (data) { state = AsyncValue.data(...); log(...) })`. There are ~25 near-identical blocks. + +- `AsyncValue.guard` (and the `Either` already in hand) could collapse most of this. +- Several methods set `AsyncValue.loading()` and then call another method that *also* sets loading and re-fetches (e.g. `putMedication` → `getMedications`), producing double loading transitions and an extra round trip. +- `state = AsyncValue.error(failure, failure.stackTrace!)` **force-unwraps** a nullable `StackTrace`. If any `Failure` is constructed without a stack trace, this throws inside the error handler. (`auth_notifier` already hedges with `?? StackTrace.current` in some methods but not others — inconsistent.) + +### 3.4 🟠 Medium — Real correctness bugs in core logic + +**`Medication.putActivityRecord` double-adds (`models/medication.dart`):** +```dart +if (activityExists) { + activityRecord.removeAt(index); + activityRecord.insert(index, newActivity); +} else { + activityRecord.add(newActivity); +} +activityRecord.add(newActivity); // <-- always runs, duplicates the record +``` +In both branches the record is added an extra time. Adherence/streak data derived from `activityRecord` will be wrong (duplicate entries per day). + +**`Failure.fromApi` assumes HTTP status codes (`error/failure.dart`):** it branches on `code.toString().startsWith('4'/'5'/...)`. But the codes flowing in are Firebase/ObjectBox codes (often non-numeric strings or null), not HTTP statuses. The mapping is effectively dead/incorrect for this app's actual error sources, and the method is marked `Todo: Properly implement`. + +**`getMedicationScheduledDoses` with `from == null && until != null`** calls `scheduleDosesList.first.date` without a guard — throws on an empty box. + +### 3.5 🟠 Medium — Privacy & security posture for a health app +- **Verbose state logging:** there are ~25 `log("Success ${state.value}")` calls plus 181 `log(...)` calls total. `state.value` for these notifiers is the full `AppUser`, medication list, water logs, etc. On Android these go to logcat in release unless stripped — i.e. **health/PII potentially written to device logs**. Logging should be gated behind `kDebugMode` and never dump full domain objects. +- **No Firestore security rules in the repo.** `firebase.json` configures only Flutter platform outputs; there is no `firestore.rules`/`firestore.indexes.json` and no Firestore deploy target. Rules may exist only in the console (unversioned, unreviewable) or may be permissive. For an app writing user health records this is a notable gap. +- **Crashlytics is always-on.** `CrashlyticsService.initialize()` enables collection unconditionally; the opt-in toggle is a `Todo`. Crash reports can contain PII; a health app generally needs consent. +- `google-services.json` and `GoogleService-Info.plist` are committed. This is *technically* acceptable (they're client config, not secrets) but should be a conscious decision; the GCP **service-account key and keystore** are correctly kept out of the repo and injected via CI. + +### 3.6 🟡 Low–Medium — Dead code, TODOs, and churn +- **~1,946 commented lines** and **71 TODO/FIXME** markers in hand-written Dart. `main.dart` carries a large commented-out `MaterialApp` block; `med_local_service.clearMedications` has a commented-out query block; notifiers carry long TODO checklists. +- This obscures the real code, inflates diffs, and makes it hard to tell intended behavior from abandoned experiments. Most of it belongs in git history, not the working tree. + +### 3.7 🟡 Low — Naming, typos, and small inconsistencies +- Filenames `lib/databse_service.dart` and `lib/crashytics_service.dart` are misspelled (the typo is even acknowledged in `CLAUDE.md`). These are referenced widely, so renaming is a small mechanical change worth doing. +- `authNotifierProviderIml` / `waterLogNotifierProviderIml` ("Iml") vs `…Impl` elsewhere — inconsistent. +- `med_notifier.dart` has an unused `forceRefresh` parameter on several methods that actually drives `updateRemote`, and `MedRepositoryImpl.updateMedication` is a public method not on the interface (dead/duplicated path with `putMedication`). +- `AuthNotifier.authStateChanges()` subscribes to a stream inside a method and returns the stream, but nothing disposes the subscription — a latent leak; this belongs in `build` with a `ref.onDispose`. + +### 3.8 🟡 Low — Lint configuration is minimal +`analysis_options.yaml` only layers a few `prefer_const_*` rules on top of `flutter_lints`. For a codebase with these issues, stricter lints would catch a lot automatically. `custom_lint` + `riverpod_lint` are installed but there's no `custom_lint` invocation in CI. Consider `flutter_lints` → a stricter base (or `very_good_analysis`), enabling `avoid_print`, `unawaited_futures`, `prefer_final_locals`, and wiring `dart run custom_lint` into CI. + +### 3.9 🟡 Low — A few architectural rough edges +- **`water` is split into three notifiers** (`water_log`, `water_prefs`) plus a derived `Provider`. The derived stats provider recomputes 5+ date-window filters on every water-log change — fine at current scale, but it's the only feature using a derived provider, so the pattern is inconsistent with the rest of the app (which would have put this in the notifier or repository). +- **`Medication` mixes `Equatable` with mutable ObjectBox fields** (`// ignore: must_be_immutable`) and JSON-string `@Transient` converters for `Dose`/`Frequency`/`Streak`. Storing structured sub-objects as `jsonEncode`d strings means you can't query them and converters can throw on malformed data. Workable, but consider ObjectBox relations or flattened fields for anything you'll query. +- **No route guards.** `LoadingScreen` is the `initial: true` route and presumably branches on auth imperatively; AutoRoute `AuthGuard`s would make the auth/unauth boundary explicit and testable. + +--- + +## 4. Dependencies Review + +The dependency set is modern, well-chosen, and current (Flutter 3.44, Firebase BoM-aligned plugins, ObjectBox 4.2, AutoRoute 9, Riverpod 2.6). Notes: + +| Package | Assessment | +|---|---| +| `flutter_riverpod` / `riverpod_annotation` 2.6 | Current 2.x. Riverpod 3 is the considered upgrade — see companion plan. | +| `fpdart` 1.1 | Good fit for the `Either` strategy. Stable. | +| `objectbox` 4.2 | Solid local-first choice; pairs well with offline-first design. | +| `auto_route` 9.3 | Fine. Route guards underused. | +| `health` 12.0 | Recently added (Health Connect feature). Verify platform permission handling. | +| `springster` 0.4, `generic_selector` 0.1.0, `hugeicons` 0.0.7 | **Pre-1.0 / niche.** Low-maintenance risk: small/young packages can stall. `generic_selector` and `hugeicons` are easily replaceable; worth tracking. | +| `mesh_gradient`, `flutter_animate`, `percent_indicator`, `fl_chart` | Reasonable UI deps. | +| Icon libraries: `fluentui_system_icons` **and** `hugeicons` **and** `cupertino_icons` | Three icon sets is more than needed; consolidating would cut app size and decision overhead. | + +`dev_dependencies` are appropriate (`mocktail`, `riverpod_lint`, `custom_lint`, generators). The gap is not the tooling — it's that the testing tooling is unused and `custom_lint` isn't enforced. + +`flutter analyze` is **clean** (only 2 `info`-level deprecation notices for `Radio.groupValue`/`onChanged` in `app_radio.dart` — a quick Material 3 `RadioGroup` migration). + +--- + +## 5. CI/CD Review + +**Strengths:** PR builds run `flutter analyze` + debug APK; release PRs auto-version, tag, build AAB, and deploy to Play internal. Secrets are injected, not committed. + +**Gaps:** +- **No test step** in CI (because there are no tests). Once tests exist, add `flutter test` as a required gate. +- `deploy.yml` triggers on `pull_request` to `main` (with a `startsWith(head_ref, 'release')` guard), so deployment happens on the **PR**, not on **merge**. That means an unmerged release PR can ship to the internal track. Consider triggering deploy on `push`/merge to `main` or on tag creation instead. +- `custom_lint`/`riverpod_lint` are not run in CI despite being installed. +- iOS build is commented out; only Android is verified by CI. + +--- + +## 6. Prioritized Recommendations + +**Now (correctness & safety):** +1. Fix `Medication.putActivityRecord` double-add and the empty-box guard in `getMedicationScheduledDoses`. (3.4) +2. Gate all `log(...)` of domain state behind `kDebugMode` and stop logging full `state.value`. (3.5) +3. Add `firestore.rules` to the repo, lock them down to per-user access, and add a Firestore deploy target. (3.5) +4. Make Crashlytics opt-in (consent) before wider release. (3.5) + +**Next (foundations):** +5. Stand up a real test suite — start with `futureHandler`, model serialization/converters, `Medication`/streak logic, then notifier tests with overridden repository providers. Add `flutter test` to CI. (3.1) +6. Replace global service/repository singletons with provider-based DI; drop the `build({required repository})` family seam. (3.2) — do this **with** or **just before** the Riverpod 3 migration. +7. Introduce a shared notifier helper (or `AsyncValue.guard`) to kill the repeated loading/fold boilerplate and the `stackTrace!` force-unwrap. (3.3) + +**Then (hygiene):** +8. Delete commented-out code and burn down TODOs (or move to issues). (3.6) +9. Rename `databse_service.dart` / `crashytics_service.dart`; fix `Iml`→`Impl`. (3.7) +10. Tighten lints and run `custom_lint` in CI; fix the `Radio` deprecations. (3.8, §4) +11. Reconsider `Failure.fromApi` to map the error sources you actually have (Firebase/ObjectBox), not HTTP codes. (3.4) +12. Consolidate icon libraries; track the pre-1.0 dependencies. (§4) + +--- + +## 7. On the Riverpod 3.0 Upgrade + +You asked specifically about effort. Short version: **the migration surface is small and the effort is moderate (~2–4 focused days), but the risk is elevated specifically because there are no tests to catch regressions.** + +The good news is the hard part of most Riverpod 3 migrations — porting legacy `StateProvider`/`StateNotifierProvider`/`ChangeNotifierProvider` — **does not apply here**: this app uses only code-generated `@Riverpod` notifiers (8 of them) plus a single functional `Provider`. The painful surface is the global-singleton DI pattern (§3.2), which you should fix anyway and which the migration is a natural moment to address. + +A full effort estimate, breaking-change inventory, and step-by-step plan are in **[`docs/riverpod_3_migration_plan.md`](./riverpod_3_migration_plan.md)**. + +--- + +## 8. Conclusion + +Circle is built on a thoughtful skeleton: the layering, error model, and feature structure reflect real architectural judgment, and the documentation is unusually good. The reason it lands at **5.5/10** rather than higher is execution rigor — the absence of tests, the DI pattern that quietly cancels Riverpod's benefits, a handful of real bugs in health-critical logic, and a privacy posture that isn't yet ready for a medical app at scale. + +None of these are structural dead-ends. The architecture is sound enough that the fixes are additive, not a rewrite. Address testing and DI first (they unblock everything else, including the Riverpod 3 upgrade), tighten the privacy/logging story for the health domain, and this codebase moves into the 7.5–8 range without changing its fundamental shape. diff --git a/docs/riverpod_3_migration_plan.md b/docs/riverpod_3_migration_plan.md new file mode 100644 index 0000000..d101053 --- /dev/null +++ b/docs/riverpod_3_migration_plan.md @@ -0,0 +1,151 @@ +# Riverpod 2.x → 3.0 Migration Plan + +**Companion to:** [`codebase_audit.md`](./codebase_audit.md) +**Date:** 2026-06-24 +**Current versions:** `flutter_riverpod ^2.6.1`, `riverpod_annotation ^2.6.1`, `riverpod_generator ^2.6.3`, `riverpod_lint ^2.6.3` +**Target:** Riverpod 3.x (unified package, codegen) + +--- + +## 1. TL;DR — Effort & Risk + +| | Assessment | +|---|---| +| **Migration surface** | **Small.** 8 code-generated `@Riverpod` notifiers + 1 functional `Provider`. ~85 `ref.watch`, 9 `ref.read`, 1 `ref.listen`. 53 `Consumer*` widgets. **No legacy `StateProvider`/`StateNotifierProvider`/`ChangeNotifierProvider`/`StateNotifier` to port** — this is the part that makes most migrations painful, and you have none of it. | +| **Mechanical effort** | **Low–moderate** — bump deps, regenerate, fix the handful of API renames. A day or less of pure code changes. | +| **Real effort driver** | **The DI refactor (audit §3.2) and the absence of tests.** Without a test suite, every behavioral change in Riverpod 3 (auto-retry, provider pausing) must be verified by hand across all screens. | +| **Total estimate** | **~2–4 focused days**, front-loaded if you also fix DI (recommended). Add ~1–2 days if you build a smoke-test suite first (strongly recommended). | +| **Overall risk** | **Medium** — low code volume, but no automated safety net and a few behavioral defaults change in v3. | + +**Recommendation:** Do this on a dedicated branch, fix the global-singleton DI as part of it (the two refactors touch the same files), and write at least a thin notifier/repository test layer *first* so you can detect regressions. + +--- + +## 2. What Actually Changes in Riverpod 3 (relevant to this codebase) + +Riverpod 3 is a smaller break than 1→2 for codegen users, but several defaults change behavior. The items below are the ones that touch *this* code. + +### 2.1 Single unified package +`riverpod`, `flutter_riverpod`, and `hooks_riverpod` are consolidated and re-versioned to 3.x. You bump all Riverpod deps together. Codegen output targets the new API. + +### 2.2 Generic `Ref` (codegen) — **affects generated code, not yours much** +In v2 codegen, each provider got a typed ref (e.g. `MedNotifierRef`). In v3 there is a single generic `Ref`. This codebase **does not reference the per-provider `…Ref` types by name** (notifiers use `ref.watch`/`ref.read` directly inside the class, and the one functional provider uses an untyped `ref`), so regeneration absorbs almost all of this. Verify after regen that no manual reference to a `…Ref` typedef remains. + +### 2.3 Automatic retry — **behavioral change, verify** +v3 providers **automatically retry on error by default** (exponential backoff). Today the app surfaces failures as `AsyncValue.error` and the UI presumably shows an error state. With auto-retry, a failed provider may silently re-run. Decide per-provider whether to keep, tune, or disable retry (`@Riverpod(retry: ...)` / scope-level `retry`). For notifiers that fail on, e.g., a permanent local error, you likely want to disable retry to avoid loops. + +### 2.4 Provider pausing when not observed — **behavioral change, verify** +v3 can pause providers whose listeners are not currently visible (e.g. off-screen). Your notifiers are all `@Riverpod(keepAlive: true)`, which mitigates disposal, but pausing semantics around `ref.watch`-driven recomputation (e.g. `waterStatsProvider`, and `MedNotifier` watching `userNotifierProviderImpl`) should be smoke-tested. + +### 2.5 `AsyncValue` / error handling refinements +- `AsyncValue` retains previous data more consistently across loading/error transitions in v3. Code that assumes `state.value` is null during loading may now see stale-but-present data. Your `log("Success ${state.value}")` and any UI reading `.value` during loading should be reviewed (you're removing most of that logging per the audit anyway). +- Error/stacktrace handling around `AsyncError` is stricter. This is a **good moment** to remove the `failure.stackTrace!` force-unwraps (audit §3.3) since you're touching every notifier. + +### 2.6 Legacy API moved behind `legacy` import +`StateProvider`, `StateNotifierProvider`, `ChangeNotifierProvider`, and `StateNotifier` are deprecated/moved. **Not used here** — no action required, which is the single biggest reason this migration is cheap. + +### 2.7 Experimental features you may *opt into* later (not required) +- **Offline persistence** (`@JsonPersist` / persistence API) — could eventually replace some of the manual ObjectBox-as-cache plumbing, but out of scope for the upgrade itself. +- **Mutations** (experimental) — a cleaner way to model the side-effecting notifier methods (`putMedication`, etc.) than the current manual loading/fold blocks. Worth piloting *after* the upgrade stabilizes. + +### 2.8 Tooling +`riverpod_generator`, `riverpod_lint`, and `custom_lint` must move to 3.x-compatible versions in lockstep. `build_runner` stays. Expect the generator to require a clean rebuild (`--delete-conflicting-outputs`). + +--- + +## 3. Pre-Migration Checklist (do these first) + +1. **Branch:** create `chore/riverpod-3` off `main` (not off `code-audit`). +2. **Green baseline:** confirm `flutter analyze` is clean (it currently is, modulo 2 `Radio` infos) and the app builds/runs on Android. +3. **Write a smoke-test safety net** (the most important prerequisite given §1): + - Unit tests for `futureHandler`/`methodHandler` mapping. + - Model round-trip tests (`Medication`/`Dose`/`Frequency` `toMap`/`fromMap`, ObjectBox converters). + - One notifier test per feature using `ProviderContainer` with an overridden repository — this *also* forces the DI fix in §4. + - A handful of widget smoke tests for the primary screens (renders without throwing, shows data/error/loading). +4. **Snapshot current behavior** of error states and loading spinners on key screens (auth, meds list, water) so you can compare after auto-retry/pausing changes. + +--- + +## 4. Fix DI as Part of the Migration (recommended, audit §3.2) + +The current pattern — global `final` singletons + `build({required Repository repository})` family seam — is awkward under v2 and stays awkward under v3. Migrate it to provider-based DI in the same pass, because it touches the same files and unblocks testing (which you need to verify the migration). + +**Target shape per feature:** +```dart +// services/repositories exposed as providers +@riverpod +MedLocalService medLocalService(Ref ref) => + MedLocalService(store: ref.watch(databaseStoreProvider)); + +@riverpod +MedService medService(Ref ref) => MedService(); + +@riverpod +MedRepository medRepository(Ref ref) => MedRepositoryImpl( + medLocalService: ref.watch(medLocalServiceProvider), + medService: ref.watch(medServiceProvider), + ); + +@Riverpod(keepAlive: true) +class MedNotifier extends _$MedNotifier { + @override + FutureOr> build() async { + _repo = ref.watch(medRepositoryProvider); // no more family param + _selfUser = ref.watch(userNotifierProvider).value; + return []; + } +} +``` +- Expose the ObjectBox `Store` as a provider (`databaseStoreProvider`) overridden in `main()` after `LocalDatabaseService.instance.initialize()`, instead of reading the global `database.store` at file-load time. +- Delete the `…ProviderImpl` / `…ProviderIml` globals; UI watches the generated provider directly (`ref.watch(medNotifierProvider)`). +- Tests then do `ProviderScope(overrides: [medRepositoryProvider.overrideWithValue(mockRepo)])`. + +If you want to keep the migration purely mechanical and defer DI, you *can* — v3 still supports family parameters — but you'd be carrying the un-testability that makes the migration risky. Doing both together is the better trade. + +--- + +## 5. Step-by-Step Migration + +1. **Bump dependencies** in `pubspec.yaml` to the 3.x line (all in lockstep): + - `flutter_riverpod`, `riverpod_annotation` → ^3.x + - `riverpod_generator`, `riverpod_lint` → ^3.x (dev) + - keep `custom_lint`, `build_runner` current/compatible + - `flutter pub get` and resolve any version conflicts (e.g. `analyzer`/`build` transitive bumps). +2. **Regenerate:** `dart run build_runner build --delete-conflicting-outputs`. Expect changed `*.g.dart` (generic `Ref`, new family/provider shapes). +3. **Fix compile errors** — they will cluster around: + - any direct use of a `…Ref` typedef (none expected here), + - the family-parameter providers if you keep them (signature shifts), or their removal if you do §4, + - `AsyncValue`/`AsyncError` construction sites (the `stackTrace!` unwraps — replace with `AsyncValue.guard` or `?? StackTrace.current`). +4. **Run `flutter analyze` and `dart run custom_lint`** — `riverpod_lint` 3.x will flag deprecated patterns; clear them. +5. **Configure retry** (§2.3): decide global vs per-provider; for local-storage-backed notifiers consider disabling auto-retry to avoid loops on deterministic failures. +6. **Run the smoke-test suite** from §3; fix regressions. +7. **Manual QA pass** on every screen, paying attention to: loading spinners, error displays, the `waterStatsProvider` recomputation, `MedNotifier`'s dependency on the user provider, and the auth stream subscription in `AuthNotifier.authStateChanges()` (move it into `build` with `ref.onDispose` while you're here — audit §3.7). +8. **Update `CLAUDE.md`** to describe provider-based DI (it currently documents the `build()`-injection pattern). + +--- + +## 6. Effort Breakdown + +| Task | Estimate | Notes | +|---|---|---| +| Pre-migration smoke tests | 1–2 days | Optional but strongly recommended; reduces risk the most | +| Dep bump + regenerate + fix compile errors | 0.5 day | Small surface; mostly generated code | +| DI refactor to providers (§4) | 0.5–1 day | Touches 4 provider files + their UI watchers (~85 `ref.watch` sites, mostly unchanged names if you keep generated provider names) | +| Retry/pausing config + behavioral verification | 0.5 day | Depends on how many screens to QA | +| Manual QA + fixes | 0.5–1 day | The long pole given no tests | +| **Total** | **~2–4 days** (+1–2 if building tests first) | | + +--- + +## 7. Go / No-Go + +**Reasons to do it now:** small surface, no legacy providers, you get auto-retry/persistence/mutations on the table, and you can fix the DI anti-pattern in the same motion. Riverpod 2 is in maintenance. + +**Reasons to wait:** the **no-tests** situation is the real blocker. Migrating a health app's state layer with zero automated coverage and shipping to users is the risk, not the API churn. + +**Recommended sequence:** +1. Build the smoke-test safety net (valuable regardless of the upgrade). +2. Fix DI (audit §3.2) — small, high-leverage, makes tests easy. +3. Then migrate to Riverpod 3 on top of that foundation. + +Doing 1→2→3 turns a medium-risk migration into a low-risk one and leaves the codebase materially better than a straight version bump would. diff --git a/firebase.json b/firebase.json index fe75cb7..26b7964 100644 --- a/firebase.json +++ b/firebase.json @@ -1 +1,34 @@ -{"flutter":{"platforms":{"android":{"default":{"projectId":"circle-60af7","appId":"1:497391679746:android:9c173adea7fad3059d2a1b","fileOutput":"android/app/google-services.json"}},"ios":{"default":{"projectId":"circle-60af7","appId":"1:497391679746:ios:f6a8a2fe808895309d2a1b","uploadDebugSymbols":true,"fileOutput":"ios/Runner/GoogleService-Info.plist"}},"dart":{"lib/firebase_options.dart":{"projectId":"circle-60af7","configurations":{"android":"1:497391679746:android:9c173adea7fad3059d2a1b","ios":"1:497391679746:ios:f6a8a2fe808895309d2a1b"}}}}}} \ No newline at end of file +{ + "firestore": { + "rules": "firestore.rules", + "indexes": "firestore.indexes.json" + }, + "flutter": { + "platforms": { + "android": { + "default": { + "projectId": "circle-60af7", + "appId": "1:497391679746:android:9c173adea7fad3059d2a1b", + "fileOutput": "android/app/google-services.json" + } + }, + "ios": { + "default": { + "projectId": "circle-60af7", + "appId": "1:497391679746:ios:f6a8a2fe808895309d2a1b", + "uploadDebugSymbols": true, + "fileOutput": "ios/Runner/GoogleService-Info.plist" + } + }, + "dart": { + "lib/firebase_options.dart": { + "projectId": "circle-60af7", + "configurations": { + "android": "1:497391679746:android:9c173adea7fad3059d2a1b", + "ios": "1:497391679746:ios:f6a8a2fe808895309d2a1b" + } + } + } + } + } +} diff --git a/firestore.indexes.json b/firestore.indexes.json new file mode 100644 index 0000000..415027e --- /dev/null +++ b/firestore.indexes.json @@ -0,0 +1,4 @@ +{ + "indexes": [], + "fieldOverrides": [] +} diff --git a/firestore.rules b/firestore.rules new file mode 100644 index 0000000..4285e0c --- /dev/null +++ b/firestore.rules @@ -0,0 +1,51 @@ +rules_version = '2'; + +// Circle — Firestore security rules +// +// Data model (all owned data is keyed by the owner's auth UID at the first +// path segment): +// /users/{uid} -> user profile document +// /users/{uid}/preferences/{prefId} -> e.g. .../preferences/water +// /meds/{uid} -> medication document (array field) +// /logs/{uid}/water/{monthYear} -> water log buckets, e.g. "June_2026" +// +// Baseline policy: deny everything, then allow a signed-in user to access ONLY +// the documents whose {uid} segment matches their own auth UID. No user can read +// or write another user's health data. +service cloud.firestore { + match /databases/{database}/documents { + + // True when the request comes from the user that owns `uid`. + function isOwner(uid) { + return request.auth != null && request.auth.uid == uid; + } + + // User profile and everything nested under it (e.g. /preferences/water). + match /users/{uid} { + allow read, write: if isOwner(uid); + + match /{document=**} { + allow read, write: if isOwner(uid); + } + } + + // Medication document for a user. + match /meds/{uid} { + allow read, write: if isOwner(uid); + } + + // Logs (water, and any future log types) for a user. + match /logs/{uid} { + allow read, write: if isOwner(uid); + + match /{document=**} { + allow read, write: if isOwner(uid); + } + } + + // Explicit default-deny for anything not matched above. + match /{document=**} { + allow read, write: if false; + } + } +}