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.5k stars 2.85k forks source link

[$250] Messages read on desktop are not marked as read on mobile, if the mobile app is closed/backgrounded when it happens #50696

Open m-natarajan opened 1 week ago

m-natarajan commented 1 week 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: Reproducible in staging?: needs reproduction Reproducible in production?: Needs reproduction 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: @quinthar Slack conversation: https://expensify.slack.com/archives/C05LX9D6E07/p1728692784614339?thread_ts=1724780554.260709&cid=C05LX9D6E07

Action Performed:

Precondition: Close or background the mobile

  1. Send a message from account A to account B
  2. View and read that report in Desktop
  3. Open the report in mobile app

    Expected Result:

    Message should display as "read"

    Actual Result:

    Message is displayed as "unread"

    Workaround:

    unknown

    Platforms:

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

    • [x] Android: Native
    • [ ] Android: mWeb Chrome
    • [ ] iOS: Native
    • [ ] iOS: mWeb Safari
    • [ ] MacOS: Chrome / Safari
    • [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021845644254812409536
  • Upwork Job ID: 1845644254812409536
  • Last Price Increase: 2024-10-14
melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @johncschuster (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @Beamanator (AutoAssignerNewDotQuality)

MelvinBot commented 1 week ago

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

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

adhorodyski commented 1 week ago

Adam from Callstack, I'd like to work on this.

Julesssss commented 1 week ago

Do we have a high level plan for this?

Do we think this data would be received on reconnect or via push? Either way this seems like a backend issue to me.

mountiny commented 1 week ago

I think the lastUpdatedTime should be sent as onyxUpdate, but we are not processing them when the client is not active, so this might be another reason to implement the background processing.

But even when you come back online, we should get the missed Onyx updates and apply them. So what an external person can do imho is they can help diagnose if we get the updates but we fail to apply them for some reason or we just do not get the updates at all

adhorodyski commented 1 week ago

We've tested this across 2 accounts together with @rinej, here's what we know:

This leads to this data just not coming through to the mobile client, the desktop client is not able to push it through. Requests are being added correctly, so background processing will help as you don't need to reopen the app to walk through the queue.

melvin-bot[bot] commented 1 week ago

@Beamanator, @johncschuster, @Ollyws, @adhorodyski Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

marcaaron commented 1 week ago

When we jam the queue, this issue starts getting reproducible. With mobile app turned off, reading a message on desktop does not produce a 'read' status on mobile when opening the app.

Just passing through here, but what does it mean to "jam the queue"? Is there a step-by-step reproduction that can be provided?

johncschuster commented 1 week ago

Bumping for Melv. Thanks for the clarifying question, @marcaaron!

rinej commented 1 week ago

@marcaaron The easiest way to add many elements to the queue and 'jam it' is to:

The more actions you perform, the longer the queue will remain jammed. If you close the app while there are still pending actions in the queue, they will not sync with other devices. (Request will be waiting on the device)

You can monitor the queue size in the logs: Log.info('[SequentialQueue] '${requestToPersist.command}' command queued. Queue length is ${getLength()}');

Another way to jam the queue is by simulating a failed request, which triggers the retry mechanism. This will also block the queue temporarily. However, this behavior I didn't observed in real-world scenarios and just used it for debugging or testing purposes.

marcaaron commented 1 week ago

However, this behavior I didn't observed in real-world scenarios and just used it for debugging or testing purposes.

Ok, I think better to take it one part at a time and focus on the observable/reproducible stuff for now.

If you close the app while there are still pending actions in the queue, they will not sync with other devices

Ah, what does "they will not sync with other devices" mean? Are the expected updates arriving correctly via Pusher and not getting merged in because of the pending actions? Or something else? Have we written down the complete problem somewhere?

rinej commented 5 days ago

Ah, what does "they will not sync with other devices" mean?

As I understand it the requests are added to a SequentialQueue and processed one after the other in sequence. Updates are made optimistically on the device, meaning changes are displayed as if they were successful, even before they are confirmed by the server. However, if the user closes the app, the processing of requests stops, and no further requests are sent to the server. The queued requests will only start being processed again when the user reopens the app.

This can create the impression that a message was read or sent on one device, but when the app is opened on another device, the message isn't there because the request didn't reach the server yet.

Have we written down the complete problem somewhere?

Here we got the issue with that problem -> https://github.com/Expensify/App/issues/50140

melvin-bot[bot] commented 5 days ago

@Beamanator, @johncschuster, @Ollyws, @adhorodyski Whoops! This issue is 2 days overdue. Let's get this updated quick!

johncschuster commented 4 days ago

Thanks for the explanation, @rinej!

johncschuster commented 1 day ago

Bumping for Melvin