[$500] Workspace switcher - Both "Expensify" and another workspace are highlighted in WS switcher

[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.44-0
Reproducible in staging?: Y
Reproducible in production?: N
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: Applause - Internal Team
Slack conversation:

Action Performed:

1. Go to staging.new.expensify.com
2. Create a workspace if there is no workspace
3. Open workspace switcher

Expected Result:

Only "Expensify" will be highlighted

Actual Result:

"Expensify" and another workspace are 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

Add any screenshot/video evidence

Bug6393389_1708986239721.bandicam_2024-02-27_06-21-36-088.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~012b29a531f8eb3037
• Upwork Job ID: 1762249377835036672
• Last Price Increase: 2024-02-26
• Automatic offers:
• akinwale | Reviewer | 0
• Krishna2323 | Contributor | 0

[melvin-bot]

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

[melvin-bot]

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

[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 @roryabraham (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[lanitochka17]

We think that this bug might be related to #wave8
CC @zanyrenney

[roryabraham]

I don't think this should be treated as a deploy blocker, demoting to regular bug.

[roryabraham]

Still keen to know where this regression is coming from.

[melvin-bot]

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

[eucool]

Still keen to know where this regression is coming from.

Regression from #36839 #35727

[Krishna2323]

Proposal

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

Workspace switcher - Both "Expensify" and another workspace are highlighted in WS switcher

What is the root cause of that problem?

The newFocusedIndex will be 0 when props.selectedOptions.length is 0 and due to this the first option gets selected.

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

We should subtract 1 from props.selectedOptions.length to get the actual focused index if we don't allow to select multiple options .

App/src/components/OptionsSelector/BaseOptionsSelector.js

Line 428 in 297bcec

const newFocusedIndex = props.selectedOptions.length;

Result

focused_issue.mp4

Alternative

We can set newFocusedIndex value to -1 and also update the focusedIndex -1 when search value changes or we toggle any option. We need to update the code below, to check for prevValue.current !== value instead of (!!prevValue.current && !value) and also add setFocusedIndex(-1) if the condition is true. By doing so we will be consistent with the selectors like IOU Request participants selector.

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 439 to 454 in 297bcec

	useEffect(() => {
	// If we just toggled an option on a multi-selection page or cleared the search input, scroll to top
	if (props.selectedOptions.length !== prevSelectedOptions.current.length || (!!prevValue.current && !value)) {
	prevSelectedOptions.current = props.selectedOptions;
	prevValue.current = value;
	scrollToIndex(0);
	return;
	}
	// Otherwise, scroll to the focused index (as long as it's in range)
	if (allOptions.length <= focusedIndex) {
	return;
	}
	scrollToIndex(focusedIndex);
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, [allOptions.length, focusedIndex, props.focusedIndex, props.selectedOptions, value]);

[roryabraham]

I think this appears to be a regression from #31606...

We should subtract 1 from props.selectedOptions.length to get the actual focused index.

@Krishna2323 wouldn't that just move the "duplicate focus" to the end of the list where it's out-of-sight in your video?

[Krishna2323]

@roryabraham, No it won't but I suggest to use alternative solution as it aligns with SelectionList. In SelectionList we don't set any initial focus index and we remove focus when selected options changes.

[akinwale]

@Krishna2323 Subtracting 1 from the props.selectedOptions.length when it's 0 will give -1, which is effectively the same as the first part of the alternate solution. I think it makes sense to just explicitly set -1 as the initial focus index and then update the rest of the logic accordingly.

We can move forward with @Krishna2323's proposal here.

🎀👀🎀 C+ reviewed.

[melvin-bot]

Current assignee @roryabraham is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[roryabraham]

sounds good. I also left a comment here with a tangentially related change that I would love to see included in your PR @Krishna2323

[melvin-bot]

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

Offer link
Upwork job

[melvin-bot]

📣 @Krishna2323 🎉 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 📖

[dragnoir]

Proposal

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

Two elements from the Wokspaces page are highlighted.

What is the root cause of that problem?

There's no issue with the 1st item to be highlighted when the page Choose a workspace is opened.
This is how the component BaseOptionsSelector works.
below some examples showing how the fist element is always selected:

below a screenshot how the active selected workspace should not be highlighted

The issue here is that the actif workspace is highlighted. The correct way is to not highlight the actif workspace and highlight the 1st available option to pick from.

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

In this case of this issue I recommande that we should remove highlights from the Everything list (Expensify itrem) and just keep the green checkmark as with when we select a workspace from the list

(as you can see here, the actif workspace is NOT highlighted).

The proposed solution is by disabling the focused option here

App/src/pages/WorkspaceSwitcherPage.tsx

Line 195 in 174a032

optionIsFocused={!activeWorkspaceID}

<OptionRow
...
- optionIsFocused={!activeWorkspaceID}
+ optionIsFocused={false}
/>

What alternative solutions did you explore? (Optional)

[dragnoir]

@akinwale pls review the choosen proposal, the -1 to props.selectedOptions.length will just make a mess and regressions.

The function BaseOptionsSelector is used by many other components. Below a regression from the update.

20240227_172233.mp4

as you can see on the video, the new choosen update will select the last item from the list.
Pls check my proposal.

[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.

[akinwale]

Based on this comment, this inadvertently got fixed in that PR.

cc @roryabraham

[Krishna2323]

@akinwale, we still have multiple issue here.

1. When an options gets selected the list doesn't get scrolled to the top, instead it scrolls to the first unselected options which.
2. The focus is not removed or moved to the first item when the input text gets changed.

baseOptions_selector_scroll_bug.mp4

[akinwale]

@Krishna2323 Those are entirely separate issues.

You may still get compensated here since your proposal was selected but that's entirely up to BZ / internal team to decide.

[Krishna2323]

@akinwale, the scroll issue may seem different but the root cause is the same.

Here we scroll to index 0, which is correct

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 441 to 445 in b447c11

	if (props.selectedOptions.length !== prevSelectedOptions.current.length || (!!prevValue.current && !value)) {
	prevSelectedOptions.current = props.selectedOptions;
	prevValue.current = value;
	scrollToIndex(0);
	return;

but this again changes the focusedIndex to props.selectedOptions.length thats why we didn't scroll to top of the selector.

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 428 to 434 in b447c11

	const newFocusedIndex = props.selectedOptions.length;
	const prevFocusedOption = _.find(newOptions, (option) => focusedOption && option.keyForList === focusedOption.keyForList);
	const prevFocusedOptionIndex = prevFocusedOption ? _.findIndex(newOptions, (option) => focusedOption && option.keyForList === focusedOption.keyForList) : undefined;
	setSections(newSections);
	setAllOptions(newOptions);
	setFocusedIndex(prevFocusedOptionIndex || (_.isNumber(props.focusedIndex) ? props.focusedIndex : newFocusedIndex));

I'm happy to fix this since I've already spent time on it, but I'll leave the final decision to @roryabraham.

[roryabraham]

This component was much more complicated than it needed to be, so I've been working on a holistic refactor here. I think I've simplified it to a point where it makes more sense and is easier to follow.

So far I've only done cursory testing, so my next step is to go through all the open issues related to the BaseOptionsSelector and see if they're reproducible on my branch.

[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.

[Piotrfj]

Update: re-refactoring is ready to be tested, it fixes all the regression issues I was aware of. Please test it (especially the tags, because I could not yet replicate the scenario on my site).
The fix is based on the @roryabraham solution, so special thanks for him!
Link to PR: #37494

[melvin-bot]

@akinwale, @sakluger, @roryabraham, @Krishna2323 Eep! 4 days overdue now. Issues have feelings too...

[sakluger]

@roryabraham what would you recommend as the next step here? Should we wait to see if your refactored component solves the issue that @Krishna2323 mentioned? Or will we still need a fix after your refactor?

Also, @Krishna2323 - did you do any work on a PR for your original proposal? If so, do you have a draft PR?

[dragnoir]

BaseOptionSelector is deprecated #37628

[roryabraham]

ok, looping back here...

1. This issue is no longer reproducible because the PR that caused it was reverted
2. As @dragnoir mentioned, we won't need a follow-up because the component in question has been deprecated
3. Since @Krishna2323 never actually opened a PR for this issue, I think we can probably just close this issue w/o payments

thanks everyone!