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 all commits
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
5 changes: 0 additions & 5 deletions src/libs/API/parameters/ReconnectToReportParams.ts
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion src/libs/API/parameters/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ export type {default as VerifyIdentityForBankAccountParams} from './VerifyIdenti
export type {default as AnswerQuestionsForWalletParams} from './AnswerQuestionsForWalletParams';
export type {default as AddCommentOrAttachementParams} from './AddCommentOrAttachementParams';
export type {default as OptInOutToPushNotificationsParams} from './OptInOutToPushNotificationsParams';
export type {default as ReconnectToReportParams} from './ReconnectToReportParams';
export type {default as ReadNewestActionParams} from './ReadNewestActionParams';
export type {default as MarkAsUnreadParams} from './MarkAsUnreadParams';
export type {default as TogglePinnedChatParams} from './TogglePinnedChatParams';
Expand Down
2 changes: 0 additions & 2 deletions src/libs/API/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ const WRITE_COMMANDS = {
RESTART_BANK_ACCOUNT_SETUP: 'RestartBankAccountSetup',
OPT_IN_TO_PUSH_NOTIFICATIONS: 'OptInToPushNotifications',
OPT_OUT_OF_PUSH_NOTIFICATIONS: 'OptOutOfPushNotifications',
RECONNECT_TO_REPORT: 'ReconnectToReport',
READ_NEWEST_ACTION: 'ReadNewestAction',
MARK_AS_UNREAD: 'MarkAsUnread',
TOGGLE_PINNED_CHAT: 'TogglePinnedChat',
Expand Down Expand Up @@ -272,7 +271,6 @@ type WriteCommandParameters = {
[WRITE_COMMANDS.RESTART_BANK_ACCOUNT_SETUP]: Parameters.RestartBankAccountSetupParams;
[WRITE_COMMANDS.OPT_IN_TO_PUSH_NOTIFICATIONS]: Parameters.OptInOutToPushNotificationsParams;
[WRITE_COMMANDS.OPT_OUT_OF_PUSH_NOTIFICATIONS]: Parameters.OptInOutToPushNotificationsParams;
[WRITE_COMMANDS.RECONNECT_TO_REPORT]: Parameters.ReconnectToReportParams;
[WRITE_COMMANDS.READ_NEWEST_ACTION]: Parameters.ReadNewestActionParams;
[WRITE_COMMANDS.MARK_AS_UNREAD]: Parameters.MarkAsUnreadParams;
[WRITE_COMMANDS.TOGGLE_PINNED_CHAT]: Parameters.TogglePinnedChatParams;
Expand Down
56 changes: 0 additions & 56 deletions src/libs/actions/Report.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import type {
OpenReportParams,
OpenRoomMembersPageParams,
ReadNewestActionParams,
ReconnectToReportParams,
RemoveEmojiReactionParams,
RemoveFromRoomParams,
ResolveActionableMentionWhisperParams,
Expand Down Expand Up @@ -888,54 +887,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,
},
},
];

const parameters: ReconnectToReportParams = {
reportID,
};

API.write(WRITE_COMMANDS.RECONNECT_TO_REPORT, 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 @@ -1182,12 +1133,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.

}

/** Deletes a comment from the report, basically sets it as empty string */
Expand Down Expand Up @@ -3031,7 +2976,6 @@ export {
searchInServer,
addComment,
addAttachment,
reconnect,
updateDescription,
updateWriteCapabilityAndNavigate,
updateNotificationPreference,
Expand Down
35 changes: 2 additions & 33 deletions src/pages/home/report/ReportActionsView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@ function ReportActionsView({
const {isSmallScreenWidth, windowHeight} = useWindowDimensions();
const contentListHeight = useRef(0);
const isFocused = useIsFocused();
const prevNetworkRef = useRef(network);
const prevAuthTokenType = usePrevious(session?.authTokenType);
const [isNavigatingToLinkedMessage, setNavigatingToLinkedMessage] = useState(!!reportActionID);
const prevIsSmallScreenWidthRef = useRef(isSmallScreenWidth);
Expand All @@ -119,14 +118,6 @@ function ReportActionsView({
Report.openReport(reportID, reportActionID);
};

const reconnectReportIfNecessary = () => {
if (!shouldFetchReport(report)) {
return;
}

Report.reconnect(reportID);
};

useLayoutEffect(() => {
setCurrentReportActionID('');
}, [route]);
Expand Down Expand Up @@ -273,35 +264,13 @@ function ReportActionsView({
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [route, indexOfLinkedAction]);

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 = prevNetwork.isOffline && !network.isOffline;
if (wasNetworkChangeDetected) {
if (isReportFullyVisible) {
openReportIfNecessary();
} else {
reconnectReportIfNecessary();
}
}
// update ref with current network state
prevNetworkRef.current = network;
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [network, isReportFullyVisible]);

useEffect(() => {
const wasLoginChangedDetected = prevAuthTokenType === CONST.AUTH_TOKEN_TYPES.ANONYMOUS && !session?.authTokenType;
if (wasLoginChangedDetected && didUserLogInDuringSession() && isUserCreatedPolicyRoom(report)) {
if (isReportFullyVisible) {
openReportIfNecessary();
} else {
reconnectReportIfNecessary();
}
openReportIfNecessary();
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [session, report, isReportFullyVisible]);
}, [session, report]);

useEffect(() => {
const prevIsSmallScreenWidth = prevIsSmallScreenWidthRef.current;
Expand Down
Loading