Skip keyless update entries in updateSnapshots instead of crashing - #818
Skip keyless update entries in updateSnapshots instead of crashing#818elirangoshen wants to merge 4 commits into
Conversation
Onyx.update already skips entries without a string key (and clear/multiset entries legitimately have none), but passed the unfiltered array to updateSnapshots, which crashed on key.startsWith when a snapshot was cached. Fixes APP-6NT Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| // `clear`/`multiset` updates legitimately carry no key (and `Onyx.update` skips other keyless | ||
| // entries), so ignore them here too instead of crashing on `key.startsWith` below. |
| // A keyless entry (e.g. a malformed server update) used to crash updateSnapshots with | ||
| // "can't access property 'startsWith', key is undefined" and reject the whole update. |
| if (typeof key !== 'string') { | ||
| continue; |
There was a problem hiding this comment.
Please, add Log message, same as it's done in Onyx.update
|
|
||
| for (const {key, value} of data) { | ||
| if (typeof key !== 'string') { | ||
| Logger.logInfo(`Invalid ${typeof key} key provided in Onyx update. Key must be of type string. Skipping snapshot update for this entry.`); |
There was a problem hiding this comment.
NAB: Maybe its worth to log something extra here instead of only typeof key? onyxMethod? Something to help with debugging later? Trimmed String(key) maybe?
There was a problem hiding this comment.
good idea, done
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 758d11fc5a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // clear/multiset entries legitimately carry no key; snapshots have nothing to update for them | ||
| if (onyxMethod !== METHOD.CLEAR && onyxMethod !== METHOD.MULTI_SET) { | ||
| Logger.logInfo(`Invalid ${typeof key} key (method: ${onyxMethod}, key: ${String(key).slice(0, 50)}) provided in Onyx update. Skipping snapshot update for this entry.`); | ||
| } | ||
| continue; |
There was a problem hiding this comment.
When an Onyx.update batch uses a keyless METHOD.MULTI_SET, the update handler in lib/Onyx.ts still applies every key from the entry's value, but this new guard drops the whole entry before updateSnapshots can inspect those keys. If any of the multiset keys is present in a cached snapshot, the real Onyx value is updated while the snapshot keeps the old value, so search/snapshot consumers can show stale data after a successful update; handle MULTI_SET by expanding its payload into per-key snapshot updates instead of continuing here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
ok, so this is not a regression with this code, so we could choose to handle it in a follow-up. What we have here is strictly better than what we have on main, which errors in this case.
That said, we probably should update updateSnapshots to work with multiSet and clear, which would be strictly better than just avoiding a crash. Right @elirangoshen ?
|
@roryabraham could you review this PR? I'm not familiar with this code or what it's trying to achieve |
| if (typeof key !== 'string') { | ||
| // clear/multiset entries legitimately carry no key; snapshots have nothing to update for them | ||
| if (onyxMethod !== METHOD.CLEAR && onyxMethod !== METHOD.MULTI_SET) { | ||
| Logger.logInfo(`Invalid ${typeof key} key (method: ${onyxMethod}, key: ${String(key).slice(0, 50)}) provided in Onyx update. Skipping snapshot update for this entry.`); |
There was a problem hiding this comment.
| Logger.logInfo(`Invalid ${typeof key} key (method: ${onyxMethod}, key: ${String(key).slice(0, 50)}) provided in Onyx update. Skipping snapshot update for this entry.`); | |
| Logger.logWarn(`Invalid ${typeof key} key (method: ${onyxMethod}, key: ${String(key).slice(0, 50)}) provided in Onyx update. Skipping snapshot update for this entry.`); |
| // clear/multiset entries legitimately carry no key; snapshots have nothing to update for them | ||
| if (onyxMethod !== METHOD.CLEAR && onyxMethod !== METHOD.MULTI_SET) { | ||
| Logger.logInfo(`Invalid ${typeof key} key (method: ${onyxMethod}, key: ${String(key).slice(0, 50)}) provided in Onyx update. Skipping snapshot update for this entry.`); | ||
| } | ||
| continue; |
There was a problem hiding this comment.
ok, so this is not a regression with this code, so we could choose to handle it in a follow-up. What we have here is strictly better than what we have on main, which errors in this case.
That said, we probably should update updateSnapshots to work with multiSet and clear, which would be strictly better than just avoiding a crash. Right @elirangoshen ?
6 participants
Details
Came from this slack thread
Fixes a production crash in
OnyxUtils.updateSnapshots:TypeError: Cannot read properties of undefined (reading 'startsWith')(Sentry APP-6NT, ~94 events / ~35 users since 2026-03-01, plus duplicate groupings APP-JT0, APP-961, APP-H8X, APP-ED8 — all on web, mostly 2FA pages).Root cause:
Onyx.updatevalidates its entries and skips any with a non-stringkey(andclear/multisetentries legitimately carry no key at all), but then passes the unfiltered update array toOnyxUtils.updateSnapshots. There,OnyxKeys.isCollectionMemberKey(snapshotCollectionKey, key)callskey.startsWith(collectionKey)with no guard, so any keyless entry throws and rejects the wholeOnyx.updatepromise. It only reproduces when the snapshot collection is non-empty (i.e. the user has cached search snapshots), which is why the volume is low.In Expensify/App this surfaced as an unhandled rejection from
QueuedOnyxUpdates.flushQueue(the deferred WRITE flush), and Sentry mis-attributed the newest grouping (APP-JT0) to Expensify/App PR #97098, which merely touched those lines.Fix: in
updateSnapshots, skip entries wheretypeof key !== 'string'before callingisCollectionMemberKey— mirroring the per-entry validationOnyx.updatealready performs.Fixes APP-6NT
Related Issues
Expensify/App#98438
Linked E/App PR
Expensify/App#97857 (TESTING ONLY pin of this PR's HEAD, so the full E/App CI suite runs against it). The fix itself is self-contained in Onyx and requires no App-side change; for context, Expensify/App#97098 is the App PR that Sentry falsely attributed the newest crash grouping to.
Automated Tests
Added a regression test in
tests/unit/onyxTest.ts— "should skip update entries without a key when updating Snapshots instead of rejecting": dispatches anOnyx.updatecontaining a keylessmergeentry alongside a valid one while a snapshot is cached. Without the fix it reproduces the exact production crash (TypeError: Cannot read properties of undefined (reading 'startsWith')rejectingOnyx.update); with the fix,Onyx.updateresolves and the valid entry still lands in the snapshot.Verification run: 173 unit tests pass (
onyxTest+onyxUtilsTest),tsc --noEmitclean, ESLint clean, Prettier clean. Also verified the new test fails with the exact production stack trace when the guard is stashed.Manual Tests
On Android / iOS (native or mWeb) against staging:
Expected: no crash and no stuck/undelivered updates in any step. (Before this fix, a malformed keyless entry in any server update batch would crash the app with a
startsWithTypeError whenever a search had previously been run — steps 2–5 keep that code path continuously active.)Deterministic developer repro (web console)
snapshot_collection entry is cached.Onyx.update([{onyxMethod: 'merge', key: 'session', value: {}}, {onyxMethod: 'merge', value: {}}]).TypeError: Cannot read properties of undefined (reading 'startsWith')fromupdateSnapshots.QA Steps
Same as Manual Tests.
Author Checklist
### Related Issuessection above### Linked E/App PRsection above, and verified this change against it (E/App CI passed and manual testing completed)TestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
N/A — non-UI library fix, covered by the unit regression test.
Android: mWeb Chrome
N/A — non-UI library fix, covered by the unit regression test.
iOS: Native
Simulator.Screen.Recording.-.iPhone.17.-.2026-08-13.at.14.49.42.mov
iOS: mWeb Safari
N/A — non-UI library fix, covered by the unit regression test.
MacOS: Chrome / Safari
N/A — non-UI library fix, covered by the unit regression test.