Remove Onyx.connect() key ONYXKEYS.COLLECTION.REPORT_ACTIONS in src/libs/OptionsListUtils.ts - part 1#85783
Remove Onyx.connect() key ONYXKEYS.COLLECTION.REPORT_ACTIONS in src/libs/OptionsListUtils.ts - part 1#85783truph01 wants to merge 9 commits intoExpensify:mainfrom
Conversation
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.
|
|
@DylanDylann 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
joekaufmanexpensify
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Add a field to sort the recent reports by the time of last IOU request for create actions | ||
| if (preferRecentExpenseReports) { | ||
| const reportPreviewAction = allSortedReportActions[option.reportID]?.find((reportAction) => isActionOfType(reportAction, CONST.REPORT.ACTIONS.TYPE.REPORT_PREVIEW)); | ||
| // eslint-disable-next-line @typescript-eslint/no-deprecated |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The fallback expression sortedReportActionsData?.sortedActions ?? deprecatedAllSortedReportActions is computed twice inside the same loop body with different variable names: resolvedSortedActions (line ~2293) and sortedActionsForLookup (line ~2308). Both resolve to the same value on every iteration.
Hoist the resolution above both if blocks so it is computed once per iteration:
// Before the if (shouldUnreadBeBold) block:
const resolvedSortedActions = sortedReportActionsData?.sortedActions ?? deprecatedAllSortedReportActions;Then use resolvedSortedActions in both the shouldUnreadBeBold and preferRecentExpenseReports blocks, removing the duplicate sortedActionsForLookup variable.
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: 670c63f2a9
ℹ️ 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 reportActionsArray = Object.values(actions); | ||
| let sortedReportActions = getSortedReportActions(withDEWRoutedActionsArray(reportActionsArray), true); | ||
| sortedActions[reportID] = sortedReportActions; |
There was a problem hiding this comment.
sortedActions[reportID] = sortedReportActions puts a full ReportAction[] for every report into ONYXKEYS.DERIVED.SORTED_REPORT_ACTIONS. Derived keys are persisted via Onyx.set() in src/libs/actions/OnyxDerived/utils.ts, so every new message now serializes a second copy of the user's report-action history into one large Onyx value. On high-traffic accounts this can balloon storage and turn each report-action update into a very expensive disk write; the previous cache lived only in memory, so this is a real regression.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@truph01 I think this is valid. Could you take a look? We should also add some performance measurements in this PR to see if we need to optimize this
src/hooks/useSearchSelector.base.ts
Outdated
| const [draftComments] = useOnyx(ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT); | ||
| const [nvpDismissedProductTraining] = useOnyx(ONYXKEYS.NVP_DISMISSED_PRODUCT_TRAINING); | ||
| const [visibleReportActionsData] = useOnyx(ONYXKEYS.DERIVED.VISIBLE_REPORT_ACTIONS); | ||
| const [sortedReportActionsData] = useOnyx(ONYXKEYS.DERIVED.SORTED_REPORT_ACTIONS); |
There was a problem hiding this comment.
useSearchSelectorBase() now subscribes to the entire SORTED_REPORT_ACTIONS derived key, and the hook also includes that object in the options useMemo dependencies. Because sortedReportActions.ts::compute() rewrites this derived value whenever any report_ or reportActions_ entry changes, an unrelated incoming message anywhere in the account will invalidate and rebuild the member/share/attendee selector results while the user is typing. On high-traffic accounts this is likely to show up as search lag/jank.
Useful? React with 👍 / 👎.
tgolen
left a comment
tgolen
left a comment
There was a problem hiding this comment.
Wow, really cool approach for this! I love it.
|
No product review needed |
|
@truph01 The idea looks impressive. Could you please add some test cases to useSearchSelector
|
| const [nvpDismissedProductTraining] = useOnyx(ONYXKEYS.NVP_DISMISSED_PRODUCT_TRAINING); | ||
| const [allPolicies] = useOnyx(ONYXKEYS.COLLECTION.POLICY); | ||
| const [recentAttendees] = useOnyx(ONYXKEYS.NVP_RECENT_ATTENDEES); | ||
| const [{sortedActions} = getEmptyObject<OnyxTypes.SortedReportActionsDerivedValue>()] = useOnyx(ONYXKEYS.DERIVED.SORTED_REPORT_ACTIONS); |
There was a problem hiding this comment.
I’d suggest avoiding a default value here. A safer approach is to let Onyx return undefined and handle the fallback (e.g., defaulting,..) inside the function logic itself. This prevents bugs if someone forgets to set a default in future useOnyx() calls
There was a problem hiding this comment.
Yeah, I agree with this as well.
tgolen
left a comment
tgolen
left a comment
There was a problem hiding this comment.
I approve, assuming the comments from @DylanDylann are addressed.
4 participants
Explanation of Change
This PR begins the migration of
Onyx.connect()forONYXKEYS.COLLECTION.REPORT_ACTIONSinOptionsListUtilsby introducing an OnyxDerived value and refactoring theprepareReportOptionsForDisplay → getValidOptionsflow.What changed:
sortedReportActions.ts): Extracts the sorting/caching logic from theOnyx.connectcallback into a derived value underONYXKEYS.DERIVED.SORTED_REPORT_ACTIONS. This computessortedActions,lastActions, andtransactionThreadIDsfromREPORT_ACTIONSandREPORTcollections — the same derivation the callback performs today.prepareReportOptionsForDisplay → getValidOptionsflow.allSortedReportActions,lastReportActions,cachedOneTransactionThreadReportIDs, andallReportActionswith adeprecatedprefix and added@deprecatedJSDoc tags to signal other developers to use the derived value instead.Fixed Issues
$ #66381
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include ""[No QA].""
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