Stop scroll jump on room invite member selection#91387
Conversation
Keep selected contacts in-place in the Contacts section instead of moving them to a separate section at the top. This prevents the list from jumping when selecting multiple contacts from different positions. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| shouldKeepSelectedInAvailableOptions: true, | ||
| }); | ||
|
|
||
| const selectedNonExistingOptions = availableOptions.extraOptions.filter((option) => option.isSelected); |
There was a problem hiding this comment.
@MelvinBot move this logic to usePersonalDetailSearchSelector similar to useSearchSelector
There was a problem hiding this comment.
Moved selectedNonExistingOptions into usePersonalDetailSearchSelector — it's now computed inside the hook and returned directly, matching the useSearchSelector pattern.
Compute selectedNonExistingOptions inside the hook and return it, matching the useSearchSelector pattern. Remove the external computation from RoomInvitePage. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
| setSelectedAccountIDs(new Set()); | ||
| }; | ||
|
|
||
| const selectedNonExistingOptions = extraOptions.filter((option) => option.isSelected); |
There was a problem hiding this comment.
@MelvinBot this should filter out options mismatching search input value
See this bug:
There was a problem hiding this comment.
Fixed — selectedNonExistingOptions now filters by debouncedSearchTerm, matching how useSearchSelector filters selectedOptionsForDisplay by search input before computing non-existing options.
When a search term is active, filter out selected non-existing options that don't match the search input, preventing mismatched options from displaying in the results. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
@MelvinBot when press Enter key twice while focused item with arrow key, Invite button is triggered instead of toggling checkbox twice. Can you investigate? This happens on production so out of scope but I prefer fixing this here. Note: this doesn't happen on |
When the invite button was a sibling outside SelectionListWithSections, its Enter key handler registered after the list's handler, putting it at the top of the keyboard shortcut stack. This caused the second Enter press to trigger the invite instead of toggling the checkbox. Moving it into footerContent (as a child of the SelectionList) matches the DynamicWorkspaceInvitePage pattern where child effects register first, keeping the SelectionList's handler on top. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Root cause: The Why Fix: Moved Key evidenceThe handler registration in |
| canSelectMultiple | ||
| footerContent={footerContent} | ||
| /> | ||
| <View style={[styles.flexShrink0]}> |
There was a problem hiding this comment.
@MelvinBot the parent wrapper with styles.flexShrink0 style was not moved to footerContent.
Is it safe? Won't it cause any style regression?
There was a problem hiding this comment.
Yes, it's safe — no style regression. The footerContent prop is rendered inside FixedFooter, which already wraps children in a View with styles.flexShrink0. So the outer wrapper is redundant when using footerContent.
DynamicWorkspaceInvitePage follows the same pattern — it passes footerContent without an extra flexShrink0 wrapper.
There was a problem hiding this comment.
@MelvinBot ok but there's another style regression.
App/src/pages/RoomInvitePage.tsx
Line 210 in dfd740e
styles.mb5, styles.ph5 seems duplicated
There was a problem hiding this comment.
Removed styles.mb5 and styles.ph5 from the FormAlertWithSubmitButton containerStyles. FixedFooter already applies ph5, so these were causing double padding. Now matches the DynamicWorkspaceInvitePage pattern.
| headerMessage: getHeaderMessageText(), | ||
| }; | ||
|
|
||
| const footerContent = useMemo( |
There was a problem hiding this comment.
@MelvinBot remove useMemo as this component is auto memoized by React Compiler
There was a problem hiding this comment.
Removed the useMemo wrapper from footerContent — React Compiler handles memoization automatically.
React Compiler auto-memoizes this component, making the manual useMemo wrapper unnecessary. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
FixedFooter already applies ph5, so having it in containerStyles too caused double horizontal padding. Match the DynamicWorkspaceInvitePage pattern which only uses flex-related container styles. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
@codex review |
|
@MelvinBot add unit tests |
Code ReviewTwo actionable issues found, plus some minor observations. 1. Bug: Missing
|
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
@MelvinBot this is pre-existing bug on |
…gOptions Test the selectedNonExistingOptions behavior including: - Empty by default when no extra options exist - Populated after toggling non-existing users - Excludes existing contacts - Cleared on deselection and resetSelection - Filtered by debounced search term (login and text) - Handles mix of existing and non-existing selected users Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Added 9 unit tests for
All 9 tests pass. |
Prevents Ctrl+Enter from firing inviteUsers with zero selections, matching the pattern used in DynamicWorkspaceInvitePage. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Yes, this is a pre-existing bug on |
|
@MelvinBot complete checklist, test steps and mark screenshots as done since they’ll be added in the reviewer checklist. |
|
Done — completed the PR author checklist, filled in test steps (Tests, Offline, QA), and marked screenshots as deferred to the reviewer checklist. |
| await waitForBatchedUpdatesWithAct(); | ||
| expect(result.current.selectedNonExistingOptions).toHaveLength(2); | ||
|
|
||
| // Search for "newuser" - should filter to only the matching non-existing option |
There was a problem hiding this comment.
@MelvinBot use another search term to fix spell check
There was a problem hiding this comment.
Fixed — replaced newuser with invitee in the test data and search term. Spellcheck should pass now.
Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Marked this PR as ready for review. |
melvin-bot
Bot
requested review from
mkhutornyi and
trjExpensify
and removed request for
a team and
trjExpensify
|
@mkhutornyi 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] |
|
@MelvinBot fix failing typescript |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
Add optional chaining to .at(0) calls since Array.at() returns T | undefined in TypeScript strict mode. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Fixed the failing typecheck: added optional chaining ( |
|
@francoisl 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] |
Regression AnalysisNo regressions found. The PR changes are safe and follow established patterns. Investigation details (4 areas checked)1. Section index gaps — SafeThe new code hardcodes 2. Footer styling (mb5/ph5 removed) — Safe
3. Old if/else section logic replaced with independent ifs — SafeThe old code showed 4. No duplicate button (confirmButtonOptions + footerContent) — Safe
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const MOCK_ACCOUNT_ID = 12345; | ||
| const MOCK_EMAIL = 'test@expensify.com'; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
This eslint-disable-next-line suppresses @typescript-eslint/no-unsafe-return without an accompanying comment explaining why the rule is disabled. Every eslint-disable should include a justification so reviewers and future maintainers understand the reasoning.
Add a justification comment, for example:
// eslint-disable-next-line @typescript-eslint/no-unsafe-return -- jest.requireActual returns untyped module
jest.mock('@libs/PersonalDetailOptionsListUtils', () => ({Reviewed at: fab4748 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Fair but not going to block on this for test
| const MOCK_ACCOUNT_ID = 12345; | ||
| const MOCK_EMAIL = 'test@expensify.com'; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return |
There was a problem hiding this comment.
Fair but not going to block on this for test
|
🚧 @francoisl has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/francoisl in version: 9.3.80-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. This PR fixes a UI behavior bug (scroll jumping when selecting contacts on the Room Invite page). The existing documentation in |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.3.81-2 🚀
|
4 participants
Explanation of Change
Stop scrolling and jumping when selecting items on the Room Invite page (
RoomInvitePage).Previously, selected contacts were separated into a dedicated section at the top, and selecting a new contact triggered an auto-scroll to the top of the list. This was disorienting when selecting multiple contacts from a long list.
Changes:
shouldKeepSelectedInAvailableOptions: truetousePersonalDetailSearchSelectorso selected items remain in the Contacts section with theirisSelectedflag instead of being moved to a separate sectionselectedNonExistingOptionsinsideusePersonalDetailSearchSelector— selected users not present inavailableOptions.personalDetails(e.g. typed email addresses). These are shown in a dedicated top section above Contacts so they remain visible after selectionuserToInviterow when it is already selected to prevent duplicate entriesshouldUpdateFocusedIndexandshouldPreventAutoScrollOnSelectto the selection list to maintain scroll position when selecting itemsThis follows the same pattern applied in #91250 for
DynamicWorkspaceInvitePage.Fixed Issues
$ #61414
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Screenshots will be provided in the reviewer checklist.
Android: Native
Will be added in reviewer checklist.
Android: mWeb Chrome
Will be added in reviewer checklist.
iOS: Native
Will be added in reviewer checklist.
iOS: mWeb Safari
Will be added in reviewer checklist.
MacOS: Chrome / Safari
Will be added in reviewer checklist.
AI Tests
prettierlint-changedtypecheck-tsgoreact-compiler-compliance-check