[$250] Mention - User profile shows the old UI when clicking on user mention after sending it

[lanitochka17]

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: 1.4.79-7
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. Go to any chat
3. Mention a user and send it
4. Click on the mention
5. Note that the user profile is the old UI
6. Dismiss the user profile
7. After a while, click on the mention again

Expected Result:

In Step 4, user profile from mention should show the new UI

Actual Result:

In Step 4, user profile from mention should show the old UI
It shows the new UI after a while when reopening the user profile (Step 7)

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

Bug6502932_1717601114418.bandicam_2024-06-05_23-20-22-036.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~010d3f9640b9f8d604
• Upwork Job ID: 1798394573273215917
• Last Price Increase: 2024-06-05

Issue Owner

Current Issue Owner: @allgandalf

[melvin-bot]

Triggered auto assignment to @tgolen (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[lanitochka17]

@tgolen FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[lanitochka17]

We think that this bug might be related to #vip-vsp

[neil-marcellini]

I'm pretty sure this problem comes from the frontend.

[melvin-bot]

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

[melvin-bot]

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

[ShridharGoel]

Proposal

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

Mention - User profile shows the old UI when clicking on user mention after sending it

What is the root cause of that problem?

In #42188, this scenario was missed (when accountId is not available and htmlAttribAccountID becomes empty then it navigates to the old page).

Below navigation route is wrong:

App/src/components/HTMLEngineProvider/HTMLRenderers/MentionUserRenderer.tsx

Line 72 in 8dbec8d

navigationRoute = ROUTES.DETAILS.getRoute(mentionDisplayText);

Why does this happen only initially?

This is because !isEmpty(htmlAttribAccountID) becomes false initially and true later, once we get the data from backend it contains the accountId which is fetched via tnode.attributes.accountid.

Navigation to DetailsPage leads to the old UI being shown.

Once we have the accountId, !isEmpty(htmlAttribAccountID) becomes true and it starts navigating to the new UI (ProfilePage).

Note: Even if we fix this issue, we'll still have another one. If the accountId is not available when the click on user-mention is done, then it will show the profile as "Hidden" and user needs to go back and click again when the data becomes available.

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

Change this to use

navigationRoute = ROUTES.PROFILE.getRoute(accountID);

This will ensure that we use the same ProfilePage even when we don't have the accountId initially.

I also see htmlAttribAccountID and htmlAttributeAccountID (two items with same functionality - we should use only one of them).

How to fix the "Hidden" issue?

Update the route:

PROFILE: {
        route: 'a',
        getRoute: (accountID?: string | number, login?: string, backTo?: string) => getUrlWithBackToParam(accountID ? `a?accountID=${accountID}` : `a?login=${encodeURIComponent(login)}`, backTo),
    },

Pass the mentionDisplayText as login and accountId as undefined here:

navigationRoute = ROUTES.PROFILE.getRoute(undefined, mentionDisplayText);

Add this condition at this place:

if (!route.params?.accountID) {
        accountID = PersonalDetailsUtils.getAccountIDsByLogins([route.params?.login])?.[0];
}

Screen.Recording.2024-06-06.at.1.48.33.AM.mov

[allgandalf]

As this is a deploy blocker and hourly issue, Reviewing the above proposal now 🟢

[dragnoir]

@allgandalf pls wait, I have a proposal in few mns

[allgandalf]

@ShridharGoel , here is some feedback on your proposal:

About RCA:

• As this is a deploy blocker please also link the offending PR for better context of what changes lead to this regression.
• As seen in the video, when we immediately click on the username we see the old UI, but after the whisper appears ,if we click the username then we see the new UI, in your RCA you have not mentioned why this happens before the whisper and why not after the whisper.

About Solution:

• I tried implementing your solution, we get redirected to the new UI, but we get a loading state and after it ends we are shown the hidden user UI, also if we close the RHP and then again click on the same username, the accountID in the URL do not match with previous accountID and fun fact: this time we see the correct user details 🔬

Screen.Recording.2024-06-05.at.11.18.54.PM.mov

[dragnoir]

Proposal

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

User mention opens in different UIs

What is the root cause of that problem?

This is not a deploy blocker, it's the same behavior on production too.

After this PR #42188
We have a new design for ProfilePage that change the Pin and Message to the top as butttons

When we create a new message with a user-mention it will be displayed in two steps:

• Optimistic comment: the returned value is like that: <mention-user>@[email protected]</mention-user> and it's missing the accountID

App/src/components/HTMLEngineProvider/HTMLRenderers/MentionUserRenderer.tsx

Lines 59 to 64 in 263e2e4

	if (!isEmpty(htmlAttribAccountID)) {
	const user = personalDetails[htmlAttribAccountID];
	accountID = parseInt(htmlAttribAccountID, 10);
	mentionDisplayText = LocalePhoneNumber.formatPhoneNumber(user?.login ?? '') || PersonalDetailsUtils.getDisplayNameOrDefault(user);
	mentionDisplayText = getShortMentionIfFound(mentionDisplayText, htmlAttributeAccountID, user?.login ?? '');
	navigationRoute = ROUTES.PROFILE.getRoute(htmlAttribAccountID);

• Comment from the backend: the returned value is like that: html": "<mention-user accountID=\"16485034\"\/> with the accountID

App/src/components/HTMLEngineProvider/HTMLRenderers/MentionUserRenderer.tsx

Lines 65 to 72 in 263e2e4

	} else if ('data' in tnodeClone && !isEmptyObject(tnodeClone.data)) {
	// We need to remove the LTR unicode and leading @ from data as it is not part of the login
	mentionDisplayText = tnodeClone.data.replace(CONST.UNICODE.LTR, '').slice(1);
	// We need to replace tnode.data here because we will pass it to TNodeChildrenRenderer below
	asMutable(tnodeClone).data = tnodeClone.data.replace(mentionDisplayText, Str.removeSMSDomain(getShortMentionIfFound(mentionDisplayText, htmlAttributeAccountID)));
	accountID = PersonalDetailsUtils.getAccountIDsByLogins([mentionDisplayText])?.[0];
	navigationRoute = ROUTES.DETAILS.getRoute(mentionDisplayText);

When we click on the rendered comment of the mentioned user, before the API turns the response and we still have the optimistic data, the action will be opening the DetailsPage

After we receive the new value from API, the action will call the ProfilePage because now we have the accountID attribute.

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

Solution 1: update the design of the DetailsPage to fit the new ProfilePage new UI

Solution 2: Add accountID for optimistic data

We can make sure also the commentText get the accountID attribute added after getParsedComment

App/src/libs/ReportUtils.ts

Line 3536 in 263e2e4

const commentText = getParsedComment(text ?? '', {shouldEscapeText, reportID});

We can do this by adding a new utility to include accountID attribute or update expensify-common for that

What alternative solutions did you explore?

[dragnoir]

@allgandalf pls check my proposal too. Thank you

[allgandalf]

@dragnoir , your RCA is correct, !isEmpty(htmlAttribAccountID) will be true when we get htmlAttribAccountID from the BE! this explains why we go to the details page before the whisper.

Solution 1: update the design of the DetailsPage to fit the new ProfilePage new UI

This is not what the expected result is, we should be able to visit the Profile page from the first time itself and not update the DetailsPage page!

Solution 2: Add accountID for optimistic data

This solution makes sense to me, but can you share/add more details in the solution how that is going to be done? or a test branch ❕

[dragnoir]

@allgandalf thank you for your review, pls allow me a few hours and I will submit a test branch.

[allgandalf]

@dragnoir yeah sure, but please be quick this is a deploy blocker, also test branch is not absolutely necessary, you can update the existing solution with some more details for me to understand your approach, cause

We can do this by adding a new utility to include accountID

This doesn't really help understand the proper approach which is going to be used during PR :) thanks

[ShridharGoel]

Proposal

Updated to include additional details.

[ShridharGoel]

@allgandalf Can you check my updated proposal?

[roryabraham]

This is not a deploy blocker, it's the same behavior on production too

This new UI is on staging only, so it's not the same behavior on production.

I'm on the fence about whether or not this is a true blocker or not. Discussing in slack: https://expensify.slack.com/archives/C01GTK53T8Q/p1717612587165159?thread_ts=1717611817.164829&cid=C01GTK53T8Q

[ShridharGoel]

@allgandalf

I think the profile page shows an account as hidden if it is opened as soon as it is mentioned and Onyx doesn't have any details about it. If you open it after few seconds then it works fine. This should be expected and not an issue caused by my solution. Thoughts?

[allgandalf]

@ShridharGoel , the expected results are:

In Step 4, user profile from mention should show the new UI

So i guess it isn't good on UX to initially show a hidden state when the user clicks on the username and Then, after retrieving the htmlAttribAccountID from the backend, displaying the correct profile details only when the user clicks the username a second time.

I guess @tgolen you would also agree with this observation

[kosmydel]

Hey I’m only for a moment, but #42385 might solve this issue.

[allgandalf]

Thanks for the bump @kosmydel , mentioned about this and tagged you in the slack thread as well

[ShridharGoel]

@allgandalf We still need to solve the hidden issue. I have updated my proposal.

[allgandalf]

We still need to solve the hidden issue.

@ShridharGoel The PR #42385 also changes the navigation logic for Profile and code in MentionUserRenderer

Lets wait for the next steps here from @roryabraham @tgolen , how should we proceed here, please see comment

[ShridharGoel]

I think that PR would take time to review since it's a bigger change. So, as a quick-fix for this blocker - can we consider the changes in my proposal and then #42385 can be continued?

@tgolen @roryabraham

[dragnoir]

@ShridharGoel you copied my RCA and you changed your proposal based on my comments.

[dragnoir]

@allgandalf I tested PR #42385 and this issue is totally solved. We can HOLD or close this one, I think.

[kosmydel]

Hey, I didn't have time yesterday to investigate deeply.

I think that this behavior is expected. In the design doc, we planned two separate issues for updating the ProfilePage and then removing DetailsPage. The testing plan in this issue mentions the second case (DetailsPage) which changes hasn't been merged yet (PR).

This is why the mentions shows old profile UI (because in our codebase it was actually a different component).

[Julesssss]

I'm removing the blocker label as this is an edge case with proposals in place, is potentially expected, and so we can speed up QA. More context here (internal comment)

[melvin-bot]

@tgolen, @allgandalf Eep! 4 days overdue now. Issues have feelings too...

[allgandalf]

Can the QA retest this ? This was fixed in another PR according to #43131 (comment)

[Julesssss]

I have asked QA to retest 👍

[isagoico]

Not able to reproduce the issue anymore in Staging build v1.4.82-0

[Julesssss]

Thank you!