[HOLD for payment 2024-07-10] [$500] Workspace switcher - Selection disappears after erasing character with workspace selected

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

Action Performed:

Precondition:

• There is at least 8 workspaces.

1. Go to staging.new.expensify.com
2. Open workspace switcher.
3. Select a workspace.
4. Open workspace switcher.
5. Type a valid search term.

Latest reproduction steps: #36140 (comment)

• Lets say the selected workspace name is Workspace 1

1. Enter Work, Observe the option is still focused.
2. Add a character Workk
3. Remove the extra k
4. Observe the first option is not focused.

Expected Result:

The selection highlight will remain on the first item in the list.

Actual Result:

The selection highlight moves to the second item in the list.

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

Bug6371676_1707394024021.20240208_112634.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~018e72ec1b032f8020
• Upwork Job ID: 1755568798530035712
• Last Price Increase: 2024-02-22
• Automatic offers:
• situchan | Contributor | 0
• Krishna2323 | Contributor | 0
• hoangzinh | Contributor | 0

Issue Owner

Current Issue Owner: @kevinksullivan

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[izarutskaya]

We think that this bug might be related to #wave8-collect-admins
@zanyrenney

[ZhenjaHorbach]

Proposal

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

Workspace switcher - Selection highlight moves to second item when searching for workspace

What is the root cause of that problem?

The problem is related to that when we search for elements we select the first element with a valid index (1) (That is, the second)

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

To fix this issue we can use focusedIndex={0} for <OptionsSelector/> so that after starting the search we select the first element

App/src/pages/WorkspaceSwitcherPage.tsx

Lines 235 to 258 in 6a889f1

	<OptionsSelector
	// @ts-expect-error TODO: remove this comment once OptionsSelector (https://github.com/Expensify/App/issues/25125) is migrated to TS
	placeholder={translate('workspace.switcher.placeholder')}
	ref={inputCallbackRef}
	sections={[usersWorkspacesSectionData]}
	value={searchTerm}
	shouldShowTextInput={usersWorkspaces.length >= CONST.WORKSPACE_SWITCHER.MINIMUM_WORKSPACES_TO_SHOW_SEARCH}
	onChangeText={setSearchTerm}
	selectedOptions={selectedOption ? [selectedOption] : []}
	onSelectRow={selectPolicy}
	shouldPreventDefaultFocusOnSelectRow
	headerMessage={headerMessage}
	highlightSelectedOptions
	shouldShowOptions
	autoFocus={false}
	disableFocusOptions={!activeWorkspaceID}
	canSelectMultipleOptions={false}
	shouldShowSubscript={false}
	showTitleTooltip={false}
	contentContainerStyles={[styles.pt0, styles.mt0]}
	textIconLeft={MagnifyingGlass}
	// Null is to avoid selecting unfocused option when Global selected, undefined is to focus selected workspace
	initiallyFocusedOptionKey={!activeWorkspaceID ? null : undefined}
	/>

The same logic we use here

App/src/components/CategoryPicker/index.js

Lines 64 to 68 in 670e044

	selectedOptions={selectedOptions}
	// Focus the first option when searching
	focusedIndex={0}
	// Focus the selected option on first load
	initiallyFocusedOptionKey={selectedOptionKey}

But maybe this is not the only case. So I think it’s a good idea to check all OptionsSelector or set by Default focusedIndex

What alternative solutions did you explore? (Optional)

NA

[Krishna2323]

Proposal

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

Workspace switcher - Selection highlight moves to second item when searching for workspace

What is the root cause of that problem?

The main problem lies in the code below, for selectors which can select multiple options we want to highlight the option after all selected options, and in WorkspaceSwitcherPage we have a default selected option so whenever we search the focus index is set to the option after all selected options, in this case we have only one selected option so the newFocusedIndex will be one and second option will be highlighted.

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 163 to 171 in caefc6e

	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?

Instead of passing focusedIndex from everywhere we use BaseOptionsSelector, we can simply set the newFocusedIndex index value to 0 when this.props.canSelectMultipleOptions is false and use this.props.selectedOptions.length if we can select multiple.

        const newFocusedIndex = this.props.canSelectMultipleOptions ? this.props.selectedOptions.length : 0;
        const isNewFocusedIndex = newFocusedIndex !== this.state.focusedIndex;

        // eslint-disable-next-line react/no-did-update-set-state
        this.setState(
            {
                sections: newSections,
                allOptions: newOptions,
                focusedIndex: newFocusedIndex,
            }, 

PS: This solution will make BaseOptionsSelector behave like BaseSelectionList without passing and extra prop (focusedIndex).

Result

focused_index.mp4

Alternative:

Update the focusedIndex to -1 because we will not reach the focusIndex state updating block until the sections has been updated, so I think the best thing is to add -1 as focusedIndex and remove all redundant code to match the behaviour of BaseSelectionList for consistency.

[melvin-bot]

@kevinksullivan, @thesahindia Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

@kevinksullivan, @thesahindia 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[melvin-bot]

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

[thesahindia]

Not getting enough time to look at this. Please reassign @kevinksullivan.

[situchan]

I am interested in reviewing this as C+

[melvin-bot]

@kevinksullivan Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@kevinksullivan Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

@kevinksullivan 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? 💸

[melvin-bot]

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

[melvin-bot]

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

[Krishna2323]

@rlinoz, waiting for your confirmation on this before opening third PR for this issue😅

[rlinoz]

Yeah, sorry for all the confusion in this issue, I can reproduce with your steps and the solution looks good, let's fix it.

I will also update the issue title to better acknowledge what we are doing

[Krishna2323]

@hoangzinh, PR ready for review.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.3-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: Workspace switcher - Selection disappears after erasing character with workspace selected. #44222

If no regressions arise, payment will be issued on 2024-07-10. 🎊

For reference, here are some details about the assignees on this issue:

• @hoangzinh requires payment automatic offer (Contributor)
• @Krishna2323 requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@hoangzinh] The PR that introduced the bug has been identified. Link to the PR:
• [@hoangzinh] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@hoangzinh] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@hoangzinh] Determine if we should create a regression test for this bug.
• [@hoangzinh] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@kevinksullivan] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[Krishna2323]

@kevinksullivan, bump for payments.

[kevinksullivan]

@hoangzinh please complete the checklist in order to receive payment. Thanks

[hoangzinh]

BugZero Checklist:

• The PR that introduced the bug has been identified. Link to the PR: fix: Focus not removed when selection list changes. #39141
• The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/39141/files#r1679532621
• A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
• Determine if we should create a regression test for this bug: I think we don't need a regression test here, it's just a small UI issue.

[melvin-bot]

@rlinoz, @hoangzinh, @kevinksullivan, @Krishna2323, @dubielzyk-expensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[kevinksullivan]

all set