[HOLD for payment 2023-12-06] [$500] Web - Profile - When selecting the last country or state extra space is displayed for a while

@sonialiap

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: 1.4.1-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:
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:

Go to Settings > Profile > Personal details > Address.
Click on the country and select "Zimbabwe."
Click on the country again and notice space below.
Now, select "United States" and choose the last state.
Click on the state again and notice extra space for a while

Expected Result:

When selecting the last country or state, extra space should not be displayed below the country list or state

Actual Result:

When selecting the last country or state, extra space is displayed for a while below the list

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

Bug6285905_1700589441726.2023-11-21_22-53-35.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01e93d2572ea063da0
Upwork Job ID: 1727075975932694528
Last Price Increase: 2023-11-21
Automatic offers:

ZhenjaHorbach | Contributor | 27786937

Issue Owner

Current Issue Owner: @sonialiap

@sonialiap

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

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

Bug0 Triage Checklist (Main S/O)

This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@burczu

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

@lanitochka17

@lanitochka17 the space looks uniform to me, am I missing something?

On macbook(Chrome) I have a bit different situation but looks similar

Screen.Recording.2023-11-21.at.23.09.02.mov

Proposal

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

Web - Profile - When selecting the last country or state extra space is displayed for a while

What is the root cause of that problem?

The main problem is how many elements we display when rendering the SectionList per batch

App/src/components/SelectionList/BaseSelectionList.js

Line 454 in da549c0

maxToRenderPerBatch={5}

https://reactnative.dev/docs/optimizing-flatlist-configuration#maxtorenderperbatch

And since we select the last element
The list needs time to display the last elements
This is why we see bugs like this.

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

To fix this bug we can increase this property from 5 to 15(for example) to Increase the list display speed
Also, we have initialNumToRender and windowSize. We can also use these properties for optimization.

App/src/components/SelectionList/BaseSelectionList.js

Lines 453 to 455 in da549c0

    
           initialNumToRender={12} 
        
           maxToRenderPerBatch={5} 
        
           windowSize={5}

As an alternative, we can pass these values like props from CountrySelectionPage, StateSelectorModal, and other screens

What alternative solutions did you explore? (Optional)

We can also make separate configs for different numbers of elements
And if the lists have more than 100 elements for example
We will increase the values (initialNumToRender, windowSize, maxToRenderPerBatch) to display elements better
Because the current values we use for long lists are not very suitable

Or at least we can use dynamic values that we will calculate depending on the number of visible elements using onLayout from SectionList. But I'm not sure this approach will work for all lists. so if we decide to use dynamic maxToRenderPerBatch, then it is better to create a new prop that will enable and disable dynamic maxToRenderPerBatch

We can update scrollToFocusedIndexOnFirstRender like

   const [maxToRenderPerBatch, setMaxToRenderPerBatch] = useState(5);
   
    const scrollToFocusedIndexOnFirstRender = useCallback(
        (layout) => {
            if(isDynamicMaxToRenderPerBatch) {
              // I added 5(or we can multiply by 2) because the scrolling speed is higher than updating elements as tall as a list. 
 But this is more of an additional functionality
             setMaxToRenderPerBatch((layout.nativeEvent.layout.height / layout.nativeEvent.layout.y) + 5); (optional Math.ceil())
            }
            if (!firstLayoutRef.current) {
                return;
            }
            scrollToIndex(focusedIndex, false);
            firstLayoutRef.current = false;
        },
        [focusedIndex, scrollToIndex],
    );

App/src/components/SelectionList/BaseSelectionList.js

Lines 313 to 319 in 6c29cab

    
           const scrollToFocusedIndexOnFirstRender = useCallback(() => { 
        
               if (!firstLayoutRef.current) { 
        
                   return; 
        
               } 
        
               scrollToIndex(focusedIndex, false); 
        
               firstLayoutRef.current = false; 
        
           }, [focusedIndex, scrollToIndex]);

And then update this line like:

maxToRenderPerBatch={maxToRenderPerBatch}

App/src/components/SelectionList/BaseSelectionList.js

Line 454 in 6c29cab

maxToRenderPerBatch={5}

And since we only need this functionality for a certain number of screens
We can use isDynamicMaxToRenderPerBatch

We can add this prop here

App/src/components/StatePicker/StateSelectorModal.js

Lines 95 to 104 in 6c29cab

    
           <SelectionList 
        
               headerMessage={headerMessage} 
        
               textInputLabel={label || translate('common.state')} 
        
               textInputValue={searchValue} 
        
               sections={[{data: searchResults, indexOffset: 0}]} 
        
               onSelectRow={onStateSelected} 
        
               onChangeText={setSearchValue} 
        
               initiallyFocusedOptionKey={currentState} 
        
               shouldStopPropagation 
        
           />

And here

App/src/pages/settings/Profile/PersonalDetails/CountrySelectionPage.js

Lines 91 to 99 in 6c29cab

    
           <SelectionList 
        
               headerMessage={headerMessage} 
        
               textInputLabel={translate('common.country')} 
        
               textInputValue={searchValue} 
        
               sections={[{data: searchResults, indexOffset: 0}]} 
        
               onSelectRow={selectCountry} 
        
               onChangeText={setSearchValue} 
        
               initiallyFocusedOptionKey={currentCountry} 
        
           />

Proposal

Change the maxToRenderPerBatch value dependent on the window height.

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

The number of initially rendered selection of countries is not enough to fill the screen.

What is the root cause of that problem?

According to the code below, the specified number of selections to be rendered initially is 12 but for some reason is not visually being applied.

App/src/components/SelectionList/BaseSelectionList.js

Lines 453 to 454 in da549c0

    
           initialNumToRender={12} 
        
           maxToRenderPerBatch={5}

It seems that the number of components being visually rendered is determined by maxToRenderPerBatch.

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

To avoid hardcoding a bigger value to make sure the selection is initially filled, we can simply replace the value 5 with:
Math.ceil((window.innerHeight - 129)/ 64).

Essentially this formula will calculate how much countries can be shown in the screen.

Why do we subtract window.innerHeight with 129?

129 is the pixel height of the Country Header component plus the height of the Search input for countries.

Why do we divide by 64?

64 is the height of a selectable country component

We then Math.ceil it since we want a whole number since if the quotient is a decimal, it would be impossible to render a component at a fraction. We use Math.ceil instead of Math.floor because in cases wherein the quotients decimal is near the value 1 (0.9, 0.8, etc), a space may still show if we decide to floor it.

The value we get from the formula Math.ceil((window.innerHeight - 129)/ 64) will give us the number of Country components that can fill the initial space.

What alternative solutions did you explore? (Optional)

N/A

Proposal : Fix for this Issue

Source of the Problem
The Initial Number of items being rendered for the list is small , not enough to fill the screen , causing this glitch while opening the country or state selection modal.

What is the root cause of that problem?
the specified number of selections to be rendered initially is 12 which is small and not enough to fill the screen on every device size causing a glitch effect.

 initialNumToRender={12} 
 maxToRenderPerBatch={5}

It seems that the number of components being visually rendered is determined by maxToRenderPerBatch.

Fix for the problem
A bigger value can be passed to the section list.A number large enough to fill the screen that will stop this glitch from occuring that shows up for a while.
We can also avoid just passing a large number statically and pass a more dynamic number that would be optimal for all screen sizes the general formula used would be windowHeight/(height of countryItem)
.This will give us the exact number of items initally required to be rendered on the screen.

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

@burczu

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

Web - Profile - When selecting the last country or state extra space is displayed for a while #31737

If no regressions arise, payment will be issued on 2023-12-06. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

External issue reporter
Contributor that fixed the issue
Contributor+ that helped on the issue and/or PR

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

@burczu does not require payment (Contractor)
@ZhenjaHorbach requires payment offer (Contributor)

@burczu

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:

[@burczu] The PR that introduced the bug has been identified. Link to the PR:
[@burczu] 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:
[@burczu] 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:
[@burczu] Determine if we should create a regression test for this bug.
[@burczu] 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.
[@sonialiap] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@ZhenjaHorbach

@ZhenjaHorbach contributer $500 - paid ✔️

@burczu

@burczu please complete the checklist :)

@burczu

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:

[@burczu] The PR that introduced the bug has been identified. Link to the PR:

#27767

[@burczu] 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/27767/files#r1422160347

[@burczu] 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:

Not found

[@burczu] Determine if we should create a regression test for this bug.

I don't think it's worth adding regression test for this issue - it was just UX fix and there was no impact on any important flow.

[@burczu] 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.
[@sonialiap] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

Thanks, Bartek!

Navigation Menu

Repository navigation

[HOLD for payment 2023-12-06] [$500] Web - Profile - When selecting the last country or state extra space is displayed for a while #31660

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Bug0 Triage Checklist (Main S/O)

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

24 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	initialNumToRender={12}
	maxToRenderPerBatch={5}
	windowSize={5}

	const scrollToFocusedIndexOnFirstRender = useCallback(() => {
	if (!firstLayoutRef.current) {
	return;
	}
	scrollToIndex(focusedIndex, false);
	firstLayoutRef.current = false;
	}, [focusedIndex, scrollToIndex]);

	<SelectionList
	headerMessage={headerMessage}
	textInputLabel={label \|\| translate('common.state')}
	textInputValue={searchValue}
	sections={[{data: searchResults, indexOffset: 0}]}
	onSelectRow={onStateSelected}
	onChangeText={setSearchValue}
	initiallyFocusedOptionKey={currentState}
	shouldStopPropagation
	/>

	<SelectionList
	headerMessage={headerMessage}
	textInputLabel={translate('common.country')}
	textInputValue={searchValue}
	sections={[{data: searchResults, indexOffset: 0}]}
	onSelectRow={selectCountry}
	onChangeText={setSearchValue}
	initiallyFocusedOptionKey={currentCountry}
	/>

[HOLD for payment 2023-12-06] [$500] Web - Profile - When selecting the last country or state extra space is displayed for a while #31660

Description

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Activity

melvin-bot commented on Nov 21, 2023

melvin-bot commented on Nov 21, 2023

melvin-bot commented on Nov 21, 2023

Bug0 Triage Checklist (Main S/O)

melvin-bot commented on Nov 21, 2023

erquhart commented on Nov 21, 2023

ZhenjaHorbach commented on Nov 21, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Fralalala commented on Nov 22, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

aum1618 commented on Nov 22, 2023

24 remaining items

melvin-bot commented on Nov 29, 2023

melvin-bot commented on Nov 29, 2023

melvin-bot commented on Nov 29, 2023

sonialiap commented on Dec 7, 2023

sonialiap commented on Dec 7, 2023

burczu commented on Dec 11, 2023

sonialiap commented on Dec 11, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions