Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
1 of 6 tasks
m-natarajan opened this issue Apr 5, 2024 · 94 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@m-natarajan
Copy link

m-natarajan commented Apr 5, 2024

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:

  1. Sign in to NewDot
  2. 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
  • 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 OwnerCurrent Issue Owner: @twisterdotcom
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 5, 2024
Copy link

melvin-bot bot commented Apr 5, 2024

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

@twisterdotcom
Copy link
Contributor

@twisterdotcom twisterdotcom changed the title HIGH: [Reliability] Multiple calls to GetNewerActions on report open HIGH: [Needs Reproduction] [Reliability] Multiple calls to GetNewerActions on report open Apr 5, 2024
@twisterdotcom twisterdotcom added Needs Reproduction Reproducible steps needed retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause Weekly KSv2 and removed Daily KSv2 labels Apr 5, 2024
@MelvinBot
Copy link

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

@twisterdotcom
Copy link
Contributor

39674.mp4

@twisterdotcom
Copy link
Contributor

I still can't reproduce this. Nobody else has reported it recently.

@twisterdotcom
Copy link
Contributor

39674-2.mp4

So, an example of a single report loading GetNewerActions twice is https://staging.new.expensify.com/r/8036391930866711

When I load it, there are two GetNewerActions calls:

First:

jsonCode : 200
onyxData : [{onyxMethod: "merge", key: "reportActions_6028698164025499", value: {,…}}]
	0 : {onyxMethod: "merge", key: "reportActions_6028698164025499", value: {,…}}
		key : "reportActions_6028698164025499"
		onyxMethod : "merge"
	value : {,…}
		3459936395858833627 : {person: [{type: "TEXT", style: "strong", text: "CFO expensivepie.com"}], actorAccountID: 14459901,…}
			actionName : "REPORTPREVIEW"
			actorAccountID : 14459901
			actorEmail : "cfo@expensivepie.com"
			automatic : false
			avatar : "https://d1wpcgnaa73g0y.cloudfront.net/f427ffa485716562c8a975c8805a447b5684b740_128.jpeg"
			childCommenterCount : 1
			childLastActorAccountID : 8698376
			childLastReceiptTransactionIDs : "1204108464410787705"
			childLastVisibleActionCreated : "2024-03-14 21:38:55.951"
			childMoneyRequestCount : 1
			childOldestFourAccountIDs : "11665625"
			childRecentReceiptTransactionIDs : {1204108464410787705: "2024-03-14 21:37:29"}
				1204108464410787705 : "2024-03-14 21:37:29"
			childReportID : 1783551381563554
			childReportNotificationPreference : "hidden"
			childStateNum : 1
			childStatusNum : 1
			childType : "iou"
			childVisibleActionCount : 1
			created : "2024-03-14 21:37:29.911"
			lastModified : "2024-03-14 21:37:29.911"
			message : [,…]
				0 : {type: "COMMENT", html: "cfo@expensivepie.com owes A$0.00", text: "cfo@expensivepie.com owes A$0.00",…}
					deleted : ""
					html : "cfo@expensivepie.com owes A$0.00"
					isDeletedParentAction : false
					isEdited : false
					reactions : []
					text : "cfo@expensivepie.com owes A$0.00"
					type : "COMMENT"
					whisperedTo : []
				originalMessage : {lastModified: "2024-03-14 21:37:29.911", linkedReportID: "1783551381563554",…}
					lastModified : "2024-03-14 21:37:29.911"
					linkedReportID : "1783551381563554"
					originalActionAccountID : 14459901
				person : [{type: "TEXT", style: "strong", text: "CFO expensivepie.com"}]
					0 : {type: "TEXT", style: "strong", text: "CFO expensivepie.com"}
						style : "strong"
						text : "CFO expensivepie.com"
						type : "TEXT"
					previousReportActionID : "90093531819613915"
					reportActionID : "3459936395858833627"
					reportActionTimestamp : 1710452249911
					shouldShow : true
					timestamp : 1710452249
					whisperedToAccountIDs : []
	requestID : "872b3661fb7e63ec-LHR"

Second:

jsonCode : 200
onyxData : [{onyxMethod: "merge", key: "reportActions_8036391930866711", value: {,…}}]
	0 : {onyxMethod: "merge", key: "reportActions_8036391930866711", value: {,…}}
		key : "reportActions_8036391930866711"
		onyxMethod : "merge"
			value : {,…}
				2397210080192955362 : {person: [{type: "TEXT", style: "strong", text: "Admin Pie User"}], actorAccountID: 8698376,…}
					actionName : "REPORTPREVIEW"
					actorAccountID : 8698376
					actorEmail : "pecan@expensivepie.com"
					automatic : false
					avatar : "https://d1wpcgnaa73g0y.cloudfront.net/f7493165a790b796c92bd0337f8cf6b9bb74c669_128.jpeg"
					childLastActorAccountID : 8698376
					childMoneyRequestCount : 3
					childReportID : 438513267536357
					childReportNotificationPreference : "hidden"
					childType : "expense"
					created : "2024-03-16 13:30:42.420"
					lastModified : "2024-03-16 13:30:42.420"
					message : [{type: "COMMENT", html: "Xero owes $132.30", text: "Xero owes $132.30", isEdited: false,…}]
						0 : {type: "COMMENT", html: "Xero owes $132.30", text: "Xero owes $132.30", isEdited: false,…}
							deleted : ""
							html : "Xero owes $132.30"
							isDeletedParentAction : false
							isEdited : false
							reactions : []
							text : "Xero owes $132.30"
							type : "COMMENT"
							whisperedTo : []
						originalMessage : {lastModified: "2024-03-16 13:30:42.420", linkedReportID: "438513267536357"}
							lastModified : "2024-03-16 13:30:42.420"
							linkedReportID : "438513267536357"
						person : [{type: "TEXT", style: "strong", text: "Admin Pie User"}]
							0 : {type: "TEXT", style: "strong", text: "Admin Pie User"}
								style : "strong"
								text : "Admin Pie User"
								type : "TEXT"
						previousReportActionID : "5789708545474438065"
						reportActionID : "2397210080192955362"
						reportActionTimestamp : 1710595842420
						shouldShow : true
						timestamp : 1710595842
						whisperedToAccountIDs : []
	requestID : "872b3661fb7d63ec-LHR"

Maybe this is normal, maybe not?

@twisterdotcom twisterdotcom added External Added to denote the issue can be worked on by a contributor and removed Needs Reproduction Reproducible steps needed retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause labels Apr 11, 2024
@melvin-bot melvin-bot bot changed the title HIGH: [Needs Reproduction] [Reliability] Multiple calls to GetNewerActions on report open [$250] HIGH: [Needs Reproduction] [Reliability] Multiple calls to GetNewerActions on report open Apr 11, 2024
Copy link

melvin-bot bot commented Apr 11, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 11, 2024
Copy link

melvin-bot bot commented Apr 11, 2024

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Apr 11, 2024
@shahinyan11
Copy link
Contributor

shahinyan11 commented Apr 11, 2024

Proposal

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

HIGH: [Needs Reproduction] [Reliability] Multiple calls to GetNewerActions on report open

What is the root cause of that problem?

There is two different bugs.

  1. When we navigate to any report screen the Report.getNewerActions is called multiple time with the sam params due to calling onStartReached multiple times which is upstream bug
  2. When we navigate from report A to report B the Report.getNewerActions is called for both reports due to calling onStartReached for both screens which is upstream bug

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

  1. Add new const isStartReachedInitCallExecuted as a ref in MVCPFlatList which will be set true after first call of onStartReached function and set to false on focus
const isStartReachedInitCallExecuted = useRef(false)
...

useFocusEffect(
    useCallback(() => {
        isStartReachedInitCallExecuted.current = false
    }, []),
);

Add below onStartReached prop to Flatlist which will prevent additional calls if it has already been called once

<FlatList
    ...
    onStartReached={(e)=>{
        if(e.distanceFromStart === 0 && isStartReachedInitCallExecuted.current){
            return
        }
  
        props.onStartReached?.(e)
        isStartReachedInitCallExecuted.current = true
    }}
/>

What alternative solutions did you explore? (Optional)

Add below props to Flatlist . maxToRenderPerBatch for bug 1 and onStartReached for bug 2

maxToRenderPerBatch={props.data.lengt}
onStartReached={(e)=>{
    if(!isFocused){
        return
    }
    props.onStartReached?.(e)
}}

@twisterdotcom twisterdotcom changed the title [$250] HIGH: [Needs Reproduction] [Reliability] Multiple calls to GetNewerActions on report open [$250] HIGH: [Reliability] Multiple calls to GetNewerActions on report open Apr 11, 2024
@s77rt
Copy link
Contributor

s77rt commented Apr 11, 2024

@shahinyan11 Thanks for the proposal. Your RCA is not complete. Can you explain why is the loadNewerChats function being called in the first place for the first report (report A)?

PS: I think that's a different bug than what's reported, I'm suspecting GetNewerActions is getting called for the same report multiple times, I didn't experience this much but got 3 total calls (one for the report A and two for report B)

Screenshot 2024-04-11 at 9 52 48 PM

@shahinyan11
Copy link
Contributor

Proposal

Updated. Added solution for two bugs.

@s77rt
Copy link
Contributor

s77rt commented Apr 12, 2024

@shahinyan11 Thanks for the update. The root cause is still not correct. The call sequence is as follow: loadNewerChats -> handleReportActionPagination -> fetchNewerAction and in fetchNewerAction we have an early return checking for isLoadingNewerReportActions

const fetchNewerAction = useCallback(
(newestReportAction: OnyxTypes.ReportAction) => {
if (isLoadingNewerReportActions || isLoadingInitialReportActions) {
return;
}

Thus not checking for isLoadingNewerReportActions in loadNewerChats is not the culprit. On the other hand I'm seeing unguarded calls to getNewerActions in loadOlderChats which could be the culprit. Let's investigate more on that

@shahinyan11
Copy link
Contributor

@s77rt In our case the getNewerActions never called from loadOlderChats function. My root cause may not be entirely correct, but you can test my solution and see if it fixes the error.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Apr 29, 2024
@janicduplessis
Copy link
Contributor

@s77rt @roryabraham Here's the PR for the patch #41189

@twisterdotcom
Copy link
Contributor

@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
Copy link
Contributor

s77rt commented Apr 29, 2024

Looks like the RN patch also fixed the RNW bug 🎉

@janicduplessis
Copy link
Contributor

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

@shahinyan11
Copy link
Contributor

@janicduplessis @janicduplessis Thank you for appreciating my work

@twisterdotcom
Copy link
Contributor

Payment Summary (to be paid after 7 days):

@twisterdotcom twisterdotcom changed the title HIGH: [API Reliability] [$500] Multiple calls to GetNewerActions on report open [HOLD until 2024-05-06] HIGH: [API Reliability] [$500] Multiple calls to GetNewerActions on report open Apr 29, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels May 1, 2024
@melvin-bot melvin-bot bot changed the title [HOLD until 2024-05-06] HIGH: [API Reliability] [$500] Multiple calls to GetNewerActions on report open [HOLD for payment 2024-05-08] [HOLD until 2024-05-06] HIGH: [API Reliability] [$500] Multiple calls to GetNewerActions on report open May 1, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label May 1, 2024
Copy link

melvin-bot bot commented May 1, 2024

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

Copy link

melvin-bot bot commented May 1, 2024

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

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

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

  • @s77rt requires payment (Needs manual offer from BZ)

Copy link

melvin-bot bot commented May 1, 2024

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:

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

@twisterdotcom twisterdotcom changed the title [HOLD for payment 2024-05-08] [HOLD until 2024-05-06] HIGH: [API Reliability] [$500] Multiple calls to GetNewerActions on report open [HOLD for payment 2024-05-08] HIGH: [API Reliability] [$500] Multiple calls to GetNewerActions on report open May 1, 2024
@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels May 7, 2024
Copy link

melvin-bot bot commented May 8, 2024

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
Copy link
Contributor

s77rt commented May 8, 2024

Can we close this? i think the next steps will be handled in #41153

Copy link

melvin-bot bot commented May 13, 2024

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

@francoisl
Copy link
Contributor

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

@s77rt
Copy link
Contributor

s77rt commented May 14, 2024

This is paid already

@twisterdotcom
Copy link
Contributor

Yes, we can close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
Development

No branches or pull requests

9 participants