[HOLD for payment 2024-05-08] HIGH: [API Reliability] [$500] Multiple calls to `GetNewerActions` on report open

m-natarajan commented 6 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.60-7 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: @quinthar Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1712180926567259

Action Performed:

Sign in to NewDot
Open any report and don't scroll anywhere

Expected Result:

Shouldn't call GetNewerActions multiple times

Actual Result:

GetNewerActions called 5times, despite not scrolling anywhere. Also, all 5 calls seem identical

Workaround:

unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

image (10)

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01d7ea3b088204e3df
Upwork Job ID: 1778413819479543808
Last Price Increase: 2024-04-26

Issue Owner

Current Issue Owner: @twisterdotcom

twisterdotcom commented 6 months ago

@janicduplessis who commented in Slack, would love to get you involved here. I'm leaning towards maybe just having @shahinyan11 handle the RNW workaround, and then assigning you to a separate issue to handle these in RN and RNW natively - does that sound alright?

janicduplessis commented 5 months ago

@twisterdotcom Sounds good, I can handle the RN part, I am pretty familiar with that code.

s77rt commented 5 months ago

@shahinyan11 Can you please update your proposal to fix the RNW bug (should fix both extra calls on focus and on blur)

janicduplessis commented 5 months ago

What do you think about merging something like this so when not using comment linking we just never try to fetch newer messages as there will never be any. I think this can be a good improvement to this for now until we can have a better integration with the backend as discussed here.

https://github.com/Expensify/App/pull/41053

janicduplessis commented 5 months ago

I also think I found a good solution to help avoid invalid calls to onStartReached in react-native so that will help for the case where we are actually using comment linking and GetNewerActions should get called, but only once.

shahinyan11 commented 5 months ago

Proposal

Updated. Added solution which will fix two bugs. I think the logic of my proposed solution can be used also in native level

twisterdotcom commented 5 months ago

@s77rt can we assign @shahinyan11?

janicduplessis commented 5 months ago

@shahinyan11 I’m pretty sure comment linking relies on the fact that onStartReached is called initially. Would you be able to test this to make sure your solution doesn’t break this?

I think it could be an ok temporary solution, but we should still implement other improvements mentioned here.

shahinyan11 commented 5 months ago

@shahinyan11 I’m pretty sure comment linking relies on the fact that onStartReached is called initially. Would you be able to test this to make sure your solution doesn’t break this?

I just added a new alternative solution. But can you explain what you mean by comment linking? . Could you describe an example ?

shahinyan11 commented 5 months ago

Proposal

Updated. Added alternative solution

janicduplessis commented 5 months ago

@shahinyan11 If you take a chat with many messages you can right click on a message in the middle and Copy Link. Then in another chat you can link to that message. This will use GetNewerActions and onStartReached to load the newer messages so we have to make sure this still works properly.

shahinyan11 commented 5 months ago

@janicduplessis Yes you are correct. My main solution will break it so I updated it to allow onStartReached to be called one time

melvin-bot[bot] commented 5 months ago

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

s77rt commented 5 months ago

@shahinyan11 On focus the callback is still executed https://snack.expo.dev/aZqOHa5m9m1AoGHGqAhoN?platform=web Also why you are adding e.distanceFromStart === 0 check?

shahinyan11 commented 5 months ago

@s77rt It is necessary that the callback be executed once during focus. See this https://github.com/Expensify/App/issues/39674#issuecomment-2079352605 . distanceFromStart returns the scroll distance that was scrolled before reaching the start and it always 0 on init

quinthar commented 5 months ago

What's the next step and ETA on this?

janicduplessis commented 5 months ago

I think there are a few things we can do to improve this. This issue has 2 main causes:

onStartReached is called more than it should. Ideally this would be fixed in react-native upstream.
We do not have a good way to know if calling GetNewerActions is needed. Ideally this would be returned by the API when opening the report so even if `onStartReached is called we don't fetch new reports.

For 1. I have a PR ready, need to be reviewed by meta engineers. We could make a patch for it to use it now.

For 2. I discussed this with @roryabraham and he will tackle this task when he can, I created this issue to track this.

In the meantime I suggest merging some of the workarounds proposed here. I think the simplest and safest option would be to just never call GetNewerActions when we are not in a comment linking report as implemented here.

s77rt commented 5 months ago

@shahinyan11

It is necessary that the callback be executed once during focus

Let's limit that only for comment linking. In other cases calling GetNewerActions on focus is redundant

distanceFromStart returns the scroll distance that was scrolled before reaching the start and it always 0 on init

This didn't answer my question. I meant what's the purpose of this condition or what would go wrong if we don't add it

s77rt commented 5 months ago

@janicduplessis Let's go with the RN patch + disabling GetNewerActions for non-comment-linked reports

s77rt commented 5 months ago

Current situation:

Bug	RCA	Fixed in Regular reports?	Fixed in Comment-Linked reports?
`GetNewerActions` called multiple times on rendering report	Bug in RN	No 🐛	No 🐛
`GetNewerActions` called 1 time on focusing/blurring report	Bug in RNW	No 🐛	No 🐛

After @janicduplessis's solution:

Bug	RCA	Fixed in Regular reports?	Fixed in Comment-Linked reports?
`GetNewerActions` called multiple times on rendering report	Bug in RN	Yes ✅	Yes ✅
`GetNewerActions` called 1 time on focusing/blurring report	Bug in RNW	Yes ✅	No 🐛

s77rt commented 5 months ago

For the remaining unresolved case, it requires a react-native-web upstream PR. From /App side the safest we can do is to prevent the call on blurring the report (partly fixing the issue, the call would still occur on focusing the report)

s77rt commented 5 months ago

@janicduplessis Can you please raise a PR for both the patch and the suggested solution?

janicduplessis commented 5 months ago

First PR is up, will do the patch one later today or tomorrow!

s77rt commented 5 months ago

🎀 👀 🎀

melvin-bot[bot] commented 5 months ago

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

shahinyan11 commented 5 months ago

This didn't answer my question. I meant what's the purpose of this condition or what would go wrong if we don't add it

@s77rt if distanceFromStart is 0 this means that cllback is called without the user scrolling ( i.e. on rendering ). if we don't add it the callback will only be called when user first time reaches to the start, other reachings will have no effect

shahinyan11 commented 5 months ago

Now what should I do next?

roryabraham commented 5 months ago

I have https://github.com/facebook/react-native/pull/44287 ready, need to be reviewed by meta engineers. We could make a patch for it to use it now.

Thanks for the upstream PR @janicduplessis - feel free to open a PR with the patch.

janicduplessis commented 5 months ago

Yes, planning to do that today!

janicduplessis commented 5 months ago

@s77rt @roryabraham Here's the PR for the patch https://github.com/Expensify/App/pull/41189

twisterdotcom commented 5 months ago

@s77rt @janicduplessis - are we getting @shahinyan11 to do anything here? If not, I'm kind of willing to pay out $250 regardless for their help here.

s77rt commented 5 months ago

Looks like the RN patch also fixed the RNW bug 🎉

janicduplessis commented 5 months ago

I think we are good for this issue. Yes please pay out @shahinyan11 also for his help and work on this.

shahinyan11 commented 5 months ago

@janicduplessis @janicduplessis Thank you for appreciating my work

twisterdotcom commented 5 months ago

Payment Summary (to be paid after 7 days):

@shahinyan11 paid $250 here
@s77rt paid $500 here
@janicduplessis does not require payment.

melvin-bot[bot] commented 5 months ago

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

melvin-bot[bot] commented 5 months ago

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

If no regressions arise, payment will be issued on 2024-05-08. :confetti_ball:

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

@s77rt requires payment (Needs manual offer from BZ)

melvin-bot[bot] commented 5 months ago

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:

[x] [@s77rt] The PR that introduced the bug has been identified. Link to the PR:
[x] [@s77rt] 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:
[x] [@s77rt] 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:
[x] [@s77rt] Determine if we should create a regression test for this bug.
[x] [@s77rt] 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.
[x] [@twisterdotcom] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

melvin-bot[bot] commented 5 months ago

Payment Summary

Upwork Job

ROLE: @s77rt paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@twisterdotcom)

[ ] I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
[ ] I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1778413819479543808/hired)
[ ] I have paid out the Upwork contracts or cancelled the ones that are incorrect
[ ] I have verified the payment summary above is correct

s77rt commented 5 months ago

Can we close this? i think the next steps will be handled in https://github.com/Expensify/App/issues/41153

melvin-bot[bot] commented 5 months ago

@francoisl, @twisterdotcom, @s77rt Huh... This is 4 days overdue. Who can take care of this?

francoisl commented 5 months ago

@twisterdotcom did you get a chance to issue the payments you had summarized in this comment?

s77rt commented 5 months ago

This is paid already

twisterdotcom commented 5 months ago

Yes, we can close.

Expensify / App