Implement RAM-only key support to replace initWithStoredValues #725
Conversation
… from initStoreValues
melvin-bot
bot
requested review from
puneetlath
and removed request for
a team
![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: 2edcd51d05
ℹ️ 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".
| function isRamOnlyKey(key: OnyxKey): boolean { | ||
| try { | ||
| const collectionKey = getCollectionKey(key); | ||
| // If collectionKey exists for a given key, check if it's a RAM-only key | ||
| return cache.isRamOnlyKey(collectionKey); | ||
| } catch { |
There was a problem hiding this comment.
The new isRamOnlyKey only checks the collection key when the input is a collection member. If a caller includes a specific member key in ramOnlyKeys (as implied by the docstring “RAM-only collection member”), getCollectionKey succeeds and the function returns cache.isRamOnlyKey(collectionKey), ignoring the member entry. In that scenario, the member will still be persisted to storage, which violates the RAM-only contract for that key. Consider checking cache.isRamOnlyKey(key) as well when the key is a collection member so explicitly marked members are honored.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think this makes sense to clarify @JKobrynski , either the docs should be updated or the individual keys should be updated
There was a problem hiding this comment.
I'm not sure I understand this problem correctly, this function is tested and should work correctly for RAM-only keys, RAM-only collection keys, and RAM-only collection member keys, the tests pass for all cases listed above. I updated the comment so every supported key is listed and added some examples. Let me know if that's what you meant, if not I will change it, thanks!
There was a problem hiding this comment.
I agree with @JKobrynski , I don't understand the comment because tests are actually covering all possible scenarios.
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Thorough Review of RAM-only Key Support Implementation
Overall this is a well-structured PR with good test coverage. I've left several comments on specific areas. Here's a summary:
Comments Left
| Issue | Priority | Description |
|---|---|---|
| #2 | Medium | Pre-existing storage data not cleaned up when keys transition to RAM-only |
| #4 | Low | O(n) filtering in multiSetWithRetry on every call |
| #5 | Low | remove() doesn't skip storage operations for RAM-only keys |
| #6 | Medium | Missing test for Onyx.merge() with RAM-only key |
| #7 | Medium | Missing test for clear() with RAM-only keys |
| #8 | Info | Memory accumulation for RAM-only keys should be documented |
| #10 | Low | Documentation could be clearer about collection member behavior |
Not Commented (excluded per request)
- Issue 1 (individual collection member RAM-only handling) - there's already an unresolved comment about this
- Issue 3 (try-catch performance) - minor optimization
- Issue 9 (DevTools action handling) - correctly implemented
Nice work on the implementation! The core functionality looks solid.
| initDevTools(enableDevTools); | ||
|
|
||
| Storage.init(); | ||
|
|
There was a problem hiding this comment.
Issue 2: Missing handling for pre-existing storage data after upgrade
If a user upgrades their app and a key that was previously persisted is now marked as RAM-only, the stale data remains in storage indefinitely. While it won't be read (since Onyx will skip storage operations), it's wasted space.
Consider adding a cleanup step here that removes RAM-only keys from storage if they already exist:
cache.setRamOnlyKeys(new Set<OnyxKey>(ramOnlyKeys));
// Clean up any pre-existing RAM-only keys from storage
if (ramOnlyKeys.length > 0) {
Storage.getAllKeys().then((keys) => {
const keysToRemove = Array.from(keys).filter(key => OnyxUtils.isRamOnlyKey(key));
if (keysToRemove.length > 0) {
Storage.removeItems(keysToRemove);
}
});
}This ensures that if a key transitions from persisted to RAM-only between app versions, the old data is cleaned up.
There was a problem hiding this comment.
Good point, I added the clean up logic and a unit test for it
| ]); | ||
| }); | ||
|
|
||
| it('should not save a RAM-only collection to storage', async () => { |
There was a problem hiding this comment.
Issue 6: Missing test for Onyx.merge() with RAM-only key
The tests cover set, multiSet, setCollection, mergeCollection, and Onyx.update(), but I don't see a dedicated test for Onyx.merge() with a RAM-only key.
The merge operation has its own code path in OnyxMerge/index.ts and OnyxMerge/index.native.ts, so it would be good to have explicit test coverage:
it('should not save a RAM-only key to storage when using merge', async () => {
await Onyx.merge(ONYX_KEYS.RAM_ONLY_TEST_KEY, {someProperty: 'value'});
expect(cache.get(ONYX_KEYS.RAM_ONLY_TEST_KEY)).toEqual({someProperty: 'value'});
expect(await StorageMock.getItem(ONYX_KEYS.RAM_ONLY_TEST_KEY)).toBeNull();
});
it('should not save a RAM-only collection member to storage when using merge', async () => {
const collectionMemberKey = `${ONYX_KEYS.COLLECTION.RAM_ONLY_COLLECTION}1`;
await Onyx.merge(collectionMemberKey, {data: 'test'});
expect(cache.get(collectionMemberKey)).toEqual({data: 'test'});
expect(await StorageMock.getItem(collectionMemberKey)).toBeNull();
});| .filter((key) => !keysToPreserve.includes(key)) | ||
| .filter((key) => !keysToPreserve.includes(key) && !OnyxUtils.isRamOnlyKey(key)) | ||
| .reduce((obj: KeyValueMapping, key) => { | ||
| // eslint-disable-next-line no-param-reassign |
There was a problem hiding this comment.
Issue 7: Missing test for clear() with RAM-only keys that have defaults
This line filters out RAM-only keys from being reset to defaults during clear(). It would be good to have a test case that verifies:
- RAM-only keys with default values are NOT written to storage during
clear() - RAM-only keys in cache are properly reset to their default values (or remain undefined if no default)
Example test:
it('should handle RAM-only keys with defaults correctly during clear', async () => {
// Set a value for RAM-only key
await Onyx.set(ONYX_KEYS.RAM_ONLY_TEST_KEY, 'some value');
await Onyx.clear();
// Verify it's not in storage
expect(await StorageMock.getItem(ONYX_KEYS.RAM_ONLY_TEST_KEY)).toBeNull();
// Verify cache state based on whether there's a default
});| ONYXKEYS.RAM_ONLY_KEY_1, | ||
| ONYXKEYS.RAM_ONLY_KEY_2, | ||
| ONYXKEYS.COLLECTION.RAM_ONLY_KEY_2, | ||
| ], |
There was a problem hiding this comment.
Issue 8: Memory consideration worth documenting
RAM-only keys are stored in the cache indefinitely. If an application creates many ephemeral RAM-only collection members (e.g., tempData_1, tempData_2, ..., tempData_10000), they will accumulate in memory until Onyx.clear() is called.
This is expected behavior, but it might be worth adding a note here like:
Note: RAM-only keys still consume memory and will remain in cache until explicitly cleared or until
Onyx.clear()is called. Use them judiciously for truly ephemeral data.
This helps users understand that "RAM-only" means "in memory" not "automatically garbage collected".
| ``` | ||
|
|
||
| ### Using RAM-only keys | ||
|
|
There was a problem hiding this comment.
Issue 10: Documentation could be clearer about collection behavior
The current docs say "It also works for collections" but it's not explicit about:
- Whether you can mark individual collection members as RAM-only (e.g.,
report_123) - Or if you must mark the entire collection key (e.g.,
report_)
Looking at the implementation of isRamOnlyKey, it appears that marking a collection key makes ALL members of that collection RAM-only. It might be helpful to clarify this:
You can mark entire collections as RAM-only by including the collection key (e.g.,
ONYXKEYS.COLLECTION.TEMP_DATA). This will make all members of that collection RAM-only. Individual collection member keys cannot be selectively marked as RAM-only.
Or if individual members ARE supported, update the implementation and add an example like:
ramOnlyKeys: [
ONYXKEYS.COLLECTION.TEMP_DATA, // Entire collection is RAM-only
`${ONYXKEYS.COLLECTION.REPORT}special123`, // Just this one member
],There was a problem hiding this comment.
Issue 4: O(n) filtering on every multiSet call
The new filter in multiSetWithRetry runs on every multiSet call, iterating through all key-value pairs. Each isRamOnlyKey call may also iterate through collection keys via getCollectionKey.
For large batch operations (e.g., syncing many reports), this compounds. While acceptable for now since RAM-only keys are expected to be rare, if performance becomes an issue in the future, consider:
- Caching a Set of collection key prefixes for O(1) prefix matching
- Or doing a single pass that both filters and builds the storage array
Low priority - just flagging for awareness.
Issue 5: Consider adding RAM-only check to remove()
When a RAM-only key is set to null (e.g., via Onyx.set(RAMONLY_KEY, null)), the code path goes through remove() which calls Storage.removeItem(). This attempts to remove a key from storage that was never stored there - it's wasteful though not incorrect.
Consider adding a RAM-only check in remove():
function remove<TKey extends OnyxKey>(key: TKey, isProcessingCollectionUpdate?: boolean): Promise<void> {
cache.drop(key);
scheduleSubscriberUpdate(key, undefined as OnyxValue<TKey>, undefined, isProcessingCollectionUpdate);
if (isRamOnlyKey(key)) {
return Promise.resolve();
}
return Storage.removeItem(key).then(() => undefined);
}Low priority since the storage operation will just be a no-op.
There was a problem hiding this comment.
I added that additional condition to remove. About the multiSetWithRetry optimisation, do you think we should do it in this PR? If so, I can address it. If we'd be ok with doing this as a followup improvement, I can assign myself to it and take care of it as soon as I finish working on the higher priority Onyx issues. What would you prefer?
There was a problem hiding this comment.
I think we can confirm Issue 4 with Reassure – we already have a test for multiSet and it didn't report regressions, right?
There was a problem hiding this comment.
That's correct! However, the perf tests don't include RAM-only keys, we should probably add them right?
| // Clean up any pre-existing RAM-only keys from storage | ||
| if (ramOnlyKeys.length > 0) { | ||
| Storage.getAllKeys().then((storedKeys) => { | ||
| const keysToRemove = Array.from(storedKeys).filter((key) => OnyxUtils.isRamOnlyKey(key)); | ||
| if (keysToRemove.length > 0) { | ||
| Storage.removeItems(keysToRemove); | ||
| } | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't really get this... is this just temporary? Otherwise, how could RAM only keys be in storage at all?
There was a problem hiding this comment.
If it's only temporary, then I think we need a better way of doing this. I think it should be done as an Onyx migration.
There was a problem hiding this comment.
I don't think this is temporary, it's for the scenario when you have a regular key and you decide to convert it into a RAM-only key - it needs to be removed from storage. It's not just about rolling out this new feature, that kind of conversion might happen anytime, so we need a permanent solution. Do you think we should change it?
There was a problem hiding this comment.
Ok the scenario @JKobrynski described is real but I agree that it's probably more appropriate to deal these scenarios with a migration file in Onyx. Since this piece of code will add some overhead to initialisation every single time, maybe it's better to just put a comment above ramOnlyKeys when declaring it in E/App code, that every addition there should be followed with a migration file.
There was a problem hiding this comment.
Thanks, Fabio, I agree. It's not the job of internal Onyx code to handle and respond to a consumer adding/removing RAM only keys from the config. I think it's up to the consumer to manage that.
| // Clean up any pre-existing RAM-only keys from storage | ||
| if (ramOnlyKeys.length > 0) { | ||
| Storage.getAllKeys().then((storedKeys) => { | ||
| const keysToRemove = Array.from(storedKeys).filter((key) => OnyxUtils.isRamOnlyKey(key)); |
There was a problem hiding this comment.
Isn't keysToRemove the same as ramOnlyKeys?
There was a problem hiding this comment.
Not really, we filter ramOnlyKeys that are saved in storage (they used to be regular keys if present in storage) and that filtered list is keysToRemove, so it's just a part of ramOnlyKeys. However, it might be a good idea to do the following:
Storage.removeItems(ramOnlyKeys);That eliminates the need to loop through all keys in storage. What do you think? CC: @mountiny @fabioh8010
| const defaultKeyValuePairs = Object.entries( | ||
| Object.keys(defaultKeyStates) | ||
| .filter((key) => !keysToPreserve.includes(key)) | ||
| .filter((key) => !keysToPreserve.includes(key) && !OnyxUtils.isRamOnlyKey(key)) |
There was a problem hiding this comment.
Why exclude the RAM only keys here?
There was a problem hiding this comment.
Because defaultKeyValuePairs are passed to Storage.multiSet(defaultKeyValuePairs) couple of lines below, we want to avoid saving RAM-only keys to storage.
There was a problem hiding this comment.
Ah, I see. Would you mind adding that as a code comment so that it's obvious to others?
| enablePerformanceMetrics = false, | ||
| enableDevTools = true, | ||
| skippableCollectionMemberIDs = [], | ||
| ramOnlyKeys = [], |
There was a problem hiding this comment.
Be sure to run the automated documentation script. I expected the docs to be updated with this parameter. It would also be great to give it some comments.
There was a problem hiding this comment.
Thanks, I ran the script and pushed the changes. I added this comment in the lib/types.ts file, do you think we should add anything else?
There was a problem hiding this comment.
Yeah, that looks good. Thanks!
…91-implement-ramonly-key-support
6 participants
Details
Related Issues
GH_LINK Expensify/App#80091
Automated Tests
Unit tests covering the newly added features were added in this PR.
Manual Tests
This is a new feature and is not used in the E/App yet, so it can't be tested there.
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop