Move useSidebarOrderedReports cache to OnyxDerived 2#92129
Conversation
cached value in hook
|
@mananjadhav 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] |
|
Hi @TMisiukiewicz, I tagged you in since you have context on the original PR. Hope you don't mind! @mananjadhav feel free to look at this now if you'd like, but I will ping you when it has test steps that you can perform the checklist with |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30065ca7ee
ℹ️ 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".
| ONYXKEYS.COLLECTION.POLICY, | ||
| ONYXKEYS.NVP_PRIORITY_MODE, | ||
| ONYXKEYS.BETAS, | ||
| ONYXKEYS.NETWORK, |
There was a problem hiding this comment.
Recompute LHN when NetworkState changes
Depending on ONYXKEYS.NETWORK here does not cover normal NetInfo transitions: NetworkState updates like setHasRadio/setInternetUnreachable only notify useNetwork subscribers and do not write this Onyx key. Since this commit moved the LHN filtering into a derived value and the hook now returns baseOrderedReportIDs unchanged on useNetwork changes, going offline/online via actual connectivity can leave rows whose visibility depends on isOffline (for example pending-deleted single-transaction threads through isOneTransactionThread/getOneTransactionThreadReportAction) stale until some other Onyx dependency changes. Please wire the derived value to NetworkState changes or force a recompute from the hook when isOffline changes.
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.
|
Explanation of Change
This PR replaces #79609 because the merge conflicts got pretty gnarly, but it's the same objective: remove sourceValue from the React lifecycle in useSidebarOrderedReports.
DERIVED.SIDEBAR_ORDERED_REPORTS, that computes the LHN'sreportsToDisplaymap and sortedorderedReportIDsoutside the React lifecycle. The incremental-update logic (only re-evaluating reports whose dependencies actually changed) moves from the hook into the derived value'scompute, where it consumessourceValuesinstead ofuseOnyxsourceValue.useSidebarOrderedReportsis now a thin consumer of that derived value — thesourceValuetracking,getUpdatedReports, the imperative cache (currentReportsToDisplay), and the cache-clearing hack are all removed.updateReportsToDisplayInLHNrather than making the derived value dependent on router stateorderedReports(fullReport[]) from the context — it now exposes onlyorderedReportIDs.LHNOptionsListresolves each row's report from Onyx by ID, so the context value no longer churns on every report mutation. I don't have evidence that this was a problem, but it was unnecessary work when consumers of this context had their own subscriptions to the report collection anyway.Fixed Issues
$ #76165
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 stepssectiontoggleReportand 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