null
/
Closer
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity
Closer/docs/standardization-review.md

4.0 KiB
Raw Blame History

Code standardization & modernization review (2026-07-06)

A grounded pass over the Android app (313 Kotlin files), Cloud Functions (TypeScript), and build. Each item is a real, measured inconsistency (with counts), not generic advice.

Applied (done + verified)

  1. Compose state collection unified on collectAsStateWithLifecycle — was 55 files on 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)

  1. 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.
  2. ~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).
  3. 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.
  4. 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.

Healthy already (no action)

Material3 only (86 files, zero Material2); no GlobalScope; only 9 !! app-wide; analytics hashing single-sourced. DI split of @Binds/@Provides is idiomatic.

Verdict

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.