feat: add context menu to messages #1292
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces several significant changes across multiple components in the chat application. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant Loader
participant MessageService
participant GroupService
User->>App: Initiate loading
App->>Loader: Show loading indicator
Loader-->>App: Loading in progress
App->>MessageService: Fetch messages
MessageService-->>App: Messages fetched
App->>Loader: Hide loading indicator
App->>User: Display messages
User->>GroupService: Request group info
GroupService-->>User: Group info received
Possibly related PRs
Suggested labels
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 (
|
thierryskoda
changed the base branch from
release/3.0.0
to
ts/message-refactoring-attachments
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (38)
components/Chat/Message/message-content-types/message-simple-text.tsx (3)
46-136:
⚠️ Potential issueRemove commented-out context menu implementation.
The PR title indicates adding a context menu feature, but the implementation is commented out with a note about missing library functionality. This raises several concerns:
- Large blocks of commented code shouldn't be merged into production.
- The code contains debugging artifacts (debugBorder).
- Multiple attempted approaches suggest the solution isn't finalized.
Recommendations:
- Remove all commented-out code
- Create a separate feature branch for the context menu implementation
- Document the attempted approaches and challenges in the project wiki or issues
- Consider splitting this PR into two:
- One for the basic MessageSimpleText component
- Another for the context menu feature when it's ready
Would you like me to help create a GitHub issue to track the context menu implementation separately?
37-45:
⚠️ Potential issueRemove unused library imports.
The commented-out imports for context menu libraries should be removed until the implementation is ready. Keeping unused imports makes the code harder to maintain and can lead to confusion.
-// import { MessageContextMenu } from "@/components/Chat/Message/MessageContextMenu"; -// import ContextMenu from "react-native-context-menu-view"; -// import * as ContextMenu from "zeego/context-menu";📝 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./** * Tried different approaches to implement native context menu, but it's not * working as expected. Still missing some pieces from libraries to achieve what we want */
12-35: 🛠️ Refactor suggestion
Consider adding error handling for message content extraction.
While the implementation is clean and well-structured, the
message.content()call on line 17 lacks error handling. This could potentially throw an error if the message content is corrupted or invalid.Consider adding a try-catch block:
- const textContent = message.content(); + let textContent: string; + try { + textContent = message.content(); + } catch (error) { + console.error('Failed to decode message content:', error); + textContent = 'Message cannot be displayed'; + }📝 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.export const MessageSimpleText = memo(function MessageSimpleText(props: { message: DecodedMessage<[TextCodec]>; }) { const { message } = props; let textContent: string; try { textContent = message.content(); } catch (error) { console.error('Failed to decode message content:', error); textContent = 'Message cannot be displayed'; } const { hasNextMessageInSeries, fromMe } = useMessageContextStoreContext( useSelect(["hasNextMessageInSeries", "fromMe"]) ); return ( <MessageLayout> <BubbleContainer fromMe={fromMe}> <BubbleContentContainer fromMe={fromMe} hasNextMessageInSeries={hasNextMessageInSeries} > <MessageText inverted={fromMe}>{textContent}</MessageText> </BubbleContentContainer> </BubbleContainer> </MessageLayout> ); });queries/useRevokeSuperAdminMutation.ts (1)
18-18: 🛠️ Refactor suggestion
Remove redundant null checks for
topicSince
topicis now a required parameter of typeConversationTopic, the null checks throughout the code (e.g., inmutationFnandonError) are unnecessary and can be safely removed.screens/Navigation/Navigation.tsx (1)
153-156: 🛠️ Refactor suggestion
Add screen options for consistency with other screens.
The DmConversation screen is missing screen options that are present in other screens. Consider adding header configuration for consistent UI/UX.
Here's a suggested improvement:
<NativeStack.Screen name="DmConversation" component={DmConversationScreen} + options={{ + headerLargeTitle: true, + headerShadowVisible: false, + headerBackTitle: "Back", + headerBackTitleVisible: false, + }} />Committable suggestion skipped: line range outside the PR's diff.
features/conversation/dm-conversation.screen.tsx (3)
48-48: 🛠️ Refactor suggestion
Avoid Suppressing TypeScript Errors with
@ts-ignoreUsing
// @ts-ignoresuppresses TypeScript errors, which can conceal underlying issues and reduce type safety. It's better to resolve the type error to ensure robust and maintainable code.Consider properly typing
props.route.paramsby adjusting the type definitions or casting to the appropriate type to eliminate the need for@ts-ignore.
77-77: 🛠️ Refactor suggestion
Simplify Boolean Expression by Removing Redundant Double Negation
The expression
!!conversationuses a double negation, which is unnecessary since JavaScript coerces expressions in conditional statements to boolean values. Removing the double negation improves readability.Apply this diff to simplify the expression:
- {!!conversation ? ( + {conversation ? (📝 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.{conversation ? (🧰 Tools
🪛 Biome (1.9.4)
[error] 77-77: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
228-236:
⚠️ Potential issueCorrect Typographical Error in Parameter Name
peerAddresssThe parameter
peerAddresssin theuseNewConversationHeaderfunction has an extra 's'. To maintain consistency and prevent potential bugs, rename it topeerAddress.Apply this diff to fix the typo:
-function useNewConversationHeader(peerAddresss: string) { +function useNewConversationHeader(peerAddress: string) {Update all occurrences within the function:
- navigation.setOptions({ - headerTitle: () => <NewConversationTitle peerAddress={peerAddresss} />, - }); + navigation.setOptions({ + headerTitle: () => <NewConversationTitle peerAddress={peerAddress} />, + });And adjust the dependency array in the
useEffecthook:- }, [peerAddresss, navigation]); + }, [peerAddress, 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.function useNewConversationHeader(peerAddress: string) { const navigation = useRouter(); useEffect(() => { navigation.setOptions({ headerTitle: () => <NewConversationTitle peerAddress={peerAddress} />, }); }, [peerAddress, navigation]); }components/Chat/Message/message-content-types/message-static-attachment.tsx (2)
66-67: 🛠️ Refactor suggestion
Handle potential errors when writing the attachment file
Currently, the code assumes that
RNFS.writeFilewill always succeed. It's good practice to handle potential errors that may occur during file writing to prevent unexpected crashes or silent failures.Consider wrapping the file writing operation in a try-catch block to handle any exceptions:
try { await RNFS.writeFile(attachmentPath, staticAttachment.data, "base64"); } catch (error) { throw new Error(`Failed to write attachment file: ${error.message}`); }📝 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.try { await RNFS.writeFile(attachmentPath, staticAttachment.data, "base64"); } catch (error) { throw new Error(`Failed to write attachment file: ${error.message}`); }
92-94:
⚠️ Potential issueAvoid potential division by zero when calculating
aspectRatioWhen calculating
aspectRatio, there is a possibility of a division by zero ifattachment.imageSize.heightis zero, which would cause a runtime error.Add a check to ensure
attachment.imageSize.heightis not zero before performing the division:const aspectRatio = attachment.imageSize && attachment.imageSize.height !== 0 ? attachment.imageSize.width / attachment.imageSize.height : undefined;📝 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.const aspectRatio = attachment.imageSize && attachment.imageSize.height !== 0 ? attachment.imageSize.width / attachment.imageSize.height : undefined;components/Chat/Message/message-gestures.tsx (1)
84-85:
⚠️ Potential issueAvoid calling
runOnJSinsiderunOnJSThe
runOnJSfunction is used inside anonStartcallback that already usesrunOnJS. This can lead to unexpected behavior and performance issues.Modify the code to use
runOnJSonce, and ensure callbacks are correctly handled:.onStart((e) => { Haptics.softImpactAsyncAnimated(); scaleAV.value = withTiming(MESSAGE_GESTURE_LONG_PRESS_SCALE, { duration: MESSAGE_GESTURE_LONG_PRESS_MIN_DURATION * 2, easing: Easing.bezier(0.31, 0.04, 0.03, 1.04), }); const measured = measure(containerRef); if (!measured) return; if (onLongPress) { - runOnJS(onLongPress)({ ...e, ...measured }); + onLongPress({ ...e, ...measured }); } })Ensure that
onLongPressis wrapped withrunOnJSonly once in the gesture configuration.Committable suggestion skipped: line range outside the PR's diff.
components/Chat/ConsentPopup/dm-consent-popup.tsx (1)
102-105: 🛠️ Refactor suggestion
Avoid swallowing errors in
handleAcceptThe
captureErrorWithToastfunction captures the error but may not provide detailed feedback to the user.Consider providing a more descriptive error message and ensure that all errors are properly logged:
} catch (error) { captureErrorWithToast(ensureError(error), { - message: `Error consenting`, + message: translate("error_consenting", { error: error.message }), }); }Committable suggestion skipped: line range outside the PR's diff.
components/Chat/Message/V3Message.tsx (1)
77-79: 🛠️ Refactor suggestion
Handle null or undefined
messagegracefullyLogging "no message found" may not be sufficient. It's essential to handle the absence of a message appropriately.
Consider displaying a placeholder or fallback UI component instead of returning
null.if (!message) { - console.log("no message found", messageId); + return <Text>Message not found</Text>; }Ensure that the
Textcomponent is imported from the appropriate library.Committable suggestion skipped: line range outside the PR's diff.
components/Chat/Attachment/remote-attachment-image.tsx (1)
77-81:
⚠️ Potential issueImplement the 'openInWebview' functionality for unsupported media types
The
onPresshandler in thePressableScalecomponent is currently empty, with a placeholder comment// openInWebview. This means that users are unable to view unsupported media types as intended.Would you like assistance in implementing the
openInWebviewfunctionality, or should I open a GitHub issue to track this task?hooks/useGroupConsent.ts (1)
150-154:
⚠️ Potential issueAwait the call to
consentToInboxIdsOnProtocolByAccountto ensure it completesCurrently, the call to
consentToInboxIdsOnProtocolByAccountis not awaited, which may result in the function returning before the consent operation completes. This could lead to unexpected behavior.Apply this diff to fix the issue:
if (inboxIdsToDeny.length > 0) { - consentToInboxIdsOnProtocolByAccount({ + await consentToInboxIdsOnProtocolByAccount({ account, inboxIds: inboxIdsToDeny, consent: "deny", }); }📝 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.if (includeCreator && groupCreator) { inboxIdsToDeny.push(groupCreator); } if (inboxIdsToDeny.length > 0) { await consentToInboxIdsOnProtocolByAccount({ account, inboxIds: inboxIdsToDeny, consent: "deny", }); }components/Chat/Message/components/message-layout.tsx (1)
145-145: 🛠️ Refactor suggestion
Remove redundant double-negation in condition
The double-negation
!!is unnecessary since the condition in theifstatement will coercemessageStringContentto a boolean automatically.Apply this diff to remove the redundant double-negation:
- if (!!messageStringContent) { + if (messageStringContent) {📝 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.if (messageStringContent) {🧰 Tools
🪛 Biome (1.9.4)
[error] 145-145: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
components/Chat/ChatGroupUpdatedMessage.tsx (1)
33-35:
⚠️ Potential issueHandle string content returned by
message.content()The check for
typeof content === "string"leads to areturn nullwith a TODO comment. This indicates that handling for string content is not implemented, which might cause issues ifmessage.content()returns a string.Would you like assistance in implementing the handling for string content, or should I open a GitHub issue to track this?
components/Chat/Message/message-content-types/message-reply.tsx (1)
174-183:
⚠️ Potential issueEnsure Safe Access of Nested Properties
When accessing
contentfromreplyMessage.content(), ensure thatcontentis defined and has the expected structure before accessing nested properties likecontent.content.remoteAttachment. Adding type checks or null checks can prevent potential runtime errors due to undefined values.components/Chat/Message/MessageReactionsList.tsx (1)
283-287:
⚠️ Potential issueReactivate Reaction Handling Logic in
EmojiItemThe core logic for adding or removing reactions in the
handlePressfunction is currently commented out. This prevents users from being able to interact with message reactions as intended. Please uncomment and verify this functionality to ensure that users can add or remove reactions.components/Chat/Message/components/message-space-between-messages.tsx (1)
5-10: 🛠️ Refactor suggestion
Consider making this component more reusable.
The component is currently very specific to message spacing, but it could be made more reusable by accepting custom spacing values.
Consider this more flexible approach:
+type Props = { + spacing?: keyof typeof theme.spacing; +}; + export const MessageSpaceBetweenMessages = memo( - function MessageSpaceBetweenMessages() { + function MessageSpaceBetweenMessages({ spacing = "4xs" }: Props) { const { theme } = useAppTheme(); - return <VStack style={{ height: theme.spacing["4xs"] }} />; + return <VStack style={{ height: theme.spacing[spacing] }} />; } );Alternatively, consider creating a more generic
Spacercomponent that could be used throughout the app:type SpacerProps = { size: keyof typeof theme.spacing; direction?: "vertical" | "horizontal"; }; export const Spacer = memo(function Spacer({ size, direction = "vertical" }: SpacerProps) { const { theme } = useAppTheme(); return ( <div style={{ [direction === "vertical" ? "height" : "width"]: theme.spacing[size], }} /> ); });components/Chat/Message/components/message-container.tsx (1)
13-26: 🛠️ Refactor suggestion
Add accessibility attributes for screen readers.
Messages should be properly announced to screen readers. Consider adding appropriate ARIA attributes.
<HStack + accessibilityRole="text" + accessibilityLabel={`Message ${fromMe ? 'sent' : 'received'}`} style={{ flex: 1, alignItems: "flex-end", ...(fromMe📝 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.return ( <HStack accessibilityRole="text" accessibilityLabel={`Message ${fromMe ? 'sent' : 'received'}`} style={{ // ...debugBorder("blue"), flex: 1, alignItems: "flex-end", ...(fromMe ? { justifyContent: "flex-end" } : { justifyContent: "flex-start" }), }} > {children} </HStack> );components/Chat/Message/message-content-types/message-remote-attachment.tsx (2)
20-28: 🛠️ Refactor suggestion
Consider adding loading and error states
The component should handle loading and error states for remote attachments to improve user experience.
return ( <MessageLayout> + <ErrorBoundary fallback={<Text>Failed to load attachment</Text>}> + <Suspense fallback={<Loader />}> <RemoteAttachmentImage messageId={message.id} remoteMessageContent={content} fitAspectRatio /> + </Suspense> + </ErrorBoundary> </MessageLayout> );Committable suggestion skipped: line range outside the PR's diff.
15-18:
⚠️ Potential issueImplement error handling for string content
The TODO comment and returning
nullfor string content might lead to silent failures. Consider implementing proper error handling or logging.if (typeof content === "string") { - // TODO - return null; + console.warn(`Unexpected string content in RemoteAttachment: ${content}`); + return <MessageLayout><Text>Invalid attachment format</Text></MessageLayout>; }Committable suggestion skipped: line range outside the PR's diff.
queries/useConversationWithPeerQuery.ts (2)
17-29: 🛠️ Refactor suggestion
Enhance error handling and logging.
The current implementation could benefit from better error handling and more specific logging.
queryFn: async () => { - logger.info("[Crash Debug] queryFn fetching conversation with peer"); + logger.info(`[Conversation] Fetching conversation for peer: ${peer}`); if (!peer) { return null; } - const conversation = await getConversationByPeerByAccount({ - account, - peer, - includeSync: true, - }); - - return conversation; + try { + return await getConversationByPeerByAccount({ + account, + peer, + includeSync: true, + }); + } catch (error) { + logger.error(`[Conversation] Failed to fetch conversation: ${error}`); + throw error; + }📝 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.queryFn: async () => { logger.info(`[Conversation] Fetching conversation for peer: ${peer}`); if (!peer) { return null; } try { return await getConversationByPeerByAccount({ account, peer, includeSync: true, }); } catch (error) { logger.error(`[Conversation] Failed to fetch conversation: ${error}`); throw error; } },
14-17:
⚠️ Potential issueRemove non-null assertion operator for safer type handling.
The non-null assertion (
!) onpeerin the query key could lead to runtime errors. Consider using a safer approach.- queryKey: conversationWithPeerQueryKey(account, peer!), + queryKey: conversationWithPeerQueryKey(account, peer ?? ''),Committable suggestion skipped: line range outside the PR's diff.
design-system/BlurView.tsx (1)
3-3:
⚠️ Potential issueRemove unused FadeIn import.
The
FadeInanimation is imported but never used in the component.-import Animated, { AnimatedProps, FadeIn } from "react-native-reanimated"; +import Animated, { AnimatedProps } from "react-native-reanimated";📝 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.import Animated, { AnimatedProps } from "react-native-reanimated";queries/useDmConstentStateQuery.ts (3)
14-14:
⚠️ Potential issueAvoid non-null assertions in TypeScript
The non-null assertions (
!) ontopicanddmConversationcould lead to runtime errors if these values are undefined. Consider implementing proper null checks or providing default values.- const { data: dmConversation } = useConversationQuery(account, topic!); + const { data: dmConversation } = useConversationQuery(account, topic); return useQuery({ - queryKey: dmConsentQueryKey(account, topic!), + queryKey: topic ? dmConsentQueryKey(account, topic) : undefined, queryFn: () => dmConversation!.consentState(),Also applies to: 18-18
18-20:
⚠️ Potential issueImprove error handling for queryFn
The
queryFnuses a non-null assertion and lacks error handling. Consider adding proper error handling and type checking.- queryFn: () => dmConversation!.consentState(), + queryFn: async () => { + if (!dmConversation) { + throw new Error('Conversation not found'); + } + return dmConversation.consentState(); + },📝 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.queryFn: async () => { if (!dmConversation) { throw new Error('Conversation not found'); } return dmConversation.consentState(); }, enabled: !!dmConversation && !!topic, initialData: dmConversation?.state,
46-54: 🛠️ Refactor suggestion
Add error handling to cancelDmConsentQuery
The
cancelDmConsentQueryfunction should handle potential errors during query cancellation.export const cancelDmConsentQuery = async (args: { account: string; topic: ConversationTopic; }) => { const { account, topic } = args; - await queryClient.cancelQueries({ - queryKey: dmConsentQueryKey(account, topic), - }); + try { + await queryClient.cancelQueries({ + queryKey: dmConsentQueryKey(account, topic), + }); + } catch (error) { + console.error('Failed to cancel DM consent query:', error); + throw error; + } };📝 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.export const cancelDmConsentQuery = async (args: { account: string; topic: ConversationTopic; }) => { const { account, topic } = args; try { await queryClient.cancelQueries({ queryKey: dmConsentQueryKey(account, topic), }); } catch (error) { console.error('Failed to cancel DM consent query:', error); throw error; } };features/conversations/components/V3ConversationFromPeer.tsx (1)
1-2: 🛠️ Refactor suggestion
Update ActivityIndicator import to use new Loader component
According to the AI summary, ActivityIndicator is being replaced with the new Loader component across the codebase.
-import { useConversationWithPeerQuery } from "@/queries/useConversationWithPeerQuery"; -import ActivityIndicator from "@components/ActivityIndicator/ActivityIndicator"; +import { useConversationWithPeerQuery } from "@/queries/useConversationWithPeerQuery"; +import { Loader } from "@design-system/loader";📝 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.import { useConversationWithPeerQuery } from "@/queries/useConversationWithPeerQuery"; import { Loader } from "@design-system/loader";queries/useConversationQuery.ts (1)
17-27: 🛠️ Refactor suggestion
Consider adding error handling for undefined topic
While the code uses non-null assertion (
topic!), it would be safer to handle undefined topics explicitly since the function acceptsundefinedas a valid type.export const useConversationQuery = ( account: string, topic: ConversationTopic | undefined, options?: Partial<UseQueryOptions<ConversationQueryData | null | undefined>> ) => { + if (!topic) { + return { + data: undefined, + isLoading: false, + error: new Error('Topic is required') + }; + } return useQuery({ ...options, - queryKey: conversationQueryKey(account, topic!), - queryFn: () => getConversation(account, topic!), + queryKey: conversationQueryKey(account, topic), + queryFn: () => getConversation(account, topic), enabled: !!topic, }); };📝 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.export const useConversationQuery = ( account: string, topic: ConversationTopic | undefined, options?: Partial<UseQueryOptions<ConversationQueryData | null | undefined>> ) => { if (!topic) { return { data: undefined, isLoading: false, error: new Error('Topic is required') }; } return useQuery({ ...options, queryKey: conversationQueryKey(account, topic), queryFn: () => getConversation(account, topic), enabled: !!topic, }); };components/Chat/ConsentPopup/group-consent-popup.tsx (2)
23-23:
⚠️ Potential issueAvoid non-null assertion on currentAccount
The non-null assertion (
!) onuseCurrentAccount()could lead to runtime errors if the account is not available. Consider handling the null case gracefully.- const currentAccount = useCurrentAccount()!; + const currentAccount = useCurrentAccount(); + if (!currentAccount) { + return null; // or handle the no-account case appropriately + }📝 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.const currentAccount = useCurrentAccount(); if (!currentAccount) { return null; // or handle the no-account case appropriately }
46-55:
⚠️ Potential issueInconsistent navigation behavior between accept and block actions
The
onBlockaction navigates on success whileonAcceptdoesn't handle navigation. This could lead to inconsistent UX.const onAccept = useCallback(async () => { try { await allowGroup({ includeCreator: false, includeAddedBy: false, }); + navigation.pop(); } catch (error) { captureErrorWithToast(error); } - }, [allowGroup]); + }, [allowGroup, 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.const onAccept = useCallback(async () => { try { await allowGroup({ includeCreator: false, includeAddedBy: false, }); navigation.pop(); } catch (error) { captureErrorWithToast(error); } }, [allowGroup, navigation]);components/Chat/Message/message-utils.tsx (1)
151-217: 🛠️ Refactor suggestion
Consider refactoring message content extraction for better maintainability
The
getMessageStringContentfunction is quite long and handles multiple message types. Consider breaking it down into smaller, type-specific functions.+function getReplyMessageContent(content: ReplyContent): string { + if (content.content.text) return content.content.text; + if (content.content.reply?.content.text) return content.content.reply.content.text; + if (content.content.attachment?.filename) return content.content.attachment.filename; + if (content.content.remoteAttachment?.url) return content.content.remoteAttachment.url; + if (content.content.groupUpdated) return "Group updated"; + return ""; +} + export function getMessageStringContent( message: DecodedMessageWithCodecsType ): string | undefined { // ... existing type checks ... if (isReplyMessage(message)) { - const content = message.content() as ReplyContent; - if (content.content.text) { - return content.content.text; - } - // ... other reply content checks ... + return getReplyMessageContent(message.content() as ReplyContent); } // ... rest of the function ... }This refactoring would:
- Improve code readability
- Make it easier to test individual message type handling
- Reduce the cognitive complexity of the main function
Committable suggestion skipped: line range outside the PR's diff.
containers/GroupScreenMembersTable.tsx (1)
45-45:
⚠️ Potential issueConsider safer null handling for topic fallback
The current implementation uses non-null assertion (!), which could lead to runtime errors if both
topicandgroup?.topicare undefined.Consider this safer implementation:
- } = useGroupMembers((topic ?? group?.topic)!); + } = useGroupMembers(topic ?? group?.topic ?? ''); const { data: groupPermissionPolicy } = useGroupPermissionPolicyQuery( currentAccount, - (topic ?? group?.topic)! + topic ?? group?.topic ?? '' );Alternatively, add runtime checks:
if (!topic && !group?.topic) { logger.error('No topic available for group members'); return null; }Also applies to: 47-48
screens/GroupInvite.tsx (1)
61-61:
⚠️ Potential issueRemove unsafe non-null assertion on optional newGroup
The non-null assertion on
newGroup?.topic!is unsafe asnewGroupis initialized as undefined and only set after successful group operations.Consider this safer implementation:
- const { allowGroup } = useGroupConsent(newGroup?.topic!); + const { allowGroup } = useGroupConsent(newGroup?.topic ?? '');Also, consider adding a guard clause:
if (!newGroup?.topic) { logger.warn('No topic available for group consent'); return null; }components/GroupAvatar.tsx (1)
43-43:
⚠️ Potential issueResolve inconsistency between prop type and usage
The
topicprop is marked as optional inGroupAvatarProps, but used with a non-null assertion inuseGroupMembers. This creates a type-safety gap.Consider one of these approaches:
- If topic should be required:
- topic?: ConversationTopic; + topic: ConversationTopic;
- If topic can be optional, handle it safely:
- topic!, + topic ?? '',Also, the
enabledcondition already checks for!!topic, making the non-null assertion redundant:enabled: !!topic && !pendingGroupMembers,Also applies to: 205-213
screens/NewConversation/NewConversation.tsx (1)
60-60: 🛠️ Refactor suggestion
Replace non-null assertion with proper type guard
The non-null assertion operator (!) bypasses TypeScript's type checking. While runtime checks exist elsewhere in the code, it's safer to handle the optional parameter explicitly.
Consider this safer approach:
- route.params?.addingToGroupTopic! + route.params?.addingToGroupTopic ?? ''Then update the
useGroupQueryhook to handle empty string cases appropriately.Committable suggestion skipped: line range outside the PR's diff.
| theme.spacing.xs + | ||
| // Account for safe area at the bottom of the screen | ||
| (safeAreaInsets?.bottom || 0); | ||
| console.log("items:", items); |
alexrisch
force-pushed
the
ts/message-refactoring-attachments
branch
2 times, most recently
from
b84d2c3 to
b425fec
Compare
|
Closed and replaced with #1314 |
Summary by CodeRabbit
New Features
Loadercomponent for loading indicators, replacing the previousActivityIndicator.AttachmentContainer,AttachmentLoading, andRemoteAttachmentImagecomponents for enhanced attachment handling in chats.DmConsentPopupandGroupConsentPopupfor managing user consent in conversations.MessageContextMenucomponent for managing context menus in chat messages.MessageContextMenuAboveMessageReactions,MessageContextMenuBackdrop, and other related components for improved user interaction with message reactions.Bug Fixes
Refactor
ChatGroupUpdatedMessageandMessageContextMenucomponents for improved functionality and readability.MessageReactionscomponent to simplify its interface and improve state management.Chores