[$500] Expense - Workspace under "To" field is highlighted after opening user avatar

[izarutskaya]

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: v1.4.42-0
Reproducible in staging?: Y
Reproducible in production?: N
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause-Internal Team
Slack conversation:

Action Performed:

1. Navigate to staging.new.expensify.com
2. Go to FAB > Request money > Manual.
3. Enter amount > Next.
4. Select a workspace > Next.
5. On confirmation page, click on the workspace under "To".
6. Click on the user avatar (not workspace avatar).
7. Return to confirmation page via back button on the top left.

Expected Result:

The workspace name under "To" will not be highlighted.

Actual Result:

The workspace name under "To" is highlighted.

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

Bug6380638_1708001573958.bandicam_2024-02-15_15-38-43-819__1_.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01bd38066315528994
• Upwork Job ID: 1758115460406108160
• Last Price Increase: 2024-03-07

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @slafortune (Bug), see https://stackoverflow.com/c/expensify/questions/14418 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.

[melvin-bot]

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

[izarutskaya]

We think that this bug might be related to #wave5-free-submitters
CC @dylanexpensify

[Beamanator]

I agree this should be changed, but I don't think it should be a blocker

[ikevin127]

Proposal

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

The workspace name option under "To" is highlighted after opening user avatar.

What is the root cause of that problem?

Offending PR is #35594 -> before this PR we were not able to click on the workspace details option as it was disabled.

Here's what causes the option to be highlighted once user views avatar (opens modal):

• when the avatar modal is opened this causes the BaseOptionsSelector to re-render which triggers its componentDidUpdate to run which sets state and changes the focusedIndex from 1 to 0 (selectedOptions.length) here:

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 163 to 171 in 32648bb

	const newFocusedIndex = this.props.selectedOptions.length;
	const isNewFocusedIndex = newFocusedIndex !== this.state.focusedIndex;
	// eslint-disable-next-line react/no-did-update-set-state
	this.setState(
	{
	sections: newSections,
	allOptions: newOptions,
	focusedIndex: _.isNumber(this.props.focusedIndex) ? this.props.focusedIndex : newFocusedIndex,

this re-render / setState within the BaseOptionsSelector component shouldn't happen in our case since focusedIndex was initialized with 1 on initial component mount, we should keep the initial focusedIndex instead of updating it when not necessary.

To be more specific about what exactly causes the re-render / setState (requested by reviewer #36577 (comment)):

• within BaseOptionsSelector the prop within this.props.sections[0].data[0].participantsList[0].isLoading changes its boolean value when OpenPublicProfilePage is called as a result of opening the avatar preview -> this causes the component's componentDidUpdate to run and update the focusedIndex to 0 unnecessarily -> which causes the item to be highlighted (this issue).

Video

Screen.Recording.2024-02-20.at.19.10.59.mov

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

Considering the most recent reviewer's #36577 (comment):

Actually, focusedIndex doesn't seem to be used on this confirm page, so some possible directions are: how to keep it unchanged when re-rendering, or whether it can be disabled, etc.

the simplest thing to do here is to take advantage of the disableFocusOptions and pass it as true or selectedOptions.length <= 1 here:

App/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js

Line 849 in cd14830

<OptionsSelector

within the MoneyTemporaryForRefactorRequestConfirmationList component, since focusedIndex doesn't seem to be used on this confirm page -> we just disable the focusing behaviour all together 🙂

Using selectedOptions.length <= 1 to disable the focusing would make sure to not create regressions for multi-select case as discussed in issue #37238, without invalidating the solution implemented there.

Videos

MacOS: Chrome / Safari

Before	After
Screen.Recording.2024-02-15.at.17.30.07.mov	Screen.Recording.2024-02-15.at.17.28.58.mov

[Krishna2323]

Not reproducible on main branch, we don't allow users to open workspace details.

Monosnap.screencast.2024-02-15.21-15-25.mp4

[Pujan92]

Proposal

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

Workspace option kept highlighted once we come back after opening the profile avatar

What is the root cause of that problem?

It seems the problem persists after this PR gets merged.
Reason: When we open the profile avatar it calls the api openPublicProfilePage and its response will update the personalDetailsList onyx data which eventually regenerates the participants array here. Due to that sections prop gets changed and the execution will reach to set a new focused index part unnecessarily in BaseOptionsSelector.

App/src/pages/iou/request/step/IOURequestStepConfirmation.js

Lines 99 to 106 in 50e07e8

	const participants = useMemo(
	() =>
	_.map(transaction.participants, (participant) => {
	const isPolicyExpenseChat = lodashGet(participant, 'isPolicyExpenseChat', false);
	return isPolicyExpenseChat ? OptionsListUtils.getPolicyExpenseReportOption(participant) : OptionsListUtils.getParticipantsOption(participant, personalDetails);
	}),
	[transaction.participants, personalDetails],
	);

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 149 to 172 in 421bbc5

	if (_.isEqual(this.props.sections, prevProps.sections)) {
	return;
	}
	const newSections = this.sliceSections();
	const newOptions = this.flattenSections();
	if (prevProps.preferredLocale !== this.props.preferredLocale) {
	this.setState({
	sections: newSections,
	allOptions: newOptions,
	});
	return;
	}
	const newFocusedIndex = this.props.selectedOptions.length;
	const isNewFocusedIndex = newFocusedIndex !== this.state.focusedIndex;
	// eslint-disable-next-line react/no-did-update-set-state
	this.setState(
	{
	sections: newSections,
	allOptions: newOptions,
	focusedIndex: _.isNumber(this.props.focusedIndex) ? this.props.focusedIndex : newFocusedIndex,
	},

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

I believe it should be || here instead of ?? bcoz currently it won't check the second condition as the first will always return the boolean value which isn't null. With that change, it returns early and won't make the unnecessary api call when the avatarURL is available.

App/src/pages/settings/Profile/ProfileAvatar.tsx

Lines 30 to 34 in 38c6c16

	useEffect(() => {
	if (!ValidationUtils.isValidAccountRoute(Number(accountID)) ?? !!avatarURL) {
	return;
	}
	PersonalDetails.openPublicProfilePage(accountID);

if (!ValidationUtils.isValidAccountRoute(Number(accountID)) || !!avatarURL) {

[ntdiary]

Under review.

[ntdiary]

I think the RCA in the above proposals are not accurate enough and need to be further clarified. Also, I'm still testing. 🤔

[brandonhenry]

Hmm Kevin's updated proposal is essentially mine but with slight more specific detail on where to disable the pressable for the "To" field (didn't notice late night that the pressable already has support for disabling the hover if that pressable doesn't need it)

[ikevin127]

Hmm Kevin's updated proposal is essentially mine but with slight more specific detail on where to disable the pressable

Nope it's not 😅 I'm not disabling any Pressable nor changing any existing styles.
I'm using an already existing prop to disable the focus behaviour since as mentioned by latest reviewer's comment:

Actually, focusedIndex doesn't seem to be used on this confirm page

which also hinted at disabling the focus behaviour if possible.

My solution is simply adding a prop (1-liner), exactly what was suggested for as possible solution since all my previous solutions were too complicated. Also your solution is pretty vague:

We should extend our BaseGenericPressable class to account for pressable that we don't want to have a hover state.

don't you think ? Besides suggesting changing the common BaseGenericPressable's styles which has nothing to do with the RCA of our issue 🙂

[brandonhenry]

@ikevin127 ahh you're right. I see your proposal doesn't target what I believe is the fix haha. Good call, I personally think the issue is in the BaseGenericPressable and not the OptionsSelector itself

[melvin-bot]

@dangrous, @ntdiary, @slafortune Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[slafortune]

@ntdiary is there a proposal here that will do the trick or are we still waiting on a better one?

[ntdiary]

@ntdiary is there a proposal here that will do the trick or are we still waiting on a better one?

@slafortune, so far, @ikevin127's proposal is a straightforward solution, but I'm still willing to wait a bit longer, There might be potentially better ideas. BTW, this issue is somewhat similar to issue #37238, so I will also discuss it there. :)

[brandonhenry]

@ntdiary i just updated my proposal. same root cause but I don't think my proposed solution was outlined enough for it to be considered.

My change is similar to @ikevin127 but I think the Section that each of these OptionRow are built off needs a new property that way we can control specifically which OptionRow should never be hoverable. If we set that new property for the to field, and update the OptionRow to not allow a change in hover style if that variable is set - we fix this issue and #37238

[ikevin127]

There might be potentially better ideas.
BTW, this issue is somewhat similar to issue #37238, so I will also discuss it there. :)

This issue's root cause is related to OpenPublicProfilePage being called which updates focusedIndex because isLoading changes its boolean value, while the other issue's accepted RCA was pretty vague, mentioning that the sections prop changes and suggested to compare previous / current sections as a fix.

Turns out the fix PR from #37238 selected proposal didn't work as expected and a regression was reported.

I think regardless of the root cause of the other issue, my latest proposed solution still fixes our issue, since as you mentioned above:

Actually, focusedIndex doesn't seem to be used on this confirm page

To me, this looks like a straight-forward slam dunk 🏀
I'm confused as to what's the hold up w/ this one really 🤔

cc @ntdiary @dangrous

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[dangrous]

I just went on parental leave so I haven't been paying attention to this one haha - @slafortune do you think you could fish for an internal volunteer in Slack? I can help if needed!

[ikevin127]

Updated proposal

• improved solution by passing selectedOptions.length <= 1 to disableFocusOptions, therefore accounting for possible regressions when it comes to a similar issue's solution (multiple options)

To +1 on my previous #36577 (comment), this is a direct quote of @ neil-marcellini from #37238 (comment):

I think we have a pretty clear problem and solution for this specific issue. There are other issues I'm sure, but let's solve those elsewhere.

My take from that is that we should handle this issue here and treat it as separate from the other similar issue - which just merged the PR that will soon arrive on staging.

I'm pretty sure our issue will still occur even after that issue's PR arrives on staging, since my proposal's RCA will still occur even though the other proposal's solution added an early return if previous - current sections are equal.

cc @ntdiary

[melvin-bot]

@dangrous @ntdiary @slafortune 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!

[melvin-bot]

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

[slafortune]

@ntdiary are you able to recreate this still?

[ntdiary]

@ntdiary are you able to recreate this still?

I can reproduce it. And I'm still investigating further. :)

[melvin-bot]

@dangrous, @ntdiary, @slafortune Whoops! This issue is 2 days overdue. Let's get this updated quick!

[ntdiary]

Not overdue

[slafortune]

@ntdiary do you have an update?

[ntdiary]

@ntdiary do you have an update?

@slafortune, ok, so far, I think we can hold this issue for the SelectionList refactor, because I personally feel this issue is an edge case, and OptionsSelector will be replaced with SelectionList, after which this issue will disappear automatically. :)

[melvin-bot]

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

[melvin-bot]

@dangrous @ntdiary @slafortune this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[slafortune]

AH - Thanks for the details and linking that @ntdiary ! I think since this is a pretty edge case and OptionsSelector is deprecated we don't really need to hold we can simply close this.