[HOLD for payment 2024-01-22] [$500] Web - tasks - Only half of the screen shows loading animation when creating tasks offline

[kbecciv]

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

---

Version Number: v1.4.18-2
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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Sign in ND
2. Disconnect device from the internet
3. Open a chat which you have not opened yet
4. While the loading screen is being shown click on + icon inside the composer and click on assign task
5. Input title and create the task
6. Observe the loading animation

Expected Result:

The loading animation should cover the available space above the task created

Actual Result:

The loading animation only covers half of the screen and half of the screen stays blank

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6326971_1703703743325.27.12.2023_21.31.59_REC.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c332001739a50ea1
• Upwork Job ID: 1740091910271926272
• Last Price Increase: 2024-01-03
• Automatic offers:
• paultsimura | Contributor | 28088275

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01c332001739a50ea1

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[paultsimura]

This is intentional. The short skeleton is shown when there is at least one action in the report (which is the new optimistically created task).
Reference: #31492 (comment), #31492 (comment)

[mananjadhav]

@Expensify/design can you confirm if the behaviors is intentional?

[paultsimura]

Proposal

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

When adding an optimistic report action, the long skeleton gets replaced with a short one.

What is the root cause of that problem?

This happens when we add a new optimistic ReportAction.
Because of this new action, isLoadingInitialReportActions becomes false here:

App/src/pages/home/ReportScreen.js

Line 186 in 5c5c862

const isLoadingInitialReportActions = _.isEmpty(filteredReportActions) && reportMetadata.isLoadingInitialReportActions;

As a result, we don't show the full-height skeleton here:

App/src/pages/home/ReportScreen.js

Line 498 in 5c5c862

{(!isReportReadyForDisplay || isLoadingInitialReportActions || isLoading) && <ReportActionsSkeletonView />}

Instead, we show one from here, which has possibleVisibleContentItems explicitly set to 3:

App/src/pages/home/report/ListBoundaryLoader/ListBoundaryLoader.js

Lines 51 to 53 in 5c5c862

	if (lastReportActionName !== CONST.REPORT.ACTIONS.TYPE.CREATED && (isLoadingInitialReportActions || isOffline)) {
	return <ReportActionsSkeletonView possibleVisibleContentItems={3} />;
	}

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

We should separate the offline and isLoadingInitialReportActions cases in the ListBoundaryLoader: show the full-height skeleton when isLoadingInitialReportActions:

if (type === CONST.LIST_COMPONENTS.FOOTER) {
        if (isLoadingOlderReportActions) {
            return <ReportActionsSkeletonView />;
        }

        const isReportLoadedTillBeginning = lastReportActionName === CONST.REPORT.ACTIONS.TYPE.CREATED;

        // Make sure the report chat is not loaded till the beginning. This is so we do not show the
        // skeleton view above the "created" action in a newly generated optimistic chat or one with not
        // that many comments.
        if (!isReportLoadedTillBeginning && isLoadingInitialReportActions) {
            return <ReportActionsSkeletonView />;
        }

        // If we are offline and the report is not yet loaded till the beginning, we assume there are more actions to load,
        // therefore show the skeleton view even though the actions are not loading.
        if (!isReportLoadedTillBeginning && isOffline) {
            return <ReportActionsSkeletonView possibleVisibleContentItems={3} />;
        }
}

With this implementation, we'll always show the full-height skeleton when the skeleton is needed, except for one specific offline flow. We'll still want to show the 3-lines-high skeleton only in the offline mode when the first set of actions was loaded before going offline:

Offline skeleton

iOS.mp4

Result:

Screen.Recording.2024-01-02.at.10.07.22.mov

What alternative solutions did you explore? (Optional)

[melvin-bot]

@mananjadhav, @sonialiap Whoops! This issue is 2 days overdue. Let's get this updated quick!

[dubielzyk-expensify]

@Expensify/design can you confirm if the behaviors is intentional?

I believe we added a bunch more skeleton comment loaders a bit back, but I could be wrong. Either way it seems a bit weird that some convos have many and some have few. I'd prefer for it to be more consistent and have similar amounts.

@shawnborton and @dannymcclain might have more context and thoughts though

[shawnborton]

Agree, would love to make this consistent everywhere and increase the amount of loader rows so it feels like the entire conversation is filled up.

[paultsimura]

so it feels like the entire conversation is filled up

Thanks for the clarification. In that case, my proposal remains relevant. Updated it now.

[dannymcclain]

I agree with Jon and Shawn!

[melvin-bot]

@mananjadhav, @sonialiap Eep! 4 days overdue now. Issues have feelings too...

[dragnoir]

Proposal

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

When adding an optimistic report action, the long skeleton gets replaced with a one of 3 items

What is the root cause of that problem?

We are setting 3 items to be loaded here:

App/src/pages/home/report/ListBoundaryLoader/ListBoundaryLoader.js

Line 52 in 5c5c862

return <ReportActionsSkeletonView possibleVisibleContentItems={3} />;

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

Remove the possibleVisibleContentItems={3}, the function ReportActionsSkeletonView is able to detect the space for possible items to be displayed

App/src/components/ReportActionsSkeletonView/index.tsx

Line 15 in 5c5c862

const contentItems = possibleVisibleContentItems || Math.ceil(Dimensions.get('window').height / CONST.CHAT_SKELETON_VIEW.AVERAGE_ROW_HEIGHT);

Math.ceil(Dimensions.get('window').height  /  CONST.CHAT_SKELETON_VIEW.AVERAGE_ROW_HEIGHT

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[mananjadhav]

Just confirming @shawnborton @dubielzyk-expensify. We want it to be the filled even for the offline mode? or retain the existing limit of 3 rows?

Considering the existing behavior, and that @paultsimura had the first RCA along with the proposal, I think @paultsimura's proposal is good here.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[shawnborton]

We want it to be the filled even for the offline mode? or retain the existing limit of 3 rows?

I kinda think we'd just fill it up more regardless of offline or not. cc @Expensify/design for thoughts.

[dannymcclain]

I kinda think we'd just fill it up more regardless of offline or not.

Same.

[dubielzyk-expensify]

Agree with that

[dragnoir]

@mananjadhav my proposal goes with the team thoughts. Fill the space regardless of offline or not.

[paultsimura]

@dragnoir as Manan mentioned, my proposal was the first to give the correct RCA along with a potential fix. The 3-lines matter was still in discussion, and it's a tiny implementation detail that's usually handled during the PR discussion.

[dragnoir]

@paultsimura pls check how a tiny implementation details is handlel here: #33709

[paultsimura]

That's a different situation: in your case, the other proposal was an addition to yours, not just "the team should decide which height to use".

Anyway, I think we'll come back to the 3-lines skeleton discussion once more during the PR development. I'll provide more reasoning as to why it might be still useful because here the discussion was a bit obscured by using "offline/online" terminology instead of the actual use-case for this skeleton.

[melvin-bot]

📣 @paultsimura 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer 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 📖

[paultsimura]

Thanks. The PR is ready for review: #34114

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

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

• fix: Use the full-height ReportActionsSkeletonView in all cases #34114

If no regressions arise, payment will be issued on 2024-01-22. 🎊

For reference, here are some details about the assignees on this issue:

• @mananjadhav requires payment through NewDot Manual Requests
• @paultsimura requires payment automatic offer (Contributor)

[melvin-bot]

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:

• [@mananjadhav] The PR that introduced the bug has been identified. Link to the PR:
• [@mananjadhav] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@mananjadhav] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@mananjadhav] Determine if we should create a regression test for this bug.
• [@mananjadhav] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@sonialiap] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[mananjadhav]

This could be treated as a change request as the skeleton view was intended to be 3 lines when added. I also don't think we need a regression test for this one.

@sonialiap Would need the payout summary on 01/22. Thanks in advance.

[sonialiap]

@mananjadhav $500 - please request through NewDot
@paultsimura $500 - paid ✔️

[JmillsExpensify]

$500 approved for @mananjadhav based on summary above.