Skip to content

fix: Tags - List jumps to “Show More” when selecting tag after 500. #66780

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rafecolton merged 16 commits into from
Aug 5, 2025
Merged

fix: Tags - List jumps to “Show More” when selecting tag after 500. #66780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
655fdca
fix: Tags - List jumps to “Show More” when selecting tag after 500.
Krishna2323 Jul 21, 2025
2d2ff2d
Merge branch 'Expensify:main' into krishna2323/issue/65977
Krishna2323 Jul 21, 2025
e186b1a
resolve merge conflicts.
Krishna2323 Jul 23, 2025
17b4e2e
resolve merge conflicts.
Krishna2323 Jul 23, 2025
05d59d8
Merge branch 'Expensify:main' into krishna2323/issue/65977
Krishna2323 Jul 23, 2025
0dbcc61
update unit tests.
Krishna2323 Jul 23, 2025
3e8a6ae
comment update
Krishna2323 Jul 23, 2025
364ac0c
update comment.
Krishna2323 Jul 23, 2025
63c507e
Merge branch 'main' into krishna2323/issue/65977
Krishna2323 Jul 24, 2025
daa7675
fix unit tests.
Krishna2323 Jul 25, 2025
ce1a622
comment update.
Krishna2323 Jul 28, 2025
334b499
fix merge conflicts.
Krishna2323 Aug 5, 2025
45070f5
Merge branch 'Expensify:main' into krishna2323/issue/65977
Krishna2323 Aug 5, 2025
cac3654
re-implement tests.
Krishna2323 Aug 5, 2025
0291827
remove redundant code.
Krishna2323 Aug 5, 2025
181f146
Merge branch 'Expensify:main' into krishna2323/issue/65977
Krishna2323 Aug 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions src/components/SelectionList/BaseSelectionList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -809,6 +809,14 @@ function BaseSelectionList<TItem extends ListItem>(
const prevSelectedOptionsLength = usePrevious(flattenedSections.selectedOptions.length);
const prevAllOptionsLength = usePrevious(flattenedSections.allOptions.length);

useEffect(() => {
if (prevTextInputValue === textInputValue) {
return;
}
// Reset the current page to 1 when the user types something
setCurrentPage(1);
}, [textInputValue, prevTextInputValue]);

useEffect(() => {
// Avoid changing focus if the textInputValue remains unchanged.
if (
Expand All @@ -831,11 +839,6 @@ function BaseSelectionList<TItem extends ListItem>(
}
}

// Reset the current page to 1 when the user types something
if (prevTextInputValue !== textInputValue) {
setCurrentPage(1);
}

// Remove the focus if the search input is empty and prev search input not empty or selected options length is changed (and allOptions length remains the same)
// else focus on the first non disabled item
const newSelectedIndex =
Expand Down
58 changes: 58 additions & 0 deletions tests/unit/BaseSelectionListTest.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,4 +211,62 @@ describe('BaseSelectionList', () => {
expect(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}70`)).toBeTruthy();
expect(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}70`)).toBeSelected();
});

it('does not reset page when only selectedOptions changes', () => {
const {rerender} = render(
<BaseListItemRenderer
sections={[{data: largeMockSections}]}
canSelectMultiple={false}
initialNumToRender={110}
/>,
);

// Click Show More button
fireEvent.press(screen.getByText('common.showMore'));

rerender(
<BaseListItemRenderer
sections={[{data: largeMockSections.map((item, index) => ({...item, isSelected: index === 3}))}]}
canSelectMultiple={false}
initialNumToRender={110}
/>,
);
Comment on lines +227 to +233

@hoangzinh hoangzinh Jul 24, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rerender to mock sections look here looks incorrect to me. Btw, we should simulate the behavior that user tap on the checkbox, not the "show more" button.

@Krishna2323 Krishna2323 Jul 28, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does re-rendering seem incorrect to you? We just need to update the selected options array and ensure that the currentPage is not reset to 1. I think this is the correct way to handle it.

@hoangzinh hoangzinh Jul 28, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm expecting we should simulate press checkbox like this

App/tests/unit/BaseSelectionListTest.tsx

Line 52 in 4b0bfd6

fireEvent.press(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}1`));

Re-render only makes sense if the props is updated from outside like this

App/tests/unit/BaseSelectionListTest.tsx

Lines 67 to 77 in 4b0bfd6

it('should update focused item when sections are updated from BE', () => {
(NativeNavigation.useIsFocused as jest.Mock).mockReturnValue(true);
const updatedMockSections = mockSections.map((section) => ({
...section,
isSelected: section.keyForList === '2',
}));
const {rerender} = render(<BaseListItemRenderer sections={[{data: mockSections}]} />);
expect(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}1`)).toBeSelected();
rerender(<BaseListItemRenderer sections={[{data: updatedMockSections}]} />);
expect(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}2`)).toBeSelected();
});

@hoangzinh hoangzinh Jul 28, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From this recording #66780 (comment). Are you saying that we can't write test for this simulate press checkbox case?

@Krishna2323 Krishna2323 Jul 28, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can simulate a checkbox press, but it will have no effect because the options with property isSelected come from the parent component, which uses SelectionList. The selected options only change if the parent component handles the onSelectRow callback and provides updated sections to the SelectionList via sections={[{data: updatedLargeMockSections}]}.

    it('does not reset page when only selectedOptions changes', async () => {
        render(
            <BaseListItemRenderer
                sections={[{data: largeMockSections}]}
                canSelectMultiple
                initialNumToRender={600}
                listItem={TableListItem}
            />,
        );

        await waitForBatchedUpdatesWithAct();

        // Click Show More button
        fireEvent.press(screen.getByText('common.showMore'));
        fireEvent.press(screen.getByTestId(`TableListItemCheckbox-Item 598`));
        expect(onSelectRowMock).toHaveBeenCalledTimes(1);

        await waitForBatchedUpdatesWithAct();
        // Should now show items from second page
        expect(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}500`)).toBeTruthy();
        expect(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}503`)).toBeTruthy();

        // Should not show, "Show more" button as we rendered whole list and search text was not changed
        expect(screen.queryByText('common.showMore')).toBeFalsy();
    });


// Should now show items from second page
expect(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}51`)).toBeTruthy();
expect(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}62`)).toBeTruthy();

// Should not show, "Show more" button as we rendered whole list and search text was not changed
expect(screen.queryByText('common.showMore')).toBeFalsy();
});

it('should reset current page when text input changes', () => {
const {rerender} = render(
<BaseListItemRenderer
sections={[{data: largeMockSections}]}
canSelectMultiple={false}
initialNumToRender={110}
/>,
);

// Click Show More button
fireEvent.press(screen.getByText('common.showMore'));
expect(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}99`)).toBeTruthy();

// Rerender with changed `searchText` to trigger `setCurrentPage(1)`
rerender(
<BaseListItemRenderer
sections={[{data: largeMockSections.map((item, index) => ({...item, isSelected: index === 3}))}]}
canSelectMultiple={false}
searchText="Item"
initialNumToRender={110}
/>,
);

// Should not show the items from second page
expect(screen.queryByText(`${CONST.BASE_LIST_ITEM_TEST_ID}52`)).toBeFalsy();

// Should show, "Show more" button as current page is reset
expect(screen.getByText('common.showMore')).toBeOnTheScreen();
});
});