Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.49k stars 2.84k forks source link

Thread - Parent message gets grayed out on going offline #38411

Closed lanitochka17 closed 7 months ago

lanitochka17 commented 7 months ago

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: 1.4.53-1 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team

Issue found when executing PR https://github.com/Expensify/App/pull/38327

Action Performed:

  1. Log in to New Expensify
  2. Navigate to any chat
  3. Hold any existing message > select Reply in thread
  4. Send a message
  5. Disable the internet connection

Expected Result:

Existing messages should remain fully opaque while offline

Actual Result:

Thread parent message gets grayed out on going offline

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/ef4b27bf-1b81-4206-92de-64aab41af019

View all open jobs on GitHub

melvin-bot[bot] commented 7 months ago

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

lanitochka17 commented 7 months ago

@slafortune FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

lanitochka17 commented 7 months ago

We think that this bug might be related to #vip-vsp

bernhardoj commented 7 months ago

Proposal

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

The thread parent message stays gray when going offline.

What is the root cause of that problem?

When we start a new thread, it will optimistically create the thread report with a pending field of createChat, https://github.com/Expensify/App/blob/2979e3c709f9e0d8cfe3b25dc132fea03d046a5a/src/libs/actions/Report.ts#L639-L641

which will gray out the thread parent message if offline. https://github.com/Expensify/App/blob/2979e3c709f9e0d8cfe3b25dc132fea03d046a5a/src/pages/home/report/ReportActionItemParentAction.tsx#L80

When the OpenReport API completes, the pending field is cleared, but the thread report data (ancestor) isn't updated.

The thread ancestor data is updated through this onyx subscription. It subscribes to the thread ancestors report. https://github.com/Expensify/App/blob/2979e3c709f9e0d8cfe3b25dc132fea03d046a5a/src/pages/home/report/ReportActionItemParentAction.tsx#L46-L70

But as you can notice, it depends on the report object, but we don't add it to the useEffect deps, so it always hold the old report object, even if the thread ancestor is updated and after the API removes the current thread pending field.

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

Add report to the useEffect deps. It would be better to add shouldHideThreadDividerLine too. This will make sure the getAllAncestorReportActions is recalculated every time the current thread report object changes.

What alternative solutions did you explore? (Optional)

Just in case we want to avoid Onyx unsubs and subs again, we can store the report on a ref

OR

We can use ReportUtils.getReport(report.reportID). setAllAncestors(ReportUtils.getAllAncestorReportActions(ReportUtils.getReport(report.reportID), shouldHideThreadDividerLine));

But we still depend on the thread ancestor to update to recalculate the getAllAncestorReportActions. (ancestorIDs.current.reportIDs doesn't include the current thread report). Even though the above works because the thread ancestor is updated when sending a message, I think it's better to subscribe to the current report too.

useEffect(() => {
    setAllAncestors(ReportUtils.getAllAncestorReportActions(report, shouldHideThreadDividerLine));
}, [report, shouldHideThreadDividerLine]);

This way, the thread ancestors data will be updated when:

  1. The current thread report is updated (this currently doesn't happen which raises this issue)
  2. The thread ancestor report is updated
melvin-bot[bot] commented 7 months ago

@slafortune Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

slafortune commented 7 months ago

I am able to recreate this but don't think it is a high enough priority to move forward with. This is simply a visual that most users won't even run into or notice.