Update mobile/README.md for development builds — Expo Go no longer runs the app #129

Activity

1. PaulgSmith

   added

   documentationImprovements or additions to documentationImprovements or additions to documentation

   severity:significantShould be fixed before shippingShould be fixed before shipping

   on Aug 13, 2026
2. Jberma23

   self-assigned this

   on Aug 16, 2026
3. Jberma23 commented on Aug 16, 2026

   

   Jberma23

   on Aug 16, 2026

   Collaborator

   More actions

   I know you already documented some of these. But review:

   Code review — PR 128, encrypted storage & biometric unlock foundation

   Verification I ran (after npm ci — my checkout's node_modules was stale, the lockfile itself is correct):

   ┌──────────────────┬───────────────────────────────┐
   │ Check │ Result │
   ├──────────────────┼───────────────────────────────┤
   │ npx tsc --noEmit │ clean │
   ├──────────────────┼───────────────────────────────┤
   │ npx expo lint │ clean │
   ├──────────────────┼───────────────────────────────┤
   │ npx jest │ 10 suites, 95 tests, all pass │
   └──────────────────┴───────────────────────────────┘

   Overall this is unusually good work. The layering (key → database → migrations → repository, with the gate
   separate) is right, the injectable get testable against real SQLite insteadof asserting on SQL strings, and ADR 0015's reasoning for rejecting requireAuthentication is correct and
   important. Comments explain why rather what follows is about gaps betweenwhat the code intends and what it enforces.

   ---

   1. Blocking: nothing verifies SQLCiphe

   src/lib/db/database.ts:53-58 keys the lite_master to "force SQLCipher toactually decrypt a page". That probe doesn't do what the comment says. I verified against plain SQLite:

   PRAGMA key on plain SQLite: ACCEPTED SILENTLY (no error)
   sqlite_master probe still works: {"c":
   PRAGMA cipher_version -> null
   SQLite ignores unknown pragmas rather than erroring, so on any binary without SQLCipher compiled in, PRAGMA key is a no-op, the probe passecrypted medical journal with no signalwhatsoever. That binary is easy to end up with: Expo Go (config plugins don't apply there, and the README still tells contributors to use it), arebuild that predates this change, or a cached EAS build.

   This is precisely the silent downgrade database.ts:42 refuses to allow on web, and ADR 0017's "failing
   loudly is better than a fallback that . The check is cheap:

   const cipher = await db.getFirstAsync<PRAGMA cipher_version');
   if (!cipher) {
   throw new DatabaseUnavailableError(
   'This build has no SQLCipher, so the journal would be stored unencrypted. Run npx expo prebuild and
   use a development build — Expo Go cann
   );
   }

   Run it before PRAGMA key, and the erroarding hint for finding 2.

   2. Blocking: mobile/README.md still do

   ADR 0016 lists this as a required consADME.md:13 lists Expo Go as aprerequisite, :31 says to scan the QR code, :53 says "Expo Go still runs the app on a physical iPhone
   without a Mac." Combined with finding the README, gets a working app, andwrites plaintext. ADR 0008 designed this area as the gentle first ticket, so the setup docs matter more
   than usual here.

   3. repository.update breaks when destr

   repository.ts:146 and :164 call this.f

   PROBE A: const { update } = contacts;
   → TypeError: _this2.find is not a function

   const { update } = repo and onPress={repo.update} are both ordinary React usage, and the failure is a
   runtime TypeError that types won't cation find(id) above the returned objectand call it directly from update.

   4. create() returns something different from what it stored

   repository.ts:136 spreads the caller's input rather than the row. Verified:

   • Probe B — create({ name: 'Sam' }) returns an object with no phone key; find() on the same id returns
     phone: null. So created ≠ find(created
   • Probe C — undeclared keys are returned but never inserted:
     ["name","phone","notAColumn","id","cre

   The existing test at repository.test.tis null, which passes while thereturned value is undefined. This will bite the first screen that renders created directly instead of
   re-listing. Building the return value null) fixes both, andexpect(created).toEqual(await find(created.id)) is a good regression test.

   5. "Delete all my data" can silently leave the file behind

   database.ts:103 swallows every error from deleteDatabaseAsync. Looking at expo-sqlite's iOS
   implementation, that call throws Deletabase is still in the module'sconnection cache and DatabaseNotFoundException if the file is absent. The blanket catch treats both the
   same, so a genuinely failed deletion r it to the not-found case gets you thebehaviour the test at database.test.ts:159 claims to cover.

   Related: closeJournalDatabase at database.ts:89 awaits db?.closeAsync() unguarded, so a close failure
   rejects destroyJournalDatabase before ordering where the user keeps areadable journal after asking for deletion.

   While there — the rationale at database.ts:96-99 and database.test.ts:153 doesn't parse: "an unreadable
   database rather than a readable one withat can't exist, since a database with no key is by definition unreadable. Deleting the key first is the stronger order, because it guarantees
   unreadability at the earliest moment els.

   6. A phone with no lock gets an impossurn

   unlock-gate.tsx:105-109 sets {status:'less of the previous state. Verified:

   PROBE D: no-lock device, Continue → ba
   → "Your journal is locked" / "Unlock with your phone to open it."

   On a device with no lock there is nothing to unlock with. The user taps Unlock, gets the "Your phone has
   no lock set" screen again, taps Continpps. Re-locking to no-device-lock whenthat's where the state came from, or re-checking availability before choosing the copy, keeps ADR 0018's
   placeholder honest.

   7. iOS app-switcher snapshot leaks jou

   Same listener: background only. On iOSand the OS snapshots the screen for the switcher card before background arrives, so the card shows journal content and persists to disk. The
   usual fix is an opaque cover rendered lock state, so it doesn't fight thebiometric prompt) plus the existing re-lock on background. Reasonable as a follow-up issue rather than in
   this PR, but the comment at :99-101 cl so the gap should be recordedsomewhere.

   ---

   Smaller things

   • key.ts:76-104 — check-then-act race.oth read null, both generate, and thesecond setItemAsync overwrites the first. Only database.ts calls this today and openPromise serialises it,
     so it can't currently fire — but the ftakes are the ones the file's ownheader describes. The same shared-in-flight-promise pattern used for openPromise would close it.
   • unlock.ts:83 — cancelLabel: 'Not nowxpo-local-authentication only callssetNegativeButtonText when disableDeviceFallback is true (which you deliberately leave false), because
     BiometricPrompt rejects a negative butAL. The copy the comment thought aboutis iOS-only; fallbackLabel is the iOS-side lever if you want more control.
   • unlock.ts:73 + unlock-gate.tsx:60 boty, so every attempt makes two nativeround-trips. Harmless, easily deduplicated by having requestUnlock accept an already-known availability.
   • No auto re-prompt on foreground. Aftser must tap Unlock. Defensible (avoids prompting mid-animation), but it's an extra tap every time for an audience the ADRs describe as finding phones difficult — worth being a decis
   • Android biometricsSecurityLevel defaults to 'weak', admitting Class 2 biometrics. Consistent with unlock.test.ts:37 treating BIOMETRIC_W intentional — just not written downanywhere.
   • user_fallback isn't in the switch ath to "That didn't work." Unlikely tofire with the device-owner policy, but it's the one error string that means the user actively chose
     something rather than failed.

   One thing worth folding back into the

   ADR 0015 says issue #115 "still has totabase file itself." That's now aniOS-only job. Adding expo-secure-store to plugins applies its backup rules with configureAndroidBackup defaulting true, and those rules use <ath="."/> as their only include — under Android's Auto Backup semantics that excludes everything else, including files/SQLite/journal.db. On iOS the database sits in Documents/SQLite/pted, with the key correctly excludedby THIS_DEVICE_ONLY, so a restore yields an unreadable file — the boundary 0015 intends). Scoping #115 to
   NSURLIsExcludedFromBackupKey on iOS wog this.

   What's good

   The requireAuthentication analysis in the most valuable thing in this PR —it's a trap most implementations walk into, the reasoning is right, and key.test.ts:55 pins it with a test that explains itself. jest/in-memory-smigration rollback test atmigrations.test.ts:83 verifies real transactional behaviour rather than a mock's opinion, and it's honesabout what it can't cover. The empty Mserting it stays empty is a nice way to keep "foundation" and "first feature" separable. And ADR 0018 recording an unmade decision, rather than letting the implementation quietly becthe right instinct.

   Findings 1 and 2 are the ones I'd hold and localised.
4. PaulgSmith

   added a commit that references this issue on Aug 20, 2026

   Document development builds, and stop `npm start` reaching for Expo Go

   ...

   This issue will close once commit 5e32f62 is merged into the 'main' branch.

   5e32f62