[$250] Task – Task creator avatar appears briefly on top of chat when create task with non-existing

[izarutskaya]

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: 9.0.18-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: https://expensify.testrail.io/index.php?/tests/view/4836876
Email or phone of affected tester (no customers): applausetester+jp_e_category_2@applause.expensifail.com
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

1. Go to https://staging.new.expensify.com/
2. Log in
3. Click on FAB and Assign task
4. Enter title
5. Click on Assignee and select non-existing user
6. Click on Confirm the task

Expected Result:

Task assignee avatar present on the top of the chat

Actual Result:

Task creator avatar appears briefly on the top of the chat

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

Bug6566324_1723174921955.Task_non-ex.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~016ea999111b3bbb3c
• Upwork Job ID: 1822945464965222378
• Last Price Increase: 2024-08-19

Issue Owner

Current Issue Owner: @c3024

[melvin-bot]

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

[izarutskaya]

We think this issue might be related to the #vip-vsb

[bfitzexpensify]

Minor bug. Will send external to see if it's easy to resolve, but I think this is the type of thing we could also close.

[melvin-bot]

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

[melvin-bot]

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

[jacobkim9881]

Proposal

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

When assigning a task to a non-existent user, the user's avatar briefly flicks.

What is the root cause of that problem?

App/src/libs/ReportUtils.ts

Lines 2163 to 2171 in 1eef411

	function getIcons(
	report: OnyxInputOrEntry<Report>,
	personalDetails: OnyxInputOrEntry<PersonalDetailsList>,
	defaultIcon: AvatarSource | null = null,
	defaultName = '',
	defaultAccountID = -1,
	policy?: OnyxInputOrEntry<Policy>,
	invoiceReceiverPolicy?: OnyxInputOrEntry<Policy>,
	): Icon[] {

Because at the above function, there are 2 icons for the non-existent user while creating a chat report. 2 icons are created because the newly added account has an account ID as optimistic one and the other ID from BE API. Till the optimistic account ID is deleted, there are 3 accounts(admin, optimistic ID, the other ID from BE API). After the optimistic account ID deleted, getIcons function executed at this condition below,

App/src/libs/ReportUtils.ts

Lines 2343 to 2352 in 1eef411

	if (isOneOnOneChat(report)) {
	const otherParticipantsAccountIDs = Object.keys(report.participants ?? {})
	.map(Number)
	.filter((accountID) => accountID !== currentUserAccountID);
	return getIconsForParticipants(otherParticipantsAccountIDs, personalDetails);
	}
	const participantAccountIDs = Object.keys(report.participants ?? {}).map(Number);
	return getIconsForParticipants(participantAccountIDs, personalDetails);
	}

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

We can keep the icon from flickering by filtering the optimistic ID while creating a new chat report. The optimistic ID(1) and the other ID(2) from BE API have the same name,

0 {id: 18064077, source: "https://d1wpcgnaa73g0y.cloudfront.net/ADMIN.jpeg", type: "avatar", name: "admin", fallbackIcon: ""}
1 {id: 18139679, source: "https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/default-avatar_24.png", type: "avatar", name: "same@abc.com", fallbackIcon: ""}
2 {id: 1457118634, source: function, type: "avatar", name: "same@abc.com", fallbackIcon: ""}

We can add a condition at getIcons function and return icons for members,

if (report.lastActionType === 'CREATED' && hasSamename(report)) {

  return getIconsForParticipants();
}

Then only a member icon will be got when assigning a task for creating a new chat with the member.

What alternative solutions did you explore? (Optional)

N/A

[c3024]

@jacobkim9881

Backend sends the report participants correctly. So, I think we should check why the merging of the sent participants into existing report participants duly removing the optimistic participant details is not happening correctly.

[jacobkim9881]

@c3024 Because non-existing user doesn't get an real participant ID on selecting an assignee stage, merging optimistic report into real report from server have made duplicate participant ID. Merging occurs on read news action function. The process is like this,

Optimistic ID made -> requesting creating a task is made -> got ID from BE -> Read newest action function -> 2 ID(both for one member ID) are merged into one report -> now 2 ID -> applyHTTPSOnyxUpdates from BE (1 ID) -> finally 1 ID

[melvin-bot]

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

[c3024]

2 ID(both for one member ID) are merged into one report

Then in my opinion, this should be fixed.

Merging incorrectly and then filtering by checking if names are same is a workaround and we should avoid it.

[jacobkim9881]

2 ID(both for one member ID) are merged into one report

This process is merging of Onyx from BE response. Then the merging should be able to edited to filter by checking ID.

[c3024]

Then could you check where this should be fixed in react-native-onyx?

[dominictb]

Edited by proposal-police: This proposal was edited at 2024-08-20 16:23:34 UTC.

Proposal

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

Task creator avatar appears briefly on the top of the chat.

This issue is hard to spot but if we screencast the whole process and try to pinpoint the exact timestamp then we can see the issue

What is the root cause of that problem?

When we call createTaskAndNavigate with non existing user, the account ID is optimistic. And after the task is created, we clear the personal details associated with the optimistic account ID in here (which is here). However, this is 2 onyx operations so there's a (small) gap that we could see both the optimistic account ID and non-optimistic account ID exist in the report.participants

And in here and here, we are getting icons of oneOnOne chat report. However, at that point the report record has 3 participant account ID (admin, optimistic assignee, and non-optimistic account ID), so isOneOnOne(chatReport) is false, hence we show 3 different icons for 3 participant records as dictated in here

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

In here, we should add a field isOptimisticPersonalDetail=true, and in here and here, we should filter out optimistic account ID in case there's a non-optimistic account ID in the report.participant that has the same login. This is a strategy used in here. We can consider creating a shared function for this purpose.

What alternative solutions did you explore? (Optional)

This can be fixed on the BE side as well. BE will need to clear the optimistic participant in the report.participant in the same Onyx operation as in here.

[dominictb]

Minor proposal update to fix wrong word

[c3024]

Given that it is recommended to update the backend response and successData successively, one after the other, as shown here, we cannot avoid the brief, intermittent presence of both optimistic and non-optimistic (backend returned) IDs among the participants.

Since we already use a pattern of omitting the optimistic ID here, as shown by @dominictb, we can apply the same approach here.

The RCA and fix suggested here by @dominictb look good to me!

🎀 👀 🎀 C+ Reviewed

[melvin-bot]

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

[marcaaron]

However, this is 2 onyx operations so there's a (small) gap that we could see both the optimistic account ID and non-optimistic account ID exist in the report.participants

Hmm this sounds like something we could maybe fix by changing the behavior of how these updates are applied? Like it sounds like you are saying because we briefly set the personal details to 'null' and then apply the correct ones after it leads to this problem.

But the personal details is just one object - so if we applied these updates at the same time the problem would not happen? Or no?

[marcaaron]

The solution we applied there looks like a hack to me kind of. Also what will be shown instead? Please post a video of the UX so we can see what this flow looks like with the change applied. Is there some empty space where an avatar should be?

[marcaaron]

This can be fixed on the BE side as well. BE will need to clear the optimistic participant in the report.participant in the same Onyx operation as in here.

This sounds better IMO, but might not be valuable enough to spend time on right now.

[c3024]

We briefly set the personal details to 'null' and then apply the correct ones, leading to this problem.

Actually, it's the other way around. Here's the flow:

1. A task is created, and the participants field in the report contains the optimistic assignee ID and the current user ID.

2. The backend returns a response with the report containing the participants field with the non-optimistic (real) assignee ID and the current user ID. This is then merged into Onyx.

   App/src/libs/actions/OnyxUpdates.ts

   Line 37 in 685bb4c

   const onyxDataUpdatePromise = response.onyxData ? updateHandler(response.onyxData) : Promise.resolve();

   Now, the participants field has three participants. The logic here:

   App/src/libs/ReportUtils.ts

   Lines 2351 to 2359 in ddec3e7

   	if (isOneOnOneChat(report)) {
   	const otherParticipantsAccountIDs = Object.keys(report.participants ?? {})
   	.map(Number)
   	.filter((accountID) => accountID !== currentUserAccountID);
   	return getIconsForParticipants(otherParticipantsAccountIDs, personalDetails);
   	}
   	const participantAccountIDs = Object.keys(report.participants ?? {}).map(Number);
   	return getIconsForParticipants(participantAccountIDs, personalDetails);

   checks if it's a one-on-one chat by excluding the current user ID and checking the number of participants in the report:

   App/src/libs/ReportUtils.ts

   Lines 1599 to 1613 in ddec3e7

   	function isOneOnOneChat(report: OnyxEntry<Report>): boolean {
   	const participantAccountIDs = Object.keys(report?.participants ?? {})
   	.map(Number)
   	.filter((accountID) => accountID !== currentUserAccountID);
   	return (
   	!isChatRoom(report) &&
   	!isExpenseRequest(report) &&
   	!isMoneyRequestReport(report) &&
   	!isPolicyExpenseChat(report) &&
   	!isTaskReport(report) &&
   	isDM(report) &&
   	!isIOUReport(report) &&
   	participantAccountIDs.length === 1
   	);
   	}

   When there are three participants in the report, this check fails. As a result, the current user ID is not excluded, and all three avatars are briefly shown.

3. Then, the successData:

   App/src/libs/ReportUtils.ts

   Line 6780 in ddec3e7

   participants: {[assigneeAccountID]: null},

   is merged into Onyx:

   App/src/libs/actions/OnyxUpdates.ts

   Lines 38 to 42 in 685bb4c

   	return onyxDataUpdatePromise
   	.then(() => {
   	// Handle the request's success/failure data (client-side data)
   	if (response.jsonCode === 200 && request.successData) {
   	return updateHandler(request.successData);

   This removes the optimistic assignee ID, and only one avatar is shown correctly because the one-on-one chat check now succeeds.

This sounds better IMO, but might not be valuable enough to spend time on right now.

I agree. I think, generally and not just in this specific case, that unless the successData is a large object that would be prohibitive to send over the network, it might be better to merge the successData on the backend and include it in the response, where practicable.

@dominictb correct me if I am wrong.

[dominictb]

Please post a video of the UX so we can see what this flow looks like with the change applied

I'll send the video in a bit.

[marcaaron]

Ok, got it, thanks for the explanation @c3024.

It still sounds like a possible "root cause" is that we are not merging together these two updates, but merging in the server data first then the successData at some delay? If we were to say, collapse the server data + successData into one single update first then this "flash" of inaccurate stuff would not happen?

it might be better to merge the successData on the backend and include it in the response, where practicable

Hmm I think perhaps the server should just be responsible for the part of successData that might cause a problem like this - but not all the successData in a general sense since issues like this are rare.

Given how our system works and applies updates the current behavior makes sense. There is no differentiation between optimistic and actual data. But exposing that detail to the frontend might not be the answer to me. So, I'd think we'd either:

1. Have the client pass the optimisticAccountID and then have the server set it to null in the same update (and really anytime a "real" accountID is created we should also be doing this in the server)

OR

2. Collapse the updates together and merge once so there isn't a "flash" of inaccurate stuff.

OR

3. A longer term fix where accountID can be client generated.

Honestly, since everything ends up looking how it should look I'm not sure if we should be doing the proposed solution. Going to ask what the internal team thinks about this one.

[kaushiktd]

@marcaaron, when I pulled the code, the steps you mentioned didn’t work as expected. The assigned task isn't showing up in the top menu.
screen-capture.webm

[c3024]

@marcaaron

It still sounds like a possible "root cause" is that we are not merging together these two updates, but merging in the server data first, then the successData with some delay?

Yes, that is the root cause. As I mentioned here:

Given that it is recommended to update the backend response and successData successively, one after the other, as shown here

App/src/libs/actions/OnyxUpdates.ts

Lines 34 to 43 in 685bb4c

	// First apply any onyx data updates that are being sent back from the API. We wait for this to complete and then
	// apply successData or failureData. This ensures that we do not update any pending, loading, or other UI states contained
	// in successData/failureData until after the component has received and API data.
	const onyxDataUpdatePromise = response.onyxData ? updateHandler(response.onyxData) : Promise.resolve();
	return onyxDataUpdatePromise
	.then(() => {
	// Handle the request's success/failure data (client-side data)
	if (response.jsonCode === 200 && request.successData) {
	return updateHandler(request.successData);
	}

the comment suggests merging response data and successData successively. I did not dig deeper into why it is required. I assumed it must be something important and felt that the frontend hack was acceptable since it had already been used elsewhere.

All three solutions you suggested are indeed more ideal than filtering the optimistic ID on the frontend.

Honestly, since everything ends up looking how it should, I'm not sure if we should be implementing the proposed solution.

Yes, the three avatars appear for a very brief time, and sometimes it happens so quickly that it’s not perceptible at all. We can leave this as it is and close this issue. Perhaps this brainstorming will be useful if we encounter a situation where the gap between updates actually causes a significant UI/UX issue.

[dominictb]

it happens so quickly that it’s not perceptible at all

It is in my local machine and possibly your local machine but not in the OP's machine, or any user's browser. It is not guaranteed at all.

the comment suggests merging response data and successData successively. I did not dig deeper into why it is required. I assumed it must be something important

It is in fact the correct and recommended design. The response data should be merged first, acknowledging that we have received data from the server before we move forward with the successData. For example in this case, if we apply successData and serverData simultaneously , the optimistic data clean up could happen first before the BE-returned data kick in, and that could lead to situation in the participant list:

• The optimistic participant record disappears
• Then after that, BE-returned participant record appears.

But the ideal UX in this way, should be the change of optimistic participant record -> BE-returned participant record without any disappearing gap.

Hmm I think perhaps the server should just be responsible for the part of successData that might cause a problem like this

I don't feel like it. BE should never be responsible for cleaning up optimistic data in the FE, because BE will never have the full context of what is optimistic data by generated by FE, and BE should not need to know that. FE should be the one responsible for cleaning up optimistic data and other related optimistic UX (including hiding/disable optimistic data), making sure that the UI makes sense to the end user. This is why I advocate for FE strategy in other issue and this issue as well.

[c3024]

if we apply successData and serverData simultaneously , the optimistic data clean up could happen first before the BE-returned data kick in, and that could lead to situation in the participant list:

The optimistic participant record disappears
Then after that, BE-returned participant record appears.

I did not mean that we can apply them simultaneously. I understand that we should have the backend response before doing anything with successData. I meant that I have not checked the difficulties of merging the (successful) response data and success data together first and then merging this merged object into Onyx.

[dominictb]

Thinking about it, since we have this PR Expensify/react-native-onyx#490 merged, can we reliably do updateHandler(...response.onyxData, ...successData) ? Because now we ensure that every individual operation passed in Onyx.update is applied reliably.

[jacobkim9881]

Then could you check where this should be fixed in react-native-onyx?

@c3024 Let me know you if I found a solution.

[jacobkim9881]

Exactly the report having the issuable merging was made just after "GetMissingOnyxMessages" request. Here is the process when after creating a task:

1. Optimistic ID on the optimistic report as there is other report having real ID from BE
2. Log:

[Debug] [info] [OnyxUpdateManager] Applying update type: https with lastUpdateID: 1295849174 - {"command":"GetMissingOnyxMessages"} – ""
[Debug] [info] [Onyx] merge called for key: OnyxUpdatesLastUpdateIDAppliedToClient hasChanged: true - "" – ""
[Debug] [info] [Onyx] set called for key: nvp_quickActionGlobalCreate properties: action,chatReportID,isFirstQuickAction,targetAccountID hasChanged: true - "" – ""
[Debug] [info] [Onyx] set called for key: userMetadata properties: accountID,email,freeTrial,planType,role,tryNewDotDismissed hasChanged: false - "" – ""
[Debug] [info] [Onyx] merge called for key: personalDetailsList properties: 17907433,18187017 hasChanged: true - "" – ""

3. The report has optimistic ID and real ID from BE.

[melvin-bot]

@marcaaron @bfitzexpensify @c3024 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[marcaaron]

Can someone maybe explain why there is a delay for some users and other times not? This seems to point even further to a problem at some lower level with Onyx. My expectation is that even if we are applying the server data followed by the successData that this would not be a noticeable event.

Anyways, I think this pretty low priority. Maybe we should just close it out @bfitzexpensify ?

[marcaaron]

Actually I'm going to close and if there is enough compelling evidence to point to we can reopen.