2026-07-06 21:36:59 -05:00
|
|
|
# Code standardization & modernization review (2026-07-06)
|
|
|
|
|
|
2026-07-11 18:29:08 -05:00
|
|
|
> **Status (stamped 2026-07-11): point-in-time review; the "Applied" section shipped, the rest is
|
|
|
|
|
> backlog.** Unapplied items overlap the refactor backlog tracked in `Future.md` (session-game
|
|
|
|
|
> engine dedupe, DispatcherProvider injection, error-surfacing standardization) — `Future.md` is
|
|
|
|
|
> the live list; this file is the evidence/rationale record behind it.
|
|
|
|
|
|
2026-07-06 21:36:59 -05:00
|
|
|
A grounded pass over the Android app (313 Kotlin files), Cloud Functions (TypeScript), and build. Each
|
2026-07-06 22:19:33 -05:00
|
|
|
item is a *real, measured* inconsistency (with counts), not generic advice.
|
2026-07-06 21:36:59 -05:00
|
|
|
|
|
|
|
|
## Applied (done + verified)
|
|
|
|
|
|
|
|
|
|
1. **Compose state collection unified on `collectAsStateWithLifecycle`** — was 55 files on
|
2026-07-06 22:19:33 -05:00
|
|
|
`collectAsState()` vs 2 lifecycle-aware; now all 57. Added `lifecycle-runtime-compose`. Upstream
|
|
|
|
|
flow collection pauses when the UI isn't visible.
|
|
|
|
|
2. **Firebase Tasks → `.await()` across the ENTIRE data layer** — all 15 data sources converted from
|
|
|
|
|
verbose `suspendCancellableCoroutine{cont-> …addOnSuccess/addOnFailure}` wrappers (and the per-file
|
|
|
|
|
`getDoc`/`voidAwait`/`queryAwait`/`refAwait` helper duplicates) to `kotlinx-coroutines-play-services`
|
|
|
|
|
`.await()`. Zero Task-callback wrappers remain. Per-block review kept `callbackFlow` snapshot
|
|
|
|
|
listeners + the one transaction untouched; failure→null/false best-effort blocks became
|
|
|
|
|
`runCatching{…await()}.getOrNull()/getOrDefault()`; no-signed-in-user guards became `?: throw`.
|
|
|
|
|
**Verified live** (real Firebase, paired account): auth + Home reads and a daily-answer write all
|
|
|
|
|
succeed.
|
|
|
|
|
3. **Gradle version catalog (`gradle/libs.versions.toml`)** — 57 hardcoded coordinates centralized;
|
|
|
|
|
root + app scripts use `libs.*` / `alias(libs.plugins.*)`. 1:1, no version changes.
|
|
|
|
|
4. **Typed Cloud Functions callable payloads** — the 5 `onCall((data: any, …))` handlers now take
|
|
|
|
|
`Record<string, unknown>`, forcing field access through the existing validators.
|
|
|
|
|
5. **ViewModels off Firebase (7 of 8 offenders):**
|
|
|
|
|
- 5 VMs (BucketList, QuestionThread, DateBuilder, MessagesInbox, Conversation) switched from the
|
|
|
|
|
`FirebaseAuth.getInstance()` static singleton to the injected `AuthRepository.currentUserId`.
|
|
|
|
|
- 2 VMs (DailyQuestion, PartnerHome) had their raw `db.…addSnapshotListener` on the partner's answer
|
|
|
|
|
doc replaced with the existing `FirestoreAnswerDataSource.observeAnswerForUser(...)` Flow (Job-managed),
|
|
|
|
|
dropping the `FirebaseFirestore` injection. Verified live (correct "waiting for partner" state).
|
|
|
|
|
|
|
|
|
|
## Remaining — scoped, deliberate follow-ons (NOT churn-avoidance)
|
|
|
|
|
|
|
|
|
|
6. **HomeViewModel's two snapshot listeners** — the last raw-Firestore-in-VM. Deliberately left because,
|
|
|
|
|
unlike the two extracted above, these use `MetadataChanges.INCLUDE` (fire on local/pending writes, not
|
|
|
|
|
just server data) and carry inline partner-answered analytics + widget-state updates, and they drive
|
|
|
|
|
the app's **core reveal trigger**. Doing this right needs a metadata-aware data-source Flow AND
|
|
|
|
|
**two-device** real-time verification (partner answers on device B → device A flips to reveal) — a
|
|
|
|
|
single-device smoke can't catch a regression here. Extract as its own focused task.
|
|
|
|
|
7. **~175 hardcoded `Color(0xFF…)` in `ui/`** → theme tokens. Right modern practice, but visual-
|
|
|
|
|
regression risk; pair with `scripts/theme-scan.sh` + screenshot diffs, per-screen (not en masse).
|
|
|
|
|
8. **Cloud Functions v1 → v2** (`firebase-functions/v2/*`). The modern target (cold-start/concurrency/
|
|
|
|
|
typed params), but changes signatures + deploy config and touches the billing webhook / callables —
|
|
|
|
|
its own project with sandbox verification, not bundled.
|
|
|
|
|
9. **`catch (err: any)` → `unknown`** narrowing and the untyped Tink handles in `wrapReleaseKeyCallable`
|
|
|
|
|
(the crypto lib ships no types) — marginal value; fold into #8 if/when it happens.
|
2026-07-06 21:36:59 -05:00
|
|
|
|
|
|
|
|
## Healthy already (no action)
|
|
|
|
|
|
2026-07-06 22:19:33 -05:00
|
|
|
Material3 only (86 files, zero Material2); no `GlobalScope`; only 9 `!!` app-wide; analytics hashing
|
|
|
|
|
single-sourced. DI split of `@Binds`/`@Provides` is idiomatic.
|
2026-07-06 21:36:59 -05:00
|
|
|
|
2026-07-06 22:19:33 -05:00
|
|
|
## Verdict
|
2026-07-06 21:36:59 -05:00
|
|
|
|
2026-07-06 22:19:33 -05:00
|
|
|
The safe, high-value standardizations are done and verified (state collection, the whole data-layer
|
|
|
|
|
Tasks→await, version catalog, TS payload typing, 7/8 ViewModel-Firebase extractions). The four
|
|
|
|
|
remaining items are each a case where a responsible change needs its own verification loop
|
|
|
|
|
(two-device real-time, screenshot diffs, or a sandbox deploy) rather than a broad sweep — sequenced
|
|
|
|
|
6 → 8 → 7 → 9.
|
