Skip to content
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.

Sign up for GitHub

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

HIGH: Kill ReconnectToReport #34902

Merged
merged 12 commits into from
Apr 3, 2024
Merged

HIGH: Kill ReconnectToReport #34902

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
404732f
Kill ReconnectToReport
aldo-expensify Jan 22, 2024
0b4b666
Lint
aldo-expensify Jan 22, 2024
bcfe3ab
Merge Main / resolve conflicts
aldo-expensify Jan 31, 2024
7cb1ab1
Delete unused params for deleted command ReconnectToReport
aldo-expensify Jan 31, 2024
10360a3
Remove ReconnectToReportParams
aldo-expensify Jan 31, 2024
148f19d
Merge main / resolve conflicts
aldo-expensify Mar 13, 2024
c369feb
Merge main / resolve conflicts
aldo-expensify Mar 28, 2024
6e2648d
Merge branch 'main' of github.com:Expensify/App into aldo_remove-reco…
aldo-expensify Mar 28, 2024
ea3f7b6
Remove unused type
aldo-expensify Mar 28, 2024
278c3f1
Remove unused variable
aldo-expensify Apr 2, 2024
325111d
Merge branch 'main' of github.com:Expensify/App into aldo_remove-reco…
aldo-expensify Apr 2, 2024
5c587e0
Merge branch 'main' of github.com:Expensify/App into aldo_remove-reco…
aldo-expensify Apr 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 0 additions & 59 deletions src/libs/actions/Report.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -767,58 +767,6 @@ function navigateToAndOpenChildReport(childReportID = '0', parentReportAction: P
}
}

/** Get the latest report history without marking the report as read. */
function reconnect(reportID: string) {
const optimisticData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {
reportName: allReports?.[reportID]?.reportName ?? CONST.REPORT.DEFAULT_REPORT_NAME,
},
},
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportID}`,
value: {
isLoadingInitialReportActions: true,
isLoadingNewerReportActions: false,
isLoadingOlderReportActions: false,
},
},
];

const successData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportID}`,
value: {
isLoadingInitialReportActions: false,
},
},
];

const failureData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportID}`,
value: {
isLoadingInitialReportActions: false,
},
},
];

type ReconnectToReportParameters = {
reportID: string;
};

const parameters: ReconnectToReportParameters = {
reportID,
};

API.write('ReconnectToReport', parameters, {optimisticData, successData, failureData});
}

/**
* Gets the older actions that have not been read yet.
* Normally happens when you scroll up on a chat, and the actions have not been read yet.
Expand Down Expand Up @@ -1095,12 +1043,6 @@ function handleReportChanged(report: OnyxEntry<Report>) {
conciergeChatReportID = report.reportID;
}
}

// A report can be missing a name if a comment is received via pusher event and the report does not yet exist in Onyx (eg. a new DM created with the logged in person)
// In this case, we call reconnect so that we can fetch the report data without marking it as read
if (report.reportID && report.reportName === undefined) {
reconnect(report.reportID);
}
Comment on lines -1188 to -1190
Copy link
Contributor Author

@aldo-expensify aldo-expensify Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Need to make sure this isn't needed. If reliable updates works properly, I don't think we should miss a report, but 🤷

Copy link
Contributor Author

@aldo-expensify aldo-expensify Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is very very very very Old (~2020), and it is very likely that things were much more unreliable back then. I think this came from this PR:

https://github.com/Expensify/App/pull/339/files

And have been modified a few times after, for example here: https://github.com/Expensify/App/pull/1075/files#r548053786

I think this can be tested by:

  1. accountA is offline and has never chatted with accountB before
  2. accountB creates a DM with accountA and sends a message. accountA misses the push event
  3. accountA goes online
  4. accountB sends another message

That way, when we push the last message sent by accountB in step 4, accountA could have been missing the report, but that is not the case anymore because accountA got the report in step 3 because of "reliable updates".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with you @aldo-expensify that the app is more reliable. I think this code is basically here for the case you have laid out i.e. it was possible to get a pusher update for a report that you didn't have locally yet. And in that case, we would fetch it.

cc @tgolen to get more eyes on this one just in case he can think of something we are not seeing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh... Good to see this code is no longer necessary! I agree with both of you and the assumptions.

}

Onyx.connect({
Expand Down Expand Up @@ -2654,7 +2596,6 @@ export {
searchInServer,
addComment,
addAttachment,
reconnect,
updateWelcomeMessage,
updateWriteCapabilityAndNavigate,
updateNotificationPreference,
Expand Down
29 changes: 2 additions & 27 deletions src/pages/home/report/ReportActionsView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,7 @@ function ReportActionsView(props) {
const isFirstRender = useRef(true);
const hasCachedActions = useInitialValue(() => _.size(props.reportActions) > 0);
const mostRecentIOUReportActionID = useInitialValue(() => ReportActionsUtils.getMostRecentIOURequestActionID(props.reportActions));

const prevNetworkRef = useRef(props.network);
const prevAuthTokenType = usePrevious(props.session.authTokenType);

const prevIsSmallScreenWidthRef = useRef(props.isSmallScreenWidth);

const isFocused = useIsFocused();
Expand All @@ -118,36 +115,14 @@ function ReportActionsView(props) {
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

useEffect(() => {
const prevNetwork = prevNetworkRef.current;
// When returning from offline to online state we want to trigger a request to OpenReport which
// will fetch the reportActions data and mark the report as read. If the report is not fully visible
// then we call ReconnectToReport which only loads the reportActions data without marking the report as read.
const wasNetworkChangeDetected = lodashGet(prevNetwork, 'isOffline') && !lodashGet(props.network, 'isOffline');
if (wasNetworkChangeDetected) {
if (isReportFullyVisible) {
openReportIfNecessary();
} else {
Report.reconnect(reportID);
}
}
// update ref with current network state
prevNetworkRef.current = props.network;
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [props.network, props.report, isReportFullyVisible]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed anymore because ReconnectApp will fetch the missing updates

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds right to me.


useEffect(() => {
const wasLoginChangedDetected = prevAuthTokenType === 'anonymousAccount' && !props.session.authTokenType;
if (wasLoginChangedDetected && didUserLogInDuringSession() && isUserCreatedPolicyRoom(props.report)) {
if (isReportFullyVisible) {
openReportIfNecessary();
} else {
Report.reconnect(reportID);
}
openReportIfNecessary();
Copy link
Contributor Author

@aldo-expensify aldo-expensify Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only case we still need to fetch report with OpenReport because this is not related to loosing/regaining connection, so ReconnectApp is not called.

In the past, we were call Report.reconnect(reportID); instead of openReportIfNecessary(); if the App is in the background/not focused, but I don't think it is important to have this consideration here because this case only happens when we are login in from an anonymous account, and we can't log in with the app in the background.

}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [props.session, props.report, isReportFullyVisible]);

}, [props.session, props.report]);
useEffect(() => {
const prevIsSmallScreenWidth = prevIsSmallScreenWidthRef.current;
// If the view is expanded from mobile to desktop layout
Expand Down
Loading