Mark conversation as read when entering chat #1311
Conversation
|
Warning Rate limit exceeded@lourou has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 29 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request introduce new hooks and modify existing functionality within the conversation handling components. The Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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 (
|
![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.
Actionable comments posted: 22
🧹 Outside diff range and nitpick comments (18)
components/Banner/AnimatedBanner.tsx (1)
Line range hint
24-84: Consider enhancing component configurability.The component implementation is solid, but consider these improvements for better flexibility:
- Make animation duration and easing configurable via props
- Allow customization of zIndex through props instead of hardcoding to 1000
- Consider making VERTICAL_MARGIN configurable
Example implementation:
type AnimatedBannerProps = { title: string; description: string; cta?: string; onButtonPress?: () => void; onDismiss?: () => void; style?: StyleProp<ViewStyle>; + animationDuration?: number; + animationEasing?: Animated.EasingFunction; + zIndex?: number; + verticalMargin?: number; }; -const VERTICAL_MARGIN = Margins.default; +const DEFAULT_VERTICAL_MARGIN = Margins.default; +const DEFAULT_ANIMATION_DURATION = 200; +const DEFAULT_Z_INDEX = 1000; const AnimatedBanner: React.FC<AnimatedBannerProps> = React.memo(({ + animationDuration = DEFAULT_ANIMATION_DURATION, + animationEasing = Easing.bezier(0.7, 0.0, 1, 1), + zIndex = DEFAULT_Z_INDEX, + verticalMargin = DEFAULT_VERTICAL_MARGIN, + ...props }) => {components/Chat/Frame/urlProtocols.ts (1)
1-6: LGTM! Consider adding TypeScript types for better type safety.The implementation correctly creates a Set of unique URL protocols. However, adding TypeScript types would improve code reliability.
Consider applying these changes:
-import config from "../../../config"; +interface Config { + scheme: string; + framesAllowedSchemes: string[]; +} +import config from "../../../config" as Config;components/AndroidBackAction.tsx (1)
4-4: Consider documenting color scheme behaviorThe component correctly handles dark/light mode via useColorScheme and textSecondaryColor utility. Consider adding a brief comment explaining the color scheme behavior for future maintainability.
import { useColorScheme } from "react-native"; export default function AndroidBackAction({ navigation, }: { navigation: NativeStackNavigationProp<any>; }) { + // Adapts back button color based on current color scheme const colorScheme = useColorScheme(); return ( <HeaderBackButton onPress={navigation.goBack} canGoBack={navigation.canGoBack()} style={{ marginLeft: -3 }} tintColor={textSecondaryColor(colorScheme)} /> ); }Also applies to: 11-18
components/Chat/ChatNullState.tsx (1)
30-34: Consider adding proper typing for navigation propsThe navigation and route props are currently typed as
any. Consider using proper types from the navigation library for better type safety and developer experience.+import { StackNavigationProp } from '@react-navigation/stack'; +import { RouteProp } from '@react-navigation/native'; +import { RootStackParamList } from '../../navigation/types'; type ChatNullStateProps = { currentAccount: string; - navigation: any; - route: any; + navigation: StackNavigationProp<RootStackParamList>; + route: RouteProp<RootStackParamList>; };components/Chat/Frame/FramePreview.tsx (1)
3-3: Address the TODO commentThe TODO comment indicates that this file should be deleted after refactoring is complete. To maintain a clean codebase, please remove this file if it's no longer needed.
Do you need assistance in removing this file or completing the refactoring process?
components/Chat/ChatDumb.tsx (1)
219-241: Confirm whether the commented-out Chat Input code should be includedThe code responsible for rendering the chat input area is currently commented out. If the chat input is intended to be part of this component, consider uncommenting this section. Otherwise, if it's no longer needed, it might be cleaner to remove it entirely to keep the codebase tidy.
components/Chat/Attachment/remote-attachment-image.tsx (2)
116-140: ReuseAttachmentContainerinstead of definingAttachmentPreviewContainerThe
AttachmentPreviewContainercomponent provides styling similar to theAttachmentContainercomponent introduced elsewhere. To adhere to the DRY principle and improve maintainability, consider reusingAttachmentContainerto avoid code duplication.Apply this diff to refactor the code:
-import { IVStackProps, VStack } from "@design-system/VStack"; +import { AttachmentContainer } from "@/components/Chat/Attachment/attachment-container"; -const AttachmentPreviewContainer = memo(function AttachmentPreviewContainer( - props: IVStackProps -) { - const { style, ...rest } = props; - - const { theme } = useAppTheme(); - - return ( - <VStack - style={[ - { - overflow: "hidden", - borderRadius: theme.borderRadius.sm, - width: "100%", - justifyContent: "center", - alignItems: "center", - backgroundColor: theme.colors.fill.tertiary, - aspectRatio: 1.5, // Default aspect ratio for attachments - }, - style, - ]} - {...rest} - /> - ); -}); +const AttachmentPreviewContainer = AttachmentContainer;
104-111: Add accessibility labels to theImagecomponentTo improve accessibility for users utilizing screen readers, add an
accessibilityLabeloraccessibleprop to theImagecomponent. This ensures that the image content is properly described.Apply this diff to include an accessibility label:
<Image source={{ uri: attachment.mediaURL }} contentFit="cover" style={{ width: "100%", height: "100%", }} + accessible={true} + accessibilityLabel={translate("attachment_image_accessibility_label")} >Ensure that the translation key
"attachment_image_accessibility_label"is added to the localization files with an appropriate description.components/Chat/ConsentPopup/dm-consent-popup.tsx (2)
87-89: Use localized error messagesConsider using the
translatefunction for error messages to support internationalization.Apply this diff:
showSnackbar({ type: "error", - message: `Error consenting`, + message: translate("error_consenting"), });
103-104: Use localized error messagesConsider using the
translatefunction for error messages to support internationalization.Apply this diff:
captureErrorWithToast(ensureError(error), { - message: `Error consenting`, + message: translate("error_consenting"), });components/Chat/ConsentPopup/consent-popup.design-system.tsx (1)
8-12: Pass the 'style' prop to the Button component and remove unused variableIn
ConsentPopupButton, thestyleprop is accepted but not passed to theButtoncomponent, and thethemevariable is declared but not used. To allow styling the button and clean up unused code, pass thestyleprop down and remove the unused variable.Apply this diff:
export function ConsentPopupButton({ style, ...rest }: IButtonProps) { - const { theme } = useAppTheme(); - return <Button {...rest} />; + return <Button style={style} {...rest} />; }android/gradle.properties (1)
53-54: Consider environment-specific network inspector configurationThe network inspector is enabled globally, which might affect release builds.
Consider moving this configuration to a development-specific properties file or using a build type condition:
-# Enable network inspector -EX_DEV_CLIENT_NETWORK_INSPECTOR=true +# Enable network inspector (dev only) +dev.EX_DEV_CLIENT_NETWORK_INSPECTOR=trueandroid/app/src/preview/AndroidManifest.xml (1)
1-31: Consider extracting common configurationsThe preview and production manifests share identical structure and paths.
Consider:
- Using manifest placeholders for environment-specific values
- Creating a base manifest template
- Using manifest merging to reduce duplication
Example approach:
<!-- base/AndroidManifest.xml --> <manifest ...> <application ...> <activity ...> <intent-filter> <data android:scheme="${appScheme}"/> <data android:scheme="${appPackageScheme}"/> </intent-filter> <intent-filter android:autoVerify="true"> <data android:host="${appHost}" .../> <!-- other paths --> </intent-filter> </activity> </application> </manifest>Then in
build.gradle:android { defaultConfig { manifestPlaceholders = [ appScheme: "converse", appPackageScheme: "com.converse.prod", appHost: "getconverse.app" ] } productFlavors { preview { manifestPlaceholders = [ appScheme: "converse-preview", appPackageScheme: "com.converse.preview", appHost: "preview.getconverse.app" ] } } }adr/adr.001.folder and file structure/example composite component structure.md (1)
3-3: Fix typo in documentation"pressented" should be "presented"
README.md (2)
43-50: Improve formatting and clarity of web limitations noteThe web limitations section needs some formatting improvements:
### Please Note -Currently Groups and V3 Identity is not supported on Web at the SDK layer, but is actively being worked on by the team +Currently, Groups and V3 Identity are not supported on Web at the SDK layer, but are actively being worked on by the team. -Until then Converse web will only show 1 to 1 conversations and the majority of testing and development are native app focused. +Until then, Converse web will only show one-on-one conversations, and the majority of testing and development are native app focused. -Web support is an end goal and the team is happy to fix any issues that are reported +Web support is an end goal, and the team is committed to resolving any reported issues.🧰 Tools
🪛 LanguageTool
[uncategorized] ~44-~44: A comma may be missing after the conjunctive/linking adverb ‘Currently’.
Context: ...uilding the App - Web ### Please Note Currently Groups and V3 Identity is not supported...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[typographical] ~46-~46: It seems that a comma is missing after this introductory phrase.
Context: ...s actively being worked on by the team Until then Converse web will only show 1 to 1 conv...(SINCE_THEN_COMMA)
[uncategorized] ~47-~47: It appears that hyphens are missing.
Context: ... Until then Converse web will only show 1 to 1 conversations and the majority of testi...(ONE_ON_ONE_HYPHEN)
[style] ~49-~49: Consider using a different verb for a more formal wording.
Context: ...is an end goal and the team is happy to fix any issues that are reported ### Insta...(FIX_RESOLVE)
🪛 Markdownlint (0.35.0)
43-43: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
81-101: LGTM: Added comprehensive testing documentationThe new sections for Jest and Performance testing provide clear instructions for developers.
Consider adding examples of expected
.envvariables for completeness.🧰 Tools
🪛 LanguageTool
[style] ~94-~94: Consider shortening or rephrasing this to strengthen your wording.
Context: ...estssh yarn test:perf:baselineMake changes to the code to see the performance impact ...(MAKE_CHANGES)
adr/adr.002.architecture for business logic and dependencies - join group/adr.002.architecture-poc-for-business-logic-and-dependencies-group-joining.md (1)
102-116: Enhance the Consequences section with additional considerationsConsider adding the following to strengthen the ADR:
- Specific success metrics to measure the impact of the refactoring
- Clear rollback strategy in case of unforeseen issues
- Performance impact analysis of introducing XState
adr/adr.001.folder and file structure/adr.001.folder and file structure.md (1)
286-286: Consider adding guidance for cross-feature interactionsThe ADR would benefit from additional sections covering:
- How to handle dependencies between features
- Strategy for feature-specific configuration management
- Guidelines for sharing types across features
Example structure:
## Cross-Feature Dependencies - Guidelines for importing from other features - Strategies to minimize coupling - Handling shared types and interfaces ## Feature-Specific Configuration - Location of feature-specific config files - Environment variable management - Feature flag implementation
🛑 Comments failed to post (22)
components/Chat/Frame/urlProtocols.ts (1)
3-6: 🛠️ Refactor suggestion
Add validation for config properties.
The code assumes that config properties exist and are properly formatted, but this should be validated.
Consider adding validation:
+const validateScheme = (scheme: string): string => { + if (!scheme || typeof scheme !== 'string') { + throw new Error('Invalid scheme format in config'); + } + return scheme.endsWith(':') ? scheme : `${scheme}:`; +}; export const AUTHORIZED_URL_PROTOCOLS = new Set([ - `${config.scheme}:`, - ...config.framesAllowedSchemes.map((s) => `${s}:`), + validateScheme(config.scheme), + ...(config.framesAllowedSchemes || []).map(validateScheme), ]);📝 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 validateScheme = (scheme: string): string => { if (!scheme || typeof scheme !== 'string') { throw new Error('Invalid scheme format in config'); } return scheme.endsWith(':') ? scheme : `${scheme}:`; }; export const AUTHORIZED_URL_PROTOCOLS = new Set([ validateScheme(config.scheme), ...(config.framesAllowedSchemes || []).map(validateScheme), ]);android/app/src/main/google-services.json (1)
21-25:
⚠️ Potential issueCRITICAL: Remove sensitive API key from version control
The Google Cloud Platform API key is exposed in the repository. This poses a significant security risk as it could be misused to access your Google Cloud services.
Recommended actions:
- Immediately rotate/revoke this API key in Google Cloud Console
- Move sensitive credentials to environment variables or secure credential management systems
- Add this file to
.gitignore- Use a template file (e.g.,
google-services.json.template) with placeholder values for version controlExample template approach:
{ "api_key": [ { "current_key": "${ANDROID_GCP_API_KEY}" } ] }🧰 Tools
🪛 Gitleaks (8.21.2)
23-23: Uncovered a GCP API key, which could lead to unauthorized access to Google Cloud services and data breaches.
(gcp-api-key)
components/ActivityIndicator/ActivityIndicator.tsx (1)
1-6: 💡 Codebase verification
This change is unrelated to the PR's objective and should be in a separate PR
The evidence shows this is an unrelated change:
- The PR description "WIP read/unread" indicates focus on read/unread conversation functionality
- The ActivityIndicator to Loader migration appears to be a design system upgrade
- While the component is used in chat-related features (V3ConversationFromPeer.tsx still uses ActivityIndicator), this specific change is about component deprecation and not about read/unread functionality
This change should be moved to a separate PR focused on design system component migrations to maintain clear change scope and traceability.
🔗 Analysis chain
Verify relevance to PR objectives.
This component change appears unrelated to the PR's stated objective of "Mark conversation as read when entering chat". Please clarify how this change contributes to the PR's goal.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if this component is used in chat-related features # to understand its relevance to marking conversations as read # Search for ActivityIndicator usage in chat-related files rg -l "ActivityIndicator|Loader" | rg -i "chat|conversation" # Search for any recent changes to chat-related files that might explain this change git diff HEAD~1 -- "**/chat/**" "**/conversation/**"Length of output: 531
Script:
#!/bin/bash # Let's check the actual usage of ActivityIndicator in these files # and see if there are any changes related to loading states in chat features # Check ActivityIndicator/Loader usage in chat and conversation files rg -A 5 "ActivityIndicator|Loader" components/Chat/Attachment/attachment-loading.tsx components/Chat/ChatPlaceholder/ChatPlaceholder.tsx features/conversations/components/V3ConversationFromPeer.tsx features/conversation/dm-conversation.screen.tsx # Check if there are any pending migrations from ActivityIndicator to Loader # in chat-related files that might explain this change rg -l "@design-system/loader" components/Chat features/conversation features/conversations screens/NewConversation # Look at the PR description to understand the context git show --format=%B -s HEADLength of output: 4578
android/app/src/prod/google-services.json (1)
21-25:
⚠️ Potential issue
⚠️ Critical: Remove API key from version controlThe Google Cloud Platform API key should not be committed to version control as it poses a significant security risk. This could lead to unauthorized access to your Google Cloud services.
Please follow these steps to secure your configuration:
- Remove the API key from the repository
- Rotate the exposed API key immediately in Google Cloud Console
- Use environment variables or a secure secret management system
- Add
google-services.jsonto.gitignoreWould you like assistance in implementing a secure configuration management solution?
🧰 Tools
🪛 Gitleaks (8.21.2)
23-23: Uncovered a GCP API key, which could lead to unauthorized access to Google Cloud services and data breaches.
(gcp-api-key)
components/Chat/Frame/FrameBottom.tsx (1)
71-73: 🛠️ Refactor suggestion
Consider adding a platform check for haptics support.
The unconditional haptic feedback might cause issues on platforms where haptics are not supported.
Consider wrapping the haptic call with a platform check:
-Haptics.impactAsync(); +if (Platform.OS !== 'web') { + Haptics.impactAsync(); +}📝 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 (Platform.OS !== 'web') { Haptics.impactAsync(); } onButtonPress(button); }}components/Chat/Frame/FramePreview.tsx (1)
7-415: 🛠️ Refactor suggestion
Remove large blocks of commented-out code
The code from lines 7 to 415 is entirely commented out. It's best practice to remove unused code rather than leaving it commented out to improve readability and maintainability. Version control systems can help retrieve previous versions if necessary.
components/Chat/ChatDumb.tsx (2)
117-122: 🛠️ Refactor suggestion
Ensure
chatInputHeight.valueis updated to reflect dynamic input heightThe shared value
chatInputHeightis used in calculatingchatInputDisplayedHeight, which influences the padding of the chat content. Currently,chatInputHeight.valueis initialized to zero and not updated in the provided code. If the chat input height changes dynamically, make sure to updatechatInputHeight.valueaccordingly to maintain the correct layout.
190-215:
⚠️ Potential issueAssign
messageListRefto therefprop ofReanimatedFlashListto enable scrollingThe
messageListRefis used in thescrollToMessagefunction to control scrolling of the chat messages. However, it's not assigned to therefprop ofReanimatedFlashList, somessageListRef.currentwill beundefined, and the scrolling functionality will not work as intended.Apply this diff to assign
messageListRefto therefprop:<ReanimatedFlashList contentContainerStyle={styles.chat} data={items} + ref={messageListRef} refreshing={refreshing} extraData={extraData} renderItem={renderItem} onLayout={handleOnLayout} keyboardDismissMode="interactive" automaticallyAdjustContentInsets={false} contentInsetAdjustmentBehavior="never" inverted keyExtractor={keyExtractor} getItemType={getItemType} keyboardShouldPersistTaps="handled" estimatedItemSize={34} showsVerticalScrollIndicator={Platform.OS === "ios"} pointerEvents="auto" ListFooterComponent={ListFooterComponent} />📝 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.<ReanimatedFlashList contentContainerStyle={styles.chat} data={items} ref={messageListRef} refreshing={refreshing} extraData={extraData} renderItem={renderItem} onLayout={handleOnLayout} keyboardDismissMode="interactive" automaticallyAdjustContentInsets={false} contentInsetAdjustmentBehavior="never" // Causes a glitch on Android, no sure we need it for now // maintainVisibleContentPosition={{ // minIndexForVisible: 0, // autoscrollToTopThreshold: 100, // }} // estimatedListSize={Dimensions.get("screen")} inverted keyExtractor={keyExtractor} getItemType={getItemType} keyboardShouldPersistTaps="handled" estimatedItemSize={34} // Size glitch on Android showsVerticalScrollIndicator={Platform.OS === "ios"} pointerEvents="auto" ListFooterComponent={ListFooterComponent} />components/Chat/Attachment/remote-attachment-image.tsx (2)
77-81:
⚠️ Potential issueImplement the
onPresshandler to open attachments in the webviewThe
onPresshandler for unsupported media types is currently empty. To enhance user experience, implement the logic to open the attachment in a webview or the default browser when the user taps on the link.
157-157:
⚠️ Potential issueEnsure
getCurrentAccount()cannot return null to prevent runtime errorsIn
fetchAttachment,getCurrentAccount()is called with a non-null assertion (!). If there's any possibility thatgetCurrentAccount()could returnnullorundefined, this may lead to runtime exceptions. Verify that an account is always available when this function is called, or add appropriate null checks.Consider modifying the code to handle the potential null case:
- account: getCurrentAccount()!, + const currentAccount = getCurrentAccount(); + if (!currentAccount) { + throw new Error("No current account available"); + } + account: currentAccount,📝 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 = getCurrentAccount(); if (!currentAccount) { throw new Error("No current account available"); } account: currentAccount,components/Chat/ChatGroupUpdatedMessage.tsx (3)
79-117: 🛠️ Refactor suggestion
Refactor to eliminate code duplication between
ChatGroupMemberLeftandChatGroupMemberJoinedThe
ChatGroupMemberLeftandChatGroupMemberJoinedcomponents share significant code duplication. Consider abstracting the shared logic into a single component or a helper function, passing in props to handle the differences such as the translation key and event type.For example, create a generic
ChatGroupMemberUpdatecomponent:type IChatGroupMemberUpdateProps = { inboxId: InboxId; eventType: 'joined' | 'left'; }; function ChatGroupMemberUpdate({ inboxId, eventType }: IChatGroupMemberUpdateProps) { // Shared logic... const translationKey = eventType === 'joined' ? 'group_member_joined' : 'group_member_left'; // Render logic... }Then replace
ChatGroupMemberLeftandChatGroupMemberJoinedwith this new component.Also applies to: 125-165
16-16:
⚠️ Potential issueCorrect the typo in the import statement
There's a typo in the import statement. The correct type name is
GroupUpdatedMetadataEntry.Apply this diff to fix the typo:
- GroupUpdatedMetadatEntry, + GroupUpdatedMetadataEntry,Ensure that all usages of
GroupUpdatedMetadataEntryare updated accordingly.📝 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.GroupUpdatedMetadataEntry,
115-115:
⚠️ Potential issueIncorrect translation key in
ChatGroupMemberLeftcomponentIn the
ChatGroupMemberLeftcomponent, the message uses the translation key for a member joining the group. It should usetranslate("group_member_left")to indicate that a member has left.Apply this diff to correct the translation key:
<ChatGroupUpdateText> - {translate("group_member_joined")} + {translate("group_member_left")} </ChatGroupUpdateText>Ensure the translation key
"group_member_left"exists in the localization files.Committable suggestion skipped: line range outside the PR's diff.
components/Chat/ActionButton.tsx (1)
15-15: 💡 Codebase verification
Update required:
ActionButtonusages need to be modified to useIPictotypeThe search results show several instances where
ActionButtonis being used with string literals for thepictoprop, which needs to be updated to use the newIPictotype:
screens/ShareProfile.tsx: Usespicto="xmark"components/Chat/Message/Message.tsx: Usespicto="arrowshape.turn.up.left"(in commented code)components/Chat/Transaction/SendMoneyButton.tsx: Usespicto="dollarsign"(in commented code)components/Chat/Frame/FramePreview.tsx: Usespicto="square.and.arrow.up"(in commented code)🔗 Analysis chain
Verify all usages of
ActionButtonare updated to passIPictotype forpictoThe
pictoprop type inChatActionPropshas been changed fromstringtoIPicto. Ensure that all components usingActionButtonare updated to pass anIPictotype to prevent type errors.Run the following script to find usages of
ActionButtonand check forpictoprop types:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of ActionButton and check if picto prop is correctly typed # Find all files importing ActionButton fd --type f --extension tsx | xargs grep -n 'import.*ActionButton' # Search for ActionButton component usage rg '<ActionButton' --type tsx -A 5 # Check for picto prop being passed as a string literal rg '<ActionButton[^>]*picto=[\'"].*[\'"]' --type tsx # Verify that picto prop is passed an IPicto variable rg '<ActionButton' --type tsx -A 5 | rg 'picto={' -A 1Length of output: 523
Script:
#!/bin/bash # Let's try a different approach using ripgrep with proper file type specification # Find all files importing ActionButton rg "import.*ActionButton" -t typescript -t typescriptreact # Search for ActionButton component usage with context rg "<ActionButton" -t typescript -t typescriptreact -A 5 # Check for picto prop being passed as a string literal rg "<ActionButton[^>]*picto=['\"].*['\"]" -t typescript -t typescriptreact # Verify that picto prop is passed an IPicto variable rg "<ActionButton.*picto=\{.*\}" -t typescript -t typescriptreactLength of output: 408
Script:
#!/bin/bash # Let's try without specifying file types and use basic pattern matching # Find imports of ActionButton rg "import.*ActionButton" # Find all ActionButton usages with surrounding context rg "<ActionButton" -A 5 # Look for picto prop usage rg "picto=" --context 2 | rg -B 2 "ActionButton"Length of output: 3126
App.tsx (3)
1-1:
⚠️ Potential issueAvoid importing
expo-dev-clientin production codeImporting
"expo-dev-client"is intended for development purposes and should not be included in production builds, as it can increase app size and potentially expose development tools. Consider conditionally importing it only in development environments or removing it from production code.
88-96:
⚠️ Potential issueEnsure
authStatusupdates whencurrentAccountchangesThe current
useEffectruns only once due to the empty dependency array[], so ifcurrentAccountchanges after the component mounts,authStatuswill not update accordingly. Consider subscribing tocurrentAccountchanges by using the hook version ofuseAccountsStoreand includingcurrentAccountin the dependency array.Apply this diff to fix the issue:
- useEffect(() => { - const currentAccount = useAccountsStore.getState().currentAccount; + const currentAccount = useAccountsStore(state => state.currentAccount); + useEffect(() => { if (currentAccount && currentAccount !== TEMPORARY_ACCOUNT_NAME) { setAuthStatus("signedIn"); } else { setAuthStatus("signedOut"); } - }, []); + }, [currentAccount]);📝 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.// For now we use persit with zustand to get the accounts when the app launch so here is okay to see if we're logged in or not const currentAccount = useAccountsStore(state => state.currentAccount); useEffect(() => { if (currentAccount && currentAccount !== TEMPORARY_ACCOUNT_NAME) { setAuthStatus("signedIn"); } else { setAuthStatus("signedOut"); } }, [currentAccount]);
62-62:
⚠️ Potential issueMove
xmtpEngine.start()inside auseEffecthookCalling
xmtpEngine.start()at the module level can cause unintended side effects during module initialization and may execute before the app is ready. It's better to place it inside auseEffecthook within theAppcomponent to ensure it runs after the component mounts.Apply this diff to move the call inside the
Appcomponent:// Remove the module-level invocation - xmtpEngine.start(); ... // Inside the App component + useEffect(() => { + xmtpEngine.start(); + }, []);Committable suggestion skipped: line range outside the PR's diff.
android/app/src/preview/google-services.json (1)
21-25:
⚠️ Potential issueCritical: Remove API key from version control.
The exposed Google Cloud Platform API key poses a security risk. API keys should never be committed to version control.
Please:
- Remove this API key immediately and revoke it
- Use environment variables or a secure secret management system
- Update the documentation to guide developers on obtaining and configuring their own API keys for development
Example approach using environment variables:
- Remove the API key from the file
- Use a placeholder:
"api_key": [ { - "current_key": "AIzaSyBLyJ66tj4-ej4Cq2BHa-EJhHNTNWp8bFE" + "current_key": "${GOOGLE_API_KEY}" } ],
- Add the file to
.gitignore- Provide a template file (e.g.,
google-services.json.template) without sensitive dataCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Gitleaks (8.21.2)
23-23: Uncovered a GCP API key, which could lead to unauthorized access to Google Cloud services and data breaches.
(gcp-api-key)
android/app/src/main/java/com/converse/MainActivity.kt (1)
1-1: 💡 Codebase verification
Package name change requires updates to deep linking configuration
The verification reveals that the deep linking configuration in AndroidManifest.xml still uses the old package name schemes:
<data android:scheme="converse-dev"/> <data android:scheme="com.converse.dev"/>Additionally, the config.ts file shows that the bundleId is still set to "com.converse.dev" in the development environment configuration, which is inconsistent with the package name change to "com.converse" in MainActivity.kt.
- Update deep linking schemes in
android/app/src/main/AndroidManifest.xml- Update bundleId in
config.tsto match the new package name- Ensure the applicationId in
android/app/build.gradleis aligned with the new package name🔗 Analysis chain
Verify impact of package name change on deep linking
The package name change from
com.converse.devtocom.conversecould affect deep linking to chat conversations. Please ensure all deep link handlers are updated accordingly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for deep link handlers and URL schemes that might need updating rg -l "com\.converse\.dev" .Length of output: 486
Script:
#!/bin/bash # Let's check the actual content of these files to understand the deep linking configuration echo "=== Checking AndroidManifest.xml ===" cat android/app/src/main/AndroidManifest.xml echo -e "\n=== Checking config.ts ===" cat config.ts echo -e "\n=== Checking build.gradle ===" cat android/app/build.gradleLength of output: 21503
android/app/src/prod/AndroidManifest.xml (2)
14-28: 💡 Codebase verification
Digital Asset Links verification needed for getconverse.app domain
The verification shows that:
- converse.xyz has proper Digital Asset Links configuration with correct package name and SHA-256 fingerprints
- getconverse.app redirects and doesn't serve the required assetlinks.json file
Since App Links with
android:autoVerify="true"requires both domains to have valid Digital Asset Links files, please:
- Set up the assetlinks.json file at https://getconverse.app/.well-known/assetlinks.json
- Ensure it contains the same package name and fingerprints as converse.xyz
- Verify the file is accessible without redirects
🔗 Analysis chain
Verify domain ownership for App Links
The
android:autoVerify="true"requires proper domain ownership verification through Digital Asset Links.Ensure both domains have proper Digital Asset Links files containing your app's package name and SHA-256 certificate fingerprint.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if Digital Asset Links files are accessible for both domains echo "Checking Digital Asset Links for production domains..." curl -s https://getconverse.app/.well-known/assetlinks.json curl -s https://converse.xyz/.well-known/assetlinks.jsonLength of output: 613
6-6: 🛠️ Refactor suggestion
Add launch mode and export configurations to MainActivity
For better deep linking behavior and security, consider adding these attributes to the activity:
- <activity android:name=".MainActivity"> + <activity + android:name=".MainActivity" + android:launchMode="singleTask" + android:exported="true">This ensures:
- Proper back stack handling for deep links (
singleTask)- Explicit export declaration (security best practice)
📝 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.<activity android:name=".MainActivity" android:launchMode="singleTask" android:exported="true">.github/workflows/build-app-release.yml (1)
51-51:
⚠️ Potential issueFix the incorrect script path.
The path
scripts/Øbuild/eas.jscontains an invalid character 'Ø' which will cause build failures.- run: node scripts/Øbuild/eas.js --env production + run: node scripts/build/eas.js --env production📝 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.run: node scripts/build/eas.js --env production
lourou
force-pushed
the
lr/convo-mark-as-read
branch
from
b623389 to
e681692
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
components/Conversation/V3Conversation.tsx (1)
160-161: Remove console.log statements from production codeThe
console.logstatements are useful for debugging but should be removed or replaced with a proper logging mechanism before deploying to production.if (isUnread && !messagesLoading && messages?.ids.length && !hasToggledReadStatus) { - console.log("== Mark as read?"); toggleReadStatus(); setHasToggledReadStatus(true); }features/conversation-list/hooks/useToggleReadStatus.ts (1)
19-20: Remove console.log statements or use a logging libraryThe
console.logstatements can clutter the console in production. Consider removing them or using a logging library that can control log levels.console.log("useToggleReadStatus", topic, isUnread); return useCallback(() => { const newStatus = isUnread ? "read" : "unread"; - console.log("newStatus:", newStatus); const timestamp = new Date().getTime(); setTopicsData({ [topic]: { status: newStatus, timestamp, }, });Also applies to: 23-23
features/conversation-list/hooks/useMessageIsUnread.ts (1)
22-26: Consider removing deprecatedconversationparameterThe
conversationprop is marked as deprecated. If it's no longer in use, consider removing it to clean up the code and prevent confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
components/Conversation/V3Conversation.tsx(2 hunks)features/conversation-list/hooks/useMessageIsUnread.ts(1 hunks)features/conversation-list/hooks/useToggleReadStatus.ts(1 hunks)utils/conversation/showUnreadOnConversation.ts(0 hunks)
💤 Files with no reviewable changes (1)
- utils/conversation/showUnreadOnConversation.ts
🔇 Additional comments (1)
components/Conversation/V3Conversation.tsx (1)
44-45: New hooks imported correctly
The imports for useConversationIsUnread and useToggleReadStatus are properly added and set up for use in the component.
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
features/conversation-list/hooks/useMessageIsUnread.ts (3)
29-35: Consider adding error handling for edge casesWhile the current implementation handles the basic cases well, consider adding error handling for potential edge cases:
- Invalid topic format
- Malformed message data
// Check if status is unread +if (!topic || typeof topic !== 'string') { + console.warn('Invalid topic format'); + return false; +} if (topicsData[topic]?.status === "unread") return true; // Check if the last message was sent by the current user +if (!lastMessage.senderAddress) { + console.warn('Message missing sender address'); + return false; +} if (lastMessage.senderAddress === currentInboxId) { return false; }
36-41: Consider memoizing timestamp normalizationThe
normalizeTimestampcall could be memoized to avoid unnecessary recalculations on every render.+const normalizedTimestamp = useMemo(() => + normalizeTimestamp(timestampNs) +, [timestampNs]); -const timestamp = normalizeTimestamp(timestampNs); +const timestamp = normalizedTimestamp;
17-42: Add JSDoc documentation for the hookConsider adding comprehensive JSDoc documentation to clarify:
- The purpose of the hook
- Parameter descriptions
- Return value semantics
- Example usage
+/** + * Hook to determine if a conversation is unread based on various conditions + * @param {Object} props - The hook parameters + * @param {string} props.topic - The conversation topic identifier + * @param {DecodedMessageWithCodecsType} props.lastMessage - The last message in the conversation + * @param {number} props.timestamp - The timestamp to compare against (in nanoseconds) + * @returns {boolean} True if the conversation is unread, false otherwise + * @example + * const isUnread = useConversationIsUnread({ + * topic: "chat-123", + * lastMessage: message, + * timestamp: Date.now() * 1_000_000 + * }); + */ export const useConversationIsUnread = ({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
components/PinnedConversations/PinnedV3DMConversation.tsx(0 hunks)components/PinnedConversations/PinnedV3GroupConversation.tsx(0 hunks)components/V3DMListItem.tsx(0 hunks)components/V3GroupConversationListItem.tsx(0 hunks)features/conversation-list/hooks/useMessageIsUnread.ts(2 hunks)features/conversation-list/hooks/useToggleReadStatus.ts(0 hunks)
💤 Files with no reviewable changes (5)
- components/PinnedConversations/PinnedV3DMConversation.tsx
- features/conversation-list/hooks/useToggleReadStatus.ts
- components/PinnedConversations/PinnedV3GroupConversation.tsx
- components/V3GroupConversationListItem.tsx
- components/V3DMListItem.tsx
🔇 Additional comments (3)
features/conversation-list/hooks/useMessageIsUnread.ts (3)
Line range hint 1-15: LGTM! Clean imports and well-defined types
The imports are properly organized and the type definition is appropriately simplified by removing the unused conversation parameter.
26-27: LGTM! Good null check addition
Early return for undefined lastMessage is a good practice to prevent potential runtime errors.
17-42: Verify integration with chat entry point
The hook implementation looks solid, but let's verify its integration with the chat entry point to ensure conversations are properly marked as read.
✅ Verification successful
Let me try a different search strategy to verify the integration.
Let me search for the integration with read status toggling since we found the usage but need to verify the marking as read functionality.
Based on the search results, I can now provide a final response as we've found the crucial integration in V3Conversation.tsx that shows the hook is properly integrated with chat entry points.
Implementation correctly marks conversations as read on entry
The hook is properly integrated with the chat system:
- The
useConversationIsUnreadhook is used in conversation list items to detect unread status - The
V3Conversationcomponent implements auto-marking as read on chat entry through:useEffect(() => { if (isUnread && !messagesLoading && messages?.ids.length) { toggleReadStatus(); } });
- The
useToggleReadStatushook handles the actual status update in the store
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of this hook in chat entry components
echo "Searching for chat entry components using this hook..."
rg -l "useConversationIsUnread.*entering|entering.*useConversationIsUnread" --type ts --type tsx
# Search for the chat entry point implementation
echo "Searching for chat entry implementation..."
ast-grep --pattern 'const $_ = () => {
$$$
useConversationIsUnread
$$$
}'
Length of output: 422
Script:
#!/bin/bash
# Search for files using the hook
echo "Searching for files importing useConversationIsUnread..."
rg -l "useConversationIsUnread"
# Search for chat entry related components
echo "Searching for chat entry related components..."
rg -l "ChatEntry|EnterChat|ChatView" --type ts
# Look for actual implementation using the hook
echo "Looking for components using the hook..."
rg "useConversationIsUnread" -A 5 -B 5
Length of output: 11683
Script:
#!/bin/bash
# Search for the toggle read status implementation
echo "Searching for toggle read status implementation..."
rg "useToggleReadStatus" -A 10 -B 2
# Search for chat entry/navigation related code
echo "Searching for chat navigation/entry code..."
rg "navigation.*chat|onPress.*chat|navigate.*chat" -A 5 -B 5 components/V3DMListItem.tsx components/V3GroupConversationListItem.tsx components/Conversation/V3Conversation.tsx
Length of output: 12404
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
components/V3GroupConversationListItem.tsx (2)
13-13: Remove unused importThe
useEffecthook is imported but not used in the component.-import { RefObject, useCallback, useEffect, useMemo, useRef } from "react"; +import { RefObject, useCallback, useMemo, useRef } from "react";
Line range hint
1-434: Overall implementation looks good with minor improvements neededThe implementation successfully addresses the PR objective of marking conversations as read when entering chat. The code is well-structured and properly integrated with existing hooks. Consider:
- Removing the unused useEffect import
- Verifying the timing of read status updates during navigation
The rest of the implementation, including the Swipeable component integration and hook usage, is clean and follows best practices.
Consider adding logging or analytics to track any potential race conditions between message prefetching and navigation, which could affect the read status update timing.
components/Conversation/V3Conversation.tsx (2)
210-224: Extract common read status logicThe read status management logic is duplicated between
DmContentandGroupContentcomponents.Consider extracting this logic into a custom hook:
const useConversationReadStatus = (messages: any, topic: any, currentAccount: any) => { const isUnread = useConversationIsUnread({ topic, lastMessage: messages?.ids?.length ? messages.byId[messages.ids[0]] : undefined, timestamp: messages?.ids?.length ? messages.byId[messages.ids[0]]?.sentNs ?? 0 : 0, }); const toggleReadStatus = useToggleReadStatus({ topic, isUnread, currentAccount, }); useMarkAsReadOnEnter({ messagesLoading: messages?.isLoading, isUnread, toggleReadStatus, }); return { isUnread, toggleReadStatus }; };
308-326: Clean implementation with minor improvements neededThe hook implementation is solid with proper use of useRef to prevent multiple mark-as-read calls. However, there are a few improvements to consider:
- Remove the debug console.log statement
- Add error handling for toggleReadStatus call
Consider this cleaner implementation:
const useMarkAsReadOnEnter = ({ messagesLoading, isUnread, toggleReadStatus, }: { messagesLoading: boolean; isUnread: boolean; toggleReadStatus: () => void; }) => { const hasMarkedAsRead = useRef(false); useEffect(() => { if (isUnread && !messagesLoading && !hasMarkedAsRead.current) { - console.log("Mark as read?"); try { toggleReadStatus(); hasMarkedAsRead.current = true; } catch (error) { console.error('Failed to mark conversation as read:', error); } } }, [isUnread, messagesLoading, toggleReadStatus]); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
components/Conversation/V3Conversation.tsx(5 hunks)components/V3GroupConversationListItem.tsx(3 hunks)features/conversation-list/hooks/useToggleReadStatus.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- features/conversation-list/hooks/useToggleReadStatus.ts
🔇 Additional comments (4)
components/V3GroupConversationListItem.tsx (2)
373-375: LGTM: Proper ref handling for Swipeable component
The useRef hook is correctly typed and integrated with the Swipeable component.
394-394: Verify read status update timing
The implementation handles navigation and prefetching, but we should verify that the read status is consistently updated when entering the chat.
✅ Verification successful
Based on the search results, I can now generate a final response. The code shows a well-coordinated flow between prefetching, navigation, and read status updates:
Read status updates are properly synchronized
The implementation is safe from race conditions because:
prefetchConversationMessagesis called before navigation and uses React Query's prefetch mechanismuseMarkAsReadOnEnterhook in V3Conversation handles the read status update after entering the conversation- Read status updates are managed through a centralized store using
setTopicsDatawith proper timestamps
The read status update timing is properly handled through:
- Prefetch initiated before navigation
- Read status toggle executed after entering conversation
- Atomic store updates with synchronized timestamps
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for read status updates in navigation context
rg -A 5 "prefetchConversationMessages|useConversationIsUnread|useToggleReadStatus" --type ts --type tsx
# Look for potential race conditions in message prefetching
ast-grep --pattern 'prefetchConversationMessages($_)'
Length of output: 189
Script:
#!/bin/bash
# Search for read status updates and navigation patterns with broader file extensions
rg -A 5 "prefetchConversationMessages|useConversationIsUnread|useToggleReadStatus" --type-add 'tsx:*.{tsx,ts}' --type tsx
# Look for navigation and read status handling patterns
rg -A 5 "markAsRead|setUnread|navigation" --type-add 'tsx:*.{tsx,ts}' --type tsx
# Search for conversation-related hooks and their implementations
rg -A 5 "useConversation|useMessages" --type-add 'tsx:*.{tsx,ts}' --type tsx
Length of output: 88447
Script:
#!/bin/bash
# Search for read status update implementation and hooks
rg -A 10 "useMarkAsReadOnEnter|markAsRead" --type-add 'tsx:*.{tsx,ts}' --type tsx
# Look for conversation message prefetching implementation
rg -A 10 "prefetchConversationMessages" --type-add 'tsx:*.{tsx,ts}' --type tsx
# Search for conversation store updates related to read status
rg -A 5 "setTopicsData|updateReadStatus" --type-add 'tsx:*.{tsx,ts}' --type tsx
Length of output: 12647
components/Conversation/V3Conversation.tsx (2)
44-45: LGTM: Clean import declarations
The new hook imports are properly organized and follow the existing import structure.
153-157: Verify read status behavior across different scenarios
The implementation looks correct, but we should verify the behavior in various scenarios:
- New messages arriving while in chat
- Multiple rapid chat entries
- Network failures during status update
Let's check for any existing test coverage:
Also applies to: 220-224
✅ Verification successful
Based on the gathered information, I can now generate the final response:
Implementation has sufficient error handling and state management
The implementation of useMarkAsReadOnEnter is robust and handles the scenarios well:
- Uses
useRefto prevent multiple mark-as-read calls - Only triggers when messages are loaded and conversation is unread
toggleReadStatusis wrapped in a store action that handles state updates safely- Error handling is managed at the store level through
useChatStore
The concerns about various scenarios are addressed:
- New messages arriving: State is managed through chat store
- Multiple rapid entries: Prevented by
hasMarkedAsReadref - Network failures: Handled at store level with state rollback capability
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests related to conversation read status
rg -t ts -t tsx "describe.*mark.*read|test.*mark.*read|it.*mark.*read" --glob "**/*.{spec,test}.{ts,tsx}"
# Check for existing error handling in toggleReadStatus
ast-grep --pattern 'try {
$$$
toggleReadStatus($$$)
$$$
} catch'
Length of output: 208
Script:
#!/bin/bash
# Search for tests with corrected file glob
rg -t typescript -t tsx "describe.*mark.*read|test.*mark.*read|it.*mark.*read" "**/*.{spec,test}.{ts,tsx}"
# Look for error handling around toggleReadStatus calls
rg -A 3 "toggleReadStatus"
# Check implementation of useMarkAsReadOnEnter hook
fd "useMarkAsReadOnEnter" --exec cat {}
Length of output: 7498
Script:
#!/bin/bash
# Search for useMarkAsReadOnEnter hook implementation
rg -A 10 "useMarkAsReadOnEnter.*=.*{"
# Look for useToggleReadStatus implementation to understand error handling
rg -A 10 "useToggleReadStatus.*=.*{"
# Check for any error boundaries or error handling patterns around the conversation component
rg -A 3 "ErrorBoundary|try.*catch" "components/Conversation/**"
Length of output: 441
Script:
#!/bin/bash
# Search for useMarkAsReadOnEnter hook implementation with fixed regex
rg -A 10 "useMarkAsReadOnEnter"
# Look for useToggleReadStatus implementation with fixed regex
rg -A 10 "const useToggleReadStatus"
# Check for error handling in conversation components
rg -A 3 "try|catch|Error" "components/"
Length of output: 45629
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
* Remove unused helper method `showUnreadOnConversation` * WIP read/unread * Fix * Remove dependency from conversation prop * Fix * Add null check for messages array * Remove log
Screencast for both DM and group convos:
Closes #1307
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation