Fix: Selection mode infinite loop causing app freeze after splitting expense#87290
Fix: Selection mode infinite loop causing app freeze after splitting expense#87290
Conversation
…eak infinite loop Move the logic that turns off selection mode when no visible transactions remain from MoneyReportHeader into useHandleSelectionMode hook. This prevents an infinite loop between MoneyReportHeader (turning selection mode off) and useHandleSelectionMode (turning it back on). Co-authored-by: Andrew Rosiclair <arosiclair@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
melvin-bot
Bot
requested review from
a team,
flaviadefaria and
parasharrajat
and removed request for
a team
|
@parasharrajat 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] |
melvin-bot
Bot
requested review from
arosiclair and
huult
and removed request for
a team
|
@huult @arosiclair One of you needs to 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] |
|
@parasharrajat see the issue for context on this as the original freezing issue will no longer be reproducible on |
![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: 0febc530ab
ℹ️ 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".
| if (visibleItemCount <= 0) { | ||
| turnOffMobileSelectionMode(); | ||
| } else if (selectedItems.length > 0 && !isMobileSelectionModeEnabled) { |
There was a problem hiding this comment.
Do not unconditionally disable selection mode when visibleItemCount is zero, because in several SelectionListWithModal consumers (for example WorkspaceMembersPage), turning selection mode off immediately clears the selected IDs. With this new branch, selecting items and then applying a search/filter that temporarily returns no visible rows will drop the user's selection state; clearing the filter no longer restores it. This is a regression in multi-select flows where filtering is expected to be non-destructive.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This sounds legit... I'll take a look later
There was a problem hiding this comment.
@chatgpt-codex-connector Can you suggest test steps for this issue?
There was a problem hiding this comment.
To use Codex here, create a Codex account and connect to github.
There was a problem hiding this comment.
To use Codex here, create a Codex account and connect to github.
There was a problem hiding this comment.
Do not unconditionally disable selection mode when visibleItemCount is zero, because in several SelectionListWithModal consumers (for example WorkspaceMembersPage), turning selection mode off immediately clears the selected IDs. With this new branch, selecting items and then applying a search/filter that temporarily returns no visible rows will drop the user's selection state; clearing the filter no longer restores it. This is a regression in multi-select flows where filtering is expected to be non-destructive.
@MelvinBot, review this bug and verify if it is a real concern. Is so, suggest the test steps to reproduce this.
|
No new product considerations here, so unsubscribing and unassigning myself. |
|
Yes, this is a real concern. The bug would affect Causal chain:
This pattern (clear selections when Test steps to reproduce
The same steps apply to any page using Suggested fixGuard the if (visibleItemCount <= 0 && selectedItems.length === 0) {
turnOffMobileSelectionMode();
}This preserves the original fix for the infinite loop (when items are truly gone and nothing is selected) while avoiding the regression when items are merely filtered out of view. |
|
Legit Bug above. I can reproduce this. @arosiclair |
|
Yes, I'll reopen the PR when it's ready |
4 participants
Explanation of Change
There is an infinite loop between
MoneyReportHeader(which callsturnOffMobileSelectionModewhen visible transactions <= 1) anduseHandleSelectionMode(which callsturnOnMobileSelectionModewhenselectedItems.length > 0). This causes the app to freeze after splitting an expense on Android.This PR moves the visible transaction count check from
MoneyReportHeaderinto theuseHandleSelectionModehook, so the decision to turn off selection mode when no visible items remain is centralized and doesn't conflict with the hook's own logic. The hook now accepts avisibleItemCountparameter and turns off selection mode whenvisibleItemCount <= 0before checkingselectedItems.Fixed Issues
$ #87220
Tests
Offline tests
N/A - this is a selection mode UI fix unrelated to network state.
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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari