search

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
2.99k stars 2.5k forks source link

HIGH: [API Reliability] Android: `GetMissingOnyxMessages` is not called in the background #39544

Closed arosiclair closed 2 weeks ago

arosiclair commented 1 month 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: N/A 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 Email or phone of affected tester (no customers): N/A Logs: N/A Expensify/Expensify Issue URL: Issue reported by: @arosiclair Slack conversation: N/A

Action Performed:

  1. Add yourself as a workspace member
  2. Kill the app
  3. Remove yourself from the workspace (this creates some missed onyx updates)
  4. Send yourself a DM
  5. Push notification received
  6. The app wakes up in the background
  7. A gap is detected and ONYX_UPDATES_FROM_SERVER gets set in Onyx to fill the gap

Expected Result:

The callback for ONYX_UPDATES_FROM_SERVER runs and calls the GetMissingOnyxMessages API in the background

Actual Result:

The callback does NOT run until the app is reopened

Workaround:

N/A

Platforms:

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

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/~011415a3700cdef632
  • Upwork Job ID: 1775605339418427392
  • Last Price Increase: 2024-04-03
melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to Contributor Plus for review of internal employee PR - @getusha (Internal)

arosiclair commented 1 month ago

As mentioned, the root problem is that Onyx.connect callbacks do not seem to run in our background process for Android (via headless js). Instead they run once the app is re-opened. That is blocking these two critical callbacks for filling update gaps in the background:

I started discussing this here.

One workaround is to optionally run the ONYX_UPDATES_FROM_SERVER code synchronously. However, that still leaves ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT as an empty value. I'm not sure if there is a way to synchronously retrieve that value from Onyx atm.

arosiclair commented 1 month ago

Okay I've dug a bit further and found this part of the docs is crucial:

You can do anything in your task such as network requests, timers and so on, as long as it doesn't touch UI. Once your task completes (i.e. the promise is resolved), React Native will go into "paused" mode (unless there are other tasks running, or there is a foreground app).

Currently our PushNotification.onReceived callback does not return any promise so the background task stops immediately after the function finishes. So to fix this issue, we need to return a promise that resolves after we get lastUpdateIDAppliedToClient from Onyx and run GetMissingOnyxMessages. I drafted these changes and it fixed the issue! I'll clean it up and post a PR soon.

arosiclair commented 1 month ago

PR is prepped and testing well however there are some conflicts that are gonna come in from #fireroom-2024-04-01-chats-do-not-load. Once those are in, I'll fix them and post the PR for review. ETA is end of next week.

arosiclair commented 1 month ago

Out for review!

quinthar commented 1 month ago

Woohoo!

quinthar commented 3 weeks ago

Can we close this?

quinthar commented 3 weeks ago

Closing, reopen if you disagree.

rushatgabhane commented 2 weeks ago

@laurenreidexpensify could you please attach payment summary for C+ review - https://github.com/Expensify/App/pull/40022#issuecomment-2059285253

rushatgabhane commented 2 weeks ago

created a manual request here - https://staging.new.expensify.com/r/6707043287393435

JmillsExpensify commented 2 weeks ago

Re-opening for payment summary so I can pay this one out.

laurenreidexpensify commented 2 weeks ago

Payment Summary:

C+ review @rushatgabhane $500 requested in newdot

JmillsExpensify commented 2 weeks ago

$500 approved for @rushatgabhane