[Structural Sharing] PR 1: Centralized key utilities with bidirectional indexes#761
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Ready to review @Julesssss @tgolen |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd7cbbbc71
ℹ️ 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".
lib/OnyxKeys.ts
Outdated
| for (const collectionKey of collectionKeySet) { | ||
| if (isCollectionMemberKey(collectionKey, key)) { | ||
| memberToCollectionKeyMap.set(key, collectionKey); |
There was a problem hiding this comment.
Resolve member keys to the most specific collection prefix
registerMemberKey() assigns the collection by taking the first startsWith() match from collectionKeySet, which is order-dependent and incorrect for valid overlapping collection keys (for example, test_ and test_level_). In that case, registering test_level_1 can be cached under test_; because getCollectionKey() returns cached mappings first, later collection routing (callbacks, grouping, RAM-only checks) uses the wrong parent collection. This matching logic should prefer the longest compatible collection key, not the first iterated one.
Useful? React with 👍 / 👎.
| function setCollectionKeys(keys: Set<OnyxKey>): void { | ||
| collectionKeySet = keys; |
There was a problem hiding this comment.
Clear stale member indexes when collection keys are reset
setCollectionKeys() only replaces collectionKeySet and leaves memberToCollectionKeyMap/collectionToMembersMap intact. If Onyx.init() runs again with a different key schema, old member mappings remain in the fast path and getCollectionKey() can return collection keys that are no longer registered, causing misclassification of subsequent updates. Reinitializing or rebuilding both indexes when setting new collection keys is needed to avoid stale lookups.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Onyx.init() should never be called again in the same session so we're safe.
| const collectionKey = getCollectionKey(key); | ||
| if (collectionKey) { | ||
| return ramOnlyKeySet.has(collectionKey); | ||
| } | ||
| return ramOnlyKeySet.has(key); |
There was a problem hiding this comment.
How about simplifying this a little?
return ramOnlyKeySet.has(key) || ramOnlyKeySet.has(getCollectionKey(key));
This also utilizes an early return for the simple key lookups without having to go through all the getCollectionKey() logic for every single key.
| test_: ['test_', ''], | ||
| test_level_: ['test_level_', ''], | ||
| test_level_1: ['test_level_', '1'], | ||
| test_level_2: ['test_level_', '2'], | ||
| test_level_last_3: ['test_level_last_', '3'], | ||
| test___FAKE__: ['test_', '__FAKE__'], | ||
| 'test_-1_something': ['test_', '-1_something'], | ||
| 'test_level_-1_something': ['test_level_', '-1_something'], |
There was a problem hiding this comment.
It would be nice to have a little comment for each of these to explain why the returned value is "correct".
Details
First PR of Expensify/App#86181
E/App PR: Expensify/App#86201
Related Issues
Expensify/App#86181
Automated Tests
Unit tests were added.
Manual Tests
Since it's 99% refactor there isn't anything specific to test, just assert the app loads and doesn't crash. Use Expensify/App#86201 for testing.
Author Checklist
### Related Issuessection aboveTestssectiontoggleReportand 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
Screen.Recording.2026-03-25.at.08.30.46-compressed.mov
Android: mWeb Chrome
Android Chrome is always crashing for me so I can't test
iOS: Native
Screen.Recording.2026-03-25.at.08.38.48-compressed.mov
iOS: mWeb Safari
Screen.Recording.2026-03-25.at.08.40.05-compressed.mov
MacOS: Chrome / Safari
Screen.Recording.2026-03-25.at.08.41.19-compressed.mov
Screen.Recording.2026-03-25.at.08.43.21-compressed.mov