[No QA][INP] Improve UserListItem-Checkbox INP: paint the checkmark optimistically and run the selection update as a transition

src/components/SelectionList/components/ListSelectionButton.tsx

@@ -5,7 +5,7 @@ import CONST from '@src/CONST';
 
 import type {StyleProp, ViewStyle} from 'react-native';
 
-import React from 'react';
+import React, {useState} from 'react';
 
 type ListSelectionButtonProps<TItem extends ListItem> = {
     /** The item to render the selection button for */
@@ -50,13 +50,34 @@ function ListSelectionButton<TItem extends ListItem>({
 }: ListSelectionButtonProps<TItem> & {role: typeof CONST.ROLE.CHECKBOX | typeof CONST.ROLE.RADIO}) {
     const label = accessibilityLabel ?? item.text ?? '';
 
+    // Paint the checkmark immediately on press, even when the parent defers the (expensive) selection-state
+    // update in a transition. The optimistic value is dropped as soon as the item prop catches up
+    // (state-adjustment-during-render, see https://react.dev/reference/react/useState#storing-information-from-previous-renders).
+    // Track the row identity (keyForList) alongside isSelected: SelectionList rows render through FlashList and are
+    // recycled, so the same component instance can receive a different item with the same isSelected value - resetting
+    // on identity change prevents a stale optimistic checkmark from leaking onto the recycled row.
+    const isCheckedProp = item.isSelected ?? false;
+    const [prevItem, setPrevItem] = useState({key: item.keyForList, checked: isCheckedProp});
+    const [optimisticChecked, setOptimisticChecked] = useState<boolean | null>(null);
+    if (prevItem.key !== item.keyForList || prevItem.checked !== isCheckedProp) {
+        setPrevItem({key: item.keyForList, checked: isCheckedProp});
+        setOptimisticChecked(null);
+    }
+    const isChecked = optimisticChecked ?? isCheckedProp;
+
     return (
         <SelectionButton
             shouldSelectOnPressEnter
             role={role}
             accessibilityLabel={label}
-            isChecked={item.isSelected ?? false}
-            onPress={() => onSelectRow(item)}
+            isChecked={isChecked}
+            onPress={() => {
+                // A checkbox toggles, so flip the current value. A radio press only ever selects, so paint it
+                // checked - inverting an already-checked radio would leave it stuck unchecked because its
+                // isSelected prop never changes to trigger the reset above.
+                setOptimisticChecked(role === CONST.ROLE.RADIO ? true : !isChecked);
+                onSelectRow(item);
+            }}
             disabled={disabled}
             style={style}
             containerStyle={containerStyle}

src/pages/NewChatPage/index.tsx

@@ -49,7 +49,7 @@ import {useFocusEffect} from '@react-navigation/native';
 import {guidedSetupAndTourStatusSelector} from '@selectors/Onboarding';
 import passthroughPolicyTagListSelector from '@selectors/PolicyTagList';
 import reject from 'lodash/reject';
-import React, {useEffect, useImperativeHandle, useRef, useState} from 'react';
+import React, {startTransition, useEffect, useImperativeHandle, useRef, useState} from 'react';
 import {Keyboard} from 'react-native';
 
 import type SelectedOption from './types';
@@ -60,6 +60,22 @@ import useGroupChatDraftParticipantSync from './useGroupChatDraftParticipantSync
 const excludedGroupEmails = new Set<string>(CONST.EXPENSIFY_EMAILS.filter((value) => value !== CONST.EMAIL.CONCIERGE));
 const PAGINATION_SIZE = CONST.MAX_SELECTION_LIST_PAGE_LENGTH;
 
+/**
+ * Whether a pressed option refers to the same person as an already-selected one. Login is the primary identity,
+ * but login-less options (e.g. DMs whose participant details haven't loaded yet) would all compare equal on login
+ * alone (undefined === undefined), so fall back to accountID, then reportID. Pairs with no shared identity are
+ * treated as distinct so they follow the add path.
+ */
+function isSameSelectedOption(selectedOption: SelectedOption, option: ListItem & Partial<OptionData>): boolean {
+    if (selectedOption.login || option.login) {
+        return selectedOption.login === option.login;
+    }
+    if (selectedOption.accountID || option.accountID) {
+        return selectedOption.accountID === option.accountID;
+    }
+    return !!selectedOption.reportID && selectedOption.reportID === option.reportID;
+}
+
 function useOptions(reportAttributesDerived: ReportAttributesDerivedValue['reports'] | undefined) {
     const [searchTerm, debouncedSearchTerm, setSearchTerm] = useDebouncedState('');
     const [selectedOptions, setSelectedOptions] = useState<SelectedOption[]>([]);
@@ -268,6 +284,13 @@ function NewChatPage({ref}: NewChatPageProps) {
         areOptionsInitialized,
     } = useOptions(reportAttributesDerived);
 
+    // Latest committed selection, kept in a ref so back-to-back toggles compose off the newest list instead of a
+    // stale render snapshot while the deferred (startTransition) selection update below is still catching up.
+    const latestSelectedOptionsRef = useRef(selectedOptions);
+    useEffect(() => {
+        latestSelectedOptionsRef.current = selectedOptions;
+    }, [selectedOptions]);
+
     // Selected rows are marked in place by getValidOptions (isSelected), so the checkmark stays with the row instead of jumping to the top.
     // In group selection mode the self DM stays visible (so the list doesn't shift and jump the scroll position) but is made non-selectable.
     const recentReportsData = selectedOptions.length ? recentReports.map((option) => (option.isSelfDM ? {...option, isDisabled: true} : option)) : recentReports;
@@ -316,23 +339,44 @@ function NewChatPage({ref}: NewChatPageProps) {
      * Removes a selected option from list if already selected. If not already selected add this option to the list.
      */
     const toggleOption = (option: ListItem & Partial<OptionData>) => {
-        const isOptionInList = !!option.isSelected;
+        // Compose off the latest selection (not a render snapshot) so back-to-back toggles don't drop each
+        // other's pending updates while the deferred list update below is still catching up.
+        const currentSelectedOptions = latestSelectedOptionsRef.current;
+        const isOptionInList = currentSelectedOptions.some((selectedOption) => isSameSelectedOption(selectedOption, option));
+        // Removal intent comes from the pressed row's rendered state, not from ref membership: while the
+        // transition is pending the row still shows its stale control, so a re-press on a still-visible
+        // "Add to group" button must stay an add (idempotent below) rather than silently cancel the pending one.
+        const shouldRemoveOption = !!option.isSelected && isOptionInList;
 
         let newSelectedOptions: SelectedOption[];
 
-        if (isOptionInList) {
-            newSelectedOptions = reject(selectedOptions, (selectedOption) => selectedOption.login === option.login);
+        if (shouldRemoveOption) {
+            newSelectedOptions = reject(currentSelectedOptions, (selectedOption) => isSameSelectedOption(selectedOption, option));
+        } else if (isOptionInList) {
+            // Already added by a previous press of the same still-visible Add button; nothing to change.
+            newSelectedOptions = currentSelectedOptions;
         } else {
-            newSelectedOptions = [...selectedOptions, {...option, isSelected: true, reportID: option.reportID, keyForList: `${option.keyForList ?? option.reportID}`}];
+            newSelectedOptions = [...currentSelectedOptions, {...option, isSelected: true, reportID: option.reportID, keyForList: `${option.keyForList ?? option.reportID}`}];
         }
 
+        // Advance the ref immediately so a second tap landing before the transition commits composes off this
+        // result instead of dropping it. External updates re-sync the ref via the effect above.
+        latestSelectedOptionsRef.current = newSelectedOptions;
+
         selectionListRef.current?.clearInputAfterSelect();
         if (!canUseTouchScreen()) {
             selectionListRef.current?.focusTextInput();
         }
-        setSelectedOptions(newSelectedOptions);
 
-        if (personalData?.login && personalData?.accountID) {
+        // The selection update fans out into the whole options pipeline (getValidOptions -> filterAndOrderOptions ->
+        // sections -> useFlattenedSections -> every visible row re-render). Run it as a transition so the tapped
+        // checkbox (which shows optimistic feedback) can paint first and the heavy re-render doesn't block the frame.
+        startTransition(() => {
+            setSelectedOptions(newSelectedOptions);
+
+            if (!personalData?.login || !personalData?.accountID) {
+                return;
+            }
             const participants: SelectedParticipant[] = [
                 ...newSelectedOptions.map((selectedOption) => ({
                     login: selectedOption.login,
@@ -344,7 +388,7 @@ function NewChatPage({ref}: NewChatPageProps) {
                 },
             ];
             setGroupDraft({participants});
-        }
+        });
     };
 
     /**
@@ -445,7 +489,16 @@ function NewChatPage({ref}: NewChatPageProps) {
         if (!personalData?.login || !personalData.accountID) {
             return;
         }
-        const selectedParticipants: SelectedParticipant[] = selectedOptions.map((option) => ({
+        // Read from the latest-selection ref (not the render snapshot): the Next button stays tappable while the
+        // deferred selection transition is still pending, so tapping "Add to group" then quickly Next must not rebuild
+        // the draft from a stale selectedOptions that drops the just-added participant.
+        const latestSelectedOptions = latestSelectedOptionsRef.current;
+        // Conversely, the stale Next button can be tapped right after the last member was removed; don't build
+        // a group draft containing only the current user.
+        if (latestSelectedOptions.length === 0) {
+            return;
+        }
+        const selectedParticipants: SelectedParticipant[] = latestSelectedOptions.map((option) => ({
             login: option?.login,
             accountID: option.accountID ?? CONST.DEFAULT_NUMBER_ID,
         }));

tests/ui/NewChatPageTest.tsx

@@ -1,10 +1,13 @@
 import {act, fireEvent, render, screen, waitFor, within} from '@testing-library/react-native';
 
+import {CurrentUserPersonalDetailsProvider} from '@components/CurrentUserPersonalDetailsProvider';
 import HTMLEngineProvider from '@components/HTMLEngineProvider';
 import {LocaleContextProvider} from '@components/LocaleContextProvider';
 import OnyxListItemProvider from '@components/OnyxListItemProvider';
 import ScreenWrapper from '@components/ScreenWrapper';
 
+import Navigation from '@libs/Navigation/Navigation';
+
 import NewChatPage from '@pages/NewChatPage';
 
 import CONST from '@src/CONST';
@@ -24,6 +27,10 @@ import waitForBatchedUpdatesWithAct from '../utils/waitForBatchedUpdatesWithAct'
 
 jest.mock('@react-navigation/native');
 jest.mock('@src/libs/Navigation/navigationRef');
+// Use the web implementation (a passthrough): Jest resolves the .native variant, whose in-flight guard would
+// swallow the rapid re-presses that the pending-transition tests below simulate. On web, where the deferred
+// selection update matters most, there is no such guard.
+jest.mock('@hooks/useSingleExecution', (): unknown => jest.requireActual('@hooks/useSingleExecution/index.ts'));
 jest.mock('react-native-permissions', () => ({
     __esModule: true,
     RESULTS: {
@@ -49,11 +56,13 @@ const triggerTransitionEnd = () => (NativeNavigation as NativeNavigationMock).tr
 
 const wrapper = ({children}: {children: React.ReactNode}) => (
     <OnyxListItemProvider>
-        <HTMLEngineProvider>
-            <LocaleContextProvider>
-                <ScreenWrapper testID="test">{children}</ScreenWrapper>
-            </LocaleContextProvider>
-        </HTMLEngineProvider>
+        <CurrentUserPersonalDetailsProvider>
+            <HTMLEngineProvider>
+                <LocaleContextProvider>
+                    <ScreenWrapper testID="test">{children}</ScreenWrapper>
+                </LocaleContextProvider>
+            </HTMLEngineProvider>
+        </CurrentUserPersonalDetailsProvider>
     </OnyxListItemProvider>
 );
 
@@ -199,6 +208,135 @@ describe('NewChatPage', () => {
         expect(scrollToSpy).not.toHaveBeenCalled();
     });
 
+    it('should toggle selection correctly via the row checkbox with the deferred selection update', async () => {
+        await act(async () => {
+            await Onyx.merge(ONYXKEYS.PERSONAL_DETAILS_LIST, fakePersonalDetails);
+        });
+        render(<NewChatPage />, {wrapper});
+        await waitForBatchedUpdatesWithAct();
+        act(() => {
+            triggerTransitionEnd();
+        });
+
+        // Enter group-selection mode by selecting one user.
+        const addButton = await waitFor(() => {
+            const button = screen.getAllByText(translateLocal('newChatPage.addToGroup')).at(0);
+            expect(button).toBeTruthy();
+            return button;
+        });
+        if (!addButton) {
+            return;
+        }
+        fireEvent.press(addButton);
+        await waitForBatchedUpdatesWithAct();
+        expect(screen.getByText(translateLocal('common.next'))).toBeVisible();
+
+        // Toggle the selected row's checkbox (it is only rendered on selected rows). The selection update now runs
+        // as a transition, so this covers that deferring it does not change the toggle semantics: the user is
+        // unselected and group-selection mode is exited.
+        const checkedCheckbox = screen.getAllByTestId(new RegExp(`^${CONST.SELECTION_BUTTON_TEST_ID}`)).at(0);
+        expect(checkedCheckbox).toBeTruthy();
+        if (!checkedCheckbox) {
+            return;
+        }
+        fireEvent.press(checkedCheckbox);
+        await waitForBatchedUpdatesWithAct();
+        expect(screen.queryByText(translateLocal('common.next'))).toBeNull();
+        expect(screen.queryAllByTestId(new RegExp(`^${CONST.SELECTION_BUTTON_TEST_ID}`))).toHaveLength(0);
+
+        // And selecting again from the empty state still works.
+        const buttonAfter = screen.getAllByText(translateLocal('newChatPage.addToGroup')).at(0);
+        if (buttonAfter) {
+            fireEvent.press(buttonAfter);
+        }
+        await waitForBatchedUpdatesWithAct();
+        expect(screen.getByText(translateLocal('common.next'))).toBeVisible();
+    });
+
+    it('should keep the user selected when the still-visible "Add to group" button is pressed twice before the deferred update commits', async () => {
+        await act(async () => {
+            await Onyx.merge(ONYXKEYS.PERSONAL_DETAILS_LIST, fakePersonalDetails);
+        });
+        render(<NewChatPage />, {wrapper});
+        await waitForBatchedUpdatesWithAct();
+        act(() => {
+            triggerTransitionEnd();
+        });
+
+        const addButton = await waitFor(() => {
+            const button = screen.getAllByText(translateLocal('newChatPage.addToGroup')).at(0);
+            expect(button).toBeTruthy();
+            return button;
+        });
+        if (!addButton) {
+            return;
+        }
+
+        // Both presses run in one act() batch: nothing commits between them, so the second press lands on the
+        // same still-visible "Add to group" button — the retry/double-tap that arrives before the deferred
+        // selection update has caught up.
+        // eslint-disable-next-line testing-library/no-unnecessary-act -- the shared act batch is the point: it keeps the deferred update from committing between the two presses
+        act(() => {
+            fireEvent.press(addButton);
+            fireEvent.press(addButton);
+        });
+        await waitForBatchedUpdatesWithAct();
+
+        // The retry press must stay an add (idempotent), not silently cancel the pending one.
+        expect(screen.getByText(translateLocal('common.next'))).toBeVisible();
+        expect(screen.getAllByTestId(new RegExp(`^${CONST.SELECTION_BUTTON_TEST_ID}`))).toHaveLength(1);
+    });
+
+    it('should not open the group confirmation when the last member was removed just before pressing Next', async () => {
+        await act(async () => {
+            await Onyx.merge(ONYXKEYS.PERSONAL_DETAILS_LIST, fakePersonalDetails);
+            await Onyx.merge(ONYXKEYS.SESSION, {accountID: 1, email: 'email1@test.com'});
+        });
+        render(<NewChatPage />, {wrapper});
+        await waitForBatchedUpdatesWithAct();
+        act(() => {
+            triggerTransitionEnd();
+        });
+
+        // Enter group-selection mode by selecting one user.
+        const addButton = await waitFor(() => {
+            const button = screen.getAllByText(translateLocal('newChatPage.addToGroup')).at(0);
+            expect(button).toBeTruthy();
+            return button;
+        });
+        if (!addButton) {
+            return;
+        }
+        fireEvent.press(addButton);
+        await waitForBatchedUpdatesWithAct();
+        const nextButton = screen.getByText(translateLocal('common.next'));
+        expect(nextButton).toBeVisible();
+
+        const navigateSpy = jest.spyOn(Navigation, 'navigate').mockImplementation(() => {});
+
+        const checkedCheckbox = screen.getAllByTestId(new RegExp(`^${CONST.SELECTION_BUTTON_TEST_ID}`)).at(0);
+        expect(checkedCheckbox).toBeTruthy();
+        if (!checkedCheckbox) {
+            return;
+        }
+
+        // Both presses run in one act() batch: nothing commits between them, so the Next button is still
+        // rendered when it is pressed even though the last member was just removed. Confirming with an empty
+        // latest selection must not navigate to the group confirmation page.
+        // eslint-disable-next-line testing-library/no-unnecessary-act -- the shared act batch is the point: it keeps the deferred update from committing between the two presses
+        act(() => {
+            fireEvent.press(checkedCheckbox);
+            fireEvent.press(nextButton);
+        });
+        expect(navigateSpy).not.toHaveBeenCalled();
+
+        // Once the deferred update commits the removal, group-selection mode is exited.
+        await waitForBatchedUpdatesWithAct();
+        expect(screen.queryByText(translateLocal('common.next'))).toBeNull();
+        expect(navigateSpy).not.toHaveBeenCalled();
+        navigateSpy.mockRestore();
+    });
+
     describe('should not display "Add to group" button on expensify emails', () => {
         const excludedGroupEmails = CONST.EXPENSIFY_EMAILS.filter((value) => value !== CONST.EMAIL.CONCIERGE && value !== CONST.EMAIL.NOTIFICATIONS).map((email) => [email]);