feat: migrate FlatList to FlashList for rooms lists#6995
Open
deepak0x wants to merge 4 commits intoRocketChat:developfrom
Open
feat: migrate FlatList to FlashList for rooms lists#6995deepak0x wants to merge 4 commits intoRocketChat:developfrom
deepak0x wants to merge 4 commits intoRocketChat:developfrom
Conversation
- Add @shopify/flash-list dependency - Replace FlatList with FlashList in RoomsListView - Replace Animated.FlatList with AnimatedFlashList in RoomView message list - Update list ref/types to FlashListRef and FlashListProps - Preserve a11y props and add List.test.tsx for accessibility parity - Adjust scroll/viewability usage for FlashList API
- Replace FlatList with FlashList in RoomsListView for better view recycling on Android
- Wrap renderItem and onPressItem in useCallback to prevent unnecessary re-renders
- Remove getItemLayout, removeClippedSubviews, initialNumToRender, windowSize (unneeded with FlashList recycler)
- Add drawDistance={300} to pre-render cells beyond the viewport
- Revert RoomView message list to Animated.FlatList; FlashList v2 lacks inverted prop support required by the chat list
- Add accessibility regression tests for RoomView List component
- Add FLASHLIST_MIGRATION.md with Android performance measurement guide
Contributor
WalkthroughReplaces RoomsListView's FlatList with FlashList, adds accessibility tests for RoomView List, adds the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/FLASHLIST_MIGRATION.md (1)
5-5: Clarify the FlashList v2invertedprop status — it's deprecated, not absent.The v2 migration guide states the
invertedprop is deprecated; the alternative ismaintainVisibleContentPosition. Saying it is "not supported" may lead future contributors to believe a RoomView migration is impossible, when in fact there is a documented migration path. Consider rewording to: "…because the migration ofinverted(deprecated in v2) andonScrollToIndexFailedto the v2 equivalents has not yet been done."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/FLASHLIST_MIGRATION.md` at line 5, Update the sentence describing RoomView's use of Animated.FlatList to clarify that FlashList v2 deprecated the inverted prop (not removed) and offers maintainVisibleContentPosition as the alternative; reword the line referencing RoomView/Animated.FlatList to say the migration of inverted (deprecated in v2) and onScrollToIndexFailed to their v2 equivalents (e.g., maintainVisibleContentPosition and appropriate failure-handling patterns) has not yet been done, mentioning RoomView, Animated.FlatList, inverted, maintainVisibleContentPosition, and onScrollToIndexFailed so future contributors see the documented migration path.app/views/RoomsListView/index.tsx (1)
136-136:extraDatamirrorsdata— it is effectively a no-op here.
extraDatais meant to carry state that affects rendering but is not indata(e.g.,subscribedRoom, theme). Passing the same array reference asdataprovides no additional signal to FlashList. SincerenderItemis already memoized withsubscribedRoomin its dependency array, highlight-on-focus updates are handled correctly through therenderItemreference change. TheextraDataline can either be removed or updated to carry the independent state that drives re-renders.♻️ Proposed simplification
- extraData={searchEnabled ? searchResults : subscriptions} + extraData={[subscribedRoom, displayMode, showAvatar]}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/RoomsListView/index.tsx` at line 136, extraData is redundant because it receives the same array as data (extraData={searchEnabled ? searchResults : subscriptions}), so remove or change it to a true independent signal; update the RoomsListView FlashList props by either deleting the extraData prop entirely or set extraData to an independent piece of state (e.g., subscribedRoom or theme) that actually affects rendering, and ensure renderItem (which already depends on subscribedRoom) remains memoized so highlights still update correctly.app/views/RoomView/List/components/List.test.tsx (1)
25-26: ReplaceUNSAFE_getByPropswith the stablegetByTestIdquery.Since the target already has
testID='room-view-messages',getByTestIdis the idiomatic, non-fragile approach. TheUNSAFE_prefix signals the query relies on internal component structure and may break across library upgrades.♻️ Proposed refactor
it('hides message list from accessibility tree while autocomplete is visible', () => { - const { UNSAFE_getByProps } = renderList(true); - const list = UNSAFE_getByProps({ testID: 'room-view-messages' }); + const { getByTestId } = renderList(true); + const list = getByTestId('room-view-messages'); expect(list.props.accessibilityElementsHidden).toBe(true); expect(list.props.importantForAccessibility).toBe('no-hide-descendants'); }); it('keeps message list visible to accessibility tree while autocomplete is hidden', () => { - const { UNSAFE_getByProps } = renderList(false); - const list = UNSAFE_getByProps({ testID: 'room-view-messages' }); + const { getByTestId } = renderList(false); + const list = getByTestId('room-view-messages'); expect(list.props.accessibilityElementsHidden).toBe(false); expect(list.props.importantForAccessibility).toBe('yes'); });Also applies to: 33-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/RoomView/List/components/List.test.tsx` around lines 25 - 26, Replace the fragile UNSAFE_getByProps usage with the stable getByTestId query: in List.test.tsx where you call renderList(true) and destructure UNSAFE_getByProps to locate the element with testID 'room-view-messages', switch to using getByTestId('room-view-messages') (and similarly replace the other occurrence around the second case on lines 33-34) so the test queries by the public testID prop instead of internal props; update any destructuring from renderList to include getByTestId and remove UNSAFE_getByProps references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/views/RoomsListView/index.tsx`:
- Around line 79-110: The FlashList render contains two distinct view types
(SectionHeader when item.separator is true and RoomItem otherwise) but no
getItemType is provided; add a getItemType prop to the FlashList that returns a
stable type string/value (e.g., item.separator ? 'section' : 'room') so
FlashList can maintain separate recycling pools and avoid recycled separators
being reused for RoomItem and vice versa; update both places where renderItem is
defined (the renderItem callback that returns SectionHeader/RoomItem and the
other similar renderer) and pass the new getItemType function into the FlashList
component instance.
---
Nitpick comments:
In `@app/views/RoomsListView/index.tsx`:
- Line 136: extraData is redundant because it receives the same array as data
(extraData={searchEnabled ? searchResults : subscriptions}), so remove or change
it to a true independent signal; update the RoomsListView FlashList props by
either deleting the extraData prop entirely or set extraData to an independent
piece of state (e.g., subscribedRoom or theme) that actually affects rendering,
and ensure renderItem (which already depends on subscribedRoom) remains memoized
so highlights still update correctly.
In `@app/views/RoomView/List/components/List.test.tsx`:
- Around line 25-26: Replace the fragile UNSAFE_getByProps usage with the stable
getByTestId query: in List.test.tsx where you call renderList(true) and
destructure UNSAFE_getByProps to locate the element with testID
'room-view-messages', switch to using getByTestId('room-view-messages') (and
similarly replace the other occurrence around the second case on lines 33-34) so
the test queries by the public testID prop instead of internal props; update any
destructuring from renderList to include getByTestId and remove
UNSAFE_getByProps references.
In `@docs/FLASHLIST_MIGRATION.md`:
- Line 5: Update the sentence describing RoomView's use of Animated.FlatList to
clarify that FlashList v2 deprecated the inverted prop (not removed) and offers
maintainVisibleContentPosition as the alternative; reword the line referencing
RoomView/Animated.FlatList to say the migration of inverted (deprecated in v2)
and onScrollToIndexFailed to their v2 equivalents (e.g.,
maintainVisibleContentPosition and appropriate failure-handling patterns) has
not yet been done, mentioning RoomView, Animated.FlatList, inverted,
maintainVisibleContentPosition, and onScrollToIndexFailed so future contributors
see the documented migration path.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
app/views/RoomView/List/components/List.test.tsxapp/views/RoomsListView/index.tsxdocs/FLASHLIST_MIGRATION.mdpackage.json
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-05T13:55:00.974Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6930
File: package.json:101-101
Timestamp: 2026-02-05T13:55:00.974Z
Learning: In this repository, the dependency on react-native-image-crop-picker should reference the RocketChat fork (RocketChat/react-native-image-crop-picker) with explicit commit pins, not the upstream ivpusic/react-native-image-crop-picker. Update package.json dependencies (and any lockfile) to point to the fork URL and a specific commit, ensuring edge-to-edge Android fixes are included. This pattern should apply to all package.json files in the repo that declare this dependency.
Applied to files:
package.json
🧬 Code graph analysis (1)
app/views/RoomView/List/components/List.test.tsx (2)
app/definitions/IMessage.ts (1)
TAnyMessageModel(185-185)app/views/RoomView/context.ts (1)
RoomContext(23-23)
🔇 Additional comments (1)
package.json (1)
54-54: No action needed — new architecture is already enabled.The project has
newArchEnabled=trueinandroid/gradle.propertiesand RN 0.79.4 defaults to new arch on iOS. FlashList v2.2.2 is fully compatible with this configuration.
Contributor
Author
|
hey @diegolmello Thanks |
Section headers and room items use distinct view types; getItemType ensures FlashList recycles them in separate pools to avoid layout glitches and incorrect renders when scrolling.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/views/RoomsListView/index.tsx (1)
82-82:getItemTypeis a stable pure function — hoist it to module level instead ofuseCallback.Since it only references
ITEM_TYPE_SECTIONandITEM_TYPE_ROOM(both module-level constants), wrapping it inuseCallbackwith[]is unnecessary overhead. A module-level function produces the same stable identity with less ceremony.♻️ Proposed refactor
-const ITEM_TYPE_SECTION = 'section'; -const ITEM_TYPE_ROOM = 'room'; +const ITEM_TYPE_SECTION = 'section'; +const ITEM_TYPE_ROOM = 'room'; + +const getItemType = (item: IRoomItem) => (item.separator ? ITEM_TYPE_SECTION : ITEM_TYPE_ROOM);Then remove the in-component declaration:
- const getItemType = useCallback((item: IRoomItem) => (item.separator ? ITEM_TYPE_SECTION : ITEM_TYPE_ROOM), []); -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/RoomsListView/index.tsx` at line 82, The getItemType function is a pure, stable function that doesn't use component state or props, so hoist it out of the component to module scope: create a top-level function named getItemType that returns ITEM_TYPE_SECTION when item.separator is truthy and ITEM_TYPE_ROOM otherwise (using the existing ITEM_TYPE_SECTION and ITEM_TYPE_ROOM constants), then remove the useCallback declaration and replace usages in the component to call the module-level getItemType directly; ensure there are no remaining imports or references to useCallback for this function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/views/RoomsListView/index.tsx`:
- Around line 138-152: The FlashList's extraData currently mirrors data and
omits subscribedRoom, so changes to subscribedRoom won't reliably re-render
RoomItem's isFocused; update the FlashList extraData to include subscribedRoom
(e.g., pass a tuple/object containing the current list and subscribedRoom) so
FlashList knows to re-render items when subscribedRoom changes, leaving
getItemType, renderItem, keyExtractor and other props intact.
---
Nitpick comments:
In `@app/views/RoomsListView/index.tsx`:
- Line 82: The getItemType function is a pure, stable function that doesn't use
component state or props, so hoist it out of the component to module scope:
create a top-level function named getItemType that returns ITEM_TYPE_SECTION
when item.separator is truthy and ITEM_TYPE_ROOM otherwise (using the existing
ITEM_TYPE_SECTION and ITEM_TYPE_ROOM constants), then remove the useCallback
declaration and replace usages in the component to call the module-level
getItemType directly; ensure there are no remaining imports or references to
useCallback for this function.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/views/RoomsListView/index.tsx
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (1)
app/views/RoomsListView/index.tsx (6)
app/containers/RoomItem/interfaces.ts (1)
IRoomItem(170-173)app/sagas/rooms.js (1)
subscribedRoom(62-62)app/lib/methods/helpers/log/index.ts (2)
logEvent(35-42)events(7-7)app/lib/methods/helpers/goRoom.ts (1)
goRoom(85-130)app/sagas/login.js (1)
isMasterDetail(54-54)app/lib/methods/helpers/helpers.ts (3)
getUidDirectMessage(17-41)getRoomTitle(43-60)getRoomAvatar(10-15)
🔇 Additional comments (1)
app/views/RoomsListView/index.tsx (1)
139-152: FlashList v2 configuration verified — new architecture is unconditionally enabled.The project has
newArchEnabled=trueset in Android's gradle.properties, and both product flavors (official and experimental) inherit this setting without overrides. iOS is configured with React Native 0.79.4 on platform 15.1, which enables new architecture by default. FlashList v2.2.2 will run without issues on supported devices.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/views/RoomsListView/index.tsx (1)
141-141:⚠️ Potential issue | 🟠 MajorStabilize
extraDatato avoid avoidable FlashList invalidations.Line 141 creates a new array literal on every render.
extraDatais used as a re-render marker in FlashList, so this triggers unnecessary row updates. Sincedataalready coverssubscriptions/searchResults, onlysubscribedRoom(used inrenderItemto determineisFocused) needs to be inextraData.♻️ Proposed fix
- extraData={[searchEnabled ? searchResults : subscriptions, subscribedRoom]} + extraData={subscribedRoom}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/RoomsListView/index.tsx` at line 141, The FlashList's extraData is currently recreated each render as an array literal (extraData={[searchEnabled ? searchResults : subscriptions, subscribedRoom]}), causing unnecessary invalidations; change it to only pass the stable value used by renderItem for focus checks (extraData={subscribedRoom}) or, if you truly need multiple markers, create a stable memoized value (useMemo) and pass that instead—update the extraData prop near the FlashList usage in RoomsListView (where searchEnabled, searchResults, subscriptions, and subscribedRoom are referenced) and ensure renderItem's isFocused logic still reads subscribedRoom.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/views/RoomsListView/index.tsx`:
- Line 141: The FlashList's extraData is currently recreated each render as an
array literal (extraData={[searchEnabled ? searchResults : subscriptions,
subscribedRoom]}), causing unnecessary invalidations; change it to only pass the
stable value used by renderItem for focus checks (extraData={subscribedRoom})
or, if you truly need multiple markers, create a stable memoized value (useMemo)
and pass that instead—update the extraData prop near the FlashList usage in
RoomsListView (where searchEnabled, searchResults, subscriptions, and
subscribedRoom are referenced) and ensure renderItem's isFocused logic still
reads subscribedRoom.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/views/RoomsListView/index.tsx
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (1)
app/views/RoomsListView/index.tsx (3)
app/containers/RoomItem/interfaces.ts (1)
IRoomItem(170-173)app/sagas/rooms.js (1)
subscribedRoom(62-62)app/lib/methods/helpers/goRoom.ts (1)
goRoom(85-130)
deepak0x
changed the title
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
1 participant
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.
Migrate the rooms list (
RoomsListView) fromFlatListto@shopify/flash-list(FlashList) for better scrolling performance on Android. FlashList recycles native views instead of creating/destroying them on scroll, reducing dropped frames and jank on long room lists.FlatListwithFlashList; removedgetItemLayout,removeClippedSubviews,initialNumToRender, andwindowSize(handled by FlashList’s recycler). WrappedrenderItemandonPressIteminuseCallbackso FlashList doesn’t re-render items unnecessarily. AddeddrawDistance={300}to pre-render cells beyond the viewport.Animated.FlatList. FlashList v2 does not support theinvertedprop oronScrollToIndexFailed, which the chat list relies on for scroll direction, jump-to-message, and maintaining position when new messages arrive. Reverting to FlatList there avoids scroll/order/FAB regressions.accessibilityElementsHidden/importantForAccessibility).docs/FLASHLIST_MIGRATION.mdwith steps to compare CPU/graphics performance on Android (e.g.adb dumpsys gfxinfo).Issue(s)
None — feature/improvement.
How to test or reproduce
List.test.tsx(RoomView List). With a screen reader, open a room and open/close the autocomplete; list should be hidden from the tree when autocomplete is visible.Screenshots
No UI changes; behavior and layout match previous implementation. Optional: before/after scroll FPS or
gfxinfosnippets if you captured them.Types of changes
Checklist
Summary by CodeRabbit
Tests
New Features
Documentation