Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: V3 Split Logic #1089

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 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
248 changes: 248 additions & 0 deletions components/Chat/ChatDumb.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,248 @@
import { FlashList, ListRenderItem } from "@shopify/flash-list";
import {
backgroundColor,
itemSeparatorColor,
tertiaryBackgroundColor,
} from "@styles/colors";
import React, { useCallback, useEffect, useMemo, useRef } from "react";
import {
Dimensions,
FlatList,
Platform,
StyleSheet,
View,
useColorScheme,
} from "react-native";
import Animated, {
useAnimatedStyle,
useDerivedValue,
useSharedValue,
} from "react-native-reanimated";
import { useSafeAreaInsets } from "react-native-safe-area-context";

import ChatInput from "./Input/Input";
import TransactionInput from "./Transaction/TransactionInput";
import { ReanimatedFlashList, ReanimatedView } from "../../utils/animations";
import { useKeyboardAnimation } from "../../utils/animations/keyboardAnimation";
import { converseEventEmitter } from "../../utils/events";

type ChatDumbProps<T> = {
onReadyToFocus: () => void;
transactionMode: boolean;
frameTextInputFocused: boolean;
items: T[];
renderItem: ListRenderItem<T>;
keyExtractor: (item: T) => string;
showChatInput: boolean;
ListFooterComponent: React.JSX.Element | null;
showPlaceholder: boolean;
key?: string;
displayList: boolean;
refreshing: boolean;
getItemType: (
item: T,
index: number,
extraData?: any
) => string | number | undefined;
placeholderComponent: React.JSX.Element | null;
extraData?: any;
itemToId: (id: T) => string;
};
Comment on lines +29 to +50
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding common list interaction props.

The ChatDumbProps interface could benefit from additional props commonly needed in chat interfaces:

  • onRefresh?: () => void - Callback for pull-to-refresh
  • onEndReached?: () => void - Callback for pagination
  • error?: Error - Error state handling
  • onRetry?: () => void - Error retry callback
 type ChatDumbProps<T> = {
   onReadyToFocus: () => void;
   transactionMode: boolean;
   frameTextInputFocused: boolean;
   items: T[];
   renderItem: ListRenderItem<T>;
   keyExtractor: (item: T) => string;
   showChatInput: boolean;
   ListFooterComponent: React.JSX.Element | null;
   showPlaceholder: boolean;
   key?: string;
   displayList: boolean;
   refreshing: boolean;
+  onRefresh?: () => void;
+  onEndReached?: () => void;
+  error?: Error;
+  onRetry?: () => void;
   getItemType: (
     item: T,
     index: number,
     extraData?: any
   ) => string | number | undefined;
   placeholderComponent: React.JSX.Element | null;
   extraData?: any;
   itemToId: (id: T) => string;
 };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
type ChatDumbProps<T> = {
onReadyToFocus: () => void;
transactionMode: boolean;
frameTextInputFocused: boolean;
items: T[];
renderItem: ListRenderItem<T>;
keyExtractor: (item: T) => string;
showChatInput: boolean;
ListFooterComponent: React.JSX.Element | null;
showPlaceholder: boolean;
key?: string;
displayList: boolean;
refreshing: boolean;
getItemType: (
item: T,
index: number,
extraData?: any
) => string | number | undefined;
placeholderComponent: React.JSX.Element | null;
extraData?: any;
itemToId: (id: T) => string;
};
type ChatDumbProps<T> = {
onReadyToFocus: () => void;
transactionMode: boolean;
frameTextInputFocused: boolean;
items: T[];
renderItem: ListRenderItem<T>;
keyExtractor: (item: T) => string;
showChatInput: boolean;
ListFooterComponent: React.JSX.Element | null;
showPlaceholder: boolean;
key?: string;
displayList: boolean;
refreshing: boolean;
onRefresh?: () => void;
onEndReached?: () => void;
error?: Error;
onRetry?: () => void;
getItemType: (
item: T,
index: number,
extraData?: any
) => string | number | undefined;
placeholderComponent: React.JSX.Element | null;
extraData?: any;
itemToId: (id: T) => string;
};


const useStyles = () => {
const colorScheme = useColorScheme();
return useMemo(
() =>
StyleSheet.create({
chatContainer: {
flex: 1,
justifyContent: "flex-end",
backgroundColor: backgroundColor(colorScheme),
},
chatContent: {
backgroundColor: backgroundColor(colorScheme),
flex: 1,
},
chatPreviewContent: {
backgroundColor: backgroundColor(colorScheme),
flex: 1,
paddingBottom: 0,
},
chat: {
backgroundColor: backgroundColor(colorScheme),
},
inputBottomFiller: {
position: "absolute",
width: "100%",
bottom: 0,
backgroundColor: backgroundColor(colorScheme),
zIndex: 0,
},
inChatRecommendations: {
borderBottomWidth: 0.5,
borderBottomColor: itemSeparatorColor(colorScheme),
marginHorizontal: 20,
marginBottom: 10,
},
}),
[colorScheme]
);
};

export function ChatDumb<T>({
onReadyToFocus,
transactionMode,
frameTextInputFocused,
items,
renderItem,
keyExtractor,
showChatInput,
showPlaceholder,
key,
displayList,
refreshing,
ListFooterComponent,
getItemType,
placeholderComponent,
extraData,
itemToId,
}: ChatDumbProps<T>) {
const colorScheme = useColorScheme();
const styles = useStyles();

const hideInputIfFrameFocused = Platform.OS !== "web";
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve platform check robustness.

The platform check could be more explicit to handle web platforms consistently.

-const hideInputIfFrameFocused = Platform.OS !== "web";
+const hideInputIfFrameFocused = Platform.OS === "ios" || Platform.OS === "android";
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const hideInputIfFrameFocused = Platform.OS !== "web";
const hideInputIfFrameFocused = Platform.OS === "ios" || Platform.OS === "android";


const DEFAULT_INPUT_HEIGHT = 58;
const chatInputHeight = useSharedValue(0);
const chatInputDisplayedHeight = useDerivedValue(() => {
return frameTextInputFocused && hideInputIfFrameFocused
? 0
: chatInputHeight.value + DEFAULT_INPUT_HEIGHT;
});
Comment on lines +116 to +121
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider memoizing shared values.

The shared values and derived values should be memoized to prevent unnecessary recalculations during re-renders.

-const chatInputHeight = useSharedValue(0);
-const chatInputDisplayedHeight = useDerivedValue(() => {
+const chatInputHeight = useMemo(() => useSharedValue(0), []);
+const chatInputDisplayedHeight = useMemo(() => useDerivedValue(() => {
   return frameTextInputFocused && hideInputIfFrameFocused
     ? 0
     : chatInputHeight.value + DEFAULT_INPUT_HEIGHT;
-});
+}), [frameTextInputFocused, hideInputIfFrameFocused, chatInputHeight]);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const chatInputHeight = useSharedValue(0);
const chatInputDisplayedHeight = useDerivedValue(() => {
return frameTextInputFocused && hideInputIfFrameFocused
? 0
: chatInputHeight.value + DEFAULT_INPUT_HEIGHT;
});
const chatInputHeight = useMemo(() => useSharedValue(0), []);
const chatInputDisplayedHeight = useMemo(() => useDerivedValue(() => {
return frameTextInputFocused && hideInputIfFrameFocused
? 0
: chatInputHeight.value + DEFAULT_INPUT_HEIGHT;
}), [frameTextInputFocused, hideInputIfFrameFocused, chatInputHeight]);


const insets = useSafeAreaInsets();

const { height: keyboardHeight } = useKeyboardAnimation();
const tertiary = tertiaryBackgroundColor(colorScheme);
const textInputStyle = useAnimatedStyle(
() => ({
position: "absolute",
width: "100%",
backgroundColor: tertiary,
height: "auto",
zIndex: 1,
transform: [
{ translateY: -Math.max(insets.bottom, keyboardHeight.value) },
] as any,
}),
[keyboardHeight, colorScheme, insets.bottom]
);

const chatContentStyle = useAnimatedStyle(
() => ({
...styles.chatContent,
paddingBottom: showChatInput
? chatInputDisplayedHeight.value +
Math.max(insets.bottom, keyboardHeight.value)
: insets.bottom,
}),
[showChatInput, keyboardHeight, chatInputDisplayedHeight, insets.bottom]
);

const messageListRef = useRef<FlatList<T> | FlashList<T> | undefined>();

const scrollToMessage = useCallback(
(data: { messageId?: string; index?: number; animated?: boolean }) => {
let index = data.index;
if (index === undefined && data.messageId) {
index = items.findIndex((m) => itemToId(m) === data.messageId);
}
if (index !== undefined) {
messageListRef.current?.scrollToIndex({
index,
viewPosition: 0.5,
animated: !!data.animated,
});
}
},
[itemToId, items]
);

useEffect(() => {
converseEventEmitter.on("scrollChatToMessage", scrollToMessage);
return () => {
converseEventEmitter.off("scrollChatToMessage", scrollToMessage);
};
}, [scrollToMessage]);

const handleOnLayout = useCallback(() => {
setTimeout(() => {
onReadyToFocus();
}, 50);
}, [onReadyToFocus]);
Comment on lines +178 to +182
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Clean up setTimeout on unmount.

The setTimeout in handleOnLayout should be cleaned up to prevent potential memory leaks.

 const handleOnLayout = useCallback(() => {
-  setTimeout(() => {
-    onReadyToFocus();
-  }, 50);
+  const timer = setTimeout(() => {
+    onReadyToFocus();
+  }, 50);
+  return () => clearTimeout(timer);
 }, [onReadyToFocus]);

Committable suggestion was skipped due to low confidence.


return (
<View style={styles.chatContainer} key={key}>
<Animated.View style={chatContentStyle}>
{displayList && (
<ReanimatedFlashList
contentContainerStyle={styles.chat}
data={items}
refreshing={refreshing}
extraData={extraData}
renderItem={renderItem}
onLayout={handleOnLayout}
ref={(r) => {
// if (r) {
// messageListRef.current = r;
// }
}}
Comment on lines +196 to +199
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Uncomment and assign messageListRef to enable scrolling functionality

The messageListRef is currently not assigned because the code assigning it is commented out. This prevents the scrollToMessage function from working as intended, as messageListRef.current will be undefined.

Apply this diff to fix the issue:

196             ref={(r) => {
197-              // if (r) {
198-              //   messageListRef.current = r;
199-              // }
197+              if (r) {
198+                messageListRef.current = r;
199+              }
200             }}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// if (r) {
// messageListRef.current = r;
// }
}}
ref={(r) => {
if (r) {
messageListRef.current = r;
}
}}

keyboardDismissMode="interactive"
automaticallyAdjustContentInsets={false}
contentInsetAdjustmentBehavior="never"
// Causes a glitch on Android, no sure we need it for now
// maintainVisibleContentPosition={{
// minIndexForVisible: 0,
// autoscrollToTopThreshold: 100,
// }}
estimatedListSize={Dimensions.get("screen")}
inverted
keyExtractor={keyExtractor}
getItemType={getItemType}
keyboardShouldPersistTaps="handled"
estimatedItemSize={80}
// Size glitch on Android
showsVerticalScrollIndicator={Platform.OS === "ios"}
pointerEvents="auto"
ListFooterComponent={ListFooterComponent}
/>
)}
{showPlaceholder && placeholderComponent}
</Animated.View>
{showChatInput && (
<>
<ReanimatedView
style={[
textInputStyle,
{
display:
frameTextInputFocused && hideInputIfFrameFocused
? "none"
: "flex",
},
]}
>
{!transactionMode && <ChatInput inputHeight={chatInputHeight} />}
{transactionMode && <TransactionInput />}
</ReanimatedView>
<View
style={[
styles.inputBottomFiller,
{ height: insets.bottom + DEFAULT_INPUT_HEIGHT },
]}
/>
</>
)}
</View>
);
}
68 changes: 68 additions & 0 deletions components/Conversation/ConversationTitleDumb.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import { headerTitleStyle, textPrimaryColor } from "@styles/colors";
import {
Platform,
StyleSheet,
Text,
TouchableOpacity,
useColorScheme,
View,
} from "react-native";

import { getTitleFontScale } from "../../utils/str";

type ConversationTitleDumbProps = {
title?: string;
avatarComponent?: React.ReactNode;
onLongPress?: () => void;
onPress?: () => void;
};
Comment on lines +13 to +18
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider making title prop required and adding accessibility props.

The title prop should be required as it's essential for component rendering and accessibility. Additionally, the component should support accessibility properties.

Apply these changes to improve type safety and accessibility:

 type ConversationTitleDumbProps = {
-  title?: string;
+  title: string;
   avatarComponent?: React.ReactNode;
   onLongPress?: () => void;
   onPress?: () => void;
+  accessibilityLabel?: string;
+  accessibilityHint?: string;
 };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
type ConversationTitleDumbProps = {
title?: string;
avatarComponent?: React.ReactNode;
onLongPress?: () => void;
onPress?: () => void;
};
type ConversationTitleDumbProps = {
title: string;
avatarComponent?: React.ReactNode;
onLongPress?: () => void;
onPress?: () => void;
accessibilityLabel?: string;
accessibilityHint?: string;
};


export function ConversationTitleDumb({
avatarComponent,
title,
onLongPress,
onPress,
}: ConversationTitleDumbProps) {
const styles = useStyles();

return (
<View style={styles.container}>
<TouchableOpacity
onLongPress={onLongPress}
onPress={onPress}
style={styles.touchableContainer}
>
{avatarComponent}
<Text style={styles.title} numberOfLines={1} allowFontScaling={false}>
{title}
</Text>
</TouchableOpacity>
</View>
);
}
Comment on lines +20 to +42
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance component implementation with accessibility and optimization.

The component could benefit from performance optimization and better accessibility support.

Consider applying these improvements:

+import React, { memo } from 'react';

-export function ConversationTitleDumb({
+export const ConversationTitleDumb = memo(function ConversationTitleDumb({
   avatarComponent,
   title,
   onLongPress,
   onPress,
+  accessibilityLabel,
+  accessibilityHint,
 }: ConversationTitleDumbProps) {
   const styles = useStyles();
+  const titleText = title || '';
 
   return (
     <View style={styles.container}>
       <TouchableOpacity
         onLongPress={onLongPress}
         onPress={onPress}
         style={styles.touchableContainer}
+        accessibilityLabel={accessibilityLabel || `Conversation: ${titleText}`}
+        accessibilityHint={accessibilityHint || 'Double tap to open conversation'}
+        accessibilityRole="button"
       >
         {avatarComponent}
         <Text style={styles.title} numberOfLines={1} allowFontScaling={false}>
-          {title}
+          {titleText}
         </Text>
       </TouchableOpacity>
     </View>
   );
-}
+});
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export function ConversationTitleDumb({
avatarComponent,
title,
onLongPress,
onPress,
}: ConversationTitleDumbProps) {
const styles = useStyles();
return (
<View style={styles.container}>
<TouchableOpacity
onLongPress={onLongPress}
onPress={onPress}
style={styles.touchableContainer}
>
{avatarComponent}
<Text style={styles.title} numberOfLines={1} allowFontScaling={false}>
{title}
</Text>
</TouchableOpacity>
</View>
);
}
import React, { memo } from 'react';
export const ConversationTitleDumb = memo(function ConversationTitleDumb({
avatarComponent,
title,
onLongPress,
onPress,
accessibilityLabel,
accessibilityHint,
}: ConversationTitleDumbProps) {
const styles = useStyles();
const titleText = title || '';
return (
<View style={styles.container}>
<TouchableOpacity
onLongPress={onLongPress}
onPress={onPress}
style={styles.touchableContainer}
accessibilityLabel={accessibilityLabel || `Conversation: ${titleText}`}
accessibilityHint={accessibilityHint || 'Double tap to open conversation'}
accessibilityRole="button"
>
{avatarComponent}
<Text style={styles.title} numberOfLines={1} allowFontScaling={false}>
{titleText}
</Text>
</TouchableOpacity>
</View>
);
});


const useStyles = () => {
const colorScheme = useColorScheme();
return StyleSheet.create({
avatar: {
marginRight: Platform.OS === "android" ? 24 : 7,
marginLeft: Platform.OS === "ios" ? 0 : -9,
},
container: { flexDirection: "row", flexGrow: 1 },
touchableContainer: {
flexDirection: "row",
justifyContent: "flex-start",
left: Platform.OS === "android" ? -36 : 0,
width: "100%",
alignItems: "center",
paddingRight: 40,
},
title: {
color: textPrimaryColor(colorScheme),
fontSize:
Platform.OS === "ios"
? 16 * getTitleFontScale()
: headerTitleStyle(colorScheme).fontSize,
},
});
};
Comment on lines +44 to +68
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve style maintainability and platform-specific handling.

The current styling implementation uses magic numbers and platform-specific values that could be better organized.

Consider extracting platform-specific values into constants and improving the structure:

+const PLATFORM_CONSTANTS = {
+  ios: {
+    avatarMarginRight: 7,
+    avatarMarginLeft: 0,
+    containerLeft: 0,
+  },
+  android: {
+    avatarMarginRight: 24,
+    avatarMarginLeft: -9,
+    containerLeft: -36,
+  },
+};

 const useStyles = () => {
   const colorScheme = useColorScheme();
+  const platform = Platform.OS;
+  const constants = PLATFORM_CONSTANTS[platform];
+
   return StyleSheet.create({
     avatar: {
-      marginRight: Platform.OS === "android" ? 24 : 7,
-      marginLeft: Platform.OS === "ios" ? 0 : -9,
+      marginRight: constants.avatarMarginRight,
+      marginLeft: constants.avatarMarginLeft,
     },
     container: { flexDirection: "row", flexGrow: 1 },
     touchableContainer: {
       flexDirection: "row",
       justifyContent: "flex-start",
-      left: Platform.OS === "android" ? -36 : 0,
+      left: constants.containerLeft,
       width: "100%",
       alignItems: "center",
       paddingRight: 40,
     },
     title: {
       color: textPrimaryColor(colorScheme),
-      fontSize:
-        Platform.OS === "ios"
-          ? 16 * getTitleFontScale()
-          : headerTitleStyle(colorScheme).fontSize,
+      fontSize: platform === "ios" 
+        ? 16 * getTitleFontScale()
+        : headerTitleStyle(colorScheme).fontSize,
     },
   });
 };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const useStyles = () => {
const colorScheme = useColorScheme();
return StyleSheet.create({
avatar: {
marginRight: Platform.OS === "android" ? 24 : 7,
marginLeft: Platform.OS === "ios" ? 0 : -9,
},
container: { flexDirection: "row", flexGrow: 1 },
touchableContainer: {
flexDirection: "row",
justifyContent: "flex-start",
left: Platform.OS === "android" ? -36 : 0,
width: "100%",
alignItems: "center",
paddingRight: 40,
},
title: {
color: textPrimaryColor(colorScheme),
fontSize:
Platform.OS === "ios"
? 16 * getTitleFontScale()
: headerTitleStyle(colorScheme).fontSize,
},
});
};
const PLATFORM_CONSTANTS = {
ios: {
avatarMarginRight: 7,
avatarMarginLeft: 0,
containerLeft: 0,
},
android: {
avatarMarginRight: 24,
avatarMarginLeft: -9,
containerLeft: -36,
},
};
const useStyles = () => {
const colorScheme = useColorScheme();
const platform = Platform.OS;
const constants = PLATFORM_CONSTANTS[platform];
return StyleSheet.create({
avatar: {
marginRight: constants.avatarMarginRight,
marginLeft: constants.avatarMarginLeft,
},
container: { flexDirection: "row", flexGrow: 1 },
touchableContainer: {
flexDirection: "row",
justifyContent: "flex-start",
left: constants.containerLeft,
width: "100%",
alignItems: "center",
paddingRight: 40,
},
title: {
color: textPrimaryColor(colorScheme),
fontSize: platform === "ios"
? 16 * getTitleFontScale()
: headerTitleStyle(colorScheme).fontSize,
},
});
};

Loading
Loading