-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Wildan/fix/21518/client pusher method #24407
Changes from all commits
97807c2
b769dc8
c87ceb9
b57b046
3ed2c75
8d92b2b
9bc367e
488e320
09aa78e
5376bcf
16106f8
ea6c6fe
fee3986
e4d1363
d7ac19a
edb9634
ab51aea
96d1c45
4ddbcc9
7ab5f2e
1952cfd
20328da
a785a00
9f47e6a
6a1f080
39c079b
70c6b30
f91b792
1ba065a
a658b18
e0f8cbb
30c7499
b9a001e
09ebf90
b3d2a0c
91c533e
8926377
b2303b2
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 | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -89,6 +89,9 @@ const propTypes = { | |||||||||||
/** All of the personal details for everyone */ | ||||||||||||
personalDetails: PropTypes.objectOf(personalDetailsPropType), | ||||||||||||
|
||||||||||||
/** Whether user is leaving the current report */ | ||||||||||||
userLeavingStatus: PropTypes.bool, | ||||||||||||
|
||||||||||||
...windowDimensionsPropTypes, | ||||||||||||
...viewportOffsetTopPropTypes, | ||||||||||||
...withCurrentReportIDPropTypes, | ||||||||||||
|
@@ -105,6 +108,7 @@ const defaultProps = { | |||||||||||
betas: [], | ||||||||||||
policies: {}, | ||||||||||||
accountManagerReportID: null, | ||||||||||||
userLeavingStatus: false, | ||||||||||||
personalDetails: {}, | ||||||||||||
...withCurrentReportIDDefaultProps, | ||||||||||||
}; | ||||||||||||
|
@@ -145,12 +149,14 @@ function ReportScreen({ | |||||||||||
viewportOffsetTop, | ||||||||||||
isComposerFullSize, | ||||||||||||
errors, | ||||||||||||
userLeavingStatus, | ||||||||||||
currentReportID, | ||||||||||||
}) { | ||||||||||||
const firstRenderRef = useRef(true); | ||||||||||||
const flatListRef = useRef(); | ||||||||||||
const reactionListRef = useRef(); | ||||||||||||
const prevReport = usePrevious(report); | ||||||||||||
const prevUserLeavingStatus = usePrevious(userLeavingStatus); | ||||||||||||
|
||||||||||||
const [skeletonViewContainerHeight, setSkeletonViewContainerHeight] = useState(0); | ||||||||||||
const [isBannerVisible, setIsBannerVisible] = useState(true); | ||||||||||||
|
@@ -175,6 +181,7 @@ function ReportScreen({ | |||||||||||
const policy = policies[`${ONYXKEYS.COLLECTION.POLICY}${report.policyID}`]; | ||||||||||||
|
||||||||||||
const isTopMostReportId = currentReportID === getReportID(route); | ||||||||||||
const didSubscribeToReportLeavingEvents = useRef(false); | ||||||||||||
|
||||||||||||
const isDefaultReport = checkDefaultReport(report); | ||||||||||||
|
||||||||||||
|
@@ -234,7 +241,7 @@ function ReportScreen({ | |||||||||||
} | ||||||||||||
|
||||||||||||
// It possible that we may not have the report object yet in Onyx yet e.g. we navigated to a URL for an accessible report that | ||||||||||||
// is not stored locally yet. If props.report.reportID exists, then the report has been stored locally and nothing more needs to be done. | ||||||||||||
// is not stored locally yet. If report.reportID exists, then the report has been stored locally and nothing more needs to be done. | ||||||||||||
// If it doesn't exist, then we fetch the report from the API. | ||||||||||||
if (report.reportID && report.reportID === getReportID(route)) { | ||||||||||||
return; | ||||||||||||
|
@@ -282,6 +289,14 @@ function ReportScreen({ | |||||||||||
useEffect(() => { | ||||||||||||
fetchReportIfNeeded(); | ||||||||||||
ComposerActions.setShouldShowComposeInput(true); | ||||||||||||
return () => { | ||||||||||||
if (!didSubscribeToReportLeavingEvents) { | ||||||||||||
return; | ||||||||||||
} | ||||||||||||
|
||||||||||||
Report.unsubscribeFromLeavingRoomReportChannel(report.reportID); | ||||||||||||
}; | ||||||||||||
|
||||||||||||
// I'm disabling the warning, as it expects to use exhaustive deps, even though we want this useEffect to run only on the first render. | ||||||||||||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||||||||||||
}, []); | ||||||||||||
|
@@ -292,24 +307,51 @@ function ReportScreen({ | |||||||||||
firstRenderRef.current = false; | ||||||||||||
return; | ||||||||||||
} | ||||||||||||
|
||||||||||||
const onyxReportID = report.reportID; | ||||||||||||
const prevOnyxReportID = prevReport.reportID; | ||||||||||||
const routeReportID = getReportID(route); | ||||||||||||
|
||||||||||||
// Navigate to the Concierge chat if the room was removed from another device (e.g. user leaving a room) | ||||||||||||
if ( | ||||||||||||
// non-optimistic case | ||||||||||||
wildan-m marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
(!prevUserLeavingStatus && userLeavingStatus) || | ||||||||||||
// optimistic case | ||||||||||||
wildan-m marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
(prevOnyxReportID && prevOnyxReportID === routeReportID && !onyxReportID && prevReport.statusNum === CONST.REPORT.STATUS.OPEN && report.statusNum === CONST.REPORT.STATUS.CLOSED) | ||||||||||||
) { | ||||||||||||
Navigation.goBack(); | ||||||||||||
Report.navigateToConciergeChat(); | ||||||||||||
return; | ||||||||||||
} | ||||||||||||
|
||||||||||||
// If you already have a report open and are deeplinking to a new report on native, | ||||||||||||
// the ReportScreen never actually unmounts and the reportID in the route also doesn't change. | ||||||||||||
// Therefore, we need to compare if the existing reportID is the same as the one in the route | ||||||||||||
// before deciding that we shouldn't call OpenReport. | ||||||||||||
const onyxReportID = report.reportID; | ||||||||||||
const routeReportID = getReportID(route); | ||||||||||||
if (onyxReportID === prevReport.reportID && (!onyxReportID || onyxReportID === routeReportID)) { | ||||||||||||
return; | ||||||||||||
} | ||||||||||||
|
||||||||||||
fetchReportIfNeeded(); | ||||||||||||
ComposerActions.setShouldShowComposeInput(true); | ||||||||||||
}, [route, report, errors, fetchReportIfNeeded, prevReport.reportID]); | ||||||||||||
}, [route, report, errors, fetchReportIfNeeded, prevReport.reportID, prevUserLeavingStatus, userLeavingStatus, prevReport.statusNum]); | ||||||||||||
|
||||||||||||
useEffect(() => { | ||||||||||||
// Ensures subscription event succeeds when the report/workspace room is created optimistically. | ||||||||||||
Comment on lines
+339
to
+340
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. We should have early returned if 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. @fedirjh Thanks for your feedback. We did it here and here, but we might also need to put it in The issue also related to this function: App/src/pages/home/ReportScreen.js Lines 128 to 131 in 6cc9053
it returned 'null' string instead of an actual I'd recommend to change it to:
@luacmartins, @allroundexperts, considering that this has passed the regression period, and any change would require re-testing. What should we do in this situation? Will there be any adjustments? 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.
We attempted this in PR #29590, but it resulted in a regression, so we reverted the changes in PR #29893. The issue arises from the fact that App/src/pages/home/ReportScreen.js Line 470 in 6cc9053
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. I think we also need to check onyx report |
||||||||||||
// Check if the optimistic `OpenReport` or `AddWorkspaceRoom` has succeeded by confirming | ||||||||||||
// any `pendingFields.createChat` or `pendingFields.addWorkspaceRoom` fields are set to null. | ||||||||||||
// Existing reports created will have empty fields for `pendingFields`. | ||||||||||||
const didCreateReportSuccessfully = !report.pendingFields || (!report.pendingFields.addWorkspaceRoom && !report.pendingFields.createChat); | ||||||||||||
if (!didSubscribeToReportLeavingEvents.current && didCreateReportSuccessfully) { | ||||||||||||
Report.subscribeToReportLeavingEvents(reportID); | ||||||||||||
didSubscribeToReportLeavingEvents.current = true; | ||||||||||||
} | ||||||||||||
}, [report, didSubscribeToReportLeavingEvents, reportID]); | ||||||||||||
|
||||||||||||
// eslint-disable-next-line rulesdir/no-negated-variables | ||||||||||||
const shouldShowNotFoundPage = useMemo( | ||||||||||||
() => (!_.isEmpty(report) && !isDefaultReport && !report.reportID && !isOptimisticDelete && !report.isLoadingReportActions && !isLoading) || shouldHideReport, | ||||||||||||
[report, isLoading, shouldHideReport, isDefaultReport, isOptimisticDelete], | ||||||||||||
() => (!_.isEmpty(report) && !isDefaultReport && !report.reportID && !isOptimisticDelete && !report.isLoadingReportActions && !isLoading && !userLeavingStatus) || shouldHideReport, | ||||||||||||
[report, isLoading, shouldHideReport, isDefaultReport, isOptimisticDelete, userLeavingStatus], | ||||||||||||
); | ||||||||||||
|
||||||||||||
return ( | ||||||||||||
|
@@ -452,5 +494,8 @@ export default compose( | |||||||||||
personalDetails: { | ||||||||||||
key: ONYXKEYS.PERSONAL_DETAILS_LIST, | ||||||||||||
}, | ||||||||||||
userLeavingStatus: { | ||||||||||||
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_USER_IS_LEAVING_ROOM}${getReportID(route)}`, | ||||||||||||
}, | ||||||||||||
}), | ||||||||||||
)(ReportScreen); |
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.
What happens if this is not sorted?
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 cloned it from typing, alternatively, we can the below code if required, but a little bit longer