[No QA][INP] Improve UserListItem-Checkbox INP: paint the checkmark optimistically and run the selection update as a transition - #97140
Conversation
dilshodmackbook-sketch
changed the title
|
@aimane-chnaif Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df2eb9eb10
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| onPress={() => onSelectRow(item)} | ||
| isChecked={isChecked} | ||
| onPress={() => { | ||
| setOptimisticChecked(!isChecked); |
There was a problem hiding this comment.
Because this shared button also backs ListRadioButton, tapping an already-selected radio now runs setOptimisticChecked(!isChecked) and immediately renders it unchecked. In single-select pickers that stay on the page and leave the value unchanged on a repeated tap (for example the spend rule merchant match-type selector, whose handler just calls setMatchType(item.value)), item.isSelected never changes, so the reset block does not run and the selected radio remains visually unchecked until another option/remount changes the prop. Restrict this optimistic toggle to checkboxes or avoid inverting an already-checked radio.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. Fixed in the latest commit. Since a radio press only ever selects (never toggles off), the optimistic paint now sets true for radios instead of inverting the current value, so re-pressing an already-selected radio keeps it checked. Only checkboxes flip. Added a unit test covering the re-press-selected-radio case.
The optimistic checkmark also backs radio rows. Re-pressing an already- selected radio inverted it to unchecked, and since its isSelected prop never changes the reset never fired, leaving it stuck unchecked. A radio press only ever selects, so paint it checked instead of toggling. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d136ff4b4c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| startTransition(() => { | ||
| setSelectedOptions(newSelectedOptions); |
There was a problem hiding this comment.
When a user already has at least one selected member, the Next button remains tappable while this new transition is still pending. In the slow-list case this change targets, tapping “Add to group” for another user and then quickly tapping Next can run createGroup from the previous render, which rebuilds NEW_GROUP_CHAT_DRAFT from stale selectedOptions and can overwrite the draft without the just-added participant. Use latestSelectedOptionsRef.current for confirmation, or otherwise prevent confirmation until the deferred selection has committed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. Fixed - createGroup now reads from latestSelectedOptionsRef.current instead of the render snapshot, so tapping "Add to group" and then quickly tapping Next while the deferred selection transition is still pending rebuilds the draft from the newest list (including the just-added participant), rather than a stale selectedOptions.
| if (prevCheckedProp !== isCheckedProp) { | ||
| setPrevCheckedProp(isCheckedProp); | ||
| setOptimisticChecked(null); | ||
| } |
There was a problem hiding this comment.
This reset only watches item.isSelected, so an optimistic value survives if the same component instance receives a different item with the same selected state. SelectionList rows are rendered through FlashList and can be recycled during filtering/scrolling; after pressing an unchecked row, a recycled cell for another unchecked row will keep optimisticChecked === true and display the wrong checkmark until some later selection prop change. Track item.keyForList/identity as part of the reset as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. Fixed - the reset now tracks item.keyForList identity alongside isSelected, so when FlashList recycles a cell to a different item with the same selected state the stale optimisticChecked is dropped instead of leaking onto the recycled row. Added a unit test covering the recycle-to-different-item case.
… latest selection Address two Codex P2 findings: - ListSelectionButton: the optimistic-check reset only watched item.isSelected, so a FlashList-recycled cell could keep a stale optimisticChecked=true and show the wrong checkmark. Track item.keyForList identity in the reset too. - NewChatPage.createGroup: read from latestSelectedOptionsRef so tapping "Add to group" then quickly Next (while the deferred selection transition is still pending) doesn't rebuild the draft from a stale selectedOptions snapshot. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No product review needed. |
|
@aimane-chnaif Gentle bump - the PR is ready for review whenever you get a chance. Thanks! |
|
@codex review |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d4940fd56
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Read membership from the latest selection (not the item or a render snapshot) so rapid toggles stay | ||
| // correct while the deferred list update below is still catching up to a previous press. | ||
| const currentSelectedOptions = latestSelectedOptionsRef.current; | ||
| const isOptionInList = currentSelectedOptions.some((selectedOption) => selectedOption.login === option.login); |
There was a problem hiding this comment.
When the list render is slow enough that the transition has not committed yet, the stale Add to group button is still visible after the first press. A second tap on that same visible Add button now sees the option in latestSelectedOptionsRef.current and takes the removal branch, so the user’s retry/double-tap silently cancels the pending add even though the UI never changed to a remove control. Treat presses from an unselected Add button as an add, or disable/optimistically replace the button until the deferred selection render catches up.
Useful? React with 👍 / 👎.
| // 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 selectedParticipants: SelectedParticipant[] = latestSelectedOptionsRef.current.map((option) => ({ |
There was a problem hiding this comment.
If the only selected member is unchecked, toggleOption updates latestSelectedOptionsRef.current to an empty list immediately, but the Next button is hidden only by the deferred setSelectedOptions transition. During that pending window the stale Next button can still be tapped, and this code then writes a draft containing only the current user and navigates to the group confirmation page. Return early when the latest selection is empty, or gate/disable confirmation from the same latest-selection source.
Useful? React with 👍 / 👎.
Review — looks solid ✅I reviewed the code, checked the app-wide blast radius of the shared What's good
Verification I ran
One boundary condition worth noting (non-blocking) Minor observations
Nice, careful PR — the comments and tests made it easy to follow. Test evidence (web)Add to group → checked checkbox + Next button
Three contacts selected — all checked, no duplicates
Group confirm page lists all selected members
|
Regression huntI traced the app-wide blast radius of the shared 🐞 Regression —
|
- A re-press on a still-visible Add to group button while the deferred selection update is pending now stays an idempotent add instead of silently cancelling the pending one; removal intent comes from the pressed row's rendered state. - createGroup returns early when the latest selection is empty, so a stale Next press right after removing the last member no longer builds a draft containing only the current user. - Membership matching falls back to accountID/reportID for login-less options so two distinct login-less DMs no longer collide on undefined === undefined. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed in 865a2b0: membership matching now falls back to accountID/reportID for login-less options, so two login-less DMs no longer collide on login equality. Same commit also makes a re-press on a stale "Add to group" button an idempotent add and guards createGroup against confirming with an empty latest selection. Added regression tests for all three cases. |
|
Done. |
4 participants
Explanation of Change
Improves the INP of the
UserListItem-Checkboxinteraction (the selection checkbox on user/list rows). Profiling on a production build (2,000 contacts,/new/chatgroup multi-select, 4x CPU throttle) showed the tap's next paint is blocked by a single ~51 ms React commit: the selection state update re-runs the whole options pipeline (getValidOptions->filterAndOrderOptions-> section flattening -> every visible row re-render) synchronously before the toggled checkmark can be shown.The change splits the tap into an urgent, tiny update and a deferred one — the same urgent-feedback +
startTransitionsplit used for thePrevNextButtons-NextButtonINP issue (#94721, PR #95737):ListSelectionButton— the checkmark is painted optimistically on press via local state, so it flips on the tap frame even while the parent's selection update is still pending. The optimistic value is dropped as soon as theitem.isSelectedprop catches up (state-adjustment-during-render, per the React docs). Since this component backs every SelectionList checkbox/radio, the instant feedback applies app-wide.NewChatPage.toggleOption— the heavy selection update (setSelectedOptions+setGroupDraft) now runs insidestartTransition, so the full pipeline re-render lands one frame later without blocking the tap's paint. Membership is read from a ref tracking the latest selection so back-to-back toggles compose off the newest list instead of a stale render snapshot while a previous transition is still catching up.The change is behavior-preserving: selection results, group draft contents, search clearing, and focus handling are unchanged; only the scheduling of the re-render moves.
Metrics improvement
Interaction:
UserListItem-Checkbox(data-sentry-label).Captured on a production (staging) web build with the INP-approximating window from the INP Improvement Workflow — 10 samples before and 10 after,
/new/chatgroup multi-select seeded with 2,000 contacts under 4x CPU throttle (recording starts on the checkbox press and stops on the next animation frame after the click):The ~51 ms options-pipeline commit is moved off the tap frame (optimistic checkmark +
startTransition), so the toggled checkmark paints on the press frame and the heavy selection re-render commits one frame later. Selection results are unchanged, so there is no visual or behavioral regression. Full trace details are in this comment.Fixed Issues
$ #94722
PROPOSAL: #94722 (comment)
Tests
Offline tests
Same as Tests — the change only affects render scheduling of local selection state; no network calls are involved. Selection must behave identically while offline.
QA Steps
[No QA] — performance-only change with no user-facing behavior change. The interaction produces the same result in the same order; only the scheduling relative to paint changes, verified by the before/after React Profiler metrics above and the Tests section.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2026-07-28.at.18.43.13.mov
Android: mWeb Chrome
94722-android-mweb-chrome.mp4
iOS: Native
Screen.Recording.2026-07-28.at.19.08.18.mov
iOS: mWeb Safari
94722-ios-mweb-safari.mp4
MacOS: Chrome / Safari
94722-macos-chrome.mp4