#1 - Remove Onyx.connect() for the key POLICY in Policy.ts#81799
#1 - Remove Onyx.connect() for the key POLICY in Policy.ts#81799bernhardoj wants to merge 2 commits intoExpensify:mainfrom
Conversation
melvin-bot
bot
requested review from
a team,
joekaufmanexpensify and
parasharrajat
and removed request for
a team
|
@parasharrajat Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
melvin-bot
bot
requested review from
cristipaval
and removed request for
a team
|
@cristipaval Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| [accountID], | ||
| ); | ||
| const [ownerPolicies] = useOnyx(ONYXKEYS.COLLECTION.POLICY, {canBeMissing: true, selector}); | ||
| const [policies] = useOnyx(ONYXKEYS.COLLECTION.POLICY, {canBeMissing: true}); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
The component subscribes to the entire POLICY collection without a selector, causing unnecessary re-renders when any policy changes, even if the owned paid policies haven't changed.
Suggested fix:
Use a selector in the useOnyx hook to filter policies at the subscription level:
const selector = useCallback(
(policies: OnyxCollection<Policy>) => {
return ownerPoliciesSelector(policies, accountID);
},
[accountID],
);
const [ownerPolicies] = useOnyx(ONYXKEYS.COLLECTION.POLICY, {canBeMissing: true, selector});For the deleteWorkspace call, you can access the full policies collection via a separate subscription if needed, or reconsider if deleteWorkspace truly needs the entire policies collection (it may only need specific policies).
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Can you check this, too? Do we need to pass the whole policies to the deleteWorkspace function?
![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: 9f81d52566
ℹ️ 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".
tests/actions/PolicyTest.ts
Outdated
| Policy.deleteWorkspace({ | ||
| policies: {[fakePolicy.id]: fakePolicy}, |
There was a problem hiding this comment.
Here the test passes policies keyed by raw policy.id, but deleteWorkspace now reads policies via the full Onyx collection key (policies?.[`${ONYXKEYS.COLLECTION.POLICY}${policyID}`] in src/libs/actions/Policy/Policy.ts). That means policy becomes undefined in the action, so fields like workspaceAccountID and oldPolicyName are missing and the optimistic/failure data no longer mirrors production behavior. This can make the test assertions unreliable or fail once the action relies on those fields. Use the full Onyx key (e.g., {[`${ONYXKEYS.COLLECTION.POLICY}${id}`]: policy}) to match real shape.
Useful? React with 👍 / 👎.
| deleteWorkspace({ | ||
| policies: {[policy.id]: policy}, | ||
| policyID: policy.id, |
There was a problem hiding this comment.
This test also passes policies keyed by raw policy.id, but deleteWorkspace now reads from the Onyx collection key prefix (policy_...). With the current shape, the action sees policy as undefined, so it won't populate workspaceAccountID/oldPolicyName, diverging from real app behavior and potentially masking regressions. Build the map with full Onyx keys to keep the test realistic.
Useful? React with 👍 / 👎.
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
2 participants
Explanation of Change
Fixed Issues
$ #66574
PROPOSAL:
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
Prerequisite: have a workspace
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.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
web.mp4