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

Chat - Content is not loading after network return if the App starts while offline #12574

Closed
kbecciv opened this issue Nov 8, 2022 · 25 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review

Comments

@kbecciv
Copy link

kbecciv commented Nov 8, 2022

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Kill the App (dismiss App's card)
  2. Turn network off
  3. Start the App
  4. Check content in different chats ( will be infinite loading)
  5. Turn network on
  6. Check content in all chats again (will be infinite loading as well)

Expected Result:

The user should see the content in chats (that he visited before) when starting offline

Actual Result:

Content is not loading after network return if the App starts while offline

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.2.25.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5812700_offline_content.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 8, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 8, 2022

Triggered auto assignment to @muttmuure (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@JmillsExpensify
Copy link

@muttmuure This one has come up in a couple of contexts. Let's triage this and get the process started.

@fedirjh
Copy link
Contributor

fedirjh commented Nov 13, 2022

we have to update IS_LOADING_INITIAL_APP_DATA in reconnectApp , this is a regression from #12169

App/src/libs/actions/App.js

Lines 162 to 180 in 0d653ca

function reconnectApp() {
API.write('ReconnectApp', {policyIDListExcludingWorkspacesCreatedOffline}, {
optimisticData: [{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: ONYXKEYS.IS_LOADING_REPORT_DATA,
value: true,
}],
successData: [{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: ONYXKEYS.IS_LOADING_REPORT_DATA,
value: false,
}],
failureData: [{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: ONYXKEYS.IS_LOADING_REPORT_DATA,
value: false,
}],
});
}

function reconnectApp() { 
    API.write('ReconnectApp', {policyIDListExcludingWorkspacesCreatedOffline}, { 
        optimisticData: [ 
            { 
                onyxMethod: CONST.ONYX.METHOD.MERGE, 
                key: ONYXKEYS.IS_LOADING_REPORT_DATA, 
                value: true, 
            }, 
+            { 
+                onyxMethod: CONST.ONYX.METHOD.MERGE, 
+                key: ONYXKEYS.IS_LOADING_INITIAL_APP_DATA, 
+                value: true, 
+            }, 
        ], 
        successData: [ 
            { 
                onyxMethod: CONST.ONYX.METHOD.MERGE, 
                key: ONYXKEYS.IS_LOADING_REPORT_DATA, 
                value: false, 
            }, 
+            { 
+                onyxMethod: CONST.ONYX.METHOD.MERGE, 
+                key: ONYXKEYS.IS_LOADING_INITIAL_APP_DATA, 
+                value: false, 
+            }, 
        ], 
        failureData: [ 
            { 
                onyxMethod: CONST.ONYX.METHOD.MERGE, 
                key: ONYXKEYS.IS_LOADING_REPORT_DATA, 
                value: false, 
            }, 
+            { 
+                onyxMethod: CONST.ONYX.METHOD.MERGE, 
+                key: ONYXKEYS.IS_LOADING_INITIAL_APP_DATA, 
+                value: false, 
+            }, 
        ], 
    }); 
}

@melvin-bot
Copy link

melvin-bot bot commented Nov 14, 2022

@muttmuure Eep! 4 days overdue now. Issues have feelings too...

@muttmuure
Copy link
Contributor

I can reproduce so adding engineering

@melvin-bot
Copy link

melvin-bot bot commented Nov 14, 2022

Triggered auto assignment to @amyevans (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@mountiny
Copy link
Contributor

cc @marcaaron I think this is mentioning your PR, what do you think of the proposal, as you might have more insight into this.

@marcaaron
Copy link
Contributor

I think we should just revert my PR here. It's more important for the offline behavior to work than the new marker behavior to work. My solution to fix it was flawed and should be reverted.

@marcaaron marcaaron added the Internal Requires API changes or must be handled by Expensify staff label Nov 14, 2022
@marcaaron marcaaron self-assigned this Nov 14, 2022
@amyevans
Copy link
Contributor

Yeah, agreed that the new marker behavior is less important, thanks @marcaaron!

@melvin-bot
Copy link

melvin-bot bot commented Nov 15, 2022

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

@Tushu17
Copy link
Contributor

Tushu17 commented Nov 15, 2022

I had reported an issue here as a regression related to skeleton ui and it got resolved by the PR(link) of this issue. so I believe I am eligible for the reporting bonus.
I had reported the issue at (2:37) and this issue was created at (3:56) it wasn't a dupe.
cc- @muttmuure

@marcaaron
Copy link
Contributor

@muttmuure sorry did most of the BZ checklist for you already 🥲

@muttmuure
Copy link
Contributor

@Tushu17 the first report is here from Carlos - https://expensify.slack.com/archives/C01GTK53T8Q/p1667839756928309 - so this doesn't seem eligible for the reporting bonus this time.

I'm going to close out.

Let me know if you disagree!

@Tushu17
Copy link
Contributor

Tushu17 commented Nov 22, 2022

the first report is here from Carlos - https://expensify.slack.com/archives/C01GTK53T8Q/p1667839756928309

Hi, @muttmuure that wasn't a bug you can see the convo here, it was marked as "not a bug" because it was just a dev build problem.

@mananjadhav
Copy link
Collaborator

@muttmuure this is eligible for C+ review of internal PR. Can you please help with the same ?

cc - @marcaaron

@JmillsExpensify
Copy link

@muttmuure Can you issue payment for @mananjadhav. Thanks!

@mananjadhav
Copy link
Collaborator

@muttmuure Quick bump on this, can you help with the payout?

@JmillsExpensify
Copy link

@mananjadhav can you apply here please? https://www.upwork.com/jobs/~011aef35eb61fef46b

@JmillsExpensify JmillsExpensify self-assigned this Nov 29, 2022
@Tushu17
Copy link
Contributor

Tushu17 commented Nov 29, 2022

Hi, @muttmuure that wasn't a bug you can see the convo here, it was marked as "not a bug" because it was just a dev build problem.

@JmillsExpensify, I'm also eligible for reporting bonus.

@JmillsExpensify
Copy link

@Tushu17 Same link above should work for you.

@mananjadhav
Copy link
Collaborator

mananjadhav commented Nov 29, 2022

@JmillsExpensify I deleted an incorrect comment earlier. The paid out comment was for another issue. I am unable to apply to this job. It says Only invited users can find, view and apply to the job

@JmillsExpensify
Copy link

Just invited you. Upwork is being difficult 😄

@mananjadhav
Copy link
Collaborator

Haha! Applied @JmillsExpensify

@JmillsExpensify
Copy link

Offers just sent to both C and C+

@JmillsExpensify
Copy link

Everyone should be handled now. Regression test also already exists, so I think we're done here. Closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

9 participants