[HOLD for payment 2024-12-19] [$250] WS chat- "Pay with expensify" button blinks to Submit and back after clicking Submit

[IuliiaHerets]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 9.0.67-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5263862
Issue reported by: Applause Internal Team

Action Performed:

1. Open staging.new.expensify.com and create a new gmail account.
2. Create a workspace.
3. Navigate to Workspace settings/ More features, enable Workflows.
4. Open Workflows page and enable Delay submissions.
5. Navigate to workspace chat and create manual expense using "+" button.
6. In WS chat click. "Submit" button on the expense preview.

Expected Result:

"Pay with expensify" button appears on the expense preview after clicking on "Submit" button. The button does not blink.

Actual Result:

"Pay with expensify" button blinks to Submit and back after clicking Submit button on the expense preview.

Workaround:

Unknown

Platforms:

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6678319_1732730867265.submit_button_blinks.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021862514913604965431
• Upwork Job ID: 1862514913604965431
• Last Price Increase: 2024-11-29
• Automatic offers:
• rojiphil | Contributor | 105125262
• jacobkim9881 | Contributor | 105183692

[melvin-bot]

Triggered auto assignment to @zanyrenney (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.

[jacobkim9881]

Proposal

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

Clicking submit button makes flicking on a workspace with delay submissions.

What is the root cause of that problem?

getMissingOnyxUpdates is executed for SubmitReport API because it has late updateID to previous updateID and because the button UI changes every time onyx data updated.

The button UI changes by reading childStatusNum from onyx data. When it is 0 then it shows Submit grey button and 2 shows Pay green button. But by getMissingOnyxUpdates API it changes from 2(green) to 0(grey) to 2(again green). In the issue getMissingOnyxUpdates doesn't have to be executed. It is because childStatusNum is already changed into 2 by SubmitReport API just before.

Every time the client app gets response from BE it checks if any update is needed:

App/src/libs/actions/OnyxUpdates.ts

Lines 176 to 179 in 10a7be8

	if (lastUpdateIDFromClient < previousUpdateID) {
	Log.info('lastUpdateIDFromClient is less than the previousUpdateID received, client needs updating', false, {lastUpdateIDFromClient, previousUpdateID});
	return true;
	}

and if updated is needed then this is activated:

App/src/libs/Middleware/SaveResponseInOnyx.ts

Lines 38 to 39 in 10a7be8

	// Save the update IDs to Onyx so they can be used to fetch incremental updates if the client gets out of sync from the server
	OnyxUpdates.saveUpdateInformation(responseToApply);

If any update isn't needed to be checked then just onyx is updated w/o any BE update.

App/src/libs/Middleware/SaveResponseInOnyx.ts

Lines 34 to 36 in 10a7be8

	if (requestsToIgnoreLastUpdateID.includes(request.command) || !OnyxUpdates.doesClientNeedToBeUpdated(Number(response?.previousUpdateID ?? 0))) {
	return OnyxUpdates.apply(responseToApply);
	}

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

We can let the UI better by passing updates from BE update for SubmitReport API.

There is already a method to check if any update from BE is needed:

App/src/libs/Middleware/SaveResponseInOnyx.ts

Lines 6 to 8 in 10a7be8

	// If we're executing any of these requests, we don't need to trigger our OnyxUpdates flow to update the current data even if our current value is out of
	// date because all these requests are updating the app to the most current state.
	const requestsToIgnoreLastUpdateID: string[] = [

In the array we can add WRITE_COMMANDS.SUBMIT_REPORT for SubmitReport.

const requestsToIgnoreLastUpdateID: string[] = [
...,

WRITE_COMMANDS.SUBMIT_REPORT,
]

Then whenever clicking submit button in a workspace with delay submission, the button isn't flicking because it doesn't update from BE.

What alternative solutions did you explore? (Optional)

N/A

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

📣 @jacobkim9881 You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

[melvin-bot]

📣 @rojiphil 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[zanyrenney]

@rojiphil please review and let me know if we can take @jacobkim9881 proposal. thanks!

[rojiphil]

@zanyrenney. The proposal works. Thanks @jacobkim9881 for the proposal.

@jacobkim9881 proposal to add SubmitReport API request to the ignore list LGTM as the request is not mandatorily depending on any latest update from server.
Passing on to internal engineer to weigh in.
🎀👀🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

📣 @jacobkim9881 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[rojiphil]

per @arosiclair's comment here we likely need to revert the change

That's interesting. Looking into this again.

[rojiphil]

So, looking at the comment here

hey I noticed this change while investigating another issue. I don't have full context on this bug, but this is almost certainly the wrong way to fix it. We're essentially disabling reliable Onyx updates for SubmitReport requests here and ignoring any Onyx updates that may have been missed. I think we should revert this change and find another way to address the root problem.

@arosiclair The context here is that we are fetching Onyx updates from server in the middle of SUBMIT_REPORT request. This would impact the optimistic onyx updates carried out as part of the API request itself when getMissingOnyxMessages comes back with outdated data. This in-turn can impact the UX experience of the user as it happens here.

Now, I am also wondering why getMissingOnyxMessages was introduced in the middle of all such requests when the response of the API request would contain the updated data. Maybe there are some use cases where this is needed. Do you have any context on this by any chance?
cc @rafecolton

[arosiclair]

Now, I am also wondering why getMissingOnyxMessages was introduced in the middle of all such requests when the response of the API request would contain the updated data. Maybe there are some use cases where this is needed. Do you have any context on this by any chance?

So our reliable updates system tries to fetch and apply all onyx updates in the correct order. We do this by adding sequential updateIDs to every onyx update. The client tracks the last update it applied (lastUpdateIDAppliedToClient) and each API command returns the previous update it expects (previousUpdateID). So when we detect that some updates were missed (the client's lastUpdateID doesn't match the previousUpdateID from the request) we call GetMissingOnyxMessages to fetch those missed updates. Since we're trying to guarantee the updates are applied in the correct order, those missed updates must be merged before the updates from the API command. So that's why you may see GetMissingOnyxMessages "interrupt" some API requests.

This is all expected behavior in most cases so it isn't the root problem of the bug we're seeing here. Instead, there's probably an issue with the Onyx updates we're merging (either on the client or from the API) or possibly the issue could be fixed by changing our rendering logic in React.

[rojiphil]

Since we're trying to guarantee the updates are applied in the correct order, those missed updates must be merged before the updates from the API command

Thanks @arosiclair for the detailed explanation.
So then, I am leaning towards the FE updating the missing onyx updates and the API specific response together to Onyx as this would ensure both the order and the latest API specific update in Onyx. What do you think of this approach?

[arosiclair]

So then, I am leaning towards the FE updating the missing onyx updates and the API specific response together to Onyx as this would ensure both the order and the latest API specific update in Onyx. What do you think of this approach?

I'm not sure I fully understand. I think we should find out what data in these onyx updates cause the button to blink. Then we can think about how to fix that.

[rojiphil]

I think we should find out what data in these onyx updates cause the button to blink. Then we can think about how to fix that.

The following excerpt from the root cause in proposal answers what data is causing this.

The button UI changes by reading childStatusNum from onyx data. When it is 0 then it shows Submit grey button and 2 shows Pay green button. But by getMissingOnyxUpdates API it changes from 2(green) to 0(grey) to 2(again green).

And the response from API request is what changes back to 2 (again green). I am still not able to think any other better way but maybe we can find one. What do you think?

[arosiclair]

The button UI changes by reading childStatusNum from onyx data. When it is 0 then it shows Submit grey button and 2 shows Pay green button. But by getMissingOnyxUpdates API it changes from 2(green) to 0(grey) to 2(again green).

And the response from API request is what changes back to 2 (again green). I am still not able to think any other better way but maybe we can find one. What do you think?

Alright that sounds accurate. So I would say we should find out why we're queuing that additional update for the childStatusNum that makes it blink (we'd have to look into that internally on the backend). It sounds like that additional update is unnecessary. cc @rafecolton

Alternatively, we could use optimisticData to set some state like report.isLoading or report.pendingAction that we can use to ignore unnecessary changes to the childStatusNum in the component. That arguably doesn't address the root problem though.

[rafecolton]

If this requires a back-end investigation, I will need to hand it off. @arosiclair do you have bandwidth to take this over or should we ask Zany to find a volunteer?

[arosiclair]

No bandwidth atm unfortunately. I'll grab the issue if I get some cycles and it's still not fixed though.

[rafecolton]

@zanyrenney I think it's best if I unassign at this point, can you please find a volunteer to take this over?

[jacobkim9881]

@arosiclair Could I ask a question please? When is the most neccessary time to call GetMissingOnyxMessages? One of my guesses could be when people are chatty to send comments at once. Like A, B chats at once then A could get B's message by the help of GetMissingOnyxMessages. Do you agree with me?

[jacobkim9881]

@arosiclair Thanks for sharing time for this issue though.

[arosiclair]

@arosiclair Could I ask a question please? When is the most neccessary time to call GetMissingOnyxMessages? One of my guesses could be when people are chatty to send comments at once. Like A, B chats at once then A could get B's message by the help of GetMissingOnyxMessages. Do you agree with me?

Basically any time Onyx updates get delivered in the wrong order, we would call GetMissingOnyxMessages to fix that. We do that check here. It can happen in situations like you say where there is a lot of rapid activity or when there's instability with the network or our servers.

[zanyrenney]

Looking for a BE engineer to pick this up -added to the right project / release.

[jacobkim9881]

For this issue, the sync will be missed while clicking submit button. In the workspace room, I think we would worry about missing Onyx data for chats from other users(if there is any risk to lose other Onyx data, then please let me know). While having missed chats, if the user didn't re-enter the chat room the comment will be missed. If I am wrong, then let me know.

Example)
Where: Workspace chat room
users: A, B, C (or other)
A clicks submit button -> B(or B, C) send a comment at once -> GetMissingOnyxMessages must be missed -> A can not see the comment sent

[jacobkim9881]

I think we would worry about missing Onyx data for chats from other users

I wanted to say the chances to lose Onyx data is rare. But blinking issue occurs every time any user clicks submit button.

[zanyrenney]

FYI @garrettmknight i chatted to @mountiny about this one and whether we could have an external contributor pick it up. He suggested this should be a Hot Pick instead as the BE contributors are not ramped up yet, so I have applied the label: if that is a call out you do in the update, please also include this issue.

Thank you!

[zanyrenney]

Waiting for an internal engineer to pick this up.

[melvin-bot]

This issue has not been updated in over 15 days. @rojiphil, @jacobkim9881, @zanyrenney eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!