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

[HOLD - should be fixed by #23125] Web - Flag as spam page always reloads when switching browser tabs #23853

Closed
1 of 6 tasks
kbecciv opened this issue Jul 29, 2023 · 9 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Jul 29, 2023

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 any chat
  2. Hover over any message from another user
  3. Click on the flag icon to open flag as spam RHN
  4. Open any other browser tab
  5. Return to the previous tab again
  6. Notice it always reloads

Expected Result:

Flag as spam RHN should not reload every time the tab is switched

Actual Result:

Flag as spam RHN always reloads when tab is switched

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.47-3
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Screen.Recording.2023-07-28.at.9.05.21.PM.mp4
Recording.3992.mp4

Expensify/Expensify Issue URL:
Issue reported by: @huzaifa-99
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690560646136359

View all open jobs on GitHub

@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 29, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 29, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 29, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@c3024
Copy link
Contributor

c3024 commented Jul 29, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

Every time we switch tabs to return to report, the RHN with flag comment action shows loading indicator

What is the root cause of that problem?

Every time we change the tab we make the api openReport call and this line here

const shouldShowLoading = props.isLoadingReportData || props.report.isLoadingReportActions;
if (shouldShowLoading) {
return <FullscreenLoadingIndicator />;

shows loading whenever openReport is called.

What changes do you think we should make in order to solve the problem?

This here
https://github.com/c3024/App/blob/eb020063789091eb2e3086b5b54af3ead8436645/src/pages/FlagCommentPage.js#L168
is sufficient to show not found page if the comment which is being flagged is deleted from another device and no separate loading indicator is required.
We can remove the above part of showing loading page there
and remove
!shouldShowLoading here also

<FullPageNotFoundView shouldShow={!shouldShowLoading && !ReportUtils.shouldShowFlagComment(getActionToFlag(), props.report)}>

and remove this isLoadingReportData here
isLoadingReportData: {
key: ONYXKEYS.IS_LOADING_REPORT_DATA,

What alternative solutions did you explore? (Optional)

@bernhardoj
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

Switching tab will show a loading indicator on the flag comment page.

What is the root cause of that problem?

In the flag comment page, we will show the loading indicator if isLoadingReportData or report.isLoadingReportActions is true.

const shouldShowLoading = props.isLoadingReportData || props.report.isLoadingReportActions;
if (shouldShowLoading) {
return <FullscreenLoadingIndicator />;
}

When we switch tabs, Report.openReport will be called that sets report.isLoadingReportActions to true which will show the loading indicator.

function openReport(reportID, participantLoginList = [], newReportObject = {}, parentReportActionID = '0', isFromDeepLink = false, participantAccountIDList = []) {
const optimisticReportData = {
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {
isLoadingReportActions: true,

The loading indicator is useful when the report and the actions are not loaded yet. If we don't show a loading indicator, it will show a not found page for a brief moment then show the flag page. It's not perfect (it won't cover all cases) but it's what we have now.

What changes do you think we should make in order to solve the problem?

We should show the loading indicator only if either report or the actions are empty.
const shouldShowLoading = (_.isEmpty(props.report) || _.isEmpty(props.reportActions)) && (props.isLoadingReportData || props.report.isLoadingReportActions);

@dukenv0307
Copy link
Contributor

This will be fixed in this issue #23125

@johncschuster johncschuster changed the title Web - Flag as spam page always reloads when switching browser tabs [HOLD - should be fixed by #23125] Web - Flag as spam page always reloads when switching browser tabs Jul 30, 2023
@johncschuster
Copy link
Contributor

Thanks for the heads up, @dukenv0307! I'll demote this to Weekly and keep an eye on the fix.

@johncschuster johncschuster added Weekly KSv2 and removed Daily KSv2 labels Jul 30, 2023
@melvin-bot melvin-bot bot added the Overdue label Aug 7, 2023
@johncschuster
Copy link
Contributor

Looks like we're still waiting on #23125 to be resolved.

@melvin-bot melvin-bot bot removed the Overdue label Aug 7, 2023
@melvin-bot melvin-bot bot added the Overdue label Aug 21, 2023
@huzaifa-99
Copy link
Contributor

Looks like #23125 fixed this. Not reproducible for me anymore.

@johncschuster
Copy link
Contributor

Awesome. Thanks for checking!

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. Weekly KSv2
Projects
None yet
Development

No branches or pull requests

6 participants