fix(mobile): clear derived data when the app or update changes, and catch render errors at the root - #1667
Conversation
janicduplessis
left a comment
There was a problem hiding this comment.
Fresh review of #1667 against #1660. No [bug] findings. pnpm test (162 passed), typecheck and lint in apps/mobile are green on this branch.
What I checked and found correct:
- Clear order.
src/storage.tsis the onlycreateMMKVcaller left.hooks/mac-connection.tsxandhooks/notifications.tsximport it, so ES module evaluation runs the clear beforecreateMachineStorereads the status cache and beforepushedMacsis read at module scope.notifications.tsxreaches storage throughmac-connectionfirst, and that order is also fine. - Marker. A new build, an OTA and a rollback to embedded each change
Updates.updateId. The embedded update has its own id when updates are enabled. In dev builds with updates off,updateIdis null, so the marker stays the same and nothing is cleared across Metro reloads. That is acceptable for dev. A crash after the marker is written, followed by an expo-updates rollback, clears again on the next launch, which is correct. - ErrorBoundary. expo-router's
fromImport(node_modules/expo-router/build/useScreens.js:210-221) wraps the root layout's default export in<Try catch={ErrorBoundary}>. That puts every provider inside the boundary, includingLocalNotifier's effect where the #1660 throw happens. React boundaries also catch effect errors. The fallback renders outsideSettingsProviderandThemeProvider, butEmptyState,Button,TextandTouchonly use Unistyles, so it renders without them. - Fingerprint and dependencies.
expo-updates,expo-constantsandreact-native-mmkvwere already dependencies. The diff adds no package and touches no native config. - Test. It feeds build 13's persisted notify shape to the real
localNotifications, so it fails when thestateclear is removed, and it checks the same-marker no-op. That matches the repo's test policy.
Findings:
-
[should-fix]
apps/mobile/src/lib/derived-data.ts:29clearspushed:*, and doing so can cause duplicate notifications.pushed:<id>is a boolean that records server-side state: that Mac holds this phone's push registration. It has no JS-defined shape that a new bundle could misread. The Mac keeps its registration across an app update. The doc comment onpushedMacs(hooks/notifications.tsx:133) says those keys exist "so a relaunch does not notify what they push". After an update,pushedMacsstarts empty, soLocalNotifierruns the status rules with the phone's categories untilpush.registerresolves (notifications.tsx:300-305). Two cases produce duplicates:- Within that window, any event is notified by the phone and also pushed by the Mac. The window is short, but it is real.
- If
push.registerfails with a non-RequestError, for example because the socket drops mid-request,pushedstays false for that connection while the Mac keeps pushing. Duplicates then continue until a later registration succeeds.
The first
LocalNotifierrun after the clear is a baseline (overseewithprevious === null), so the launch itself is quiet; the risk is limited to the cases above. Suggest keepingpushed:*and clearing onlystateinstim.notifications. The test's expected keys would become['prefs', 'pushToken', 'pushed:mac']. The PR body's table and theclearDerivedDataOnChangedoc comment would need the same change. -
[should-fix]
apps/mobile/README.md:30-34says a cold launch shows each machine's last status, dimmed with "Last seen". That is no longer true on the first launch after an app update or OTA, when the saved status is cleared. Add one sentence, for example: "An app update clears the saved status, so the first launch after one shows no rows until each machine connects." It may also help to note in the notifications section (around line 322) that notification state resets on an update. -
[nit]
apps/mobile/src/app/_layout.tsx:21-29: Try again re-renders the same tree against the same MMKV contents. A throw caused by persisted data under the current marker loops back to the error screen. This does not block the PR, because the marker clear covers the crash in #1660. If a persisted payload can still throw, one option is to haveretryalso clear the derived keys. -
[nit]
apps/mobile/src/lib/derived-data.test.ts:29stores'pushed:mac'as the string'true'. The app writes the booleantrue(notifications.tsx:303). This is harmless because the code only checks key names, but matching the real value keeps the fixture faithful to what build 13 wrote. -
[nit]
apps/mobile/src/lib/derived-data.ts:26: only the test reads the boolean return value ofclearDerivedDataOnChange. The test could assert on store contents, which it already does, and the function could returnvoid. This is minor.
|
Review addressed in 1e6f354:
|
…atch render errors at the root The OTA built from fad82c4 read the notify state that build 13's JS stored under the same MMKV keys in an older shape, and oversee() threw on it. At startup the app now compares a marker of the running JS (app version, runtime, update id) with the one it stored, and on a mismatch clears the status cache, the notify state and the push acceptances before any module reads them. Notification settings, the push token and everything in SecureStore stay. A root ErrorBoundary shows the error with a retry instead of a fatal exception that expo-updates turns into an abort. Fixes #1660
…t clears the previous release's data
1e6f354 to
5cb83d7
Compare
janicduplessis
left a comment
There was a problem hiding this comment.
Fresh review of 5cb83d7 on the rebased branch. I found no bugs and nothing should-fix. There are two nits.
What I checked:
- Clear before read.
src/storage.tsis the onlycreateMMKVcaller in the app.mac-connection.tsx(the module-scopecreateMachineStorestatus cache read) andnotifications.tsx(readState,pushedMacs) take their instances from it, so the clear runs on import before either can read. In the boot test, thereact-native-mmkvmock is loaded lazily:storage.tsis first required duringrenderRouter, afterseedPreviousInstall(). The test proves this: the seeded marker is replaced and the seededstatus:mac1entry is gone. - Upgrade path. I disabled the
clearDerivedDataOnChange(...)call insrc/storage.tsand rannpx jest src/app-boot. The test fails atexpect(errors).toEqual([]). The captured error isTypeError: Cannot read properties of undefined (reading '/Users/dev/Developer/stim')atoversee (src/lib/oversight.ts:627), which is the production throw. I then restored the file. - parseNotifyState. No references remain anywhere in the repo (
git grep).readStateis back to the exact pre-#1662 body. - Checks.
npm run lint,npm run typecheckandnpm test(22 suites, 163 tests) all pass. - README. The statements match the code. The rollback limitation from the PR body (build 13's embedded JS does not clear) is not in the README. That is fine, because the README describes this code's behavior.
Nits:
-
[nit]
apps/mobile/src/app-boot.test.tsx:241: With the clear disabled, Jest does not report the TypeError above. It reportsCouldn't find a LinkingContext context.The new rootErrorBoundarymakes React logCaught error:with an errorInfo object, and Jest's diff throws while pretty-printing that object. The test still fails, but the message points away from the cause. Mapping the captured args to strings before the assertion would surface the real error, for exampleerrors.map((args) => args.map((a) => (a instanceof Error ? a.message : typeof a === 'string' ? a : typeof a))). -
[nit]
apps/mobile/src/app-boot.test.tsx:244andapps/mobile/README.md:647: The test name and the README say the launch clears the cached status "before reading" it. The assertion only checks that the MMKV key is gone at the end, which would also pass if the clear ran aftercreateMachineStorehad loaded the old entry into memory. The notify state ordering is proven, because the old-shape throw would fire. The status cache ordering is guaranteed by the import structure, not by this test. Two ways to close the gap: assert that no cached "Last seen" row renders before the socket opens, or word the README and test name to match what is asserted.
|
Both re-review nits are addressed in e3c9125:
|
Description
The production app (TestFlight build 13) aborts at launch after the OTAs built from e3da74c and fad82c4. This PR clears data the app derives and caches whenever the running JS changes (new build, OTA or rollback), instead of adding compatibility code for old stored shapes. It also adds a root error boundary so a render or effect throw no longer becomes a hard abort.
The crash reports resolve against build 13's binary to expo-updates'
ErrorRecovery.crash(), which re-raises a JS fatal that error recovery couldn't fall back from. The confirmed throw is in fad82c4, atoversight.ts:627:previous?.workspaces[env.path]. Build 13's JS stored the notify state in MMKV (stim.notifications/state) under the samestatus:<id>keys, but in an older shape with noworkspaces. The new JS reads it back unchecked and throws once a live status arrives with notifications on. No test fed a previous version's persisted bytes to the new code.Solution
src/storage.tsowns the MMKV instances. On import, it compares a marker of the running JS (expoConfig.version,Updates.runtimeVersion,Updates.updateId) with the one stored instim.app. On a mismatch, it clears the derived keys.createMachineStorereading the status cache,pushedMacs).updateId.stim.statusstim.notificationsstateprefs,pushToken,pushed:*,handledResponsepushed:*stays because the Mac keeps the registration across an app update; clearing it would make the phone repeat what the Mac pushes until it registers again.handledResponsestays so a relaunch after an update doesn't re-open the last tapped notification.This replaces the notify state shape check from #1662 (
parseNotifyStateand its test): with the clear, no code reads another version's shapes, so there is nothing to parse defensively.A root
ErrorBoundary, exported fromapp/_layout.tsx, turns a render or effect throw into an error screen with Try again, instead of a fatal that expo-updates converts into an abort. Throws outside React, such as in the WebSocket message handler, are still fatal.Limitations:
npx expo-updates runtimeversion:resolve --platform iosprints87cad17f256996c2023667c721a6115c4dcd0015before and after), so it can ship as an OTA onto build 13.Not yet confirmed
LocalNotifierrun happens before the pairings load and rewritesstatewith{}. iOS starts the JSinactive, so on a phone the notifier can first run after the pairings load. The boot test from ci(mobile): roll back or republish OTA updates from the release workflow, and gate publishes on a boot test #1663 models that.Test plan
src/lib/derived-data.test.ts:prefs,pushTokenandpushed:*keeps onlyprefs,pushTokenandpushed:*, andlocalNotificationson a live status no longer throws. The test fails when thestateclear is removed.src/app-boot.test.tsxnow seeds another release's marker along with its old-shape notify state and cached status. It asserts the launch clears the cache and the state before reading them, keeps the pairing and the preferences, and reports no error. With the clear disabled, it fails on the old-shape throw.stim-serverbuilt from main.LocalNotifier's effect shows the error screen, and the process stays alive.Fixes #1660