[CP Staging] Revert "fix: handle selection mode in search context"#73107
Conversation
|
🚧 @lakchote has triggered a test Expensify/App build. You can view the workflow run here. |
Codecov Report❌ Patch coverage is
... and 9 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
@blimpich looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
@chuckdries 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] |
|
Not an emergency, straight revert to fix a deploy blocker |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| turnOffMobileSelectionMode(); | ||
| } | ||
| // eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps | ||
| }, [sections, selectedItemsProp, isMobileSelectionModeEnabled, isSmallScreenWidth, isSelected, isFocused]); |
There was a problem hiding this comment.
❌ PERF-6 (docs)
The sections array is included as a dependency, which could cause unnecessary re-executions of this effect whenever any property in any section changes, even if unrelated to selection logic.
Consider extracting specific properties that are actually used:
const selectedItems = useMemo(() =>
selectedItemsProp ??
sections[0].data.filter((item) => {
if (isSelected) {
return isSelected(item);
}
return !!item.isSelected;
}), [selectedItemsProp, sections[0].data, isSelected]
);
useEffect(() => {
// ... rest of logic
}, [selectedItems.length, isMobileSelectionModeEnabled, isSmallScreenWidth, isFocused]);
| // We can access 0 index safely as we are not displaying multiple sections in table view | ||
| const selectedItems = | ||
| selectedItemsProp ?? | ||
| sections[0].data.filter((item) => { |
There was a problem hiding this comment.
❌ Array Index Access Without Bounds Check
Accessing sections[0].data without checking if sections array is non-empty could cause a runtime error if sections is empty.
Add a bounds check:
const selectedItems =
selectedItemsProp ??
(sections.length > 0 ? sections[0].data.filter((item) => {
if (isSelected) {
return isSelected(item);
}
return !!item.isSelected;
}) : []);
| }, []); | ||
|
|
||
| if (isMobileSelectionModeEnabled && shouldUseNarrowLayout) { | ||
| if (isMobileSelectionModeEnabled) { |
There was a problem hiding this comment.
❌ Logic Issue: Condition Removal May Cause Unintended Behavior
The removed && shouldUseNarrowLayout condition means the mobile selection mode logic will now execute on all screen sizes, not just narrow layouts. This could cause selection mode to be unexpectedly turned off on larger screens where it shouldn't apply.
Consider if this change is intentional. If mobile selection mode should only apply to narrow layouts, the condition should be restored:
if (isMobileSelectionModeEnabled && shouldUseNarrowLayout) {
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
blimpich
changed the title
(cherry picked from commit 90739aa) (cherry-picked to staging by blimpich)
|
🚀 Cherry-picked to staging by https://github.com/blimpich in version: 9.2.35-3 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.2.35-4 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/blimpich in version: 9.2.36-0 🚀
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.2.36-7 🚀
|
4 participants
Reverts #71700
Fixes
$ #73089