-
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
Changes from 37 commits
3be5e69
6125c38
be7ea56
caed994
37926ee
90c9119
9829fd8
bbeb353
426c3cb
9fd2f94
b3b33eb
40d48bb
14808d9
4f75359
6effb37
3619a25
eb44749
a77072a
76fafe0
5d3f53b
cfa7db6
ceda644
12c8fa9
d761db0
ffaf7a8
3df387b
8d5031b
063d658
f74d674
f6b382c
42d54fa
6985019
035874c
5628d27
492f847
dd43fd0
9eddbea
66670d5
8f27192
4ea2342
4c2abca
6ecee83
e3fd39e
3825371
24710b7
ee7f4c7
f006f1d
3aaef2f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -63,5 +63,6 @@ ExceededCommentLength.displayName = 'ExceededCommentLength'; | |||||
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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Not sure how |
||||||
}, | ||||||
})(ExceededCommentLength); |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,129 @@ | ||||||
import {useEffect} from 'react'; | ||||||
import PropTypes from 'prop-types'; | ||||||
import lodashGet from 'lodash/get'; | ||||||
import {withOnyx} from 'react-native-onyx'; | ||||||
import ONYXKEYS from '../../../ONYXKEYS'; | ||||||
import * as ReportUtils from '../../ReportUtils'; | ||||||
import reportPropTypes from '../../../pages/reportPropTypes'; | ||||||
import {withNavigationPropTypes} from '../../../components/withNavigation'; | ||||||
import * as App from '../../actions/App'; | ||||||
import usePermissions from '../../../hooks/usePermissions'; | ||||||
import CONST from '../../../CONST'; | ||||||
import Navigation from '../Navigation'; | ||||||
|
||||||
const propTypes = { | ||||||
/** Available reports that would be displayed in this navigator */ | ||||||
reports: PropTypes.objectOf(reportPropTypes), | ||||||
|
||||||
/** The policies which the user has access to */ | ||||||
policies: PropTypes.objectOf( | ||||||
PropTypes.shape({ | ||||||
/** The policy name */ | ||||||
name: PropTypes.string, | ||||||
|
||||||
/** The type of the policy */ | ||||||
type: PropTypes.string, | ||||||
}), | ||||||
), | ||||||
|
||||||
isFirstTimeNewExpensifyUser: PropTypes.bool, | ||||||
|
||||||
/** Navigation route context info provided by react navigation */ | ||||||
route: PropTypes.shape({ | ||||||
/** Route specific parameters used on this screen */ | ||||||
params: PropTypes.shape({ | ||||||
/** If the admin room should be opened */ | ||||||
openOnAdminRoom: PropTypes.bool, | ||||||
|
||||||
/** The ID of the report this screen should display */ | ||||||
reportID: PropTypes.string, | ||||||
}), | ||||||
}).isRequired, | ||||||
|
||||||
...withNavigationPropTypes, | ||||||
}; | ||||||
|
||||||
const defaultProps = { | ||||||
reports: {}, | ||||||
policies: {}, | ||||||
isFirstTimeNewExpensifyUser: false, | ||||||
}; | ||||||
|
||||||
/** | ||||||
* 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 commentThe 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
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Yeah but the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||
* @param {Object} policies | ||||||
* @param {Boolean} isFirstTimeNewExpensifyUser | ||||||
* @param {Boolean} openOnAdminRoom | ||||||
* @returns {Number} | ||||||
*/ | ||||||
const getLastAccessedReportID = (reports, ignoreDefaultRooms, policies, isFirstTimeNewExpensifyUser, openOnAdminRoom) => { | ||||||
// If deeplink url is of an attachment, we should show the report that the attachment comes from. | ||||||
const currentRoute = Navigation.getActiveRoute(); | ||||||
const matches = CONST.REGEX.ATTACHMENT_ROUTE.exec(currentRoute); | ||||||
const reportID = lodashGet(matches, 1, null); | ||||||
if (reportID) { | ||||||
return reportID; | ||||||
} | ||||||
|
||||||
const lastReport = ReportUtils.findLastAccessedReport(reports, ignoreDefaultRooms, policies, isFirstTimeNewExpensifyUser, openOnAdminRoom); | ||||||
|
||||||
return lodashGet(lastReport, 'reportID'); | ||||||
}; | ||||||
|
||||||
// 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 commentThe 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? |
||||||
const {canUseDefaultRooms} = usePermissions(); | ||||||
|
||||||
useEffect(() => { | ||||||
// Don't update if there is a reportID in the params already | ||||||
if (lodashGet(props.route, 'params.reportID', null)) { | ||||||
App.confirmReadyToOpenApp(); | ||||||
return; | ||||||
} | ||||||
|
||||||
// If there is no reportID in route, try to find last accessed and use it for setParams | ||||||
const reportID = getLastAccessedReportID( | ||||||
props.reports, | ||||||
!canUseDefaultRooms, | ||||||
props.policies, | ||||||
props.isFirstTimeNewExpensifyUser, | ||||||
lodashGet(props.route, 'params.openOnAdminRoom', false), | ||||||
); | ||||||
|
||||||
// It's possible that props.reports aren't fully loaded yet | ||||||
// in that case the reportID is undefined | ||||||
if (reportID) { | ||||||
performance.mark('ReportScreenConfirmer_hook'); | ||||||
performance.measure('ReportScreenConfirmer_hook_meassure', {start: 'ReportScreenConfirmer_hook', duration: 100}); | ||||||
mountiny marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
props.navigation.setParams({reportID: String(reportID)}); | ||||||
} else { | ||||||
App.confirmReadyToOpenApp(); | ||||||
} | ||||||
}, [props.route, props.navigation, props.reports, canUseDefaultRooms, props.policies, props.isFirstTimeNewExpensifyUser]); | ||||||
|
||||||
// The ReportScreen without the reportID set will display a skeleton | ||||||
// until the reportID is loaded and set in the route param | ||||||
return null; | ||||||
} | ||||||
|
||||||
ReportScreenIDSetter.propTypes = propTypes; | ||||||
ReportScreenIDSetter.defaultProps = defaultProps; | ||||||
ReportScreenIDSetter.displayName = 'ReportScreenIDSetter'; | ||||||
|
||||||
export default withOnyx({ | ||||||
reports: { | ||||||
key: ONYXKEYS.COLLECTION.REPORT, | ||||||
allowStaleData: true, | ||||||
}, | ||||||
policies: { | ||||||
key: ONYXKEYS.COLLECTION.POLICY, | ||||||
allowStaleData: true, | ||||||
}, | ||||||
isFirstTimeNewExpensifyUser: { | ||||||
key: ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER, | ||||||
initialValue: false, | ||||||
}, | ||||||
})(ReportScreenIDSetter); |
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 inreportMetadata_
. 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
andisLoadingMoreReportActions
intoreportMetadata_
, 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.