remove remaining inline selectors #72546
Open
+162
−84
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
melvin-bot
bot
requested review from
dukenv0307
and removed request for
a team
|
@dukenv0307 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] |
|
3 more places App/src/hooks/usePersonalPolicy.ts Line 19 in d057ac4 |
dukenv0307
reviewed
Oct 15, 2025
| } | ||
| }); | ||
|
|
||
| const transactionViolationSelector = (violations: OnyxCollection<TransactionViolations>) => { |
dukenv0307
reviewed
Oct 15, 2025
| * `true` means the onboarding flow is loading | ||
| * `false` means the onboarding flow is not loading | ||
| */ | ||
| function isOnboardingLoadingSelector(onboarding: OnyxValue<typeof ONYXKEYS.NVP_ONBOARDING>): boolean | undefined { |
There was a problem hiding this comment.
why don't we move it to selectors folder?
There was a problem hiding this comment.
Good call, I missed the fact that it's not there
|
Updated ✅ |
roryabraham
requested changes
Oct 16, 2025
There was a problem hiding this comment.
@TMisiukiewicz Can you please upgrade eslint-config-expensify to include the no-inline-useOnyx-selector rule we added in Expensify/eslint-config-expensify#160, now that the ESLint 9 upgrade is complete?
|
|
|
@roryabraham amazing! Bumped version to 2.0.92 |
TMisiukiewicz
force-pushed
the
remove-remaining-inline-selectors
branch
from
2e64e20 to
6133146
Compare
|
@TMisiukiewicz Can you please fix the eslint check? |
|
Let's see if it works now |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-10-18.at.17.56.34.movAndroid: mWeb ChromeScreen.Recording.2025-10-18.at.17.54.55.moviOS: HybridAppScreen.Recording.2025-10-18.at.17.56.44.moviOS: mWeb SafariScreen.Recording.2025-10-18.at.17.55.11.movMacOS: Chrome / SafariScreen.Recording.2025-10-18.at.17.54.29.movMacOS: DesktopScreen.Recording.2025-10-18.at.17.59.34.mov |
|
Looks good, but we have a conflict |
|
resolved |
dukenv0307
approved these changes
Oct 20, 2025
carlosmiceli
previously approved these changes
Oct 20, 2025
roryabraham
reviewed
Oct 20, 2025
| import BaseListItem from './BaseListItem'; | ||
| import type {ListItem, UserListItemProps} from './types'; | ||
|
|
||
| const reportExistsSelector = (report: OnyxEntry<Report>) => !!report; |
There was a problem hiding this comment.
NAB but maybe it's cleaner architecturally and will naturally lead to more reuse if all pure selectors (i.e: no dependencies on component props or state) are declared in the @selectors directory/namespace.
| (policies: OnyxCollection<Policy>) => { | ||
| return Object.values(policies ?? {}).some((policy) => isPaidGroupPolicy(policy) && isPolicyAdmin(policy, currentUserPersonalDetails.login)); | ||
| }, | ||
| [currentUserPersonalDetails.login], |
There was a problem hiding this comment.
The leaves me wondering if we could improve selector composition further and promote reuse. Because this selector "is the current user an admin of a paid policy" is not specific only to this component, and should not be.
I think you could compose this into a custom hook:
function useIsCurrentUserPolicyAdmin() {
// NAB TODO: add a selector for just `currentUserLogin`?
const currentUserPersonalDetails = useCurrentUserPersonalDetails();
const isUserPaidPolicyAdminSelector = useCallback(
(policies: OnyxCollection<Policy>) => Object.values(policies ?? {}).some((policy) => isPaidGroupPolicy(policy) && isPolicyAdmin(policy, currentUserPersonalDetails.login)),
[currentUserPersonalDetails.login],
);
const [isCurrentUserPolicyAdmin = false] = useOnyx(ONYXKEYS.COLLECTION.POLICY, {
canBeMissing: true,
selector: isUserPaidPolicyAdminSelector,
});
return isCurrentUserPolicyAdmin;
}There was a problem hiding this comment.
Also, maybe it should be using session instead of currentUserPersonalDetails?
| @@ -1,9 +1,9 @@ | |||
| import {createPoliciesSelector} from '@selectors/Policy'; | |||
There was a problem hiding this comment.
I find it odd/suspicious that this would be the first import (before react)
| } | ||
| }); | ||
|
|
||
| const transactionViolationSelector = useCallback( |
There was a problem hiding this comment.
Hmmm so now I'm wondering - does this selector get re-created every time we import and use this hook? Ideally we want all uses of this hook to result in 1 cached selector, but I don't think that's the case here.
So I feel like there's some piece we're missing to properly compose selectors for reuse....
|
Will get back to this as soon as possible. Still working on some critical stuff |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation of Change
Migrating the remaining inline selectors to stable selectors. They were most likely accidentally missed during migration or created while the migration was ongoing.
Fixed Issues
$ #70272
PROPOSAL:
Tests
Offline tests
n/a
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
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.mov
Android: mWeb Chrome
iOS: Native
ios.mov
iOS: mWeb Safari
ios.web.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov