diff --git a/app/src/main/java/app/closer/data/remote/FirestoreUserDataSource.kt b/app/src/main/java/app/closer/data/remote/FirestoreUserDataSource.kt index daee12fd..4aabebed 100644 --- a/app/src/main/java/app/closer/data/remote/FirestoreUserDataSource.kt +++ b/app/src/main/java/app/closer/data/remote/FirestoreUserDataSource.kt @@ -7,6 +7,7 @@ import app.closer.domain.model.User import com.google.firebase.firestore.DocumentSnapshot import com.google.firebase.firestore.FirebaseFirestore import com.google.firebase.firestore.SetOptions +import com.google.firebase.firestore.Source import kotlinx.coroutines.channels.awaitClose import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.callbackFlow @@ -66,6 +67,23 @@ class FirestoreUserDataSource @Inject constructor( return if (snap.exists()) snapshotToUser(snap.id, snap) else null } + /** + * The user, straight from the server — never the local cache. + * + * [getUser] is `Source.DEFAULT`, so right after a fresh sign-in it answers from cache in about a + * millisecond with a *hollow* document: `exists() == true`, every field null. The real document + * lands ~200ms later. That is fine for a screen that re-renders, and wrong for a one-shot + * decision made once at startup — a hollow read is indistinguishable from "this user has no + * profile", which is how a paired, fully set-up user got routed into new-user profile setup. + * + * Throws when the server can't be reached (offline), which is the point: callers making a + * destructive decision must be able to tell "no profile" from "don't know yet". + */ + suspend fun getUserFromServer(uid: String): User? { + val snap = userRef(uid).get(Source.SERVER).await() + return if (snap.exists()) snapshotToUser(snap.id, snap) else null + } + fun observeUser(uid: String): Flow = callbackFlow { val listener = userRef(uid).addSnapshotListener { snap, error -> if (error != null) { diff --git a/app/src/main/java/app/closer/data/repository/UserRepositoryImpl.kt b/app/src/main/java/app/closer/data/repository/UserRepositoryImpl.kt index ea42b719..65130650 100644 --- a/app/src/main/java/app/closer/data/repository/UserRepositoryImpl.kt +++ b/app/src/main/java/app/closer/data/repository/UserRepositoryImpl.kt @@ -15,6 +15,8 @@ class UserRepositoryImpl @Inject constructor( override suspend fun getUser(uid: String): User? = dataSource.getUser(uid) + override suspend fun getUserFromServer(uid: String): User? = dataSource.getUserFromServer(uid) + override fun observeUser(uid: String): Flow = dataSource.observeUser(uid) override suspend fun createUser(user: User) = dataSource.createUser(user) diff --git a/app/src/main/java/app/closer/domain/repository/UserRepository.kt b/app/src/main/java/app/closer/domain/repository/UserRepository.kt index 72387ebb..2e75e8c4 100644 --- a/app/src/main/java/app/closer/domain/repository/UserRepository.kt +++ b/app/src/main/java/app/closer/domain/repository/UserRepository.kt @@ -6,6 +6,9 @@ import kotlinx.coroutines.flow.Flow interface UserRepository { suspend fun getUser(uid: String): User? + + /** Server-authoritative read; throws when offline. Use for decisions that must not act on a cache miss. */ + suspend fun getUserFromServer(uid: String): User? fun observeUser(uid: String): Flow suspend fun createUser(user: User) suspend fun updateDisplayName(uid: String, displayName: String) diff --git a/app/src/main/java/app/closer/ui/onboarding/OnboardingViewModel.kt b/app/src/main/java/app/closer/ui/onboarding/OnboardingViewModel.kt index c9e0c833..a33f62f3 100644 --- a/app/src/main/java/app/closer/ui/onboarding/OnboardingViewModel.kt +++ b/app/src/main/java/app/closer/ui/onboarding/OnboardingViewModel.kt @@ -4,6 +4,7 @@ import android.util.Log import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope import app.closer.domain.model.AuthState +import app.closer.domain.model.User import app.closer.domain.repository.AuthRepository import app.closer.domain.repository.UserRepository import dagger.hilt.android.lifecycle.HiltViewModel @@ -49,38 +50,65 @@ class OnboardingViewModel @Inject constructor( * Where a signed-in user belongs: the profile step only if they genuinely have no profile yet. * * "Couldn't read the profile" and "has no profile" are NOT the same thing, and treating them the - * same was a data-loss trap. A returning user whose profile read failed (slow network, token not - * propagated yet — a real race seen right after sign-in) was sent to CREATE_PROFILE, which - * greets them with "What should your partner call you?" over a `🔒 Couldn't unlock on this - * device` value. Tapping through it re-encrypts and overwrites the very name we had just failed - * to read — and they never reach the Recovery screen that would have fixed them. + * same was a data-loss trap. Sending a returning user to CREATE_PROFILE greets them with "What + * should your partner call you?" over a `🔒 Couldn't unlock on this device` value, and tapping + * through it calls `createUser`, which is a whole-document `set()` — it overwrites their real + * name with the placeholder and drops `coupleId`/`partnerId`, unpairing the couple. They never + * reach the Recovery screen that would have fixed them. * - * The repository distinguishes the two cleanly: a missing document is `success(null)`, while a - * failed read throws. So only a *successful* read may route to the profile step. If the read - * never succeeds we send them Home, which is non-destructive and has its own offline/error - * handling — and which routes to Recovery by itself when the couple key is missing. + * Reading it once at sign-in is not enough, and this was measured rather than reasoned about. In + * the first ~200ms after sign-in the read comes back with a document that *exists* and has no + * fields at all — no email, no name, no `coupleId` — because signing in registers the FCM token, + * and that merge-write materialises `users/{uid}` before the real profile has synced. The real + * document arrives ~200ms later. A returning user on a new phone therefore hit the stub every + * time and was sent to profile setup; this was reproduced end to end with a paired account. + * + * So the read has to be *trusted* before it is acted on: a stub is treated as "ask again", not as + * "new user". Only a document with real identity on it decides, and if none ever arrives we send + * them Home — non-destructive, and it routes to Recovery by itself when the couple key is missing. + * A genuinely new user keeps working: their document is created with an email and `createdAt` + * before they ever reach here, and if it truly stays empty we fall through to profile setup. */ private suspend fun resolveDestination(uid: String): String { + var sawNoProfile = false repeat(PROFILE_READ_ATTEMPTS) { attempt -> - val result = runCatching { userRepository.getUser(uid) } + val result = runCatching { userRepository.getUserFromServer(uid) } if (result.isSuccess) { val user = result.getOrNull() - val needsProfile = user == null || user.displayName.isBlank() || user.sex.isBlank() - return if (needsProfile) "create_profile" else "home" + if (user != null && !user.isSignInStub()) { + val needsProfile = user.displayName.isBlank() || user.sex.isBlank() + return if (needsProfile) "create_profile" else "home" + } + // Absent or still a stub: too early to tell a new user from an unsynced one. + sawNoProfile = true + } else { + Log.w( + TAG, + "Could not load user profile during onboarding (attempt ${attempt + 1})", + result.exceptionOrNull() + ) } - Log.w( - TAG, - "Could not load user profile during onboarding (attempt ${attempt + 1})", - result.exceptionOrNull() - ) if (attempt < PROFILE_READ_ATTEMPTS - 1) delay(RETRY_DELAY_MS * (attempt + 1)) } - // Never read it. They are signed in, so they are not a new user: send them somewhere safe - // rather than inviting them to overwrite a profile we simply couldn't see. - Log.w(TAG, "Profile unreadable after $PROFILE_READ_ATTEMPTS attempts; routing Home, not to profile setup") - return "home" + // Out of attempts. A document that stayed empty really is a new user; a read that never + // succeeded at all tells us nothing, so don't invite them to overwrite a profile we couldn't see. + return if (sawNoProfile) { + "create_profile" + } else { + Log.w(TAG, "Profile unreadable after $PROFILE_READ_ATTEMPTS attempts; routing Home, not to profile setup") + "home" + } } + /** + * True for the field-less `users/{uid}` that FCM-token registration leaves behind at sign-in, + * before the profile has synced. Every path that creates a real user writes `email` and + * `createdAt` first, so their absence means "nothing has been written here yet" — never "this + * person has no profile". + */ + private fun User.isSignInStub(): Boolean = + email.isBlank() && createdAt == 0L && coupleId == null + fun onNavigated() = _uiState.update { it.copy(navigateTo = null) } companion object { diff --git a/docs/Engineering_Reference_Manual.md b/docs/Engineering_Reference_Manual.md index 9efbc2ad..930c5e0f 100644 --- a/docs/Engineering_Reference_Manual.md +++ b/docs/Engineering_Reference_Manual.md @@ -1478,6 +1478,13 @@ OOB code = `truncate6(SHA-256(pubkey ‖ nonce))`. **Fix (R15)**: `scripts/theme-scan.sh` is a mandatory Pass C pre-check that statically detects hardcoded colors, direct `painterResource`/`Image` usage outside `BrandIllustration`, mismatched container/surface colors, and missing `preview`s. Run it before any manual visual sweep; file findings in `ClaudeReport.md` and counts in `ClaudeQACoverage.md`. **Re-introduction risk / still-open**: any new art rendered via a **direct** `painterResource(R.drawable.illustration_*)` (NOT `BrandIllustration`) bypasses theme-correct loading and feathering. Always route opaque rounded-rect illustrations through `BrandIllustration`; keep `MaterialTheme.colorScheme` for surfaces; add Tier 1/2 previews for new screens. A hardcoded `CloserPalette.*` dark value or a raw `Color.White`/`Color.Black` will be flagged by `theme-scan.sh`. +### C-AUTH-001 - a `users/{uid}` read right after sign-in returns a field-less document +**Symptom**: signing in on a *new device* with a fully set-up, paired account routed to `CREATE_PROFILE` ("What should your partner call you?", over a `🔒 Couldn't unlock on this device` value) instead of `RECOVERY`. Reproduced end to end on a throwaway emulator with a real paired account. +**Cause**: for roughly the first 200ms after sign-in, `getUser(uid)` returns a document that **exists with no fields at all** - no `email`, no `displayName`, no `coupleId` - and the real document only lands afterwards. Sign-in registers the FCM token, and that `SetOptions.merge()` write materialises `users/{uid}` before the profile has synced. Measured on-device: two reads at t+0ms field-less, the real document at t+215ms. `Source.SERVER` does **not** dodge it, so "read from the server" is not the fix. +**Why it was destructive**: `FirestoreUserDataSource.createUser` is a whole-document `set()`, not a merge, and `CreateProfileViewModel` submit calls it. Tapping through that mistakenly-shown screen overwrites the real `displayName` with the locked placeholder and drops `coupleId`/`partnerId`, unpairing the couple - and the user never reaches Recovery. +**Fix (R30)**: `OnboardingViewModel.resolveDestination` treats a field-less document as "ask again", not as "new user": it retries (3x, 400ms backoff) until a document with real identity on it arrives, and only that decides. A read that never succeeds routes Home (non-destructive; Home routes to Recovery by itself when the couple key is missing). `UserRepository.getUserFromServer` (`Source.SERVER`) backs the decision so an offline read throws instead of guessing from cache. +**Re-introduction risk**: **any** one-shot `users/{uid}` read on a startup path that branches on a field being blank. `exists() == true` does not mean "populated". Blank fields this early mean "not synced yet", and `createUser`'s `set()` semantics make guessing wrong expensive. If you need "does this person have a profile", require identity (`email`/`createdAt`) to be present before believing the answer. + --- ## Where to look first