-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use initialValue in withOnyx and other optimizations for ReportScreen #26772
Use initialValue in withOnyx and other optimizations for ReportScreen #26772
Conversation
# Conflicts: # src/pages/home/ReportScreen.js
# Conflicts: # src/pages/home/report/ReportActionItem.js
Co-authored-by: Vit Horacek <[email protected]>
Co-authored-by: Vit Horacek <[email protected]>
I did another round on profiling on this branch and found some more improvements Prevent SidebarLinks from re-rendering on opening report
Before: After: The perf impact is pretty minimal, but it is also a good change to cleanup Diff: janicduplessis@b47663e Reduce SidebarLinksData render timeI was able to get this render down more significantly by changing the memoization to separate Diff: janicduplessis@2c14b62 Will update this as I find more things |
- Add initial values to different components to allow immediate mounting/rendering - Delay the initialization of certain values to allow React to flush the queue immediately - Remove unnecessary Onyx values
566e19b
to
3619a25
Compare
I applied the first performance update suggested by @janicduplessis, tested it and everything is working fine. I will wait for the green light from someone in expensify to approve the change in the second suggestion. |
@ospfranco can you also sync up with main please? |
I think we have a weird bug, I've been testing some reports , and sometimes, instead of the skeleton view, the "not found" view is displayed. This issue has occurred with older reports when opening reports with old tasks tasks. This bug is also affects distance requests and money requests. Edit : Affects threads as well, I guess it affects all nested reports. Steps to reproduce
CleanShot.2023-09-25.at.18.57.04.mp4 |
|
||
const isOptimisticDelete = lodashGet(report, 'statusNum') === CONST.REPORT.STATUS.CLOSED; | ||
|
||
const shouldHideReport = !ReportUtils.canAccessReport(report, policies, betas); | ||
|
||
const isLoading = !reportID || !isSidebarLoaded || _.isEmpty(personalDetails) || firstRenderRef.current; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am suspecting this change has caused this bug #26772 (comment) , isLoading
is used inside shouldShowNotFoundPage
which display the not found view. I guess before this change isLoading
was initially set as true
but it has changed to be set initially as false
and that will affect the value of shouldShowNotFoundPage
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with that first render is that it would delay the rendering one extra cycle at which point onyx would be already trying to shove data into the component and would delay the rendering again. However I don't think there is a way around it at this point. I will just revert it and we can revisit this again later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However I don't think there is a way around it at this point.
@ospfranco Instead of reverting and losing the optimisation effect , why don't we just update shouldShowNotFoundPage
to account for the first render ?
@fedirjh I've reverted the change to the |
@ospfranco Yes that fixes the bug , but it brings back the skeleton bug , when you navigate between opened reports, the skeleton is briefly displayed. |
# Conflicts: # src/libs/DateUtils.js
Reviewer Checklist
Screenshots/VideosWebCleanShot.2023-09-26.at.13.08.44.mp4Mobile Web - ChromeCleanShot.2023-09-26.at.13.23.15.mp4Mobile Web - SafariCleanShot.2023-09-26.at.13.18.57.mp4DesktopCleanShot.2023-09-26.at.14.41.41.mp4iOSCleanShot.2023-09-26.at.13.59.24.mp4AndroidCleanShot.2023-09-26.at.14.39.11.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All yours @mountiny.
* Get the most recently accessed report for the user | ||
* | ||
* @param {Object} reports | ||
* @param {Boolean} [ignoreDefaultRooms] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does not seem to have a default value set, also there are required params coming after
* @param {Boolean} [ignoreDefaultRooms] | |
* @param {Boolean} ignoreDefaultRooms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah but the []
in js docs means optional parameter, but this is not optional parameter but the looks of things
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this wasn't my code, just moved it from the wrapper. But it is fixed.
@@ -68,5 +68,6 @@ SilentCommentUpdater.displayName = 'SilentCommentUpdater'; | |||
export default withOnyx({ | |||
comment: { | |||
key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`, | |||
initialValue: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you forget to apply this suggestion?
if (didManuallyMarkReportAsUnread) { | ||
// Clearing the current unread marker so that it can be recalculated | ||
setCurrentUnreadMarker(null); | ||
setMessageManuallyMarkedUnread(new Date().getTime()); | ||
} else { | ||
setMessageManuallyMarkedUnread(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have an early return here as that is preferred
}; | ||
|
||
// This wrapper is reponsible for opening the last accessed report if there is no reportID specified in the route params | ||
function ReportScreenIDSetter(props) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a new component and we prefer destructuring the props, can you please do that here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! looking great!
Ah, unit test is failing, let me take a look |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.75-0 🚀
|
@@ -242,6 +242,7 @@ const ONYXKEYS = { | |||
POLICY_RECENTLY_USED_TAGS: 'policyRecentlyUsedTags_', | |||
WORKSPACE_INVITE_MEMBERS_DRAFT: 'workspaceInviteMembersDraft_', | |||
REPORT: 'report_', | |||
REPORT_METADATA: 'reportMetadata_', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request: can we please add a comment to explain what belongs in report_
and what belongs in reportMetadata_
. Without all the context the distinction isn't very clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now we've separated the loading states isLoadingReportActions
and isLoadingMoreReportActions
into reportMetadata_
, as they were causing many re-renders on smaller components every time the report actions were being fetched. It makes sense to add it a comment for future reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, sounds good. Maybe the distinction is that REPORT_METADATA is for client-generated data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a performance optimization, not a hard cut between functionality/logic.
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.75-12 🚀
|
🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.76-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.76-6 🚀
|
Details
This PR makes further improvements to the ReportScreen. It is based on the changes of the previous PR.
A summary of the changes:
initialValue
to the withOnyx HOC. This allows components to mount ASAP. This had to be manually added to the components that were causing a re-render.HeaderView.js
had unused onyx connections and was returning a new object every time. Causing a re-renderThis PR is also dependant on changes we made to
react-native-onyx
in this PR those need to be merged and published first.Fixed Issues
$ #26087
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android