Chat Composer Glow Up / Changed Functionality #1513
Conversation
WalkthroughThis pull request introduces significant changes to the application's conversation and navigation flow. The modifications include removing several components related to recommendations, conversation initiation, and onboarding, while introducing new components for search and chat functionality. The changes streamline the user experience by simplifying navigation, updating terminology from "conversation" to "chat," and enhancing search and group invitation processes. Changes
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
this remains the old screen to handle adding members to an existing group
There was a problem hiding this comment.
going to clean this up, just wanted to verify first pass the scope is correct
Performance Comparison ReportSignificant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
Render Count ChangesThere are no entries Render IssuesThere are no entries Added ScenariosThere are no entries Removed ScenariosThere are no entries |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (1)
screens/NewConversation/NewConversation.tsx (1)
Line range hint
170-349: Include 'pendingChatMembers.members' inuseEffectdependenciesIn the
useEffecthook starting at line 170,pendingChatMembers.membersis used but not included in the dependency array. This may cause issues ifpendingChatMembers.memberschanges but the effect doesn't rerun.Apply this diff to fix the dependency array:
- }, [searchQueryState]); + }, [searchQueryState, pendingChatMembers.members]);
🧹 Nitpick comments (11)
features/groups/invite-to-group/invite-users-to-exisiting-group.screen.tsx (2)
358-359: Remove placeholder text from the UIThe
<Text>components displaying "at the end of the road" appear to be placeholders or debug statements. These should be removed to clean up the UI and prevent confusion for users.Apply this diff to remove the unnecessary text:
- <Text> at the end of the road</Text>This applies to lines 358-359, 387-388, and 395-396.
Also applies to: 387-388, 395-396
91-95: Improve error handling and user feedbackWhen an error occurs during the member addition process, a generic alert "An error occured" is displayed. Consider providing a more user-friendly and informative error message to enhance the user experience.
For example:
- Alert.alert("An error occured"); + Alert.alert("Failed to add members", "Please try again later.");screens/NewConversation/NewConversation.tsx (2)
61-61: Remove unnecessary empty object pattern in function parametersThe
NewConversationfunction destructures an empty object{}as its parameter, which is unnecessary. Removing it simplifies the function signature.Apply this diff to fix the issue:
-export default function NewConversation({}) { +export default function NewConversation() {🧰 Tools
🪛 Biome (1.9.4)
[error] 61-61: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
499-502: Remove hardcoded 'inputValue' in 'ConversationComposerStoreProvider'The
inputValueprop is set to"testing", which seems unintended for production code. Consider removing it or setting it to an appropriate default value.Apply this diff to address the issue:
- inputValue={"testing"} + inputValue={""}screens/Navigation/NewConversationNav.tsx (1)
13-16: Avoid using an empty object type in TypeScriptThe
NewChatNavParamstype is currently an empty object{}, which is not useful and can lead to confusion. If no parameters are needed, consider usingRecord<string, never>or removing the type altogether.Apply this diff to fix the issue:
-export type NewChatNavParams = { - // peer?: string; - // addingToGroupTopic?: ConversationTopic; -}; +export type NewChatNavParams = Record<string, never>;Or remove the type if it's unnecessary.
🧰 Tools
🪛 Biome (1.9.4)
[error] 13-16: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
features/groups/invite-to-group/InviteUsersToExistingGroup.nav.tsx (1)
20-37: Consider adding error boundaries for navigation.While the implementation is solid, consider adding error boundaries to handle navigation failures gracefully, especially since this is part of a new chat creation flow.
Example implementation:
export function InviteUsersToExistingGroupNav() { const colorScheme = useColorScheme(); + const [error, setError] = React.useState<Error | null>(null); + + if (error) { + return ( + <View style={{ padding: 16 }}> + <Text>Failed to load invite screen: {error.message}</Text> + </View> + ); + } + return ( <NativeStack.Screen name="InviteUsersToExistingGroup" component={InviteUsersToExistingGroupScreen} + listeners={{ + focus: () => setError(null), + }} options={({ route }) => ({screens/NewConversation/NewConversationModal.tsx (1)
38-41: Consider environment-based logging.While debug logging is valuable, consider making it conditional based on the environment to avoid unnecessary logging in production.
+const isDevelopment = process.env.NODE_ENV === 'development'; + +const debugLog = (message: string, data?: any) => { + if (isDevelopment) { + logger.debug(message, data); + } +}; -logger.debug("[NewConversationModal] Rendering with params:", { +debugLog("[NewConversationModal] Rendering with params:", { addingToGroupTopic: route.params?.addingToGroupTopic, peer: route.params?.peer });Also applies to: 63-65, 79-80
🧰 Tools
🪛 GitHub Check: tsc
[failure] 39-39:
Property 'addingToGroupTopic' does not exist on type 'Readonly'.
[failure] 40-40:
Property 'peer' does not exist on type 'Readonly'.features/search/components/ProfileSearchResultsList.tsx (1)
46-71: Consider removing or documenting commented code.Large blocks of commented code (header/footer components) make the file harder to maintain. Consider either:
- Removing if no longer needed
- Adding a comment explaining why they're preserved
- Moving to a separate file if they'll be used later
containers/GroupScreenAddition.tsx (1)
62-74: Enhance admin status calculation logs.Consider adding the calculated values to the debug log to make debugging admin-related issues easier.
logger.debug("[GroupScreenAddition] Calculating admin status", { members, currentAccount, + isAdmin: getAddressIsAdmin(members, currentAccount), + isSuperAdmin: getAddressIsSuperAdmin(members, currentAccount), });i18n/translations/fr.ts (1)
346-351: LGTM! The translations align with the new chat terminology.The translations are consistent with the PR's objective of transitioning from "conversation" to "chat" terminology.
Consider adding a translation for a back button tooltip/aria-label to improve accessibility.
utils/xmtpRN/conversations.ts (1)
Line range hint
140-173: Improved error handling in findDm function.The addition of the
throwIfNotFoundparameter with a default value oftruemaintains backward compatibility while allowing more flexible error handling.Consider adding JSDoc comments to document the new parameter and its behavior:
/** * Finds a DM conversation with a peer * @param args.throwIfNotFound - When true, throws if DM is not found. When false, returns undefined. * Defaults to true for backward compatibility. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ios/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
components/Onboarding/WarpcastConnect.tsx(0 hunks)components/Recommendations/Recommendation.tsx(0 hunks)components/Recommendations/Recommendations.tsx(0 hunks)containers/GroupScreenAddition.tsx(5 hunks)features/conversation-list/conversation-list.screen.tsx(3 hunks)features/conversation/conversation-composer/conversation-composer.tsx(2 hunks)features/conversation/hooks/use-send-message.ts(1 hunks)features/groups/invite-to-group/InviteUsersToExistingGroup.nav.tsx(1 hunks)features/groups/invite-to-group/invite-users-to-exisiting-group.screen.tsx(1 hunks)features/new-chat/readme.md(1 hunks)features/search/components/NavigationChatButton.tsx(0 hunks)features/search/components/ProfileSearchItem.tsx(2 hunks)features/search/components/ProfileSearchResultsList.tsx(1 hunks)features/search/components/SearchBar.tsx(1 hunks)features/search/screens/ProfileSearch.tsx(0 hunks)i18n/translations/en.ts(1 hunks)i18n/translations/fr.ts(1 hunks)queries/useDmQuery.ts(2 hunks)screens/Main.tsx(2 hunks)screens/Navigation/ConverseMatchMakerNav.tsx(0 hunks)screens/Navigation/Navigation.tsx(5 hunks)screens/Navigation/NewConversationNav.tsx(2 hunks)screens/NewConversation/NewConversation.tsx(9 hunks)screens/NewConversation/NewConversationModal.tsx(4 hunks)screens/NewConversation/NewGroupSummary.tsx(2 hunks)utils/addressBook.ts(0 hunks)utils/recommendations.ts(0 hunks)utils/xmtpRN/conversations.ts(6 hunks)
💤 Files with no reviewable changes (8)
- screens/Navigation/ConverseMatchMakerNav.tsx
- components/Recommendations/Recommendation.tsx
- components/Onboarding/WarpcastConnect.tsx
- utils/recommendations.ts
- components/Recommendations/Recommendations.tsx
- features/search/screens/ProfileSearch.tsx
- features/search/components/NavigationChatButton.tsx
- utils/addressBook.ts
✅ Files skipped from review due to trivial changes (2)
- features/conversation/hooks/use-send-message.ts
- features/conversation/conversation-composer/conversation-composer.tsx
🧰 Additional context used
🪛 eslint
features/conversation-list/conversation-list.screen.tsx
[error] 91-91: React Hook useEffect has a missing dependency: 'navigation'. Either include it or remove the dependency array.
(react-hooks/exhaustive-deps)
features/groups/invite-to-group/invite-users-to-exisiting-group.screen.tsx
[error] 101-101: React Hook useCallback has an unnecessary dependency: 'route.params.addingToGroupTopic'. Either exclude it or remove the dependency array.
(react-hooks/exhaustive-deps)
screens/NewConversation/NewConversation.tsx
[error] 84-84: React Hook useEffect has a missing dependency: 'pendingChatMembers.members'. Either include it or remove the dependency array.
(react-hooks/exhaustive-deps)
[error] 107-107: React Hook useCallback has missing dependencies: 'conversationCreationMode' and 'pendingChatMembers'. Either include them or remove the dependency array.
(react-hooks/exhaustive-deps)
[error] 349-349: React Hook useEffect has a missing dependency: 'pendingChatMembers.members'. Either include it or remove the dependency array.
(react-hooks/exhaustive-deps)
🪛 GitHub Check: lint
features/conversation-list/conversation-list.screen.tsx
[failure] 91-91:
React Hook useEffect has a missing dependency: 'navigation'. Either include it or remove the dependency array
features/groups/invite-to-group/invite-users-to-exisiting-group.screen.tsx
[failure] 101-101:
React Hook useCallback has an unnecessary dependency: 'route.params.addingToGroupTopic'. Either exclude it or remove the dependency array
screens/NewConversation/NewConversation.tsx
[failure] 84-84:
React Hook useEffect has a missing dependency: 'pendingChatMembers.members'. Either include it or remove the dependency array
[failure] 107-107:
React Hook useCallback has missing dependencies: 'conversationCreationMode' and 'pendingChatMembers'. Either include them or remove the dependency array
[failure] 349-349:
React Hook useEffect has a missing dependency: 'pendingChatMembers.members'. Either include it or remove the dependency array
🪛 GitHub Check: tsc
features/groups/invite-to-group/invite-users-to-exisiting-group.screen.tsx
[failure] 401-401:
Type '{ navigation: NativeStackNavigationProp<NavigationParamList, "InviteUsersToExistingGroup", undefined>; profiles: { [x: string]: IProfileSocials; }; groupMode: boolean; addToGroup: (member: any) => Promise<...>; }' is not assignable to type 'IntrinsicAttributes & ProfileSearchProps'.
[failure] 419-419:
Parameter 'member' implicitly has an 'any' type.
screens/NewConversation/NewConversationModal.tsx
[failure] 39-39:
Property 'addingToGroupTopic' does not exist on type 'Readonly'.
[failure] 40-40:
Property 'peer' does not exist on type 'Readonly'.
screens/NewConversation/NewConversation.tsx
[failure] 540-540:
Property 'replace' does not exist on type 'Omit<NavigationProp, "getState"> & { getState(): Readonly<{ key: string; index: number; routeNames: string[]; history?: unknown[] | undefined; routes: NavigationRoute<...>[]; type: string; stale: false; }> | undefined; }'.
[failure] 563-563:
Property 'replace' does not exist on type 'Omit<NavigationProp, "getState"> & { getState(): Readonly<{ key: string; index: number; routeNames: string[]; history?: unknown[] | undefined; routes: NavigationRoute<...>[]; type: string; stale: false; }> | undefined; }'.
🪛 Biome (1.9.4)
screens/Navigation/NewConversationNav.tsx
[error] 13-16: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
screens/NewConversation/NewConversation.tsx
[error] 61-61: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
🪛 LanguageTool
features/new-chat/readme.md
[style] ~1-~1: Consider shortening or rephrasing this to strengthen your wording.
Context: going to make changes to existing new conversation flow/new grou...
(MAKE_CHANGES)
[uncategorized] ~1-~1: You might be missing the article “the” here.
Context: going to make changes to existing new conversation flow/new grouip screen...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~3-~3: Did you mean “I”?
Context: ...rsation flow/new grouip screen and then i'm goiung to move those files here and r...
(I_LOWERCASE_PREMIUM)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: performance-test
- GitHub Check: build
🔇 Additional comments (18)
features/groups/invite-to-group/InviteUsersToExistingGroup.nav.tsx (1)
16-18: LGTM! Well-defined type for navigation params.The
InviteUsersToExistingGroupParamstype is clearly defined and properly typed withConversationTopic.features/search/components/SearchBar.tsx (1)
18-18: LGTM! Named export improves maintainability.The change from default export to named export is a good practice as it makes imports more explicit and refactoring easier.
features/search/components/ProfileSearchItem.tsx (2)
18-21: LGTM! Props refactoring improves component reusability.The removal of navigation props in favor of a callback makes the component more reusable and decoupled from navigation logic.
50-50: Address TODO comment about socials parameter.The TODO comment suggests that only the address might be needed. Consider reviewing if the entire socials object is necessary for the callback.
features/search/components/ProfileSearchResultsList.tsx (1)
75-84: Review commented FlatList props.Several FlatList props are commented out:
ListHeaderComponentListFooterComponentonTouchStartshowsVerticalScrollIndicatorConsider documenting why these props are preserved or remove them if not needed.
screens/Main.tsx (1)
32-32: Verify consistent terminology change across the codebase.The change from "conversation" to "chat" terminology should be consistent across the entire application.
Also applies to: 52-52
✅ Verification successful
UI terminology change is consistent with existing patterns
The change from "conversation" to "chat" in UI elements follows an established pattern where user-facing components use "chat" terminology while core functionality maintains "conversation". This provides a more familiar experience for users while preserving technical precision in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for mixed usage of "conversation" and "chat" terminology echo "Checking for 'conversation' usage:" rg -i "conversation" --type ts --type tsx echo "Checking for 'chat' usage:" rg -i "chat" --type ts --type tsxLength of output: 285
Script:
#!/bin/bash # Search for both terms without file type restrictions echo "Files containing 'conversation':" rg -l -i "conversation" echo -e "\nFiles containing 'chat':" rg -l -i "chat" # Get some context around the matches to verify the usage echo -e "\nContext for 'conversation' usage:" rg -i "conversation" -B 1 -A 1 echo -e "\nContext for 'chat' usage:" rg -i "chat" -B 1 -A 1Length of output: 66191
containers/GroupScreenAddition.tsx (2)
96-97: LGTM! Navigation change aligns with new terminology.The navigation target change from "NewConversation" to "InviteUsersToExistingGroup" better describes the action being performed.
111-114: LGTM! Comprehensive logging implementation.The logging implementation provides good observability for debugging group-related operations:
- Creation attempts
- Successful creations with IDs
- Failures with error details
- Successful deletions with IDs
Also applies to: 122-124, 132-132, 156-158
screens/Navigation/Navigation.tsx (4)
35-38: LGTM! New import for group invitation functionality.The addition of
InviteUsersToExistingGroupNavand its params enhances the navigation capabilities for group management.
41-41: Update import to align with new chat terminology.The import change from
NewConversationNavParamstoNewChatNavParamsmaintains consistency with the new chat-focused terminology.
82-82: LGTM! Navigation params updated for consistency.The
NavigationParamListupdates align with the new chat terminology and add support for group invitations.Also applies to: 90-90
146-146: LGTM! New navigation route added.The addition of
InviteUsersToExistingGroupNav()to the navigation stack enhances group management capabilities.screens/NewConversation/NewGroupSummary.tsx (2)
41-44: LGTM! Improved permission policy management.The addition of
defaultPermissionPolicySetcentralizes permission policy definitions, making them easier to maintain and update.
58-58: LGTM! Using default permission policy set.State initialization now uses the centralized
defaultPermissionPolicySetinstead of hardcoded values.features/conversation-list/conversation-list.screen.tsx (1)
51-51: LGTM! Added useEffect import.The import statement has been updated to include the
useEffecthook.i18n/translations/en.ts (1)
343-348: LGTM! Updated translations for consistency.The translations have been updated to use "chat" terminology consistently, aligning with the broader refactoring effort.
utils/xmtpRN/conversations.ts (2)
744-789: Well-structured group creation utilities with sensible defaults.The implementation of
createGroupWithDefaultsByAccountand its supporting functions provides a clean abstraction for group creation with reasonable default permissions.Let's verify the usage of these new group creation utilities:
✅ Verification successful
Group creation utilities are properly implemented and consistently used.
The codebase shows consistent usage of group creation functions with proper permission handling across all implementations. The default permission set is appropriately applied, and custom permissions are handled correctly where needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of the new group creation functions # Test: Search for direct usage of createGroup vs createGroupWithDefaultsByAccount rg -A 2 'createGroup(?:WithDefaultsByAccount|ByAccount)?\('Length of output: 1207
Script:
#!/bin/bash # Get implementation details of the group creation functions ast-grep --pattern 'function createGroup($$$) { $$$ }' ast-grep --pattern 'function createGroupByAccount($$$) { $$$ }' # Check for permission-related parameters in group creation rg -A 3 'permissionPolicySet|addMemberPolicy|removeMemberPolicy'Length of output: 9443
845-857: Added new utility function for optional conversation retrieval.The
getOptionalConversationByPeerByAccountfunction provides a non-throwing alternative for conversation retrieval, which is useful for checking conversation existence.Let's verify that the new function is being used appropriately:
✅ Verification successful
The optional conversation retrieval function is well-implemented and appropriately used.
The function provides a clear API for non-throwing conversation checks, complementing the existing throwing variant. Its usage in
NewConversation.tsxfollows the correct pattern for checking conversation existence during new conversation creation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage patterns of the new optional conversation retrieval rg -A 3 'getOptionalConversationByPeerByAccount'Length of output: 1089
Script:
#!/bin/bash # Find similar conversation retrieval functions and their implementations ast-grep --pattern 'export const getConversation$_($$$) { $$$ }' # Also check for the getConversationByPeer implementation ast-grep --pattern 'export const getConversationByPeer = $_($$$) { $$$ }'Length of output: 146
Script:
#!/bin/bash # Search for conversation-related functions with more context rg -A 10 "export const getConversation" utils/xmtpRN/conversations.ts # Also check for throwIfNotFound usage to understand error handling patterns rg -A 3 "throwIfNotFound" utils/xmtpRN/conversations.tsLength of output: 2146
| <ProfileSearchResultsList | ||
| // todo add back and rename profile search result list item component | ||
| navigation={navigation} | ||
| profiles={(() => { | ||
| const searchResultsToShow = { ...status.profileSearchResults }; | ||
| if (group.enabled && group.members) { | ||
| group.members.forEach((member) => { | ||
| delete searchResultsToShow[member.address]; | ||
| }); | ||
| } | ||
| if (members) { | ||
| members?.ids?.forEach((memberId: InboxId) => { | ||
| const member = members.byId[memberId]; | ||
| const address = getCleanAddress(member.addresses[0]); | ||
| delete searchResultsToShow[address]; | ||
| }); | ||
| } | ||
| return searchResultsToShow; | ||
| })()} | ||
| groupMode={group.enabled} | ||
| addToGroup={async (member) => { | ||
| setGroup((g) => ({ ...g, members: [...g.members, member] })); | ||
| setValue(""); | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Resolve type incompatibility with 'ProfileSearchResultsList' component
There is a type incompatibility when passing props to ProfileSearchResultsList. The props provided do not match the expected type ProfileSearchProps, leading to a TypeScript error.
Ensure that the ProfileSearchResultsList component accepts the props being passed—navigation, profiles, groupMode, and addToGroup—and that their types align with the component's prop definitions. You might need to update the component's prop types or adjust the props you're passing.
🧰 Tools
🪛 GitHub Check: tsc
[failure] 401-401:
Type '{ navigation: NativeStackNavigationProp<NavigationParamList, "InviteUsersToExistingGroup", undefined>; profiles: { [x: string]: IProfileSocials; }; groupMode: boolean; addToGroup: (member: any) => Promise<...>; }' is not assignable to type 'IntrinsicAttributes & ProfileSearchProps'.
[failure] 419-419:
Parameter 'member' implicitly has an 'any' type.
| return searchResultsToShow; | ||
| })()} | ||
| groupMode={group.enabled} | ||
| addToGroup={async (member) => { |
There was a problem hiding this comment.
Explicitly define the type of 'member' parameter
The parameter member in the addToGroup function implicitly has an any type. To ensure type safety and avoid TypeScript errors, explicitly define the type of member.
Apply this diff to fix the type error:
- addToGroup={async (member) => {
+ addToGroup={async (member: IProfileSocials & { address: string }) => {Assuming that member has the same structure as elements in group.members.
📝 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.
| addToGroup={async (member) => { | |
| addToGroup={async (member: IProfileSocials & { address: string }) => { |
🧰 Tools
🪛 GitHub Check: tsc
[failure] 419-419:
Parameter 'member' implicitly has an 'any' type.
| // members: group.members, | ||
| // }); | ||
| // } | ||
| }, [addMembers, group.members, navigation, route.params?.addingToGroupTopic]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary dependency in useCallback dependency array
The dependency route.params?.addingToGroupTopic in your useCallback hook is unnecessary. Removing it will eliminate the ESLint warning and adhere to best practices for React Hooks.
Apply this diff to fix the issue:
- }, [addMembers, group.members, navigation, route.params?.addingToGroupTopic]);
+ }, [addMembers, group.members, navigation]);📝 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.
| }, [addMembers, group.members, navigation, route.params?.addingToGroupTopic]); | |
| }, [addMembers, group.members, navigation]); |
🧰 Tools
🪛 eslint
[error] 101-101: React Hook useCallback has an unnecessary dependency: 'route.params.addingToGroupTopic'. Either exclude it or remove the dependency array.
(react-hooks/exhaustive-deps)
🪛 GitHub Check: lint
[failure] 101-101:
React Hook useCallback has an unnecessary dependency: 'route.params.addingToGroupTopic'. Either exclude it or remove the dependency array
| // This will not happen because the above throws if not found | ||
| if (!conversation) { | ||
| throw new Error("Conversation not found"); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant error check.
This error check is redundant as noted in the comment since getConversationByPeerByAccount already throws if the conversation is not found.
- // This will not happen because the above throws if not found
- if (!conversation) {
- throw new Error("Conversation not found");
- }| if (!dm) { | ||
| // todo: better type handling of undefineds | ||
| throw new Error("DM not found"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error message and address TODO.
The error message could be more descriptive, and the TODO about better type handling should be addressed.
if (!dm) {
- // todo: better type handling of undefineds
- throw new Error("DM not found");
+ throw new Error(
+ `Failed to set DM data for peer ${peer}: DM object is undefined`
+ );
}Consider using TypeScript's strict null checks and making the dm parameter non-nullable in the type definition to handle this at compile time.
Committable suggestion skipped: line range outside the PR's diff.
| logger.debug("[NewConversationModal] Rendering with params:", { | ||
| addingToGroupTopic: route.params?.addingToGroupTopic, | ||
| peer: route.params?.peer | ||
| }); |
There was a problem hiding this comment.
Fix type issues with route parameters.
The static analysis indicates that addingToGroupTopic and peer properties don't exist on NewChatNavParams.
Update the type definition to include these properties:
export type NewChatNavParams = {
addingToGroupTopic?: ConversationTopic;
peer?: string;
// ... other existing properties
};🧰 Tools
🪛 GitHub Check: tsc
[failure] 39-39:
Property 'addingToGroupTopic' does not exist on type 'Readonly'.
[failure] 40-40:
Property 'peer' does not exist on type 'Readonly'.
| <Button | ||
| variant="text" | ||
| text={translate("new_chat.create")} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
Add onPress handler to the Button component.
The Button component is missing an onPress handler, which is required for creating the group.
<Button
variant="text"
text={translate("new_chat.create")}
+ onPress={() => {
+ // TODO: Implement group creation logic
+ logger.debug("[NewConversationModal] Create group button pressed");
+ }}
/>📝 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.
| <Button | |
| variant="text" | |
| text={translate("new_chat.create")} | |
| /> | |
| ); | |
| <Button | |
| variant="text" | |
| text={translate("new_chat.create")} | |
| onPress={() => { | |
| // TODO: Implement group creation logic | |
| logger.debug("[NewConversationModal] Create group button pressed"); | |
| }} | |
| /> | |
| ); |
| useEffect(() => { | ||
| navigation.navigate("NewConversation", {}); | ||
| }, []); |
There was a problem hiding this comment.
Review the automatic navigation behavior and fix the useEffect dependency.
The automatic navigation to "NewConversation" raises several concerns:
- It might create a confusing user experience by immediately redirecting users from the conversation list.
- The
useEffecthook is missing the 'navigation' dependency.
Consider these alternatives:
- Remove the automatic navigation if it's not intentional.
- If the navigation is intended, add the missing dependency:
useEffect(() => {
navigation.navigate("NewConversation", {});
- }, []);
+ }, [navigation]);📝 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.
| useEffect(() => { | |
| navigation.navigate("NewConversation", {}); | |
| }, []); | |
| useEffect(() => { | |
| navigation.navigate("NewConversation", {}); | |
| }, [navigation]); |
🧰 Tools
🪛 eslint
[error] 91-91: React Hook useEffect has a missing dependency: 'navigation'. Either include it or remove the dependency array.
(react-hooks/exhaustive-deps)
🪛 GitHub Check: lint
[failure] 91-91:
React Hook useEffect has a missing dependency: 'navigation'. Either include it or remove the dependency array
| going to make changes to existing | ||
| new conversation flow/new grouip screen | ||
| and then i'm goiung to move those files here | ||
| and reorganize them |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve documentation formatting and clarity.
The readme needs several improvements:
- Fix typos ("goiung" → "going", "grouip" → "group")
- Use proper capitalization ("i'm" → "I'm")
- Add proper Markdown formatting
Here's a suggested revision:
-going to make changes to existing
-new conversation flow/new grouip screen
-and then i'm goiung to move those files here
-and reorganize them
+# New Chat Feature
+
+## Planned Changes
+
+1. Update existing conversation flow
+2. Enhance new group screen functionality
+3. Migrate relevant files to this directory
+4. Reorganize component structure
+
+## Implementation Notes
+
+This feature introduces a more fluid chat creation composer flow, replacing the existing conversation-based terminology.📝 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.
| going to make changes to existing | |
| new conversation flow/new grouip screen | |
| and then i'm goiung to move those files here | |
| and reorganize them | |
| # New Chat Feature | |
| ## Planned Changes | |
| 1. Update existing conversation flow | |
| 2. Enhance new group screen functionality | |
| 3. Migrate relevant files to this directory | |
| 4. Reorganize component structure | |
| ## Implementation Notes | |
| This feature introduces a more fluid chat creation composer flow, replacing the existing conversation-based terminology. |
🧰 Tools
🪛 LanguageTool
[style] ~1-~1: Consider shortening or rephrasing this to strengthen your wording.
Context: going to make changes to existing new conversation flow/new grou...
(MAKE_CHANGES)
[uncategorized] ~1-~1: You might be missing the article “the” here.
Context: going to make changes to existing new conversation flow/new grouip screen...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~3-~3: Did you mean “I”?
Context: ...rsation flow/new grouip screen and then i'm goiung to move those files here and r...
(I_LOWERCASE_PREMIUM)
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (3)
screens/NewConversation/NewConversation.tsx (1)
Line range hint
70-563: Consider splitting the component into smaller, focused componentsThe NewConversation component is handling multiple responsibilities:
- Search functionality
- Member management
- Conversation creation
- Navigation
- Message composition
This makes the component complex and harder to maintain.
Consider splitting this into smaller components:
- SearchContainer
- MemberList
- ConversationCreator
- MessageComposer
This would improve maintainability and make the code easier to test.
🧰 Tools
🪛 Biome (1.9.4)
[error] 70-70: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
screens/Navigation/navHelpers.ts (1)
Update screen names and navigation configurations to use consistent "chat" terminology
The review comment is valid. Several navigation and screen configurations still use "conversation" terminology while other parts have been updated to "chat":
features/conversation/conversation.nav.tsx: RenameConversationScreenConfigscreens/Main.tsx: Update route configuration usingConversationScreenConfigscreens/Profile.tsx: Update navigation reference to "Conversation"screens/Navigation/navHelpers.test.ts: Update test configuration using "Conversation"🔗 Analysis chain
Line range hint
1-257: Update terminology to align with PR objectives.The PR aims to change terminology from "conversation" to "chat", but several instances still use the old terminology:
- Screen name "Conversation"
- Function names like
handleConversationLink- Event names like "setCurrentConversationInputValue"
- URL paths and parameters
Let's verify all occurrences that need to be updated:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all instances of "conversation" terminology in the codebase rg -i "conversation" --type ts --type tsx # Find all route definitions to verify screen names ast-grep --pattern 'createStackNavigator({ $$$ })'Length of output: 124
Script:
#!/bin/bash # Search for conversation-related terms with context rg -i "conversation" -B 2 -A 2 # Search for navigation route definitions ast-grep --pattern 'const $_ = createNativeStackNavigator()' ast-grep --pattern 'routes: [ $$$ ]'Length of output: 75653
Script:
#!/bin/bash # Search for screen and navigation definitions rg -B 2 -A 2 "export const \w+Screen(Config)? = {" --type ts rg -B 2 -A 2 "export const \w+Nav(igation)? = {" --type ts # Search for route names in navigation config rg -B 2 -A 2 "name: ['\"]Conversation['\"]" --type tsLength of output: 721
Script:
#!/bin/bash # Search for screen definitions with fixed pattern rg -B 2 -A 2 "export const.*Screen.*= {" --type ts rg -B 2 -A 2 "export const.*Nav.*= {" --type ts # Search for route configurations rg -B 2 -A 2 "screens: {" --type ts rg -B 2 -A 2 "linking: {" --type tsLength of output: 568
Script:
#!/bin/bash # Search for screen definitions with escaped braces rg -B 2 -A 2 "export const.*Screen.*= \{" --type ts rg -B 2 -A 2 "export const.*Nav.*= \{" --type ts # Search for route configurations with fixed strings rg -F -B 2 -A 2 "screens: {" --type ts rg -F -B 2 -A 2 "name: \"Conversation\"" --type tsLength of output: 3709
screens/Navigation/NewConversationNav.tsx (1)
Line range hint
8-16: Update path to match new screen name.The path in NewChatScreenConfig still references the old "newConversation" path. Consider updating it to maintain consistency with the new naming.
export const NewChatScreenConfig = { - path: "/newConversation", + path: "/newChat", parse: { peer: decodeURIComponent, }, stringify: { peer: encodeURIComponent, }, };
♻️ Duplicate comments (1)
screens/NewConversation/NewConversation.tsx (1)
531-531:⚠️ Potential issueRemove @ts-expect-error and fix navigation typing
The use of @ts-expect-error suggests underlying type issues with navigation. This should be properly typed instead of suppressed.
The navigation.replace method is not properly typed. Consider using proper navigation typing or falling back to navigation.navigate if replace is not available.
Also applies to: 551-551
🧹 Nitpick comments (15)
features/search/components/OldProfileSearchResultsListItem.tsx (4)
23-33: Consider adding migration guidance to the deprecation notice.While the deprecation notice is well-documented with context and links, it would be helpful to add guidance about which component should be used instead.
/** * @deprecated * We are redoing our Create new chat flow, and this screen was shared between * that and the add members to existing group flow. * * This screen will need some design work, but is outside of scope of the * current work. * + * @see ProfileSearchResultListItem for the new implementation * * @see https://github.com/ephemeraHQ/converse-app/issues/1498 * @see https://www.figma.com/design/p6mt4tEDltI4mypD3TIgUk/Converse-App?node-id=5026-26989&m=dev */
34-49: Consider memoizing derived values for performance.The derived values could be memoized using
useMemoto prevent unnecessary recalculations on re-renders.+import { useMemo } from 'react'; export function OldProfileSearchResultsListItem({ address, socials, navigation, groupMode, addToGroup, }: ProfileSearchItemProps) { const { theme, themed } = useAppTheme(); - const preferredName = getPreferredName(socials, address); - const preferredAvatar = getPreferredAvatar(socials); - const primaryNames = getPrimaryNames(socials); - const primaryNamesDisplay = [ - ...primaryNames.filter((name) => name !== preferredName), - shortAddress(address), - ]; + const preferredName = useMemo(() => getPreferredName(socials, address), [socials, address]); + const preferredAvatar = useMemo(() => getPreferredAvatar(socials), [socials]); + const primaryNamesDisplay = useMemo(() => { + const primaryNames = getPrimaryNames(socials); + return [...primaryNames.filter((name) => name !== preferredName), shortAddress(address)]; + }, [socials, preferredName, address]);
51-51: Remove redundant key prop.The
keyprop is unnecessary here since this View isn't rendered in a list.- <View key={address} style={themed($container)}> + <View style={themed($container)}>
113-116: Consider extracting platform-specific margin to a constant.The platform-specific margin could be extracted to a constant at the top of the file for better maintainability.
+const PLATFORM_SPECIFIC_MARGIN = Platform.OS === "ios" ? spacing.md : 0; const $right: ThemedStyle<ViewStyle> = ({ spacing }) => ({ justifyContent: "center", - marginLeft: Platform.OS === "ios" ? spacing.md : 0, + marginLeft: PLATFORM_SPECIFIC_MARGIN, });features/search/components/OldProfileSearchResultsList.tsx (2)
25-36: Well-documented deprecation notice with clear migration path.The deprecation notice effectively communicates the rationale and future plans. The component renaming from
ProfileSearchtoOldProfileSearchResultsListaligns with the deprecation strategy.Consider adding a
@deprecated sincetag to help track when this deprecation was introduced, which can be useful for future cleanup efforts.
76-76: Consider extracting the magic number.The margin adjustment of
55units appears to be a magic number. Consider extracting this into a named constant or theme variable to improve maintainability and document its purpose.- <View style={[themed($footer), { marginBottom: insets.bottom + 55 }]}> + const FOOTER_EXTRA_MARGIN = 55; // Add comment explaining why 55 + <View style={[themed($footer), { marginBottom: insets.bottom + FOOTER_EXTRA_MARGIN }]}>features/groups/invite-to-group/invite-users-to-exisiting-group.screen.tsx (3)
1-1: Fix typo in filenameThe filename contains a typo: "exisiting" should be "existing". This should be corrected to maintain proper naming conventions.
Rename the file from
invite-users-to-exisiting-group.screen.tsxtoinvite-users-to-existing-group.screen.tsx.
148-152: Abstract debounce logic into a custom hookThe TODO comment correctly identifies the need to abstract the debounce logic. This would improve code reusability and maintainability.
Consider creating a custom hook like this:
function useDebounceSearch<T>( searchFn: (query: string) => Promise<T>, delay: number = 500 ) { const [value, setValue] = useState(""); const [results, setResults] = useState<T | null>(null); const [loading, setLoading] = useState(false); useEffect(() => { const handler = setTimeout(async () => { if (value.length >= 3) { setLoading(true); try { const results = await searchFn(value); setResults(results); } finally { setLoading(false); } } }, delay); return () => clearTimeout(handler); }, [value, searchFn, delay]); return { value, setValue, results, loading }; }
213-213: Improve error message handlingThe error message "No address has been set for this domain." could be more specific and should be internationalized using the translation system.
Apply this change:
- error: "No address has been set for this domain.", + error: translate("errors.no_address_for_domain"),Also, consider adding more context to help users resolve the issue.
screens/NewConversation/NewConversation.tsx (1)
1-27: Convert inline documentation to GitHub issuesThe file contains valuable documentation about bugs and UI improvements. These should be tracked properly in GitHub issues instead of inline comments.
Would you like me to help create GitHub issues for each of these items? The issues would cover:
- Chat loading from cold start
- Chat flow performance on app first launch
- Group flow performance
- Search results list updates
- UI improvements for search results
screens/Navigation/navHelpers.ts (1)
245-249: Handle potential JSON.stringify failures.The debug log uses JSON.stringify which could throw an error if params contain circular references.
Consider using a try-catch:
- logger.debug( - `[screenListeners] shouldReplace replacing ${newRoute.name} with params ${JSON.stringify( - newRoute.params - )}` - ); + try { + logger.debug( + `[screenListeners] shouldReplace replacing ${newRoute.name} with params ${JSON.stringify( + newRoute.params + )}` + ); + } catch (error) { + logger.debug( + `[screenListeners] shouldReplace replacing ${newRoute.name} with params [Failed to stringify]` + ); + }screens/NewConversation/newChat.styles.tsx (3)
11-23: Consider extracting platform-specific styles into separate utilities.There's significant platform-specific styling scattered across multiple components. Consider creating a utility function to handle platform-specific styles, making the code more maintainable and reducing duplication.
Example approach:
const createPlatformStyle = <T>(defaultStyle: T, androidStyle?: Partial<T>) => Platform.select({ default: defaultStyle, android: androidStyle ? { ...defaultStyle, ...androidStyle } : defaultStyle, }); // Usage example: const $messageContainer: ThemedStyle<ViewStyle> = ({ spacing }) => createPlatformStyle( { marginTop: spacing.lg, paddingHorizontal: spacing.md, }, { marginRight: spacing.md, marginLeft: spacing.md, marginTop: spacing.md, } );Also applies to: 25-33, 35-38, 40-51, 65-69
25-33: Consider adding accessibility properties for text components.The text styles should include accessibility considerations for better user experience.
const $message: ThemedStyle<TextStyle> = ({ colors }) => ({ color: colors.text.secondary, ...textSizeStyles.sm, + // Ensure minimum contrast ratio for accessibility + ...(Platform.OS === 'ios' ? { accessibilityRole: 'text' } : {}), ...Platform.select({ default: { textAlign: "center", }, }), });
66-69: Standardize spacing values across components.The spacing values vary between components (e.g., "6xs" vs "xs"). Consider standardizing these values for consistency.
const $newGroupButton: ThemedStyle<ViewStyle> = ({ spacing }) => ({ - marginLeft: spacing["6xs"], + marginLeft: spacing.xs, // Use consistent spacing scale paddingTop: Platform.OS === "ios" ? spacing.sm : spacing.xs, paddingBottom: Platform.OS === "ios" ? 0 : spacing.xs, });screens/Navigation/NewConversationNav.tsx (1)
21-23: Remove or use proper logging level for navigation debugging.Consider using a more appropriate logging level (trace/debug) or removing the debug statement if not needed for production.
- logger.debug("[NewConversationNav] Configuring navigation", { + logger.trace("[NewConversationNav] Configuring navigation", { colorScheme, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
components/ClickableText.tsx(3 hunks)features/conversation-list/components/ChatNullState.tsx(0 hunks)features/conversation-list/components/NewConversationButton.tsx(0 hunks)features/conversation-list/conversation-list.screen.tsx(1 hunks)features/groups/invite-to-group/invite-users-to-exisiting-group.screen.tsx(1 hunks)features/search/components/NoResult.tsx(0 hunks)features/search/components/OldProfileSearchResultsList.tsx(8 hunks)features/search/components/OldProfileSearchResultsListItem.tsx(1 hunks)i18n/translations/en.ts(1 hunks)package.json(1 hunks)screens/ConverseMatchMaker.tsx(0 hunks)screens/Navigation/Navigation.tsx(4 hunks)screens/Navigation/NewConversationNav.tsx(2 hunks)screens/Navigation/navHelpers.ts(3 hunks)screens/NewConversation/NewConversation.tsx(7 hunks)screens/NewConversation/NewConversationModal.tsx(1 hunks)screens/NewConversation/NewGroupSummary.tsx(1 hunks)screens/NewConversation/newChat.styles.tsx(1 hunks)utils/xmtpRN/conversations.ts(6 hunks)
💤 Files with no reviewable changes (4)
- features/search/components/NoResult.tsx
- features/conversation-list/components/NewConversationButton.tsx
- features/conversation-list/components/ChatNullState.tsx
- screens/ConverseMatchMaker.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- features/conversation-list/conversation-list.screen.tsx
- i18n/translations/en.ts
- utils/xmtpRN/conversations.ts
🧰 Additional context used
🪛 Biome (1.9.4)
screens/NewConversation/NewConversation.tsx
[error] 70-70: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: tsc
- GitHub Check: test
- GitHub Check: performance-test
- GitHub Check: lint
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (7)
features/search/components/OldProfileSearchResultsListItem.tsx (1)
1-21: LGTM! Well-structured imports and type definitions.The imports are properly organized, and the
ProfileSearchItemPropstype is well-defined with appropriate optional properties.screens/NewConversation/NewConversationModal.tsx (1)
1-21: Consider removing commented imports.While keeping commented code might be helpful during the transition, it's better to rely on version control history for reference. Consider removing these commented imports as part of the code cleanup phase.
Let's verify if these imports are still needed elsewhere:
features/search/components/OldProfileSearchResultsList.tsx (2)
145-147: LGTM: Clean platform-specific text alignment.The new
$footerTextstyle cleanly handles platform-specific text alignment requirements.
118-128: Verify the impact of style type change.The
$sectionTitleSpacinghas been changed fromTextStyletoViewStyle. While the current margin properties are valid for both types, ensure this change doesn't affect any text-specific styling that might have been previously applied.✅ Verification successful
Style type change is safe and properly handled
The change of
$sectionTitleSpacingfromTextStyletoViewStyleis safe as it only contains margin properties, and when used with Text components, it's properly cast toTextStyle. No text-specific styling properties are affected by this change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any usage of $sectionTitleSpacing that might expect TextStyle properties rg -l "(\$sectionTitleSpacing.*style.*Text|Text.*style.*\$sectionTitleSpacing)" --type tsxLength of output: 118
Script:
#!/bin/bash # Search for any usage of $sectionTitleSpacing without file type restriction echo "=== Direct style references ===" rg "\$sectionTitleSpacing" -A 2 -B 2 echo -e "\n=== Component usage patterns ===" ast-grep --pattern 'style={$_.$sectionTitleSpacing}'Length of output: 246
Script:
#!/bin/bash echo "=== Current file content ===" cat features/search/components/OldProfileSearchResultsList.tsx echo -e "\n=== Text components in the file ===" rg "Text.*style" features/search/components/OldProfileSearchResultsList.tsx -A 2 -B 2Length of output: 4198
features/groups/invite-to-group/invite-users-to-exisiting-group.screen.tsx (1)
360-363: Explicitly define the type of 'member' parameterThe parameter
memberin theaddToGroupfunction implicitly has ananytype.screens/Navigation/navHelpers.ts (1)
12-12: LGTM! Good addition of logging capabilities.Adding structured logging will help with debugging navigation state changes.
screens/Navigation/Navigation.tsx (1)
82-82: Verify navigation type changes impact.The navigation param changes from a typed param to undefined and addition of InviteUsersToExistingGroup need verification across the codebase.
Also applies to: 90-90
✅ Verification successful
Navigation type changes verified and safe
The changes to navigation params are consistent with the codebase:
- NewConversation's change to
undefinedmatches its usage as it's only called with empty params- InviteUsersToExistingGroup is properly integrated with correct type definitions and usage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for navigation calls that might still pass params rg "navigate\(['\"]NewConversation['\"].*,.*\)" --type ts --type tsx # Search for type references that might need updating rg "NavigationParamList\[['\"](NewConversation|InviteUsersToExistingGroup)['\"]\]" --type ts --type tsxLength of output: 240
Script:
#!/bin/bash # Search for navigation calls to NewConversation rg "navigate\(['\"]NewConversation['\"]" -A 2 # Search for any references to these screens in navigation rg "(NewConversation|InviteUsersToExistingGroup)" -A 2 # Look for navigation type definitions ast-grep --pattern 'type NavigationParamList = { $$$ }'Length of output: 26336
| @@ -229,6 +229,7 @@ | |||
| "eslint-plugin-react-hooks": "^5.0.0", | |||
| "eslint-plugin-react-native": "^4.1.0", | |||
| "eslint-plugin-unused-imports": "^4.1.4", | |||
| "expo-module-scripts": "^4.0.3", | |||
There was a problem hiding this comment.
💡 Codebase verification
Remove unnecessary expo-module-scripts dependency
This dependency appears to be unnecessary for the chat composer changes as:
- No custom Expo modules are being created in this PR
- The chat composer UI changes can be implemented using existing Expo components
- The dependency is only referenced in package files but not used in the source code
🔗 Analysis chain
Verify the necessity and compatibility of expo-module-scripts.
While the addition of expo-module-scripts is valid, please clarify:
- Why this dependency is needed specifically for the chat composer changes
- If you're planning to create custom Expo modules as part of this PR
Run this script to check version compatibility with the project's Expo setup:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify compatibility between expo-module-scripts and other Expo packages
# Check if expo-module-scripts is used in any new or modified files
rg -l "expo-module-scripts"
# List all Expo-related dependencies and their versions for compatibility check
jq -r '.dependencies | with_entries(select(.key | startswith("expo"))) | to_entries | .[] | "\(.key): \(.value)"' package.json
Length of output: 972
| // export type NewConversationModalParams = { | ||
| // NewChatComposerScreen: NewChatNavParams; | ||
| // NewGroupSummary: { | ||
| // members: (IProfileSocials & { address: string })[]; | ||
| // }; | ||
| // }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update type name to follow new terminology.
The PR introduces a terminology change from "conversation" to "chat". For consistency, this type should be renamed from NewConversationModalParams to NewChatModalParams.
| // const NewConversationModal = ({ | ||
| // route, | ||
| // }: NativeStackScreenProps<NavigationParamList, "NewConversation">) => { | ||
| // const colorScheme = useColorScheme(); | ||
|
|
||
| export default NewConversationModal; | ||
| // logger.debug("[NewConversationModal] Rendering with params:", { | ||
| // addingToGroupTopic: route.params?.addingToGroupTopic, | ||
| // peer: route.params?.peer | ||
| // }); | ||
|
|
||
| // return ( | ||
| // <ModalStack.Navigator | ||
| // screenOptions={{ | ||
| // headerShown: true, | ||
| // headerStyle: { | ||
| // backgroundColor: navigationSecondaryBackgroundColor(colorScheme), | ||
| // } as any, | ||
| // headerTitleStyle: Platform.select({ | ||
| // default: headerTitleStyle(colorScheme), | ||
| // }), | ||
| // animation: navigationAnimation, | ||
| // }} | ||
| // > | ||
| // <ModalStack.Screen | ||
| // name="NewChatComposerScreen" | ||
| // component={NewConversation} | ||
| // /> | ||
| // </ModalStack.Screen> | ||
| // </ModalStack.Navigator> | ||
| // ); | ||
| // }; |
There was a problem hiding this comment.
💡 Codebase verification
Type safety issues found in new chat implementation
The new implementation still contains unsafe type handling that should be addressed:
screens/NewConversation/NewConversation.tsx:- Line 109: Unsafe navigation cast
navigation as anyin AndroidBackAction - Lines 530 and 550: Uses
@ts-expect-errorcomments that should be properly typed
- Line 109: Unsafe navigation cast
Consider properly typing these instances to ensure type safety and prevent potential runtime errors.
🔗 Analysis chain
Ensure type safety in the new implementation.
The old implementation used unsafe type assertions (as any). When implementing the new chat composer, ensure proper type safety without using any assertions.
Let's check for similar type assertions in the new implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for unsafe type assertions in TypeScript files
rg --type typescript "as any" -l
Length of output: 69
Script:
#!/bin/bash
# Search for TypeScript files with type assertions and ignores
fd '\.(ts|tsx)$' | xargs rg 'as any|as unknown|@ts-ignore|@ts-expect-error' --with-filename --line-number
Length of output: 7252
| }; | ||
| }; | ||
| // export type NewConversationModalParams = { | ||
| // NewChatComposerScreen: NewChatNavParams; | ||
| // NewGroupSummary: { | ||
| // members: (IProfileSocials & { address: string })[]; | ||
| // }; | ||
| // }; | ||
|
|
||
| const ModalStack = createNativeStackNavigator<NewConversationModalParams>(); | ||
| // const ModalStack = createNativeStackNavigator<NewConversationModalParams>(); | ||
|
|
||
| const NewConversationModal = ({ | ||
| route, | ||
| }: NativeStackScreenProps<NavigationParamList, "NewConversation">) => { | ||
| const colorScheme = useColorScheme(); | ||
| return ( | ||
| <ModalStack.Navigator | ||
| screenOptions={{ | ||
| headerShown: true, | ||
| headerStyle: { | ||
| backgroundColor: navigationSecondaryBackgroundColor(colorScheme), | ||
| } as any, | ||
| headerTitleStyle: Platform.select({ | ||
| default: headerTitleStyle(colorScheme), | ||
| web: { left: -20, color: textPrimaryColor(colorScheme) } as any, | ||
| }), | ||
| animation: navigationAnimation, | ||
| }} | ||
| > | ||
| <ModalStack.Screen | ||
| name="NewConversationScreen" | ||
| options={{ | ||
| headerTitle: route.params?.addingToGroupTopic | ||
| ? translate("new_group.add_members") | ||
| : translate("new_conversation.new_conversation"), | ||
| presentation: "modal", | ||
| }} | ||
| > | ||
| {(props) => { | ||
| const newRoute = { ...props.route }; | ||
| if (route.params) { | ||
| newRoute.params = route.params; | ||
| } | ||
| return ( | ||
| <NewConversation route={newRoute} navigation={props.navigation} /> | ||
| ); | ||
| }} | ||
| </ModalStack.Screen> | ||
| <ModalStack.Screen | ||
| name="NewGroupSummary" | ||
| component={NewGroupSummary} | ||
| options={{ | ||
| headerBackTitle: translate("new_conversation.back"), | ||
| headerTitle: translate("new_conversation.create_group"), | ||
| headerRight: () => ( | ||
| <Button | ||
| variant="text" | ||
| title={translate("new_conversation.create")} | ||
| /> | ||
| ), // Dummy button for style | ||
| }} | ||
| /> | ||
| </ModalStack.Navigator> | ||
| ); | ||
| }; | ||
| // const NewConversationModal = ({ | ||
| // route, | ||
| // }: NativeStackScreenProps<NavigationParamList, "NewConversation">) => { | ||
| // const colorScheme = useColorScheme(); | ||
|
|
||
| export default NewConversationModal; | ||
| // logger.debug("[NewConversationModal] Rendering with params:", { | ||
| // addingToGroupTopic: route.params?.addingToGroupTopic, | ||
| // peer: route.params?.peer | ||
| // }); | ||
|
|
||
| // return ( | ||
| // <ModalStack.Navigator | ||
| // screenOptions={{ | ||
| // headerShown: true, | ||
| // headerStyle: { | ||
| // backgroundColor: navigationSecondaryBackgroundColor(colorScheme), | ||
| // } as any, | ||
| // headerTitleStyle: Platform.select({ | ||
| // default: headerTitleStyle(colorScheme), | ||
| // }), | ||
| // animation: navigationAnimation, | ||
| // }} | ||
| // > | ||
| // <ModalStack.Screen | ||
| // name="NewChatComposerScreen" | ||
| // component={NewConversation} | ||
| // /> | ||
| // </ModalStack.Screen> | ||
| // </ModalStack.Navigator> | ||
| // ); | ||
| // }; | ||
|
|
||
| // export default NewConversationModal; |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Update dependencies before removing NewConversationModal
Before removing the commented-out NewConversationModal.tsx, these dependencies need to be addressed:
screens/NewConversation/NewGroupSummary.tsx: Update or remove the type importNewConversationModalParamsfeatures/conversation-list/conversation-list.screen.tsx: Update the navigation call to use the new chat composer flow
🔗 Analysis chain
Document changes and remove the file.
Since this component is being replaced as part of the new chat composer flow:
- Document the changes in the PR description or migration guide
- Remove this file entirely instead of keeping it commented out
- Update any remaining references to this component in other files
Let's check for any remaining references to this component:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to NewConversationModal
rg --type typescript "NewConversationModal" -l
# Check for import statements referencing this file
rg --type typescript "from.*NewConversationModal" -l
Length of output: 171
Script:
#!/bin/bash
# Search in both .ts and .tsx files for any references to NewConversationModal
rg "NewConversationModal" -g "*.ts" -g "*.tsx" -l
# Search specifically for import statements with different syntax variations
rg "import.*NewConversationModal|from.*NewConversationModal" -g "*.ts" -g "*.tsx" -l
# Search for potential navigation references
rg "navigate.*NewConversation|navigation.*NewConversation" -g "*.ts" -g "*.tsx" -l
Length of output: 414
Script:
#!/bin/bash
# Check the exact usage in NewGroupSummary.tsx
rg "NewConversationModal" screens/NewConversation/NewGroupSummary.tsx -C 2
# Check navigation usage in conversation-list screen
rg "NewConversation" features/conversation-list/conversation-list.screen.tsx -C 2
Length of output: 792
| // } | ||
|
|
||
| const useStyles = () => { | ||
| const colorScheme = useColorScheme(); | ||
| return useMemo( | ||
| () => | ||
| StyleSheet.create({ | ||
| avatar: { | ||
| marginBottom: 10, | ||
| marginTop: 23, | ||
| }, | ||
| group: { | ||
| backgroundColor: backgroundColor(colorScheme), | ||
| }, | ||
| groupContent: { | ||
| paddingHorizontal: Platform.OS === "ios" ? 18 : 0, | ||
| }, | ||
| sectionTitle: { | ||
| marginBottom: -8, | ||
| color: textSecondaryColor(colorScheme), | ||
| fontSize: 13, | ||
| fontWeight: "500", | ||
| }, | ||
| activitySpinner: { | ||
| marginRight: 5, | ||
| }, | ||
| listSectionContainer: { | ||
| alignItems: "center", | ||
| }, | ||
| buttonText: { | ||
| fontWeight: "500", | ||
| }, | ||
| nameInput: { | ||
| fontSize: 16, | ||
| }, | ||
| p: { | ||
| textAlign: "center", | ||
| marginLeft: 32, | ||
| marginRight: 32, | ||
| ...Platform.select({ | ||
| default: { | ||
| fontSize: 13, | ||
| lineHeight: 17, | ||
| color: textSecondaryColor(colorScheme), | ||
| }, | ||
| }), | ||
| }, | ||
| }), | ||
| [colorScheme] | ||
| ); | ||
| }; | ||
| // const useStyles = () => { | ||
| // const colorScheme = useColorScheme(); | ||
| // return useMemo( | ||
| // () => | ||
| // StyleSheet.create({ | ||
| // avatar: { | ||
| // marginBottom: 10, | ||
| // marginTop: 23, | ||
| // }, | ||
| // group: { | ||
| // backgroundColor: backgroundColor(colorScheme), | ||
| // }, | ||
| // groupContent: { | ||
| // paddingHorizontal: Platform.OS === "ios" ? 18 : 0, | ||
| // }, | ||
| // sectionTitle: { | ||
| // marginBottom: -8, | ||
| // color: textSecondaryColor(colorScheme), | ||
| // fontSize: 13, | ||
| // fontWeight: "500", | ||
| // }, | ||
| // activitySpinner: { | ||
| // marginRight: 5, | ||
| // }, | ||
| // listSectionContainer: { | ||
| // alignItems: "center", | ||
| // }, | ||
| // buttonText: { | ||
| // fontWeight: "500", | ||
| // }, | ||
| // nameInput: { | ||
| // fontSize: 16, | ||
| // }, | ||
| // p: { | ||
| // textAlign: "center", | ||
| // marginLeft: 32, | ||
| // marginRight: 32, | ||
| // ...Platform.select({ | ||
| // default: { | ||
| // fontSize: 13, | ||
| // lineHeight: 17, | ||
| // color: textSecondaryColor(colorScheme), | ||
| // }, | ||
| // }), | ||
| // }, | ||
| // }), | ||
| // [colorScheme] | ||
| // ); | ||
| // }; |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Remove commented-out code and clean up remaining references
The component appears to be in the process of being removed, with all its usages already commented out. To complete this cleanup:
- Remove the commented-out code in
screens/NewConversation/NewGroupSummary.tsx - Remove the route type
NewGroupSummaryfromNavigationParamListinscreens/Navigation/Navigation.tsx - Remove commented imports in
NewConversationModal.tsx - Clean up commented references in translation files
🔗 Analysis chain
Consider removing commented-out code instead of keeping it
The entire file has been commented out. While this might be useful during the restructuring phase, keeping commented-out code in the codebase can lead to maintenance issues and confusion. Consider either:
- Removing the code entirely and relying on version control history if needed
- Adding a TODO comment explaining why the code is temporarily commented out and when it will be addressed
Let's verify if this component is still referenced elsewhere in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to NewGroupSummary
rg "NewGroupSummary" --type ts --type tsx
Length of output: 69
Script:
#!/bin/bash
# Search for NewGroupSummary references without type restriction
rg "NewGroupSummary"
# Search for route definitions and navigation
rg "NewGroupSummary.*Stack|\"NewGroupSummary\""
# Find potential route type definitions
ast-grep --pattern 'type $_ = {
$$$
NewGroupSummary: $_
$$$
}'
Length of output: 3943
| logger.debug("[screenListeners] NewConversation", newRoute.params); | ||
| // whats happening here | ||
| logger.debug( | ||
| "[screenListeners] currentRoute.params", | ||
| currentRoute.params | ||
| ); | ||
| logger.debug("setting shouldReplace to true"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate debug logs and update screen name.
- The comment "whats happening here" suggests uncertainty about the code's purpose. Consider adding a proper comment explaining the logic.
- Multiple debug statements can be consolidated into a single log entry.
- Screen name "NewConversation" should be updated to "NewChat" as per PR objectives.
Consider this refactor:
- logger.debug("[screenListeners] NewConversation", newRoute.params);
- // whats happening here
- logger.debug(
- "[screenListeners] currentRoute.params",
- currentRoute.params
- );
- logger.debug("setting shouldReplace to true");
+ logger.debug("[screenListeners] NewChat route change", {
+ newParams: newRoute.params,
+ currentParams: currentRoute.params,
+ action: "Replacing route due to peer change"
+ });📝 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.
| logger.debug("[screenListeners] NewConversation", newRoute.params); | |
| // whats happening here | |
| logger.debug( | |
| "[screenListeners] currentRoute.params", | |
| currentRoute.params | |
| ); | |
| logger.debug("setting shouldReplace to true"); | |
| logger.debug("[screenListeners] NewChat route change", { | |
| newParams: newRoute.params, | |
| currentParams: currentRoute.params, | |
| action: "Replacing route due to peer change" | |
| }); |
components/ClickableText.tsx
Outdated
| useEffect(() => { | ||
| console.log("ClickableText", children); | ||
| }, [children]); |
There was a problem hiding this comment.
Remove debug logging before production deployment.
The useEffect hook with console.log should be removed before deploying to production as it may impact performance and pollute logs.
- useEffect(() => {
- console.log("ClickableText", children);
- }, [children]);| @@ -38,7 +42,7 @@ | |||
| }, []); | |||
|
|
|||
| const handleNewConversationPress = useCallback((peer: string) => { | |||
| navigate("NewConversation", { peer }); | |||
| navigate("Conversation", { peer }); | |||
There was a problem hiding this comment.
💡 Codebase verification
Navigation route inconsistency detected
The change to use "Conversation" in ClickableText.tsx aligns with the majority of navigation patterns in the codebase. However, there is still one instance using the old "NewConversation" route:
features/conversation-list/conversation-list.screen.tsx:350:navigation.navigate("NewConversation", {});
🔗 Analysis chain
Verify navigation flow changes.
The navigation destination has changed from "NewConversation" to "Conversation". Ensure this change is intentional and all related navigation flows have been updated accordingly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old navigation route
rg '"NewConversation"' --type ts --type tsx
# Search for navigation.navigate calls to verify consistency
ast-grep --pattern 'navigate("Conversation"'
Length of output: 120
Script:
#!/bin/bash
# Search for both old and new navigation routes
rg -t typescript -t javascriptreact -t typescriptreact '"NewConversation"|"Conversation"'
# Search for navigation patterns using ast-grep
ast-grep --pattern 'navigate($ROUTE, $$$)'
# Also search for navigation.navigate pattern
ast-grep --pattern 'navigation.navigate($ROUTE, $$$)'
Length of output: 4641
screens/Navigation/Navigation.tsx
Outdated
| @@ -135,12 +138,12 @@ export function SignedInNavigation() { | |||
| <NativeStack.Screen name="Blocked" component={BlockedChatsScreen} /> | |||
| {ConversationRequestsListNav()} | |||
| {ConversationNav()} | |||
| {NewConversationNav()} | |||
| {ConverseMatchMakerNav()} | |||
| {/* {NewConversationNav()} */} | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clean up commented code and verify navigation flow.
Remove the commented-out NewConversationNav line and ensure the new InviteUsersToExistingGroupNav is properly integrated into the navigation flow.
- {/* {NewConversationNav()} */}
{ShareProfileNav()}
{WebviewPreviewNav()}
{ProfileNav()}
{GroupNav()}
{InviteUsersToExistingGroupNav()}Also applies to: 146-146
There was a problem hiding this comment.
Amaaaaazing! Gave some comments. Obvs I know it's WIP but gave all the comments anyway in case it's helpful.
IMO there will be some stuff we'll need to align all together. We should talk on Slack or plan a small meeting.
Might also gave some comments on previous deprecated stuff by mistake. Sorry in advance about that 😵
| @@ -47,6 +47,8 @@ export function sendMessage(args: { | |||
| }); | |||
| } | |||
|
|
|||
| // todo: where do we optimistically handle adding the message | |||
| // to our query cache? | |||
There was a problem hiding this comment.
It's WIP, but if you look in useMutation it should be there.
If you work on that I would just maybe put it in Slack because i think Ry might also work on that.
Basically we do it with onMutate but because of the messages having client from the SDK it's kind of buggy so for now I don't do optimistic updates.
I'll let you and Naomi and Ry decide what to do with this but I think overall we should be very aligned on how we'll do it and what to do first. Example maybe remove the client in the SDK?
There was a problem hiding this comment.
I don't even think we have to remove client at sdk layer because we should have a serialize call and deserialize call (where we can remove unwanted values and then recreate the sdk#conversation class object on deserialization time.
Did a quick little exploration here: (couldn't commit due to annoying git stuff so here's the App.tsx file)
Actually managed to get exploration up here: b81853e
TLDR: serialization and sanitization is not typesafe at the react query level so I think it'll be worth an exploration of just delete the client from our sdk method invocation return values. I'll epxlore that tomorrow after I get more ui done, but ultimately removing from the sdk would fix the problem - just isn't required because we can modify our sdk return values before they hit our react query cache
@ -43,6 +43,10 @@ import "./utils/splash/splash";
import "./features/notifications/utils";
import { setupAppAttest } from "@utils/appCheck";
import { saveApiURI } from "./utils/sharedData";
import { PersistQueryClientProvider } from "@tanstack/react-query-persist-client";
import { createSyncStoragePersister } from "@tanstack/query-sync-storage-persister";
import { MMKV } from "react-native-mmkv";
import { v } from "@privy-io/expo/dist/EmbeddedWalletState-d3db6772";
LogBox.ignoreLogs([
"Privy: Expected status code 200, received 400", // Privy
@ -126,6 +130,45 @@ const App = () => {
const AppKeyboardProvider =
Platform.OS === "ios" ? KeyboardProvider : React.Fragment;
const mmkvStorage = new MMKV({
id: "persist-query-client",
});
/**
* Recursively removes 'client' keys from all objects in the data structure.
*
* @param obj - The object to sanitize
* @returns A sanitized copy without any 'client' keys
*/
function sanitizeForSerialization(obj: unknown): unknown {
logger.debug("[sanitizeForSerialization] Processing:", {
type: typeof obj,
isArray: Array.isArray(obj),
});
// Handle primitives
if (!obj || typeof obj !== "object") {
return obj;
}
// Handle arrays
if (Array.isArray(obj)) {
return obj.map((item) => sanitizeForSerialization(item));
}
// Handle objects
const result: Record<string, unknown> = {};
for (const [key, value] of Object.entries(obj)) {
if (key === "client") {
logger.debug("[sanitizeForSerialization] Removing client key");
continue;
}
result[key] = sanitizeForSerialization(value);
}
return result;
}
export default function AppWithProviders() {
const colorScheme = useColorScheme();
@ -137,7 +180,115 @@ export default function AppWithProviders() {
useThemeProvider();
return (
<QueryClientProvider client={queryClient}>
<PersistQueryClientProvider
client={queryClient}
persistOptions={{
hydrateOptions: {
defaultOptions: {
deserializeData(data) {
logger.debug("[deserializeData] Starting:", {
dataType: typeof data,
dataLength: typeof data === "string" ? data.length : "N/A",
});
try {
const result =
typeof data === "string" ? JSON.parse(data) : data;
logger.debug("[deserializeData] Success:", {
resultType: typeof result,
resultKeys:
result && typeof result === "object"
? Object.keys(result)
: [],
});
return result;
} catch (error) {
logger.error(
"[PersistQueryClientProvider] Failed to deserialize:",
{
error,
dataSnippet:
typeof data === "string" ? data.slice(0, 100) : "N/A",
}
);
return {};
}
},
},
},
dehydrateOptions: {
shouldDehydrateQuery: (query) => {
return true;
// const shouldPersist =
// query.queryKey.includes("conversations") &&
// query.queryKey.includes(
// "0x9aefd0c98e67d3470f1c46df758a2c9ec504d57a"
// );
// logger.debug("[shouldDehydrateQuery]:", {
// queryKey: query.queryKey,
// shouldPersist,
// });
// return shouldPersist;
},
serializeData: (data) => {
logger.debug("[serializeData] Starting:", {
dataType: typeof data,
isArray: Array.isArray(data),
topLevelKeys:
data && typeof data === "object" ? Object.keys(data) : [],
});
try {
const sanitized = sanitizeForSerialization(data);
const result = JSON.stringify(sanitized);
logger.debug("[serializeData] Success:", {
sanitizedKeys:
sanitized && typeof sanitized === "object"
? Object.keys(sanitized as object)
: [],
resultLength: result.length,
});
return result;
} catch (error) {
logger.error(
"[PersistQueryClientProvider] Failed to serialize:",
{
error,
dataKeys:
data && typeof data === "object" ? Object.keys(data) : [],
}
);
return JSON.stringify({});
}
},
},
persister: createSyncStoragePersister({
storage: {
getItem: (key) => {
logger.debug("[storage] getItem:", { key });
const value = mmkvStorage.getString(key);
logger.debug("[storage] getItem result:", {
key,
hasValue: Boolean(value),
valueLength: value?.length,
});
return value || null;
},
setItem: (key, value) => {
logger.debug("[storage] setItem:", {
key,
valueLength: value?.length,
});
return mmkvStorage.set(key, value);
},
removeItem: (key) => {
logger.debug("[storage] removeItem:", { key });
return mmkvStorage.delete(key);
},
},
}),
}}
>
<PrivyProvider appId={config.privy.appId} storage={privySecureStorage}>
<ThirdwebProvider>
<AppKeyboardProvider>
@ -156,7 +307,7 @@ export default function AppWithProviders() {
</AppKeyboardProvider>
</ThirdwebProvider>
</PrivyProvider>
</QueryClientProvider>
</PersistQueryClientProvider>
);
}
| @@ -120,3 +141,7 @@ const $footer: ThemedStyle<ViewStyle> = ({ spacing }) => ({ | |||
| }, | |||
| }), | |||
| }); | |||
|
|
|||
| const $footerText: ThemedStyle<TextStyle> = () => ({ | |||
| textAlign: Platform.OS === "ios" ? "center" : "left", | |||
There was a problem hiding this comment.
Shouldn't have speciific for Android
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
features/groups/invite-to-group/invite-users-to-exisiting-group.screen.tsx (1)
Line range hint
487-544: Improve error handling and type safety in message sending logic.
- Remove
@ts-expect-errorby properly typing the navigation- Add error handling for message sending failures
- Consider extracting the message sending logic into a separate service
Example error handling:
try { await sendMessage({ conversation: dm, params: { content: { text: dmCreationMessageText }, }, }); } catch (error) { logger.error('[NewConversation] Failed to send message', { error }); Alert.alert('Failed to send message'); return; }screens/NewConversation/NewConversation.tsx (1)
Line range hint
1-552: Consider consolidating duplicate functionality with InviteUsersToExistingGroup screen.Both screens share similar functionality for:
- User search
- Member management
- Message sending
Consider extracting shared logic into reusable hooks and components.
♻️ Duplicate comments (1)
screens/NewConversation/NewConversation.tsx (1)
85-85:⚠️ Potential issueAdd proper type annotations for the component props.
The empty object pattern suggests missing type annotations. This should be properly typed, especially since it's a screen component.
Apply this change:
-export default function NewConversation({}) { +export default function NewConversation({ + navigation, + route, +}: NativeStackScreenProps<NavigationParamList, "NewConversation">) {
🧹 Nitpick comments (9)
design-system/chip.tsx (2)
40-64: Add accessibility attributes for better screen reader support.The component could benefit from ARIA attributes for better accessibility.
<TouchableOpacity onPress={onPress} style={[themed($chip), isActive && themed($chipActive), style]} + accessibilityRole="button" + accessibilityState={{ selected: isActive }} + accessibilityLabel={`${text} ${isActive ? 'selected' : ''}`} >
66-79: Remove commented debug code.The
debugBorderutility is commented out and should be removed if not needed.const $chip: ThemedStyle<ViewStyle> = ({ spacing, borderRadius, borderWidth, colors, }) => ({ borderRadius: borderRadius.sm, borderWidth: borderWidth.sm, borderColor: colors.border.subtle, paddingVertical: spacing.xxs, paddingHorizontal: spacing.xs, minHeight: 36, - // ...debugBorder("orange"), });design-system/IconButton/IconButton.tsx (1)
7-7: Remove unused View import.The
Viewimport is not used in the component.- View,utils/api.ts (1)
222-224: Improve error message formatting.The error message has unnecessary newlines and indentation.
- throw new Error(` - No App Check Token Available. This indicates that we believe the app is not running on an authentic build of - our application on a device that has not been tampered with. - `); + throw new Error( + "No App Check Token Available. This indicates that we believe the app is not running on an authentic build of our application on a device that has not been tampered with." + );features/search/components/ProfileSearchResultsList.tsx (2)
34-42: Add performance optimizations to FlatList.Consider adding performance-related props to optimize the FlatList for large datasets.
<FlatList keyboardShouldPersistTaps="handled" data={Object.keys(profiles)} keyExtractor={keyExtractor} renderItem={renderItem} onTouchStart={Keyboard.dismiss} + initialNumToRender={10} + maxToRenderPerBatch={10} + windowSize={5} + removeClippedSubviews={true} />
21-21: Consider memoizing the address array.The
Object.keys(profiles)in the FlatList's data prop will be recalculated on every render.+ const addressList = useMemo(() => Object.keys(profiles), [profiles]); return ( <FlatList keyboardShouldPersistTaps="handled" - data={Object.keys(profiles)} + data={addressList}features/search/components/ProfileSearchItem.tsx (1)
24-31: Consider enhancing the JSDoc documentation.While the Figma link is helpful, consider adding:
- A brief description of the component's purpose
- Example usage
- Prop descriptions
features/groups/invite-to-group/invite-users-to-exisiting-group.screen.tsx (2)
1-41: Consider moving TODOs to more appropriate locations.
- Move bugs to GitHub issues
- Move UI tasks to a separate tracking document or project management tool
- Move code organization tasks to
.editorconfigor similar- Keep only component-specific documentation in the JSDoc
151-165: Consider refactoring the search state management.
- Extract the debounced search logic into a custom hook
- Combine related state into a single reducer
- Use TypeScript discriminated unions for the status state
Example custom hook:
function useDebounceSearch(searchTerm: string, delay: number) { const [state, dispatch] = useReducer(searchReducer, initialState); useEffect(() => { if (searchTerm.length < 3) { dispatch({ type: 'RESET' }); return; } const timer = setTimeout(() => { dispatch({ type: 'START_SEARCH', payload: searchTerm }); }, delay); return () => clearTimeout(timer); }, [searchTerm, delay]); return state; }Also applies to: 170-261
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
design-system/IconButton/IconButton.tsx(2 hunks)design-system/chip.tsx(1 hunks)features/conversation-list/conversation-list.screen.tsx(3 hunks)features/conversation/conversation-composer/conversation-composer.tsx(4 hunks)features/groups/invite-to-group/invite-users-to-exisiting-group.screen.tsx(1 hunks)features/search/components/ProfileSearchItem.tsx(3 hunks)features/search/components/ProfileSearchResultsList.tsx(1 hunks)screens/Navigation/Navigation.tsx(4 hunks)screens/NewConversation/NewConversation.tsx(5 hunks)screens/NewConversation/newChat.styles.tsx(1 hunks)utils/api.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- features/conversation-list/conversation-list.screen.tsx
- features/conversation/conversation-composer/conversation-composer.tsx
- screens/NewConversation/newChat.styles.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
screens/NewConversation/NewConversation.tsx
[error] 86-86: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (6)
design-system/chip.tsx (2)
1-22: Well-documented component with clear examples and design reference!The documentation includes:
- Clear description of the component's purpose
- Link to Figma design specs
- Usage examples for both avatar and non-avatar variants
32-38: LGTM! Type definitions are clear and well-structured.Props interface follows TypeScript best practices with proper optional properties.
design-system/IconButton/IconButton.tsx (1)
98-107: LGTM! Improved disabled state handling.Good addition of the disabled check before executing the press handler and proper update of the dependency array.
features/search/components/ProfileSearchItem.tsx (2)
11-21: LGTM! Props and imports have been updated to support the new search functionality.The changes align well with the component's new purpose, removing navigation-related props and adding a callback for handling search result item presses.
98-120: LGTM! Styling improvements enhance consistency.The updated styles:
- Use proper spacing and colors from the theme
- Maintain consistent avatar sizing
- Follow proper text styling conventions
screens/Navigation/Navigation.tsx (1)
28-28: Clean up navigation imports and verify navigation flow.
- The
NewConversationNavimport might be unused- Verify that
InviteUsersToExistingGroupNavis properly integrated into the navigation flowRun this script to verify the navigation flow:
Also applies to: 146-146
features/groups/invite-to-group/invite-users-to-exisiting-group.screen.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
screens/NewConversation/NewConversation.tsx (2)
1-42: Convert TODO comments into tracked issuesThe file contains several untracked bugs and tasks. These should be converted into GitHub issues for proper tracking and visibility.
Would you like me to help create GitHub issues for each of these items? I can help generate properly formatted issue descriptions for:
- Chat loading issues (lines 4-5)
- Performance bottlenecks (lines 7-11)
- Group name creation bug (lines 13-14)
- UI improvements (lines 19-26)
- Code organization tasks (lines 28-33)
Line range hint
152-350: Extract search logic into a custom hookThe search functionality is complex and could be reused in other parts of the application. Consider extracting it into a custom hook like
useProfileSearchto improve maintainability and reusability.This would:
- Reduce component complexity
- Make the search logic reusable
- Improve testability
- Separate concerns
Example structure:
function useProfileSearch() { const [query, setQuery] = useState(""); const [status, setStatus] = useState({/*...*/}); // ... rest of the search logic return { query, setQuery, status, // ... other relevant state and methods }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
screens/NewConversation/NewConversation.tsx(5 hunks)screens/NewConversation/newChat.styles.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- screens/NewConversation/newChat.styles.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
screens/NewConversation/NewConversation.tsx
[error] 87-87: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (1)
screens/NewConversation/NewConversation.tsx (1)
86-86: Add proper type annotations for component propsThe empty object pattern suggests missing type annotations. This should be properly typed, especially since it's a screen component.
| onSend={async (something) => { | ||
| const dmCreationMessageText = something.content.text || ""; | ||
| if ( | ||
| !dmCreationMessageText || | ||
| dmCreationMessageText.length === 0 | ||
| ) { | ||
| return; | ||
| } | ||
| logger.info( | ||
| "[NewConversation] Sending message", | ||
| something.content.text | ||
| ); | ||
|
|
||
| if (conversationCreationMode === ConversationVersion.DM) { | ||
| let dm = await getOptionalConversationByPeerByAccount({ | ||
| account: currentAccount(), | ||
| peer: pendingChatMembers.members[0].address, | ||
| }); | ||
| if (!dm) { | ||
| dm = await createConversationByAccount( | ||
| currentAccount(), | ||
| pendingChatMembers.members[0].address | ||
| ); | ||
| } | ||
| await sendMessage({ | ||
| conversation: dm, | ||
| params: { | ||
| content: { text: dmCreationMessageText }, | ||
| }, | ||
| }); | ||
| setConversationQueryData({ | ||
| account: currentAccount(), | ||
| topic: dm.topic, | ||
| conversation: dm, | ||
| }); | ||
| // @ts-expect-error | ||
| navigation.replace("Conversation", { topic: dm.topic }); | ||
| } else { | ||
| const group = await createGroupWithDefaultsByAccount({ | ||
| account: currentAccount(), | ||
| peerEthereumAddresses: pendingChatMembers.members.map( | ||
| (m) => m.address | ||
| ), | ||
| }); | ||
| await sendMessage({ | ||
| conversation: group, | ||
| params: { | ||
| content: { text: dmCreationMessageText }, | ||
| }, | ||
| }); | ||
| setConversationQueryData({ | ||
| account: currentAccount(), | ||
| topic: group.topic, | ||
| conversation: group, | ||
| }); | ||
| // @ts-expect-error | ||
| navigation.replace("Conversation", { topic: group.topic }); | ||
| } | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in message sending logic
The message sending logic lacks proper error handling for network failures or other potential issues.
Wrap the async operations in try-catch blocks and handle potential failures gracefully:
try {
// Existing message sending logic
} catch (error) {
logger.error('[NewConversation] Failed to send message', { error });
// Show error to user
// Consider retry logic for transient failures
}
Happy to chat about any patterns you want to align with any time @thierryskoda |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
screens/NewConversation/NewConversation.tsx (4)
127-166: Optimize useEffect dependencies.The effect's dependency array includes
themedandtheme.spacingwhich aren't used within the effect. Remove these unused dependencies to prevent unnecessary re-renders.}, [ pendingChatMembersCount, conversationCreationMode, loading, navigation, handleBack, - themed, - theme.spacing, ]);
Line range hint
167-365: Consider extracting search logic into a custom hook.The search functionality is complex and handles multiple responsibilities. Consider extracting it into a custom hook (e.g.,
useDebounceSearch) to improve maintainability and reusability.Example structure:
const useDebounceSearch = (initialQuery = '') => { const [query, setQuery] = useState(initialQuery); const [status, setStatus] = useState({ loading: false, error: '', results: {} }); useEffect(() => { // Current search logic }, [query]); return { query, setQuery, status }; };
402-495: Consider extracting conditional rendering logic into separate components.The component has multiple conditional renders that could be extracted into separate components for better maintainability:
- PendingMembers
- SearchResults
- ErrorMessage
This would make the main component more concise and easier to test.
498-500: Remove TODO comment if the pattern has been reviewed.The TODO comment about reviewing the pattern with Thierry should be removed if the discussion has already taken place.
- {/* todo: review this pattern with Thierry */} <ConversationComposerStoreProvider storeName={"new-conversation" as ConversationTopic} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/ClickableText.tsx(2 hunks)screens/NewConversation/NewConversation.tsx(5 hunks)utils/api.ts(1 hunks)utils/xmtpRN/conversations.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- utils/api.ts
- components/ClickableText.tsx
- utils/xmtpRN/conversations.ts
🔇 Additional comments (2)
screens/NewConversation/NewConversation.tsx (2)
86-101: Well-typed component definition.The component has proper type definitions for navigation props, which is a good practice for type safety.
505-562:⚠️ Potential issueAdd error handling for message sending operations.
The message sending logic lacks try-catch blocks for handling potential failures in conversation creation and message sending operations.
Add proper error handling:
onSend={async (something) => { + try { const dmCreationMessageText = something.content.text || ""; if ( !dmCreationMessageText || dmCreationMessageText.length === 0 ) { return; } // ... existing logic ... + } catch (error) { + logger.error('[NewConversation] Failed to send message', { error }); + // Show error to user + } }}Likely invalid or redundant comment.
| /** | ||
| * BUGS: | ||
| * | ||
| * chat is not loaded properly from cold start until chat is visited | ||
| * solution: proper persistence | ||
| * | ||
| * chat flow feels slow on app first launch | ||
| * solution: proper persistence | ||
| * | ||
| * group flow feels slow | ||
| * solution: requires diagnosing bottleneck | ||
| * | ||
| * conversation list is not updated with most recent dm when sent from composer | ||
| * solution: maybe using the hook instead of imperatively doing stuff? | ||
| * | ||
| * group name creation not correct: getting first preferred name correct but | ||
| * ✅ not using correct preferred name for last two when ephemeral accounts | ||
| * | ||
| * messed up old add user to existing group screen | ||
| * ✅ solution: copy existing screen back in | ||
| * | ||
| * UI: | ||
| * | ||
| * ✅ search results list needs updating | ||
| * ✅ create chips for search results | ||
| * ✅ composer disable send button | ||
| * ✅ composer hide plus button | ||
| * ✅ pending members list | ||
| * | ||
| * new group joined the group welcome message needs wrapping (not my problem atm) | ||
| * | ||
| * CODE ORG: | ||
| * change file names to lower-kebab-case | ||
| * move files to features/new-conversation | ||
| * rename files to indicate where they live within new-conversation domain | ||
| * use proper suffixes | ||
| * | ||
| * --- | ||
| * | ||
| * GITHUB: | ||
| * https://github.com/ephemeraHQ/converse-app/issues/1498 | ||
| * | ||
| * FIGMA: | ||
| * new composer: https://www.figma.com/design/p6mt4tEDltI4mypD3TIgUk/Converse-App?node-id=5026-26989&m=dev | ||
| * search results list: https://www.figma.com/design/p6mt4tEDltI4mypD3TIgUk/Converse-App?node-id=5191-4200&t=KDRZMuK1xpiNBKG9-4 | ||
| */ |
There was a problem hiding this comment.
💡 Codebase verification
Files need to be moved and renamed to match feature-based architecture
The codebase follows a clear feature-based organization pattern, and these files should be reorganized:
- Move all NewConversation files to
features/new-chat/ - Rename files to follow kebab-case convention:
NewConversation.tsx→new-chat.screen.tsxNewConversationNav.tsx→new-chat.nav.tsxNewConversationModal.tsx→new-chat-modal.tsxnewChat.styles.tsx→new-chat.styles.ts
This matches the established patterns seen in other features like conversation-list and blocked-chats.
🔗 Analysis chain
Documentation provides good visibility but highlights pending tasks.
The extensive documentation of bugs and tasks is helpful. However, several code organization tasks are still pending:
- Files need to be moved to features/new-conversation
- File names need to be changed to lower-kebab-case
- Files need proper suffixes
Let's verify the current file organization:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check current file organization
echo "Current file location:"
fd -p "NewConversation.tsx$"
echo "\nFiles that should be moved to features/new-conversation:"
fd -e tsx -e ts "NewConversation|newChat"
Length of output: 478
Script:
#!/bin/bash
# Check if features directory exists and its structure
echo "Features directory structure:"
fd -t d "features" -d 1
echo "\nChecking for any additional related files:"
rg -l "NewConversation|newChat" --type ts --type tsx
echo "\nChecking imports in NewConversation files:"
rg "^import.*from.*NewConversation" --type ts --type tsx
Length of output: 470
Script:
#!/bin/bash
# Check features directory content
echo "Features directory content:"
fd . features/
echo "\nChecking for additional related files:"
rg -t typescript -t tsx -l "NewConversation|newChat"
echo "\nChecking imports in NewConversation files:"
rg -t typescript -t tsx "^import .* from ['\"].*NewConversation.*['\"]"
echo "\nChecking current imports in NewConversation.tsx:"
cat screens/NewConversation/NewConversation.tsx | grep -E "^import"
Length of output: 18491
|
discovered this while testing on Android |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
features/notifications/utils/index.ts (1)
40-41: Improve type safety for notification data handling.The use of
anytype and duplicate type casting could be improved with a proper type definition.Consider defining an interface for the notification data structure:
interface NotificationData { notification?: { data?: { body?: string; data?: string; }; }; }Then update the extraction:
const notificationBody = ((data as NotificationData).notification?.data?.body) || ((data as NotificationData).notification?.data?.data);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
App.tsx(2 hunks)features/notifications/utils/index.ts(1 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: performance-test
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (4)
features/notifications/utils/index.ts (2)
32-34: Well-structured error logging format!The template literal formatting with platform and task context improves error message readability and debugging capabilities.
42-47: Good defensive programming with early return!The null check and early return with error logging improves the robustness of the notification handling.
Let's verify the error logging implementation across the codebase:
✅ Verification successful
Error logging implementation verified and consistent!
The error logging pattern in the code follows established practices across the codebase, using consistent formatting with Platform.OS for notification-related errors and proper error context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent error logging patterns rg -g '*.ts' -g '*.tsx' 'logger\.error.*Platform\.OS.*BACKGROUND.*TASK'Length of output: 73
Script:
#!/bin/bash # Search for logger.error usage patterns rg "logger\.error" -A 2 # Search for Platform.OS usage in logging rg "Platform\.OS.*logger\." -A 2 # Look for BACKGROUND_NOTIFICATION_TASK constant definition and usage rg "BACKGROUND_NOTIFICATION_TASK" -A 2 # Use AST to find error logging in if statements ast-grep --pattern 'if ($condition) { logger.error($$$) }'Length of output: 14991
App.tsx (2)
3-6: LGTM!The new imports from react-native-reanimated are properly structured and necessary for the logger configuration.
Line range hint
186-195: Review the commented theme implementation.The
useStyleshook has commented out theme-related code. This could lead to inconsistent theming across the app. Either:
- Remove the commented code if it's no longer needed
- Implement the theme properly if it's required
Let's check theme usage across the app:
✅ Verification successful
Remove the commented theme code
The commented theme-related code in
App.tsxcan be safely removed. The codebase shows extensive and consistent theme implementation across components, indicating that this commented code is likely obsolete or intentionally moved elsewhere.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for theme usage patterns rg "useAppTheme|theme\.colors" # Check if backgroundColor is consistently applied rg "backgroundColor:"Length of output: 53565
| // This is the default configuration | ||
| configureReanimatedLogger({ | ||
| level: ReanimatedLogLevel.warn, | ||
| strict: /* | ||
| Ignores the following warning: | ||
| "[Reanimated] Reading from `value` during component render. Please ensure that you do not access the `value` property or use `get` method of a shared value while React is rendering a component.", | ||
| todo investigate | ||
|
|
||
| */ false, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve the TODO comment and reconsider strict mode configuration.
- The TODO comment should follow a standard format with ownership and timeline:
-todo investigate
+TODO(owner): Investigate shared value access during render and determine if strict mode can be enabled
+Target: DD/MM/YYYY- Disabling strict mode to ignore warnings is not recommended as it may mask other issues. Consider keeping strict mode enabled and properly fixing the underlying shared value access issue.
Committable suggestion skipped: line range outside the PR's diff.
| "[Reanimated] Reading from `value` during component render. Please ensure that you do not access the `value` property or use `get` method of a shared value while React is rendering a component.", | ||
| "Attempted to import the module", |
There was a problem hiding this comment.
💡 Codebase verification
Fix shared value access in JoinGroup screen
The warning is caused by direct access to state.value during render in features/GroupInvites/joinGroup/JoinGroup.screen.tsx. To fix this:
- Move the value access into a derived value using
useDerivedValue - Or store the value in a regular React state if animation isn't needed
// Before
const pollingTimedOut = state.value === "Attempting to Join Group Timed Out";
// After
const pollingTimedOut = useDerivedValue(() =>
state.value === "Attempting to Join Group Timed Out"
);🔗 Analysis chain
Investigate the root cause of the Reanimated warning.
While ignoring the warning allows the app to function, it masks a potential performance or correctness issue. The warning indicates that shared values are being accessed during component rendering, which could lead to unnecessary re-renders or race conditions.
Let's search for potential causes:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct value access patterns in animation code
rg -A 2 "useAnimatedStyle|useSharedValue|SharedValue"
# Search for .value access during render
ast-grep --pattern 'const $_ = $_?.value'
Length of output: 20729
Script:
#!/bin/bash
# Search for shared value access in hooks and handlers
rg -A 2 "useEffect.*\.value|useMemo.*\.value|const.*=.*\.value"
# Look for value access in event handlers
ast-grep --pattern 'onPress={$_}'
# Search for any remaining direct value access
ast-grep --pattern '$_.value'
Length of output: 10328
| // import { NewChatNavParams } from "../Navigation/NewConversationNav"; | ||
| // import { translate } from "@/i18n"; | ||
| // import logger from "@/utils/logger"; | ||
| // import { Button } from "@/design-system/Button/Button"; |
There was a problem hiding this comment.
I don't love things being commented out I'd rather just see them deleted.
closes #1498
Finished Loom ios: https://www.loom.com/share/c1de6efd778745e2893e8e42d793f3b1
Finished Loom android: https://www.loom.com/share/b24bfe7fe9624c3abe238608e2b932c0
I'm okay to get this merged and fast follow with code cleanup and perf 👌
TODO
Summary by CodeRabbit
Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the high-level release notes:
New Features
Chipcomponent for user selection and filtering.User Interface Updates
Performance and Maintenance
Internationalization
Developer Experience
expo-module-scripts.These release notes provide a concise overview of the key changes while maintaining confidentiality about internal implementation details.