Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/components/SelectionList/BaseSelectionList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ function BaseSelectionList<TItem extends ListItem>({
shouldUseUserSkeletonView,
shouldShowTooltips = true,
shouldIgnoreFocus = false,
shouldShowRightCaret = false,
shouldStopPropagation = false,
shouldHeaderBeInsideList = false,
shouldScrollToFocusedIndex = true,
Expand Down Expand Up @@ -375,6 +376,7 @@ function BaseSelectionList<TItem extends ListItem>({
shouldSyncFocus={!isTextInputFocusedRef.current && hasKeyBeenPressed.current}
shouldDisableHoverStyle={shouldDisableHoverStyle}
shouldStopMouseLeavePropagation={false}
shouldShowRightCaret={shouldShowRightCaret}
/>
</View>
);
Expand Down
14 changes: 14 additions & 0 deletions src/components/SelectionList/ListItem/BaseListItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import * as Expensicons from '@components/Icon/Expensicons';
import OfflineWithFeedback from '@components/OfflineWithFeedback';
import PressableWithFeedback from '@components/Pressable/PressableWithFeedback';
import useHover from '@hooks/useHover';
import {useMemoizedLazyExpensifyIcons} from '@hooks/useLazyAsset';
import {useMouseContext} from '@hooks/useMouseContext';
import useStyleUtils from '@hooks/useStyleUtils';
import useSyncFocus from '@hooks/useSyncFocus';
Expand Down Expand Up @@ -45,12 +46,14 @@ function BaseListItem<TItem extends ListItem>({
shouldHighlightSelectedItem = true,
shouldDisableHoverStyle,
shouldStopMouseLeavePropagation = true,
shouldShowRightCaret = false,
}: BaseListItemProps<TItem>) {
const theme = useTheme();
const styles = useThemeStyles();
const StyleUtils = useStyleUtils();
const {hovered, bind} = useHover();
const {isMouseDownOnInput, setMouseUp} = useMouseContext();
const icons = useMemoizedLazyExpensifyIcons(['ArrowRight']);

const pressableRef = useRef<View>(null);

Expand Down Expand Up @@ -167,6 +170,17 @@ function BaseListItem<TItem extends ListItem>({
)}

{rightHandSideComponentRender()}
{shouldShowRightCaret && (
<View style={[styles.justifyContentCenter, styles.alignItemsCenter, styles.ml2]}>
<Icon
src={icons.ArrowRight}
fill={theme.icon}
additionalStyles={[styles.alignSelfCenter, !hovered && styles.opacitySemiTransparent]}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ PERF-4 (docs)

Creating a new array inline [styles.alignSelfCenter, !hovered && styles.opacitySemiTransparent] breaks memoization of the Icon component (which is optimized), causing unnecessary re-renders.

Suggested fix: Memoize the array using useMemo:

const arrowIconStyles = useMemo(
    () => [styles.alignSelfCenter, !hovered && styles.opacitySemiTransparent],
    [styles.alignSelfCenter, styles.opacitySemiTransparent, hovered]
);

// Then use:
<Icon
    src={icons.ArrowRight}
    fill={theme.icon}
    additionalStyles={arrowIconStyles}
    isButtonIcon
    medium
/>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zfurtak thoughts on this comment?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, now we have a react-compiler-check that doesn't allow manual memoization, so this type of comment is outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know who worked on that? Can make an issue to address this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamgrzybowski is working on the AI reviewer, I let him know about the issue 👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future readers: we verified it and AI reviewer acted correctly here

isButtonIcon
medium
/>
</View>
)}
</View>
{FooterComponent}
</PressableWithFeedback>
Expand Down
2 changes: 2 additions & 0 deletions src/components/SelectionList/ListItem/ListItemRenderer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ function ListItemRenderer<TItem extends ListItem>({
shouldHighlightSelectedItem,
shouldDisableHoverStyle,
shouldStopMouseLeavePropagation,
shouldShowRightCaret,
}: ListItemRendererProps<TItem>) {
const handleOnCheckboxPress = () => {
if (isTransactionGroupListItemType(item)) {
Expand Down Expand Up @@ -102,6 +103,7 @@ function ListItemRenderer<TItem extends ListItem>({
shouldHighlightSelectedItem={shouldHighlightSelectedItem}
shouldDisableHoverStyle={shouldDisableHoverStyle}
shouldStopMouseLeavePropagation={shouldStopMouseLeavePropagation}
shouldShowRightCaret={shouldShowRightCaret}
/>
{item.footerContent && item.footerContent}
</>
Expand Down
Loading