Conversation
rafecolton
requested review from
a team,
luacmartins and
mountiny
and removed request for
Valforte
![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: c676a20aa8
ℹ️ 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".
| // 2.1 However, if a default key was explicitly set to null, we need to reset it to the default value | ||
| for (const key of allKeys) { | ||
| const isKeyToPreserve = keysToPreserve.includes(key); | ||
| const isKeyToPreserve = keysToPreserve.some((preserveKey) => OnyxUtils.isKeyMatch(preserveKey, key)); |
There was a problem hiding this comment.
If an app defines overlapping collection prefixes (which this repo explicitly supports in tests/unit/onyxUtilsTest.ts:73-78, e.g. test_ and test_level_), OnyxUtils.isKeyMatch() will treat test_level_1 as a match for test_ because it only checks startsWith (lib/OnyxUtils.ts:557-559). After this change, Onyx.clear([someCollectionKey]) will therefore keep members of any longer collection whose prefix starts the same way, so unrelated data can survive a clear/sign-out whenever collection keys overlap.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Hm, this is true, though I'm not sure it's actually a problem
There was a problem hiding this comment.
I think it's a safe change because you are passing the collection key to isKeyMatch and it also check if the passed key is a collection one
| // 2.1 However, if a default key was explicitly set to null, we need to reset it to the default value | ||
| for (const key of allKeys) { | ||
| const isKeyToPreserve = keysToPreserve.includes(key); | ||
| const isKeyToPreserve = keysToPreserve.some((preserveKey) => OnyxUtils.isKeyMatch(preserveKey, key)); |
There was a problem hiding this comment.
I found one more place where we should probably change the implementation:
lib/DevTools/RealDevTools.ts
/**
* This clears the internal state of the DevTools, preserving the keys included in `keysToPreserve`
*/
clearState(keysToPreserve: string[] = []): void {
const newState = Object.entries(this.state).reduce((obj: Record<string, unknown>, [key, value]) => {
// eslint-disable-next-line no-param-reassign
obj[key] = keysToPreserve.includes(key) ? value : this.defaultState[key];
return obj;
}, {});
this.registerAction('CLEAR', undefined, newState);
}3 participants
Details
Supports adding collections to keysToPreserve to alleviate the need to add each key individually.
cc @chuckdries @dariusz-biela
Related Issues
Required for Expensify/App#86005
Automated Tests
Added unit tests
Manual Tests
Tested manually with App PR:
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