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.

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

[Matt Pay] Avatar online status icon will keep showing syncing icon if user close browser tab when sync icon appears - reported by @K4tsuki #7730

Closed
mvtglobally opened this issue Feb 14, 2022 · 18 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@mvtglobally
Copy link

mvtglobally commented Feb 14, 2022

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Open staging.new.expensify.com
  2. Turn off and turn on wifi
  3. When synching icon appear, close browser tab
  4. Go to http://staging.new.expensify.com/ again
  5. Avatar online status will keep showing sync icon.

Expected Result:

Avatar online status must display solid green and not syncing icon.

Actual Result:

Avatar online status keep displaying syncing icon even after refreshing the page over and over.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platform:

Where is this issue occurring?

  • Web

Version Number: 1.1.37-0
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

sync_icon2.mp4

Expensify/Expensify Issue URL:
Issue reported by: @K4tsuki & @adeel0202
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1643735860857629

View all open jobs on GitHub

@mvtglobally mvtglobally added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Feb 14, 2022
@MelvinBot
Copy link

Triggered auto assignment to @puneetlath (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Feb 14, 2022
@K4tsuki
Copy link
Contributor

K4tsuki commented Feb 14, 2022

proposal

The problem is from:

isSyncingData: {
key: ONYXKEYS.IS_LOADING_AFTER_RECONNECT,

The syncing indicator is depend on IS_LOADING_AFTER_RECONNECT.
IS_LOADING_AFTER_RECONNECT is set by triggerReconnectionCallbacks. On reconnect to network, set it to true and after all reconnectionCallbacks is finish set it to false:

const triggerReconnectionCallbacks = _.throttle((reason) => {
Log.info(`[NetworkConnection] Firing reconnection callbacks because ${reason}`);
Network.setIsLoadingAfterReconnect(true);
promiseAllSettled(_.map(reconnectionCallbacks, callback => callback()))
.then(() => Network.setIsLoadingAfterReconnect(false));
}, 5000, {trailing: false});

When user close the browser/tab before all reconnectionCallbacks is finished, then IS_LOADING_AFTER_RECONNECT value is keep being true.

So, we must not init isSyncData with local data by adding initWithStoredValues: false, in the code below:

isSyncingData: {
key: ONYXKEYS.IS_LOADING_AFTER_RECONNECT,

`

@puneetlath puneetlath removed their assignment Feb 14, 2022
@puneetlath puneetlath added the External Added to denote the issue can be worked on by a contributor label Feb 14, 2022
@MelvinBot
Copy link

Triggered auto assignment to @JmillsExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@JmillsExpensify
Copy link

Upwork job posted here: https://www.upwork.com/jobs/~01e447f8670b3d1f2a

@MelvinBot MelvinBot removed the Overdue label Feb 17, 2022
@JmillsExpensify
Copy link

JmillsExpensify commented Feb 21, 2022

Adding the exported label so we can get a review on @K4tsuki's proposal.

@botify botify removed the Daily KSv2 label Feb 21, 2022
@MelvinBot MelvinBot added the Weekly KSv2 label Feb 21, 2022
@MelvinBot
Copy link

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (Exported)

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 21, 2022
@MelvinBot
Copy link

Triggered auto assignment to @puneetlath (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@parasharrajat
Copy link
Member

@K4tsuki 's proposal LGTM.

cc: @puneetlath

🎀 👀 🎀 C+ reviewed

@puneetlath
Copy link
Contributor

Yep, agreed. Simple and straightforward. Let's do it.

@MelvinBot MelvinBot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 21, 2022
@MelvinBot
Copy link

📣 @K4tsuki You have been assigned to this job by @puneetlath!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@JmillsExpensify
Copy link

Active discussion in the linked PR. Check there for more.

@MelvinBot MelvinBot removed the Overdue label Mar 2, 2022
@puneetlath
Copy link
Contributor

Hm, interesting. Not sure why @flodnv got assigned to the PR instead of me. In any case, I'm about to head out on my two-month sabbatical, so I won't be able to handle the CME review.

@JmillsExpensify and @flodnv I'll let y'all take it from here!

@puneetlath puneetlath removed their assignment Mar 3, 2022
@JmillsExpensify
Copy link

Cool, thanks Puneet! Still reviews and discussion happening in the linked PR.

@parasharrajat
Copy link
Member

We were waiting for a similar issue to be covered on the same PR. Based on the slack thread that would be handled separately. So I am moving ahead with the review.

@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Mar 17, 2022
@MelvinBot MelvinBot removed the Overdue label Mar 17, 2022
@botify botify changed the title Avatar online status icon will keep showing syncing icon if user close browser tab when sync icon appears - reported by @K4tsuki [HOLD for payment 2022-03-24] Avatar online status icon will keep showing syncing icon if user close browser tab when sync icon appears - reported by @K4tsuki Mar 17, 2022
@botify
Copy link

botify commented Mar 17, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.43-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2022-03-24. 🎊

@mallenexpensify mallenexpensify changed the title [HOLD for payment 2022-03-24] Avatar online status icon will keep showing syncing icon if user close browser tab when sync icon appears - reported by @K4tsuki [Matt Pay] Avatar online status icon will keep showing syncing icon if user close browser tab when sync icon appears - reported by @K4tsuki Mar 18, 2022
@mallenexpensify
Copy link
Contributor

Assigned myself to manage payment, @parasharrajat , can you accept the offer I sent?
Invited @K4tsuki , will pay once accepted.

@botify botify removed the Weekly KSv2 label Mar 23, 2022
@MelvinBot
Copy link

@K4tsuki, @mallenexpensify, @parasharrajat Eep! 4 days overdue now. Issues have feelings too...

@mallenexpensify
Copy link
Contributor

@parasharrajat paid 4250
@K4tsuki paid $250 for reporting and $250 for fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

8 participants