Android - Reports -Empty space created at the end of filters row when clearing "From" filter#74920
Merged
Merged
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@dukenv0307 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
joekaufmanexpensify
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| scrollRef.current?.scrollToEnd(); | ||
| }, [filters.length]); | ||
|
|
||
| const renderFilterItem = useCallback( |
Contributor
There was a problem hiding this comment.
❌ PERF-4 (docs)
The renderFilterItem callback is passed to FlatList's renderItem prop but has an empty dependency array. This means it's only created once during the component's lifetime. However, the DropdownButton component receives item.PopoverComponent as a prop, which is a function reference that comes from the filters array.
Since the renderFilterItem callback doesn't include any dependencies, it won't receive updated values if the filters change. While the empty dependency array prevents re-creation of the function (which is good), you should verify that DropdownButton and its PopoverComponent prop don't need to react to changes in the parent component's state.
Suggested fix: Verify that the empty dependency array is intentional and that DropdownButton properly handles updates to item.PopoverComponent. If the callback needs to react to changes in props or state, add the appropriate dependencies:
const renderFilterItem = useCallback(
({item}: {item: FilterItem}) => (
<DropdownButton
label={item.label}
value={item.value}
PopoverComponent={item.PopoverComponent}
/>
),
[/* add dependencies if needed */],
);
| PopoverComponent={item.PopoverComponent} | ||
| /> | ||
| ), | ||
| [], |
Contributor
There was a problem hiding this comment.
❌ PERF-4 (docs)
The renderListFooter callback creates a new string concatenation on every render: translate('search.filtersHeader') + (hiddenSelectedFilters.length > 0 ? (${hiddenSelectedFilters.length}) : ''). This inline computation creates a new object reference that's passed to the Button component, potentially causing unnecessary re-renders.
Suggested fix: Compute the text value using useMemo and pass it to the callback, or include it in the dependency array to ensure proper memoization:
const filterButtonText = useMemo(
() => translate('search.filtersHeader') + (hiddenSelectedFilters.length > 0 ? ` (${hiddenSelectedFilters.length})` : ''),
[translate, hiddenSelectedFilters.length]
);
const renderListFooter = useCallback(
() => (
<Button
link
small
shouldUseDefaultHover={false}
text={filterButtonText}
iconFill={theme.link}
iconHoverFill={theme.linkHover}
icon={Expensicons.Filter}
textStyles={[styles.textMicroBold]}
onPress={openAdvancedFilters}
/>
),
[filterButtonText, theme.link, theme.linkHover, styles.textMicroBold, openAdvancedFilters],
);
joekaufmanexpensify
previously approved these changes
Nov 12, 2025
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
Contributor
There was a problem hiding this comment.
Good from a product perspective 👍
dukenv0307
reviewed
Nov 13, 2025
| // When the FlatList is scrolled to the end and the last item is deleted, a blank space is left behind. | ||
| // To fix this, we detect when onEndReached is triggered due to an item deletion, | ||
| // and programmatically scroll to the end to fill the space. | ||
| if (filters.length >= prevFiltersLength.current || !shouldAdjustScroll) { |
Contributor
There was a problem hiding this comment.
we shouldn't check filters.length >= prevFiltersLength.current because when we reset From, it won't be hidden
Contributor
|
@thelullabyy Can you please fix the issue above and resolve the conflicts? |
Contributor
Author
Thanks. I fixed them @dukenv0307 |
dukenv0307
reviewed
Nov 19, 2025
Comment on lines
+665
to
+666
| // Workaround for a known React Native bug on Android (https://github.com/facebook/react-native/issues/27504): | ||
| // When the FlatList is scrolled to the end and the last item is deleted, a blank space is left behind. |
Contributor
There was a problem hiding this comment.
Can you please adjust the comment? I think we don't need to detect for deleted item
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-11-20.at.09.40.32.movAndroid: mWeb ChromeScreen.Recording.2025-11-13.at.15.47.02.moviOS: HybridAppScreen.Recording.2025-11-13.at.16.57.25.moviOS: mWeb SafariScreen.Recording.2025-11-13.at.15.48.09.movMacOS: Chrome / SafariScreen.Recording.2025-11-13.at.15.45.22.movMacOS: DesktopScreen.Recording.2025-11-13.at.15.55.15.mov |
Contributor
|
The lint error is not related to this PR |
dukenv0307
approved these changes
Nov 20, 2025
Contributor
|
@thelullabyy Can you please merge main? |
…2/empty-space-issue
Contributor
Author
|
@dukenv0307 I merged main cc @nkuoch for further review |
nkuoch
approved these changes
Nov 25, 2025
Contributor
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
70 tasks
Contributor
|
🚀 Deployed to staging by https://github.com/nkuoch in version: 9.2.64-0 🚀
|
Contributor
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.2.64-5 🚀
|
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
Fixed Issues
$ #74582
PROPOSAL: #74582 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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.mov
Android: mWeb Chrome
web-android.mov
iOS: Native
ios.mov
iOS: mWeb Safari
web-ios.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov