-
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
HIGH: Kill ReconnectToReport #34902
HIGH: Kill ReconnectToReport #34902
Conversation
if (isReportFullyVisible) { | ||
openReportIfNecessary(); | ||
} else { | ||
Report.reconnect(reportID); | ||
} | ||
openReportIfNecessary(); |
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 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.
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]); |
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.
Not needed anymore because ReconnectApp
will fetch the missing updates
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.
Sounds right to me.
if (report.reportID && report.reportName === undefined) { | ||
reconnect(report.reportID); | ||
} |
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.
TODO: Need to make sure this isn't needed. If reliable updates works properly, I don't think we should miss a report, but 🤷
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 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:
accountA
is offline and has never chatted withaccountB
beforeaccountB
creates a DM withaccountA
and sends a message.accountA
misses the push eventaccountA
goes onlineaccountB
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".
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.
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.
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.
Huh... Good to see this code is no longer necessary! I agree with both of you and the assumptions.
Looks good! seems like testing steps will cover everything |
Testing this today. |
Something I've noticed (which is happening on staging too) is that a new chat recieved while offline will not show up in the LHN. This somewhat impedes the given testing steps: Screen.Recording.2024-01-24.at.12.09.41.mov |
hmm, this didn't happen when I tested in dev, but I never tested with staging backend. Do you see the missing report and reportActions in the |
@aldo-expensify No Onyx data is returned by ReconnectApp, just: |
This is unexpected for me and different from what I was getting while testing in dev. You can HOLD on reviewing for now, I have to fix conflicts anyway and then I'll retest. Old screenshot from my dev testing, |
What's the next step for this PR? I see it has a few conflicts. |
I should test again and see why ReconnectApp is not getting the missing report anymore. |
I'm going to make this a DRAFT because I don't think we can merge this until the update ordering problem is solved. |
Thanks for that update. Sorry I wasn't following the updates in the issue.
I'll subscribe over there so I follow what's happening!
…On Mon, Mar 25, 2024 at 2:47 PM Aldo Canepa Garay ***@***.***> wrote:
@aldo-expensify <https://github.com/aldo-expensify> what's the status of
this? I think we need to find a way to increase the #urgency of finishing
this. What is blocking you or what else are you focused on right now? Can
you hand off some of that work to others so you can focus on this?
I've been putting the status in the issue here: Expensify/Expensify#325231
(comment)
<Expensify/Expensify#325231 (comment)>
This is being handled with #urgency, I have spent a lot of hours the last
week on it and it was my main focus.
What is blocking you or what else are you focused on right now?
Gave an update here: Expensify/Expensify#325231 (comment)
<Expensify/Expensify#325231 (comment)>
Other stuff that last week was taking time from me and is supposed to be
high priority is:
- Implement the backend changes for QBO in NewDot (spring release)
(done)
- Fix auth/command/AdminUpdateInvoicedCustomer.cpp so it doesn't
timeout for clients with lots of big policies. We need this done before the
end of the month so they can run it before billing (almost done)
Can you hand off some of that work to others so you can focus on this?
I have been helping with deploy blockers / PR reviews in the morning, it
would be great if I don't have to deal with this one: #38839
<#38839>
—
Reply to this email directly, view it on GitHub
<#34902 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJMAB35QPJOFI26MQ2REJ3Y2CENXAVCNFSM6AAAAABCFQUIYGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJYHA4DKNBRHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@Ollyws this is ready for review, the backend changes have been deployed. Can you retest this from scratch please 🙏 |
Backend sends a 'deprecated version' error. Clicking on update or refreshing the page does not fix it. 🤔 Installed node-modules afresh and tried again still does not work. deprecatedVersion.mp4 |
Please merge I am testing after merging |
Reviewer Checklist
Screenshots/VideosAndroid: NativereconnectAndroid.mp4Android: mWeb ChromereconnectAndroidChrome.mp4iOS: mWeb SafarireconnectiOSSafari.mp4MacOS: Chrome / SafariTest 1 reportCreatedWhileOffline.mp4Test 2 switchingFromAnonAccount.mp4Test 3 toggleNetworkWithReportScreenOpen.mp4MacOS: Desktoptest1And3Desktop.mp4 |
@c3024 I updated with main now |
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.
LGTM and tests well!
@cristipaval Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Thanks @c3024! Merging since Marc approved in the past and Tim already approved too. |
✋ 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 production by https://github.com/Julesssss in version: 1.4.60-13 🚀
|
Details
In the context of optimizing
ReconnectToReport
, we are proposing to just get rid of it since it became redundant afterreliable updates
was implemented.ReconnectToReport
command was introduced to get the missingreportActions
after coming back online. This was necessary becauseReconnectApp
didn't getreportActions
, but now that we have "reliable updates", we are able to fetch and retrieve the missed Onyx updates which contain the missing report Actions (The missed updates are returned byReconnectApp
).Fixed Issues
$ #39464
$ https://github.com/Expensify/Expensify/issues/325231
PROPOSAL:
Tests
1. Report created while offline
accountA
andaccountB
, and log them in on different sessionsaccountA
go offlineaccountB
, create a DM chat withaccountA
and send a messageaccountA
go onlineaccountA
gets the DM chat withaccountB
and gets all messagesaccountB
accountA
received the last message2. Switching from being an
anonymousAccount
to a normal one while looking at a public room3. Going offline/online with the report screen open
Almost the same as test 1. Report created while offline, but the
accountA
goes offline after loading the report.accountA
andaccountB
, and log them in on different sessionsaccountB
, create a DM chat withaccountA
and send a messageaccountA
receives the DM chat withaccountB
and the messagesaccountA
, leave the DM chat open and go offlineaccountB
accountA
go onlineaccountA
receives the last messageOffline tests
QA Steps
1. Report created while offline
accountA
andaccountB
, and log them in on different sessionsaccountA
go offlineaccountB
, create a DM chat withaccountA
and send a messageaccountA
go onlineaccountA
gets the DM chat withaccountB
and gets all messagesaccountB
accountA
received the last message2. Switching from being an
anonymousAccount
to a normal one while looking at a public room3. Going offline/online with the report screen open
Almost the same as test 1. Report created while offline, but the
accountA
goes offline after loading the report.accountA
andaccountB
, and log them in on different sessionsaccountB
, create a DM chat withaccountA
and send a messageaccountA
receives the DM chat withaccountB
and the messagesaccountA
, leave the DM chat open and go offlineaccountB
accountA
go onlineaccountA
receives the last messagePR 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)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
1. Report created while offline
Screen.Recording.2024-01-22.at.12.21.45.PM.mov
2. Switching from being an
anonymousAccount
to a normal one while looking at a public roomScreen.Recording.2024-01-22.at.4.56.34.PM.mov
3. Going offline/online with the report screen open
Screen.Recording.2024-01-22.at.5.35.03.PM.mov
MacOS: Desktop