replace FlatList with .map solution #53655
Merged
Conversation
This file contains 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
melvin-bot
bot
requested review from
DylanDylann
and removed request for
a team
|
@DylanDylann 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] |
Closed
8 tasks
|
@jacobkim9881 Let's do the same thing on DebugReportActions |
This comment was marked as resolved.
This comment was marked as resolved.
|
@DylanDylann done! |
|
Sorry, @jacobkim9881 I want to tag @JKobrynski |
youneshenniwrites
suggested changes
Dec 6, 2024
| /> | ||
| {/* This list was previously rendered as a FlatList, but it turned out that it caused the component to flash in some cases, | ||
| so it was replaced by this solution. */} | ||
| {sortedAllReportActions?.map((item, index) => renderItem(item, index))} |
There was a problem hiding this comment.
Switching to .map() isn't ideal since FlatList provides built-in virtualization and better performance for large lists.
The flashing issue you mentioned might be due to the missing keyExtractor prop in the FlatList.
I suggest re-adding FlatList with a keyExtractor like this to see if it resolves the issue:
<FlatList
data={sortedAllReportActions}
renderItem={renderItem}
keyExtractor={(item) => item.reportActionID.toString()}
scrollEnabled={false}
/>There was a problem hiding this comment.
I tried it before (forgot to mention it in the proposal) and unfortunately this doesn't fix it.
Here is a demo:
keyextractor-compressed.mov
|
@jacobkim9881 I still get flicker Screen.Recording.2024-12-09.at.17.20.13.mov |
|
@JKobrynski From my investigation, the bug comes from We update Onyx data and then navigate to a new page, but the navigation action is executed before completely updating ONYX. This is a common way to address this bug in other places |
|
@DylanDylann all right, I will try that solution. Thanks! |
|
@DylanDylann I'm back from my vacation, so I'll take it from here 😄 |
|
@DylanDylann I tried your fix but the issue still happens. I found a way to easily reproduce the issue by throttling the CPU on Google Chrome. Tomorrow, I'll investigate it further 😄 |
|
@DylanDylann I'm able to reproduce this issue on track/submit expense flow as well: Screen.Recording.2024-12-12.at.14.47.05.mp4I suspect this issue is caused by |
|
Last friday (12/13/24)
Today:
I just noticed that there as linter issues. I'll fix these earlier in the morning 😄 |
|
@DylanDylann sorry for the delay here, yesterday I thought I had addressed all eslint issues 😅 It's ready now for your review 😄 |
|
Thanks |
|
@pac-guerreiro Thanks, this is a great refactor |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-12-23.at.12.19.57.movAndroid: mWeb ChromeScreen.Recording.2024-12-23.at.12.13.52.moviOS: NativeScreen.Recording.2024-12-23.at.12.20.33.moviOS: mWeb SafariScreen.Recording.2024-12-23.at.12.14.35.movMacOS: Chrome / SafariScreen.Recording.2024-12-20.at.14.38.28.movMacOS: DesktopScreen.Recording.2024-12-20.at.14.50.16.mov |
|
@pac-guerreiro App crash on native Screen.Recording.2024-12-20.at.15.40.49.mov |
|
@DylanDylann I fixed the crash, thanks! 😄 |
|
I'll be away until January 2nd but someone should take care of my work 😄 Happy holidays! 🎉 |
blimpich
approved these changes
Dec 23, 2024
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
github-actions
bot
added
the
DeployBlockerCash
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
|
Hmm, I don't think this should be considered a deploy blocker since this should only be affecting internal debugging tools. We intentionally used a less performant method of rendering a list so that we could fix a bug. But maybe it has wider impacts. Asking internally: https://expensify.slack.com/archives/C03TQ48KC/p1734983936104459 And also asking in open source channel: https://expensify.slack.com/archives/C01GTK53T8Q/p1734984097996759 |
|
I need to be afk for the rest of the day, but I will check in on this tomorrow morning my time (PST). If we still think it might be a legit issue then we should probably just revert. For now I'll give time for other people to come online and take a look at this. |
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.0.78-0 🚀
|
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.0.78-0 🚀
|
2 similar comments
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.0.78-0 🚀
|
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.0.78-0 🚀
|
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.0.78-0 🚀
|
blimpich
removed
the
DeployBlockerCash
|
Removing the deploy blocker label because after running the performance tests again the linking actually took less time than the previous run. 1st performance regression run: 2nd performance regression run: Conversation ongoing here, but I think its safe to not block the deploy on this. |
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.0.78-0 🚀
|
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.0.78-0 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.0.78-6 🚀
|
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
FlatList was replaced with a data.map(() => {}) solution to get rid of the flashing effect.
Fixed Issues
$ #53027
PROPOSAL: #53027 (comment)
Tests
Offline tests
N/A
QA Steps
Same as Tests section above
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))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
MacOS: Chrome / Safari
noflatlist-compressed.mov