perf: remove redundant O(employees) member scans#96080
Conversation
getPolicyEmployeeAccountIDs enumerated the full policy employeeList (~21k entries on large accounts) on every SettlementButton and usePaymentOptions render, feeding doesReportBelongToWorkspace — a helper built for the removed workspace switcher whose expensive DM branch is unreachable at every remaining call site. Replace both with field comparisons (isConciergeChatReport || isPolicyRelatedReport), harden isPolicyRelatedReport against undefined/_FAKE_ policyIDs, and switch composer mention weighting to an O(1) employeeList lookup.
# Conflicts: # src/hooks/usePaymentOptions.ts # src/pages/inbox/report/ReportActionCompose/SuggestionMention.tsx # tests/unit/PolicyUtilsTest.ts # tests/unit/SuggestionMentionTest.tsx
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.
|
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
adhorodyski
changed the title
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
melvin-bot
Bot
requested review from
sobitneupane
and removed request for
a team
|
@sobitneupane 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
JmillsExpensify
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const {accountID} = useCurrentUserPersonalDetails(); | ||
|
|
||
| // The app would crash due to subscribing to the entire report collection if chatReportID is an empty string. So we should have a fallback ID here. | ||
| // eslint-disable-next-line rulesdir/no-default-id-values |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The expression isConciergeChatReport(chatReport, conciergeReportID) || isPolicyRelatedReport(chatReport, policyID) is duplicated verbatim here and in src/components/SettlementButton/index.tsx, and the same workspace-membership check also appears in src/pages/inbox/report/ReportActionCompose/SuggestionMention.tsx. This logic previously lived in the now-removed doesReportBelongToWorkspace helper. Re-inlining it in multiple places violates DRY and means every future change must be applied in each copy.
Consolidate the check into a single reusable helper (e.g. a slimmed-down doesReportBelongToWorkspace(report, policyID, conciergeReportID) in @libs/ReportUtils) and call it here instead of duplicating the expression.
Reviewed at: 6a65d95 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
![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: 6a65d950b6
ℹ️ 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".
| const reportBelongsToWorkspace = policyID ? isConciergeChatReport(chatReport, conciergeReportID) || isPolicyRelatedReport(chatReport, policyID) : false; | ||
| const policyIDKey = reportBelongsToWorkspace ? policyID : CONST.POLICY.ID_FAKE; |
There was a problem hiding this comment.
When this hook is used by the report header/selection pay menus with an iouReport.policyID but a linked 1:1 chat whose policyID is missing or _FAKE_, this new check now falls through to CONST.POLICY.ID_FAKE. The removed helper used to classify those DM chats as belonging to the workspace when a participant was in the policy, and payment writes the preferred method under iouReport.policyID (PayMoneyRequest.ts stores NVP_LAST_PAYMENT_METHOD keyed by that policy), so the previously selected workspace payment method will no longer be read/preselected for those reports.
Useful? React with 👍 / 👎.
1 participant
Explanation of Change
Every rendered
SettlementButton(report header, each report preview, each Pay row on the Reports page) calledgetPolicyEmployeeAccountIDs, which walks the whole policy employee list. On a ~21k-member policy that is a 21k-entry array built per button, per render — and the result was thrown away unused.The array fed one function:
doesReportBelongToWorkspace. It was built for the workspace switcher (its call site was removed in #79940) and only reads the array for DMs — a case that cannot occur at any of its remaining call sites, because every caller derivespolicyIDfrom the report chain itself. The payment code only uses the boolean to pick theNVP_LAST_PAYMENT_METHODkey, and that NVP is only ever written keyed byiouReport.policyID.This PR deletes
doesReportBelongToWorkspaceandgetPolicyEmployeeAccountIDsand replaces them with two field comparisons (isConciergeChatReport || isPolicyRelatedReport). Composer mention weighting now checkspolicy.employeeList[login]directly (O(1)) instead of scanning the array per suggestion.usePaymentOptionsalso drops a now-unused whole-policy subscription.This does not break userspace. Two places differ on purpose:
isPolicyRelatedReportnow returnsfalsefor an undefined or_FAKE_policyID. Before, an undefined ID matched every report that also had no policyID, souseTransactionViolationOfWorkspacecomputed archive data before any delete was started. The delete flow itself is unchanged.employeeList[login]by exact key. The old path went through a lowercased email cache. If a key's casing ever differs from the login, that person sorts one tier lower in suggestions — nothing else changes.For a 21k members workspace this took ~850ms each time to call
getPolicyEmployeeAccountIDsduring every render. With this PR it disappears.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/591294
PROPOSAL:
Tests
@followed by a few letters. Verify report participants rank first, workspace members second, others last.@. Verify suggestions are plain alphabetical (no member weighting).Offline tests
N/A (no network-dependent logic changed).
QA Steps
Same as tests.
@followed by a few letters. Verify report participants rank first, workspace members second, others last.@. Verify suggestions are plain alphabetical (no member weighting).PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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