Noe/android prebuild #1356
Noe/android prebuild #1356
Conversation
WalkthroughThis pull request involves a comprehensive update to the Android project structure, build configurations, and workflow files. The changes include removing Android-specific files, updating Node.js versions across GitHub workflows, modifying package dependencies, and adjusting various code references from Changes
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant CI as GitHub Actions
participant Build as Build System
participant App as Application
Dev->>CI: Trigger build workflow
CI->>Build: Use Node.js 20
Build->>Build: Install dependencies
Build->>Build: Configure Android/iOS settings
Build->>App: Generate application package
App-->>CI: Build artifact
Possibly related PRs
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)ios/ConverseNotificationExtension/Spam.swift (1)
Verify the impact of commented-out consent synchronization Several sections of consent synchronization code are commented out throughout the file. This could impact the effectiveness of spam detection. Please clarify:
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.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
app.config.ts (1)
17-22: Simplify nested ternary operators for better readabilityThe nested ternary operators used to define
schemecan reduce code readability and maintainability. Consider using a helper function or an object mapping to make the code clearer.Suggestion: Refactor using a helper function
const getEnvValue = (devValue: string, previewValue: string, prodValue: string) => isDev ? devValue : isPreview ? previewValue : prodValue; const scheme = getEnvValue("converse-dev", "converse-preview", "converse");package.json (1)
5-5: Consider documenting the new build processWith the transition to Expo-managed builds, consider adding documentation about:
- The new build process and configuration
- Steps to verify deep links and notifications in preview builds
- Any required environment setup for local development
Also applies to: 112-112
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (15)
android/app/src/main/ic_launcher-playstore.pngis excluded by!**/*.pngandroid/app/src/main/ic_launcher_preview-playstore.pngis excluded by!**/*.pngandroid/app/src/main/res/drawable-hdpi/ic_default_expo_notifications.pngis excluded by!**/*.pngandroid/app/src/main/res/drawable-mdpi/ic_default_expo_notifications.pngis excluded by!**/*.pngandroid/app/src/main/res/drawable-xhdpi/ic_default_expo_notifications.pngis excluded by!**/*.pngandroid/app/src/main/res/drawable-xxhdpi/ic_default_expo_notifications.pngis excluded by!**/*.pngandroid/app/src/main/res/drawable-xxxhdpi/ic_default_expo_notifications.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-hdpi/bootsplash_logo.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-mdpi/bootsplash_logo.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xhdpi/bootsplash_logo.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xxhdpi/bootsplash_logo.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xxxhdpi/bootsplash_logo.pngis excluded by!**/*.pngandroid/gradle/wrapper/gradle-wrapper.jaris excluded by!**/*.jarassets/adaptive-icon.pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (42)
.github/workflows/build-app-release.yml(2 hunks).github/workflows/build-internal.yml(2 hunks)android/.gitignore(0 hunks)android/app/build.gradle(0 hunks)android/app/proguard-rules.pro(0 hunks)android/app/src/debug/AndroidManifest.xml(0 hunks)android/app/src/main/AndroidManifest.xml(0 hunks)android/app/src/main/google-services.json(0 hunks)android/app/src/main/java/com/converse/MainActivity.kt(0 hunks)android/app/src/main/java/com/converse/MainApplication.kt(0 hunks)android/app/src/main/res/drawable-v24/ic_launcher_background.xml(0 hunks)android/app/src/main/res/drawable/rn_edit_text_material.xml(0 hunks)android/app/src/main/res/mipmap-anydpi-v26/ic_launcher.xml(0 hunks)android/app/src/main/res/mipmap-anydpi-v26/ic_launcher_preview.xml(0 hunks)android/app/src/main/res/mipmap-anydpi-v26/ic_launcher_preview_round.xml(0 hunks)android/app/src/main/res/mipmap-anydpi-v26/ic_launcher_round.xml(0 hunks)android/app/src/main/res/values-night/colors.xml(0 hunks)android/app/src/main/res/values/colors.xml(0 hunks)android/app/src/main/res/values/ic_launcher_background.xml(0 hunks)android/app/src/main/res/values/ic_launcher_preview_background.xml(0 hunks)android/app/src/main/res/values/strings.xml(0 hunks)android/app/src/main/res/values/styles.xml(0 hunks)android/app/src/preview/AndroidManifest.xml(0 hunks)android/app/src/preview/google-services.json(0 hunks)android/app/src/preview/res/values/strings.xml(0 hunks)android/app/src/prod/AndroidManifest.xml(0 hunks)android/app/src/prod/google-services.json(0 hunks)android/app/src/prod/res/values/strings.xml(0 hunks)android/build.gradle(0 hunks)android/gradle.properties(0 hunks)android/gradle/wrapper/gradle-wrapper.properties(0 hunks)android/gradlew(0 hunks)android/gradlew.bat(0 hunks)android/react-settings-plugin/build.gradle.kts(0 hunks)android/react-settings-plugin/src/main/kotlin/expo/plugins/ReactSettingsPlugin.kt(0 hunks)android/sentry.properties(0 hunks)android/settings.gradle(0 hunks)app.config.ts(4 hunks)eas.json(0 hunks)package.json(3 hunks)scripts/build/android/androidDependenciesExpoPlugin.js(1 hunks)scripts/build/android/notifeeExpoPlugin.js(1 hunks)
💤 Files with no reviewable changes (36)
- android/app/src/main/res/values/ic_launcher_preview_background.xml
- android/app/src/main/res/values/ic_launcher_background.xml
- android/app/src/main/res/mipmap-anydpi-v26/ic_launcher.xml
- android/app/src/main/res/mipmap-anydpi-v26/ic_launcher_preview_round.xml
- android/app/proguard-rules.pro
- android/app/src/main/res/drawable-v24/ic_launcher_background.xml
- android/react-settings-plugin/src/main/kotlin/expo/plugins/ReactSettingsPlugin.kt
- android/app/src/main/res/mipmap-anydpi-v26/ic_launcher_round.xml
- android/.gitignore
- android/app/src/main/res/mipmap-anydpi-v26/ic_launcher_preview.xml
- android/react-settings-plugin/build.gradle.kts
- android/app/src/prod/res/values/strings.xml
- android/app/src/debug/AndroidManifest.xml
- android/app/src/main/google-services.json
- android/app/src/prod/google-services.json
- android/app/src/preview/google-services.json
- android/gradle/wrapper/gradle-wrapper.properties
- android/app/src/main/res/values-night/colors.xml
- android/app/src/preview/res/values/strings.xml
- android/app/src/main/res/drawable/rn_edit_text_material.xml
- android/sentry.properties
- android/build.gradle
- android/settings.gradle
- android/gradlew.bat
- android/gradlew
- android/app/src/prod/AndroidManifest.xml
- eas.json
- android/app/src/main/res/values/colors.xml
- android/app/src/preview/AndroidManifest.xml
- android/app/build.gradle
- android/app/src/main/java/com/converse/MainApplication.kt
- android/gradle.properties
- android/app/src/main/AndroidManifest.xml
- android/app/src/main/res/values/styles.xml
- android/app/src/main/java/com/converse/MainActivity.kt
- android/app/src/main/res/values/strings.xml
🔇 Additional comments (6)
app.config.ts (1)
220-221: Verify compatibility of the 'satisfies' operator with your TypeScript version
The satisfies operator is a TypeScript 4.9 feature. Ensure that your project is using TypeScript 4.9 or later to avoid compilation errors.
Run the following script to check the TypeScript version:
✅ Verification successful
TypeScript version is compatible with the 'satisfies' operator
The project uses TypeScript version ^5.3.0, which fully supports the 'satisfies' operator introduced in TypeScript 4.9. No compatibility issues are expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the TypeScript version specified in your project
# Expected: Version should be >= 4.9.0
grep '"typescript"' package.json
Length of output: 62
scripts/build/android/androidDependenciesExpoPlugin.js (1)
22-46: Ensure dependency resolution strategy does not introduce conflicts
Hardcoding dependency overrides can lead to unexpected behavior if other libraries require different versions. Verify that forcing bcprov-jdk15to18 version 1.70 and lifecycle-viewmodel-ktx version 2.5.0 is compatible with all other dependencies.
Run the following script to identify potential version conflicts:
✅ Verification successful
Dependency resolution strategy appears to be intentional and scoped
The dependency overrides are implemented as an Expo config plugin that's explicitly included in the app configuration. This is a controlled approach to handle specific known conflicts:
- The plugin is properly documented in
app.config.tswith the comment "Handle some conflicting dependencies manually" - It's implemented as a
createRunOncePluginwhich ensures consistent application - The changes are properly tagged and isolated with
converse-android-dependencies-fix
Since this is an intentional configuration to resolve specific dependency conflicts and is properly implemented as a plugin, the approach is valid. The manual verification of the exact versions can be done during the regular testing cycle.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for dependencies that might conflict with the specified versions
# Generate the dependency tree and look for conflicts
./gradlew app:dependencies > deps.txt
# Search for bcprov-jdk15on and lifecycle-viewmodel-ktx versions
grep -E 'bcprov-jdk15on|lifecycle-viewmodel-ktx' deps.txt
Length of output: 196
Script:
#!/bin/bash
# Let's check the repository structure and look for build-related files
fd -t f -e gradle -e properties
# Also search for any references to these dependencies in the codebase
rg -i "bcprov-jdk|lifecycle-viewmodel" --type-add 'gradle:*.{gradle,gradle.kts}' --type gradle
Length of output: 301
Script:
#!/bin/bash
# Let's examine the context around this file and its usage
fd androidDependenciesExpoPlugin
# Check for any Android-related build files or configurations
fd -t f -e gradle -e properties -e json android
# Look for any references to these dependencies in all file types
rg -i "bcprov-jdk|lifecycle-viewmodel"
Length of output: 762
Script:
#!/bin/bash
# Let's understand how this plugin is used and what it modifies
rg -A 5 -B 5 "androidDependenciesExpoPlugin"
# Check the contents of the plugin file to understand its full context
cat scripts/build/android/androidDependenciesExpoPlugin.js
Length of output: 2030
.github/workflows/build-internal.yml (1)
57-57: LGTM! Verify Sentry configuration
The addition of SENTRY_AUTH_TOKEN to environment files is implemented securely using GitHub secrets. The implementation is consistent across both Android and iOS build jobs.
Let's verify the Sentry configuration:
Also applies to: 115-115
✅ Verification successful
Let me check the Sentry initialization implementation to verify the configuration.
Sentry configuration is properly implemented and secure
The Sentry integration is well-structured with:
- Proper initialization in
utils/sentry.tsusing the DSN from config - Secure token handling via GitHub secrets
- Environment-aware configuration (disabled in dev)
- Error filtering and breadcrumb tracking implementation
- Platform-specific handling for iOS
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for Sentry configuration files and usage
# Check for Sentry initialization in the codebase
rg -l "Sentry.init"
# Check for Sentry configuration files
fd -e ts -e js -e json "sentry.config"
Length of output: 74
Script:
#!/bin/bash
# Check the Sentry initialization implementation
cat utils/sentry.ts
# Look for any Sentry-related configuration in package.json
rg -l "sentry" package.json
# Search for Sentry DSN configuration
rg "dsn" -A 3 -B 3
Length of output: 3282
package.json (3)
5-5: Script changes align with Expo prebuild strategy
The simplification of Android and iOS scripts aligns with moving to Expo-managed builds, removing custom Android configuration.
Also applies to: 13-13
53-53: Verify notification functionality with updated Notifee
The update to @notifee/react-native (9.1.2 → 9.1.3) requires verification of notification functionality.
Let's check the notification setup:
✅ Verification successful
Based on the results, I can see comprehensive usage of Notifee throughout the codebase, particularly in the features/notifications directory. The update from 9.1.2 to 9.1.3 is a patch version change, and the codebase shows well-structured notification handling including:
- Android channel setup
- Background notification handling
- Group message notifications
- Group welcome notifications
- Group join request notifications
Minor version update appears safe
The change from 9.1.2 to 9.1.3 is a patch version update that shouldn't introduce breaking changes. The codebase shows proper implementation of Notifee's core features and follows recommended patterns. The notification functionality should continue working as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check notification configuration and usage
# Check for Notifee initialization and usage
rg "notifee" -A 5
# Check for notification-related configuration in app config
fd "app.config" -x cat {} | rg -A 10 "notification"
Length of output: 10185
112-112: Essential addition for Expo prebuild configuration
The addition of expo-build-properties is crucial for configuring the Expo prebuild process, which is essential for the transition away from the custom Android folder.
Let's verify the build properties configuration:
✅ Verification successful
Verified: expo-build-properties is correctly configured and essential
The package is properly configured in app.config.ts with crucial Android build settings:
- Configures SDK versions (compile, target, min)
- Sets up manifest queries for wallet app integrations (Coinbase, MetaMask, etc.)
- Properly typed with PluginConfigTypeAndroid
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check build properties configuration
# Look for build properties configuration in app config
fd "app.config" -x cat {} | rg -A 10 "expo-build-properties"
# Check for any custom build configurations
fd "plugin" -e js -e ts | xargs rg "expo-build-properties"
Length of output: 751
Script:
#!/bin/bash
# Let's check for the complete app.config.* file content and any related configuration
fd "app.config" -x cat {}
# Also check for any EAS configuration that might use build properties
fd "eas\.json" -x cat {}
Length of output: 8425
| if (!contents.includes('@notifee/react-native')) { | ||
| const replacement = `maven { url 'https://www.jitpack.io' } | ||
| maven { | ||
| url "$rootDir/../node_modules/@notifee/react-native/android/libs" | ||
| }` | ||
| config.modResults.contents = contents.replace("maven { url 'https://www.jitpack.io' }", replacement) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a more robust method to modify Gradle repositories
Directly replacing a hard-coded string in build.gradle may lead to issues if the file structure changes. Consider parsing the Gradle file or using a regex to accurately insert the required repositories.
Suggestion: Use a regex pattern to find the repositories block
// Replace the entire repositories block if necessary
const repositoriesPattern = /repositories\s*{([\s\S]*?)}/;
if (repositoriesPattern.test(contents)) {
const replacement = `repositories {
maven { url 'https://www.jitpack.io' }
maven { url "$rootDir/../node_modules/@notifee/react-native/android/libs" }
}`;
config.modResults.contents = contents.replace(repositoriesPattern, replacement);
}| @@ -46,6 +46,7 @@ jobs: | |||
| echo "EXPO_PUBLIC_EXPO_PROJECT_ID=${{ secrets.EXPO_PUBLIC_EXPO_PROJECT_ID }}" >> .env.production | |||
| echo "EXPO_PUBLIC_PRIVY_APP_ID=${{ secrets.EXPO_PUBLIC_PRIVY_APP_ID }}" >> .env.production | |||
| echo "EXPO_PUBLIC_EVM_RPC_ENDPOINT=${{ secrets.EXPO_PUBLIC_EVM_RPC_ENDPOINT }}" >> .env.production | |||
| echo "SENTRY_AUTH_TOKEN=${{ secrets.SENTRY_AUTH_TOKEN }}" >> .env.production | |||
There was a problem hiding this comment.
Ensure secure handling of SENTRY_AUTH_TOKEN in build workflow
Including SENTRY_AUTH_TOKEN in .env.production can pose a security risk if the file is exposed. Confirm that .env.production is not checked into version control and that logs do not expose secret values.
Also applies to: 92-92
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
🧹 Nitpick comments (3)
scripts/build/android/buildGradleProperties.js (1)
3-24: Consider adding error handling for invalid inputsThe
setGradlePropertiesValuefunction should validate its inputs to ensure robustness.Consider adding these checks:
function setGradlePropertiesValue(config, key, value) { + if (!config || !key || value === undefined) { + throw new Error('Invalid arguments provided to setGradlePropertiesValue'); + } return withGradleProperties(config, (exportedConfig) => {app.config.ts (2)
147-220: Review SDK version configurationThe Android SDK configuration looks appropriate:
compileSdkVersionandtargetSdkVersionset to 34 (Android 14)minSdkVersionset to 26 (Android 8.0)- Comprehensive wallet app integration via manifest queries
Consider documenting the SDK version choices in a comment, especially the minimum SDK version decision, to help future maintainers understand the compatibility requirements.
Line range hint
1-234: Testing Recommendations for Expo MigrationSince this PR removes the Android folder in favor of Expo-managed Android configuration, please ensure:
- Generate a preview build and verify all deep links mentioned in the configuration
- Test push notifications functionality
- Verify wallet connections work as expected
- Test biometric authentication
- Validate file upload/download functionality (due to storage permissions)
Consider creating a test plan document that covers all these aspects to ensure nothing is missed during the transition.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app.config.ts(4 hunks)scripts/build/android/buildGradleProperties.js(1 hunks)
🔇 Additional comments (5)
scripts/build/android/buildGradleProperties.js (1)
30-30: Verify JVM memory settings
The JVM memory settings (-Xmx4096m -XX:MaxMetaspaceSize=1024m) are quite high. While this might help with build performance, ensure these values are actually needed and won't cause issues in CI/CD environments with limited resources.
app.config.ts (4)
17-36: LGTM: Environment-specific configuration is well structured
The environment-specific constants are well organized and follow a consistent pattern. The separation between dev, preview, and production environments is clear and maintainable.
77-89: Verify Android permissions scope
The list of permissions seems extensive. Some permissions might require additional user consent or justification in the Play Store listing.
Particularly check:
READ_CONTACTSWRITE_EXTERNAL_STORAGEUSE_FINGERPRINT(consider using onlyUSE_BIOMETRICfor modern Android versions)
Consider documenting why each permission is needed and ensure they align with Play Store policies.
90-141: Verify deep linking configuration completeness
The deep linking configuration appears comprehensive, covering multiple paths and domains. However, since the PR mentions verifying deep links, let's ensure all previous paths are covered.
✅ Verification successful
Based on the search results, I can now generate a final response as we have sufficient information about the deep linking implementation across the codebase.
Deep linking configuration is complete and consistent
The current deep linking configuration in app.config.ts aligns with all the paths being used in the codebase:
- The configuration covers all required paths:
/dm,/group,/group-invite,/coinbase, and/desktopconnect - These paths are properly handled in the navigation code (navHelpers.ts)
- Test coverage exists for deep linking functionality (navHelpers.test.ts)
- The implementation includes proper handling of both universal links and scheme-based deep links
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for deep link related configurations in the old android folder
rg -A 2 "android:scheme|android:host|android:pathPrefix"
Length of output: 58
Script:
#!/bin/bash
# Search for deep link related configurations in the codebase
rg -i "scheme.*host.*path|deeplink|intent.*filter" -A 5
Length of output: 5840
223-234: Verify Sentry configuration and custom plugins
The plugin configuration includes:
- Sentry integration
- Custom plugins for notifications and dependencies
- Gradle properties plugin (reviewed earlier)
✅ Verification successful
All plugin configurations are properly implemented and functional
The verification confirms:
- All three custom plugins exist and are correctly implemented:
notifeeExpoPlugin.js: Properly configures Maven repositories for @notifee/react-nativeandroidDependenciesExpoPlugin.js: Handles dependency conflicts with correct version resolutionsbuildGradleProperties.js: Sets appropriate JVM memory settings for Android builds
- Sentry is properly configured with the correct dependency (@sentry/react-native v5.31.1) and organization/project settings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all referenced plugin files exist
for plugin in "./scripts/build/android/notifeeExpoPlugin.js" "./scripts/build/android/androidDependenciesExpoPlugin.js" "./scripts/build/android/buildGradleProperties.js"; do
if [ ! -f "$plugin" ]; then
echo "Missing plugin file: $plugin"
fi
done
Length of output: 720
Script:
#!/bin/bash
# Let's check the content of these plugin files to verify their implementation
echo "=== notifeeExpoPlugin.js ==="
cat ./scripts/build/android/notifeeExpoPlugin.js
echo -e "\n=== androidDependenciesExpoPlugin.js ==="
cat ./scripts/build/android/androidDependenciesExpoPlugin.js
echo -e "\n=== buildGradleProperties.js ==="
cat ./scripts/build/android/buildGradleProperties.js
# Also check Sentry configuration in package.json
echo -e "\n=== Sentry dependencies in package.json ==="
cat package.json | grep -A 5 "@sentry/react-native"
Length of output: 3717
nmalzieu
had a problem deploying
to
Production
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app.config.ts (1)
Line range hint
1-238: Verify end-to-end functionality with Expo-generated Android code.The configuration looks comprehensive and aligns with the PR objective of removing the Android folder. However, please ensure:
- Deep links work correctly in preview builds
- Push notifications function as expected
- All wallet integrations are tested
- Biometric authentication works properly
- The app builds successfully with the new memory settings
Consider setting up automated end-to-end tests to verify these functionalities in your CI pipeline.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app.config.ts(4 hunks)scripts/build/android/google-services/preview.json(1 hunks)scripts/build/android/google-services/production.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- scripts/build/android/google-services/production.json
- scripts/build/android/google-services/preview.json
🔇 Additional comments (5)
app.config.ts (5)
Line range hint 3-36: LGTM! Well-structured environment-specific constants.
The constants are well-organized and follow a consistent pattern across environments. The naming is clear and the domain structure is properly set up for deep linking.
235-238: Verify custom plugin implementations.
The implementation of custom plugins should be reviewed:
notifeeExpoPlugin.js: For notification handlingandroidDependenciesExpoPlugin.js: For dependency conflictsbuildGradleProperties.js: For memory settings
Let's check these plugin implementations:
✅ Verification successful
Custom plugin implementations have been reviewed and are properly implemented
The plugins are well-structured and serve their intended purposes:
notifeeExpoPlugin.jscorrectly adds the required Maven repository for @notifee/react-nativeandroidDependenciesExpoPlugin.jsproperly handles dependency conflicts by fixing versions of bcprov and lifecycle-viewmodel-ktxbuildGradleProperties.jsappropriately configures JVM memory settings for Android builds
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the plugin files exist and examine their content
echo "Checking notifee plugin..."
cat ./scripts/build/android/notifeeExpoPlugin.js
echo "Checking dependencies plugin..."
cat ./scripts/build/android/androidDependenciesExpoPlugin.js
echo "Checking gradle properties plugin..."
cat ./scripts/build/android/buildGradleProperties.js
Length of output: 3184
156-223: Verify wallet integration configuration.
The manifest queries configuration includes support for multiple wallet apps (Coinbase, Ledger, Rainbow, MetaMask, etc.). Ensure that:
- All listed wallet integrations are actively supported
- Deep linking with these wallets is properly tested
- No deprecated wallet schemes are included
Let's check the wallet integration code:
✅ Verification successful
Wallet integration configuration is properly maintained and actively used
The verification shows that:
- All listed wallet schemes in app.config.ts are actively supported and match the
SUPPORTED_WALLETSarray in ConnectViaWalletSupportedWallets.ts - Deep linking is properly implemented with:
- URL scheme declarations in app.config.ts
- Runtime validation through
Linking.canOpenURL()checks - Proper wallet connection handling via WalletConnect
- The 1inch wallet is currently commented out in the code, which correctly reflects its removal from app.config.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for wallet integration code
echo "Searching for wallet-related code..."
rg -l "cbwallet|ledgerlive|rainbow|metamask|trust|uniswap|zerion|exodus|oneinch"
# Search for deep linking tests
echo "Searching for wallet deep linking tests..."
rg -l "test.*wallet.*deep.*link|test.*deep.*link.*wallet"
Length of output: 597
Script:
#!/bin/bash
# Check the wallet integration implementation details
echo "Checking ConnectViaWalletSupportedWallets.ts..."
cat components/Onboarding/ConnectViaWallet/ConnectViaWalletSupportedWallets.ts
# Check for any wallet-related deep linking implementation
echo "Checking deep linking implementation..."
rg -A 5 "scheme.*(?:cbwallet|ledgerlive|rainbow|metamask|trust|uniswap|zerion|exodus|oneinch)"
# Check for any deprecated or removed wallet schemes
echo "Checking for deprecated wallet references..."
git log -p -- app.config.ts | rg -A 3 "^\-.*scheme.*(?:cbwallet|ledgerlive|rainbow|metamask|trust|uniswap|zerion|exodus|oneinch)"
Length of output: 9488
151-156: Verify Android SDK configuration.
The Android SDK configuration looks good with modern versions:
compileSdkVersionandtargetSdkVersionset to 34 (Android 14)minSdkVersionset to 26 (Android 8.0)
However, ensure that the minimum SDK version aligns with your user base requirements.
Let's check the Play Store statistics for Android version distribution:
✅ Verification successful
Let me verify if there are any configuration files or documentation that might indicate the target Android version requirements.
Let me check the app configuration and any related files for Android version requirements or compatibility notes.
Android SDK configuration is appropriate for modern app development
The SDK configuration is well-aligned with current Android development standards:
- Using Android 14 (API 34) for
compileSdkVersionandtargetSdkVersionshows the app is up-to-date with the latest platform features minSdkVersion: 26(Android 8.0) is a reasonable choice as it:- Covers the vast majority of active Android devices (>95% according to official Android Studio statistics)
- Provides a good balance between modern API features and broad device compatibility
- Aligns with Google Play Store's minimum API level requirements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Note: This is a reminder to check Play Store Console for Android version distribution
echo "Please verify in Play Store Console that Android 8.0+ covers your target user base"
Length of output: 174
Script:
#!/bin/bash
# Check for any documentation about Android version requirements
fd -e md -e txt | xargs rg -i "android.*version|sdk.*version" -A 2
# Check build configuration files for Android version references
fd "build.gradle|gradle.properties|app.json|package.json" | xargs rg -i "android.*version|sdk.*version|min.*sdk" -A 2
Length of output: 2247
Script:
#!/bin/bash
# Check app.config.ts for any comments or documentation about SDK versions
rg -B 3 -A 3 "android.*version|sdk.*version|minSdk" app.config.ts
# Look for any related configuration files
fd "eas.json|app.config.js|expo-config" | xargs cat
Length of output: 2470
94-145: Verify deep linking configuration.
The intent filters configuration looks comprehensive. However, we should verify that all paths are properly handled in the app.
Let's check the deep link handlers:
✅ Verification successful
Let me analyze the deep linking configuration verification based on the results.
The search results show:
- Deep link handlers are implemented in:
- components/StateHandlers/InitialStateHandler.tsx
- utils/coinbaseWallet.ts
- Path handlers are found in multiple files:
- Features/GroupInvites/joinGroup/JoinGroupNavigation.tsx handles "/group-invite/:groupInviteId"
- Features/ExternalWalletPicker handles "/coinbase" callback
- Features/conversation handles group and DM paths
- App.tsx handles coinbase wallet deep links
Let me verify the implementation details:
Let me check the URL handling implementation to verify the deep link paths are properly handled:
Based on the verification results, I can now provide a final response:
Deep linking configuration is properly implemented and handled.
The implementation shows:
- Proper URL handling through
getSchemedURLFromUniversalURLthat processes all configured deep link paths - Initial deep link handling in
InitialStateHandlerusingLinking.getInitialURL() - URL event listener for Coinbase wallet callbacks
- Corresponding route configurations for all intent filter paths:
- /dm
- /group
- /group-invite
- /coinbase
- /desktopconnect
The deep linking implementation matches the intent filter configuration in app.config.ts, with proper URL transformation and navigation handling for each path.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for deep link handling code
echo "Searching for deep link handlers..."
rg -l "Linking.addEventListener|Linking.getInitialURL|universal-links|app-links"
# Search for specific path handlers
echo "Searching for specific path handlers..."
rg "/dm|/group-invite|/group|/coinbase|/desktopconnect"
Length of output: 67383
Script:
#!/bin/bash
# Check deep link handler implementation
echo "Checking deep link handler implementation..."
rg -A 5 "Linking.addEventListener|Linking.getInitialURL"
# Check route definitions
echo "Checking route definitions..."
ast-grep --pattern 'path: "$_"'
Length of output: 1201
Script:
#!/bin/bash
# Check URL handling implementation
echo "Checking URL handling implementation..."
rg -A 10 "getSchemedURLFromUniversalURL"
# Check navigation handling
echo "Checking navigation handling..."
rg -A 10 "handleInitialDeeplink"
# Check route configuration
echo "Checking route configuration..."
rg "path:|route:" screens/Navigation/
Length of output: 6017
| permissions: [ | ||
| "INTERNET", | ||
| "READ_EXTERNAL_STORAGE", | ||
| "SYSTEM_ALERT_WINDOW", | ||
| "VIBRATE", | ||
| "POST_NOTIFICATIONS", | ||
| "READ_CONTACTS", | ||
| "RECEIVE_BOOT_COMPLETED", | ||
| "WRITE_EXTERNAL_STORAGE", | ||
| "WAKE_LOCK", | ||
| "USE_FINGERPRINT", | ||
| "USE_BIOMETRIC", | ||
| ], |
There was a problem hiding this comment.
💡 Codebase verification
Permissions review reveals necessary but improvable implementation
Based on the codebase analysis:
READ_CONTACTS: Justified as the app usesContacts.getContactsAsync()in addressBook.ts for core functionalityWRITE_EXTERNAL_STORAGE: Used for legitimate file operations (attachments, caching) through RNFSUSE_FINGERPRINT/USE_BIOMETRIC: Biometric authentication is planned but not fully implemented (found TODO comment in keychain/helpers.ts)
Recommendations:
- Keep
READ_CONTACTSas it's essential for core functionality - Consider migrating to scoped storage APIs for Android 10+ while keeping
WRITE_EXTERNAL_STORAGEfor backward compatibility - Remove
USE_FINGERPRINTpermission as it's deprecated and you're already requestingUSE_BIOMETRIC
🔗 Analysis chain
Review sensitive permissions usage.
Some permissions requested are sensitive and require careful consideration:
READ_CONTACTS: Ensure this is essential for core functionalityWRITE_EXTERNAL_STORAGE: Consider if scoped storage could be used insteadUSE_FINGERPRINT: This is deprecated,USE_BIOMETRICis the modern alternative
Let's verify the permission usage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of sensitive permissions in the codebase
echo "Searching for contacts-related code..."
rg -l "READ_CONTACTS|ContactsProvider|getContacts"
echo "Searching for storage-related code..."
rg -l "WRITE_EXTERNAL_STORAGE|getExternalFilesDir|getExternalStorageDirectory"
echo "Searching for biometric-related code..."
rg -l "USE_FINGERPRINT|BiometricPrompt|FingerprintManager"
Length of output: 518
Script:
#!/bin/bash
# Let's examine the actual usage of these permissions in the codebase
echo "Checking contacts usage in addressBook.ts:"
rg -A 5 "READ_CONTACTS|ContactsProvider|getContacts" utils/addressBook.ts
echo -e "\nChecking for actual biometric implementation:"
rg -A 5 "biometric|fingerprint|authentication" --ignore-case
echo -e "\nChecking for file storage operations:"
rg "writeFile|readFile|saveFile|downloadFile|uploadFile" --ignore-case
Length of output: 10312
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
features/notifications/utils/background/notificationSpamScore.ts (1)
Line range hint
126-128: Improve error handling instead of silently ignoring errors.The current implementation silently ignores all errors, which could mask important issues and make debugging difficult. Consider:
- Logging errors for monitoring
- Handling specific error types differently
- Returning a conservative spam score on error
} catch (error) { - // Ignore errors + console.warn('Error computing spam score:', error); + // Return a conservative spam score to err on the side of caution + return 0.5; }features/notifications/utils/background/notificationContent.ts (1)
Migration from senderAddress to senderInboxId is incomplete
The codebase shows an ongoing migration from
senderAddresstosenderInboxId, but there are still several files that need to be updated:
features/conversation/hooks/use-send-message.ts: Still usingsenderAddresspropertyfeatures/conversation/conversation-message/conversation-message-reactions/conversation-message-reactions.types.ts: UsessenderAddressas key in type definitionutils/api.ts: UsessenderAddressas key in return type- Multiple instances in iOS code under
ios/ConverseNotificationExtension/still usesenderAddressdata/store/chatStore.ts: ContainssenderAddressproperty definition🔗 Analysis chain
Line range hint
1-1: Verify complete migration from senderAddress to senderInboxIdLet's ensure all instances of
senderAddresshave been migrated tosenderInboxIdacross the codebase:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of senderAddress rg "senderAddress" --type typescript # Search for new pattern to verify consistent usage rg "senderInboxId" --type typescriptLength of output: 143
Script:
#!/bin/bash # Search for any remaining instances of senderAddress without file type filtering rg "senderAddress" # Search for new pattern to verify consistent usage rg "senderInboxId"Length of output: 11849
♻️ Duplicate comments (1)
app.config.ts (1)
81-93: 🛠️ Refactor suggestionOptimize Android permissions configuration
Building upon the previous review:
permissions: [ "INTERNET", "READ_EXTERNAL_STORAGE", "SYSTEM_ALERT_WINDOW", "VIBRATE", "POST_NOTIFICATIONS", "READ_CONTACTS", "RECEIVE_BOOT_COMPLETED", - "WRITE_EXTERNAL_STORAGE", + "WRITE_EXTERNAL_STORAGE", // TODO: Consider implementing scoped storage for Android 10+ "WAKE_LOCK", - "USE_FINGERPRINT", // Remove deprecated permission "USE_BIOMETRIC", ],
🧹 Nitpick comments (6)
features/notifications/utils/background/groupMessageNotification.ts (3)
46-48: Clarify group conversation detection.
The check (conversation.version === ConversationVersion.GROUP) is a straightforward approach. Be sure to confirm that older or alternative conversation types won't inadvertently trigger false positives. If new conversation types are introduced, this logic might need updates.
79-117: Optimize or unify group notification logic.
The group notification block includes a fair bit of logic for building and displaying the notification. The subsequent block for individual messages has a similar pattern. To reduce repetitive code, consider abstracting shared notification-building logic into a helper function that handles the overlap between group and individual scenarios.
118-148: Focus on consistency between group and single-user notifications.
Most of the code is duplicated with slight differences. If the logic diverges in the future, duplication might become error-prone. Modularizing the notification assembly helps maintain consistency and handles future additions more gracefully.features/conversation/utils/is-latest-message-settled-from-peer.ts (1)
21-21: Confirm intended behavior for peer identification.
Switching the check from senderAddress to senderInboxId is consistent with the new approach. If some addresses map to the same inbox ID in certain edge cases, confirm that it doesn't cause unintended grouping or break the "settled from peer" logic.features/notifications/utils/background/notificationContent.ts (1)
Line range hint
13-46: Consider adding error handling for unsupported content typesThe
getNotificationContentfunction silently returns undefined for unsupported content types. Consider adding error logging or handling to track unsupported types in production.const supportedContentType = !!getMessageContentType(message.contentTypeId); - if (!supportedContentType) return; + if (!supportedContentType) { + console.warn(`Unsupported content type: ${message.contentTypeId}`); + return; + }features/conversation/conversation-composer/conversation-composer-reply-preview.tsx (1)
Line range hint
66-71: Consider simplifying the nested ternary expressionWhile the property update is correct, the nested ternary could be simplified for better readability.
Consider this more readable alternative:
- const replyingTo = replyMessage - ? replyMessage.senderInboxId === currentAccountInboxId - ? `Replying to you` - : inboxName - ? `Replying to ${inboxName}` - : "Replying" - : ""; + const replyingTo = (() => { + if (!replyMessage) return ""; + if (replyMessage.senderInboxId === currentAccountInboxId) return "Replying to you"; + return inboxName ? `Replying to ${inboxName}` : "Replying"; + })();
📜 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)
app.config.ts(4 hunks)features/conversation-list/hooks/useMessageIsUnread.ts(1 hunks)features/conversation/conversation-composer/conversation-composer-reply-preview.tsx(1 hunks)features/conversation/conversation-message/conversation-message-content-types/conversation-message-reply.tsx(1 hunks)features/conversation/conversation-message/conversation-message.store-context.tsx(1 hunks)features/conversation/conversation-message/conversation-message.utils.tsx(1 hunks)features/conversation/conversation.tsx(1 hunks)features/conversation/hooks/use-react-on-message.ts(1 hunks)features/conversation/hooks/use-remove-reaction-on-message.ts(1 hunks)features/conversation/hooks/use-send-message.ts(1 hunks)features/conversation/utils/has-next-message-in-serie.ts(1 hunks)features/conversation/utils/has-previous-message-in-serie.ts(1 hunks)features/conversation/utils/is-latest-message-settled-from-peer.ts(1 hunks)features/conversation/utils/message-is-from-current-user.ts(1 hunks)features/notifications/utils/background/groupMessageNotification.ts(3 hunks)features/notifications/utils/background/notificationContent.ts(1 hunks)features/notifications/utils/background/notificationSpamScore.ts(1 hunks)package.json(4 hunks)queries/useConversationMessages.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🔇 Additional comments (22)
features/notifications/utils/background/notificationSpamScore.ts (2)
91-91: Verify the sender identification refactor.
The change from senderAddress to senderInboxId is part of a broader refactor. While the type assertion provides some safety, we should ensure this change maintains the intended spam scoring behavior.
✅ Verification successful
Let me gather more specific information about the usage of senderInboxId to verify the refactoring consistency.
Based on the comprehensive analysis of the codebase, I can now generate the final response:
The sender identification refactor is consistent and properly implemented.
The change from senderAddress to senderInboxId is part of a broader, well-executed refactor across the codebase. The type assertion to InboxId is correctly used and the spam scoring behavior is maintained through:
- Proper consent state checks using
inboxIdConsentState - Consistent fallback to address-based checks when needed
- Integration with the existing spam scoring system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of senderAddress
# to ensure the refactor is complete
rg "senderAddress" --type ts
# Search for the pattern of InboxId usage to verify consistency
ast-grep --pattern 'as InboxId'
Length of output: 2054
Script:
#!/bin/bash
# Search for senderInboxId usage patterns
rg "senderInboxId" -A 2 --type ts
# Look for InboxId type usage and imports
rg "InboxId" --type ts
# Check the specific file for context
cat features/notifications/utils/background/notificationSpamScore.ts
Length of output: 49127
83-83:
Verify the impact of disabling consent synchronization.
The commented-out consent sync operation could lead to stale consent states being used for spam scoring. This might result in incorrect spam classifications if the user's consent preferences have changed.
Consider either:
- Removing the commented code if sync is no longer needed
- Re-enabling the sync if it's still required
- Adding a comment explaining why sync was disabled
features/notifications/utils/background/groupMessageNotification.ts (2)
36-44: Ensure conversation synchronization and error handling.
The double-check approach of syncing conversations and then retrying is good for reliability. However, if the repeated call still fails to find the conversation, the function throws an error. Ensure that the calling context properly handles this error case, and consider logging relevant message details for easier debugging.
49-51: Validate skipping self-sent notifications.
Skipping notifications if message.senderInboxId matches xmtpClient.inboxId prevents self-notifications. Confirm this is the intended behavior for all scenarios, especially when message acknowledgement or read receipts might be desired in the future.
features/conversation/utils/has-next-message-in-serie.ts (1)
13-13: Confirm the use of inbox IDs to indicate continuity.
Previously, senderAddress was used to check if consecutive messages belonged to the same sender. Now that senderInboxId is used, confirm that both fields represent equivalent checks for continuity to avoid unexpected grouping or separation of messages.
features/conversation/utils/has-previous-message-in-serie.ts (1)
13-13: Check for logical inversion.
Here, returning (previousMessage.senderInboxId !== currentMessage.senderInboxId) indicates that a new message series starts whenever the sender changes. Verify that logic remains consistent with desired UI displays or grouping, especially if prior logic or naming was reversed.
features/conversation/utils/message-is-from-current-user.ts (1)
15-15: Verify consistent usage of account ID retrieval methods
The two functions use different methods to get the current account ID:
messageIsFromCurrentUserusesgetCurrentAccount()messageIsFromCurrentUserV3usesgetCurrentUserAccountInboxId()
While both work, we should ensure they return consistent results.
Also applies to: 22-22
✅ Verification successful
Both account retrieval methods are correctly used and consistent
The verification shows that both methods are used appropriately:
getCurrentUserAccountInboxId()internally usesgetCurrentAccount()and transforms it to an inbox ID formatmessageIsFromCurrentUserandmessageIsFromCurrentUserV3are consistent in their approach:- The first function manually compares with
getCurrentAccount() - The second function uses
getCurrentUserAccountInboxId()which is a higher-level helper that handles the inbox ID transformation
- The first function manually compares with
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of these methods to ensure consistent patterns
rg -A 2 "getCurrentAccount\(\)" --type typescript
rg -A 2 "getCurrentUserAccountInboxId\(\)" --type typescript
Length of output: 184
Script:
#!/bin/bash
# Search for other usages of these methods to ensure consistent patterns
rg -A 2 "getCurrentAccount\(\)"
rg -A 2 "getCurrentUserAccountInboxId\(\)"
# Also search for their declarations to understand their implementations
ast-grep --pattern 'function getCurrentAccount() { $$$ }'
ast-grep --pattern 'function getCurrentUserAccountInboxId() { $$$ }'
Length of output: 9311
features/conversation-list/hooks/useMessageIsUnread.ts (1)
33-33: LGTM! Clean and consistent change
The update from senderAddress to senderInboxId is implemented correctly, and the memoization dependencies are properly maintained.
features/notifications/utils/background/notificationContent.ts (1)
67-67: 🛠️ Refactor suggestion
Consider adding case-insensitive comparison for consistency
Other similar comparisons in the codebase (e.g., in message-is-from-current-user.ts) use toLowerCase() for case-insensitive comparison. Consider applying the same pattern here:
- return message?.senderInboxId === client.inboxId;
+ return message?.senderInboxId.toLowerCase() === client.inboxId.toLowerCase();Likely invalid or redundant comment.
features/conversation/hooks/use-remove-reaction-on-message.ts (2)
41-41: LGTM: Explicit type casting improves type safety
The explicit type casting of getRandomId() to MessageId enhances type safety and ensures compatibility with the XMTP SDK.
48-48: LGTM: Consistent property naming
The rename from senderAddress to senderInboxId aligns with XMTP SDK terminology and improves code clarity.
features/conversation/hooks/use-react-on-message.ts (1)
42-42: LGTM: Consistent implementation with remove-reaction hook
The changes mirror those in use-remove-reaction-on-message.ts, maintaining consistency across reaction-related hooks:
- Type casting of message ID
- Updated property name to
senderInboxId
Also applies to: 49-49
features/conversation/hooks/use-send-message.ts (1)
75-75: LGTM: Type assertion improves type safety
The addition of the type assertion as MessageId ensures type safety when handling message IDs in the optimistic update flow.
features/conversation/conversation-message/conversation-message-content-types/conversation-message-reply.tsx (1)
118-118: LGTM: Consistent with senderInboxId refactor
The change to use senderInboxId with optional chaining and proper type assertion maintains type safety and aligns with the broader refactor.
features/conversation/conversation-composer/conversation-composer-reply-preview.tsx (1)
62-62: LGTM: Property name update is consistent
The change from senderAddress to senderInboxId maintains type safety with the InboxId assertion.
queries/useConversationMessages.ts (1)
193-193: LGTM: Consistent property name update in reaction processing
The variable rename maintains type safety and correctly integrates with the existing reaction processing logic.
features/conversation/conversation.tsx (1)
198-199: LGTM: Property update maintains message filtering functionality
The change from senderAddress to senderInboxId in the message filtering logic is consistent and type-safe.
app.config.ts (5)
17-36: LGTM: Well-structured environment-based configuration
The environment-specific constants are well-organized and follow consistent naming patterns across development, preview, and production environments.
151-156: LGTM: Up-to-date Android SDK configuration
The Android SDK configuration is properly set up with the latest stable versions:
- compileSdkVersion and targetSdkVersion: 34 (Android 14)
- buildToolsVersion: 34.0.0
- minSdkVersion: 26 (Android 8.0)
235-237: Verify custom plugin implementations
These custom plugins are crucial for the Android build process. Let's ensure they're properly implemented.
✅ Verification successful
Custom Android plugins are properly implemented and serve their intended purposes
All three plugins are well-implemented Expo config plugins with clear, specific functionality:
notifeeExpoPlugin.js: Correctly adds the required Maven repository for @notifee/react-nativeandroidDependenciesExpoPlugin.js: Properly handles dependency conflicts by forcing specific versions of bcprov and lifecycle-viewmodel-ktxbuildGradleProperties.js: Appropriately configures JVM memory settings for Android builds
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify custom plugin implementations
echo "Checking notifeeExpoPlugin.js:"
cat ./scripts/build/android/notifeeExpoPlugin.js
echo -e "\nChecking androidDependenciesExpoPlugin.js:"
cat ./scripts/build/android/androidDependenciesExpoPlugin.js
echo -e "\nChecking buildGradleProperties.js:"
cat ./scripts/build/android/buildGradleProperties.js
Length of output: 3222
94-145: Verify deep linking configuration
As mentioned in the PR objectives, deep linking functionality needs verification. The configuration looks comprehensive, but let's verify the implementation.
✅ Verification successful
Let me generate additional verification scripts to check the deep linking implementation more thoroughly.
This script will help verify:
- The URL scheme handling implementation
- Deep link path handling in components
- Deep link related imports and configurations
Let me analyze the results to provide a comprehensive verification of the deep linking functionality.
Based on the verification results, I can now provide a final response:
Deep linking configuration appears properly implemented
The deep linking implementation is verified and properly configured:
- URL scheme handling is implemented in
InitialStateHandler.tsxusingLinking.getInitialURL() - URL event listeners are set up for handling incoming links (e.g., in
coinbaseWallet.ts) - Deep linking imports and configurations are present across multiple components
- All configured paths in
app.config.ts(/dm, /group-invite, /group, /coinbase, /desktopconnect) have corresponding handling logic in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify deep linking implementation and usage
# Check for deep link handlers in the codebase
echo "Checking for deep link handlers:"
rg -l "Linking.addEventListener|Linking.getInitialURL"
# Check for usage of configured paths
echo -e "\nVerifying configured deep link paths:"
for path in "/dm" "/group-invite" "/group" "/coinbase" "/desktopconnect"; do
echo "Checking usage of path: $path"
rg -A 3 "$path"
done
Length of output: 68153
Script:
#!/bin/bash
# Check for deep link handling implementation in the codebase
# Search for URL scheme handling
echo "Checking URL scheme handling:"
rg -A 5 "Linking.(parse|getInitialURL|addEventListener)"
# Check for deep link path handling in components
echo -e "\nChecking deep link path handling in components:"
rg -A 5 "(handleDeepLink|onDeepLink|parseDeepLink)"
# Check for deep link related imports and configurations
echo -e "\nChecking deep link related imports and configurations:"
rg -A 3 "import.*Linking"
Length of output: 7509
156-223: Verify wallet integration functionality
The configuration includes extensive wallet scheme declarations. Let's verify their implementation.
✅ Verification successful
Let me analyze the implementation further by checking the wallet integration code specifically.
Let me check the actual wallet integration implementation to verify the functionality.
Let me modify the search to not rely on file type detection.
Based on the analysis of the code, I can now provide a final response:
Wallet integration implementation is properly configured and functional
The wallet scheme declarations in app.config.ts are properly implemented with:
- Complete wallet integration through WalletConnect protocol in
ConnectViaWalletSupportedWallets.ts - Proper URL scheme handling with
Linking.canOpenURLchecks for each wallet - Robust wallet connection flow with error handling and state management
- Support for multiple wallet types including Coinbase, Ledger, Rainbow, MetaMask, Trust, Uniswap, Zerion, and Exodus
- Proper navigation flow for both new accounts and onboarding processes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify wallet integration implementation
# Check for wallet-related code
echo "Checking for wallet integration code:"
rg -l "cbwallet|ledgerlive|rainbow|metamask|trust|uniswap|zerion|exodus|oneinch"
# Check for scheme handling implementation
echo -e "\nChecking for URL scheme handlers:"
rg -A 5 "Linking.openURL|Linking.canOpenURL"
Length of output: 7799
Script:
#!/bin/bash
# Check wallet integration implementation details
echo "Checking wallet integration in ConnectViaWalletSupportedWallets.ts:"
cat components/Onboarding/ConnectViaWallet/ConnectViaWalletSupportedWallets.ts
echo -e "\nChecking for WalletConnect implementation:"
rg -A 5 "WalletConnect|walletconnect"
Length of output: 56477
Script:
#!/bin/bash
# Check wallet integration implementation
echo "Checking wallet integration implementation:"
rg -A 10 "getInstalledWallets|useInstalledWallets" --type typescript
echo -e "\nChecking wallet connection handling:"
rg -A 10 "connectWallet|ConnectWallet" --type typescript
Length of output: 385
Script:
#!/bin/bash
# Check wallet integration implementation
echo "Checking wallet integration implementation:"
rg -A 10 "getInstalledWallets|useInstalledWallets"
echo -e "\nChecking wallet connection handling:"
rg -A 10 "connectWallet|ConnectWallet|WalletConnect"
echo -e "\nChecking URL scheme handling implementation:"
rg -A 5 "Linking.openURL.*wallet|Linking.canOpenURL.*wallet"
Length of output: 36280
| const senderAddress = (await conversation.members()).find( | ||
| (m) => m.inboxId === message.senderInboxId |
There was a problem hiding this comment.
Handle potential empty addresses array.
Here, addresses[0] is used without checking for empty arrays, which might cause runtime errors if the member has no known addresses. Consider a fallback or guard clause to handle that possibility gracefully.
- const senderAddress = (await conversation.members()).find(
- (m) => m.inboxId === message.senderInboxId
- )?.addresses[0];
+ const addressMember = (await conversation.members()).find(
+ (m) => m.inboxId === message.senderInboxId
+ );
+ if (!addressMember || !addressMember.addresses?.length) return;
+ const senderAddress = addressMember.addresses[0];Committable suggestion skipped: line range outside the PR's diff.
| @@ -82,7 +82,7 @@ function getStoreStateBasedOnProps(props: IMessageContextStoreProps) { | |||
| previousMessage: props.previousMessage, | |||
| }), | |||
| sentAt: convertNanosecondsToMilliseconds(props.message.sentNs), | |||
| senderInboxId: props.message.senderAddress, | |||
| senderInboxId: props.message.senderInboxId, | |||
There was a problem hiding this comment.
💡 Codebase verification
Property naming inconsistency detected across the codebase
The verification reveals mixed usage of both senderAddress and senderInboxId properties across the codebase:
- Several files still use
senderAddress:features/conversation/hooks/use-send-message.tsfeatures/conversation/conversation-message/conversation-message-reactions/conversation-message-reactions.types.tsdata/store/chatStore.ts- Multiple instances in notification-related files
While the change to senderInboxId in the current file is correct, there are inconsistencies that need to be addressed across the codebase to maintain uniformity.
🔗 Analysis chain
Verify consistent property usage across the codebase
The update to use senderInboxId aligns with the changes in other files. Let's verify the consistency of this property usage across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of senderInboxId vs senderAddress
# Check for any remaining instances of senderAddress
echo "Checking for remaining senderAddress usage:"
rg "senderAddress" --type ts
# Check for consistent senderInboxId usage
echo "Verifying senderInboxId usage:"
rg "senderInboxId" --type ts
Length of output: 6556
| @@ -97,7 +97,7 @@ export function useMessageSenderReadableProfile( | |||
| if (!currentAccountAdress) { | |||
| return ""; | |||
| } | |||
| return getReadableProfile(currentAccountAdress, message.senderAddress); | |||
| return getReadableProfile(currentAccountAdress, message.senderInboxId); | |||
There was a problem hiding this comment.
💡 Codebase verification
Migration from senderAddress to senderInboxId is incomplete
The codebase still contains several instances of senderAddress that need to be migrated:
features/conversation/hooks/use-send-message.tsfeatures/conversation/conversation-message-reactions/conversation-message-reactions.types.tsfeatures/notifications/utils/background/groupMessageNotification.tsdata/store/chatStore.tsutils/api.tsutils/transaction.ts
Some of these usages appear to be in type definitions and core functionality, suggesting this refactoring effort needs to be completed for consistency.
🔗 Analysis chain
Verify consistent usage of senderInboxId across the codebase
The change from senderAddress to senderInboxId aligns with the broader refactor. Let's verify consistent usage across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of senderAddress to ensure complete migration
rg "senderAddress" --type ts
# Search for consistent usage of senderInboxId
rg "senderInboxId" --type ts
Length of output: 6388
|
This fixes #1384 |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
.github/workflows/check-android-build.yml (1)
Line range hint
8-11: Update path triggers to reflect Android folder removalSince this PR removes the Android folder, the path trigger
android/**should be removed or updated to reflect the new Expo-based structure.paths: - - "android/**" - "package.json" + - "app.json" + - "eas.json"
🧹 Nitpick comments (1)
app.config.ts (1)
Line range hint
3-37: LGTM! Consider enhancing type safety for environment variables.The environment-specific constants are well-structured and follow a consistent pattern. However, consider replacing
process.env as anywith proper type definitions.-const env = process.env as any; +type EnvVars = { + EXPO_ENV: 'dev' | 'preview' | 'prod'; + EXPO_PUBLIC_DEV_API_URI?: string; +}; +const env = process.env as EnvVars;🧰 Tools
🪛 GitHub Check: lint
[warning] 39-39:
Prefer named exports
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/build-app-release.yml(5 hunks).github/workflows/build-internal.yml(5 hunks).github/workflows/check-android-build.yml(1 hunks).github/workflows/check-ios-build.yml(1 hunks).github/workflows/create-new-release.yml(1 hunks).gitignore(1 hunks)app.config.ts(4 hunks)package.json(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/build-internal.yml
- .github/workflows/build-app-release.yml
- package.json
🔇 Additional comments (10)
.github/workflows/check-ios-build.yml (2)
22-22: LGTM! Verify package.json compatibility with Node.js 20
The update to Node.js 20.x is appropriate as it's the latest LTS version. However, let's ensure the package.json configuration aligns with this change.
✅ Verification successful
Node.js 20.x update is compatible with package.json requirements
The package.json specifies ">=18" for Node.js version requirement, which means Node.js 20.x is fully compatible. No dependencies have explicit Node.js version constraints that could cause conflicts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check package.json for Node.js version constraints and potential compatibility issues
# Check engines field in package.json
echo "Checking Node.js version constraints in package.json:"
jq '.engines?.node' package.json
# Check if any dependencies have explicit Node.js version requirements
echo "Checking dependencies with Node.js version constraints:"
jq -r '.dependencies + .devDependencies | to_entries[] | select(.value | contains("node"))' package.json
Length of output: 386
22-22: Verify Expo compatibility with Node.js 20
Since this PR involves Expo configuration changes, let's ensure there are no compatibility issues between Expo and Node.js 20.x.
✅ Verification successful
Node.js 20.x is compatible with the project requirements
The verification shows that:
- The project uses Expo SDK 51 (
~51.0.38) - The minimum Node.js requirement is
>=18as specified in package.json - Expo SDK 51 is fully compatible with Node.js 20.x
The upgrade to Node.js 20.x in the GitHub Actions workflow is safe to proceed with.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Expo configuration and dependencies for Node.js compatibility
# Check Expo version and configuration
echo "Checking Expo version and configuration:"
jq '.dependencies.expo, .dependencies."expo-dev-client"' package.json
# Check for any Expo-related warnings about Node.js compatibility
echo "Checking app.json/app.config.js for Expo configuration:"
fd -e json -e js -g 'app.{json,config.js}'
Length of output: 358
Script:
#!/bin/bash
# Let's check the content of app.json and package.json for more details
echo "Content of app.json:"
cat app.json
echo -e "\nContent of package.json (relevant sections):"
jq '{
engines,
dependencies: {
expo,
"expo-dev-client",
"react-native"
}
}' package.json
# Check if there are any explicit Node.js version requirements in package.json
echo -e "\nChecking for Node.js version requirements:"
jq '.engines' package.json
Length of output: 799
.github/workflows/check-android-build.yml (2)
30-30: Verify Node.js 20.x compatibility
The upgrade to Node.js 20.x needs verification with the Expo build system and all dependencies.
✅ Verification successful
Node.js 20.x compatibility is confirmed
The codebase is already configured to work with Node.js 20.x:
package.jsonspecifies"node": ">=18", which includes Node.js 20.xeas.jsonexplicitly uses Node.js 20.15.1 in its build configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check package.json for potential compatibility issues
rg -A 5 '"engines":|"node":' package.json
# Check for any explicit Node.js version requirements in Expo config
rg -A 5 '"node":|"nodejs":|"runtime":|"version":' app.json eas.json
Length of output: 872
Line range hint 52-52: Verify EAS build configuration
Since we're transitioning from native Android to Expo builds, ensure the development profile in eas.json is properly configured to match the previous Android setup.
✅ Verification successful
EAS development profile is properly configured for Android builds
The development profile in eas.json is correctly set up with appropriate Android configuration:
- Uses internal distribution for development builds
- Configures medium resource class for Android builds
- Sets development channel and development client mode
- Includes necessary environment variables
The transition from native Android to Expo builds appears to be complete, with only iOS-specific native files remaining (sentry.properties).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check EAS build configuration
cat eas.json
# Look for any remaining native Android configuration files
fd -t f . -e gradle -e properties
Length of output: 1996
.github/workflows/create-new-release.yml (2)
22-22: Verify version increment script compatibility
The Node.js version upgrade to 20 requires verification of the version increment script compatibility.
✅ Verification successful
Node.js version 20 upgrade is compatible with version increment script
The version increment script uses standard Node.js features (fs, path) and basic JavaScript syntax that are fully supported in Node.js 20. The script:
- Uses CommonJS require statements
- Employs basic file operations (fs.readFile, fs.writeFile)
- Utilizes standard JavaScript array/string operations
- Handles JSON parsing/stringifying
All these features are stable across Node.js LTS versions and will work correctly with Node.js 20.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check version increment script for potential compatibility issues
cat scripts/build/incrementVersion.js
# Look for any Android-specific version management that needs updating
rg -A 5 'versionCode|versionName|version' app.json eas.json
Length of output: 2343
Line range hint 58-61: Update version management for Expo builds
Since we're transitioning to Expo builds, ensure the version increment process updates all necessary Expo configuration files.
✅ Verification successful
Let me examine the version increment script to understand if it handles all the version fields we found in app.json and eas.json.
Version management is properly configured for Expo builds
The version increment process already handles all necessary Expo configuration files. The incrementVersion.js script correctly updates:
- The main version number in
app.json(expo.version) - iOS build number (
expo.ios.buildNumber) - Android version code (
expo.android.versionCode)
These are all the version-related fields that need to be managed for Expo builds, as confirmed by their presence in app.json.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any version-related fields in Expo config
rg -A 5 '"version":|"buildNumber":|"versionCode":' app.json eas.json
# Verify if any Android-specific version files remain
fd -t f . -e gradle -e properties -e xml | rg -l 'version'
Length of output: 522
Script:
#!/bin/bash
# Check the content of the version increment script
cat scripts/build/incrementVersion.js
# Look for any other version-related scripts
fd -t f . -e js | rg -l 'version|Version' scripts/
Length of output: 2244
🧰 Tools
🪛 actionlint (1.7.4)
20-20: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
app.config.ts (4)
58-63: LGTM! Extra configuration is well-structured.
The extra configuration properly includes the EAS project ID and environment indicator.
82-94: Review and justify Android permissions
The permissions configuration needs review as previously discussed.
148-239: LGTM! Plugin configuration is comprehensive.
The configuration includes:
- Up-to-date Android SDK versions
- Proper Sentry integration
- Well-documented custom plugins for notifications and dependencies
Let's verify the custom plugins exist and are properly implemented:
✅ Verification successful
Custom plugins are properly implemented and exported
All three custom plugins are present and correctly export their configurations:
notifeeExpoPlugin.jsexports a plugin function for Notifee integrationandroidDependenciesExpoPlugin.jsexports a run-once pluginbuildGradleProperties.jsexports a custom plugin function
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the custom plugin files exist and contain the necessary exports
echo "Checking custom plugin files..."
for file in "./scripts/build/android/notifeeExpoPlugin.js" "./scripts/build/android/androidDependenciesExpoPlugin.js" "./scripts/build/android/buildGradleProperties.js"; do
if [ -f "$file" ]; then
echo "Found $file"
# Check if file exports a config
rg "module.exports|export default" "$file"
else
echo "Missing $file"
fi
done
Length of output: 1545
95-146: LGTM! Deep linking configuration is comprehensive.
The intent filters are well-structured and properly handle:
- App-specific URI schemes for each environment
- Universal links for both domains
- All required deep link paths
Let's verify the deep link paths match the web app routes:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ios/Podfile (1)
Line range hint
1-97: LGTM with version management recommendationsThe Podfile configuration looks solid with:
- Careful version pinning for critical dependencies (MMKV, OpenSSL, Sentry)
- Proper build settings and code signing configuration
- Appropriate handling of static/dynamic linkage
For better version management, consider:
- Adding comments explaining the rationale for specific versions (like you did for MMKV)
- Documenting the version update process for critical pods
- Setting up automated dependency update checks
📜 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 (1)
ios/Podfile(1 hunks)
🔇 Additional comments (1)
ios/Podfile (1)
Line range hint 89-97: Verify notification extension compatibility with Expo prebuild
Since this PR involves migrating to Expo prebuild and the objectives mention verifying notifications, please ensure that:
- The notification extension target remains properly configured
- The XMTP pod version update (3.0.19) maintains compatibility with notification handling
- The notification extension continues to work with the new Expo setup
Consider documenting the notification setup process with Expo prebuild to help future maintenance, as this is a critical feature that needs special attention during the Android-to-Expo migration.
| @@ -18,7 +18,7 @@ install! 'cocoapods', | |||
|
|
|||
| # Version must match version from XMTP Podspec (matching @xmtp/react-native-sdk from package.json) | |||
| # https://github.com/xmtp/xmtp-react-native/blob/v2.6.2/ios/XMTPReactNative.podspec#L29 | |||
| $xmtpVersion = '3.0.14' | |||
| $xmtpVersion = '3.0.19' | |||
There was a problem hiding this comment.
💡 Codebase verification
Version mismatch detected between package.json and Podfile
The XMTP versions are inconsistent across the codebase:
- package.json uses @xmtp/react-native-sdk ^3.1.4
- Podfile specifies $xmtpVersion = '3.0.19'
This version mismatch could lead to compatibility issues. The iOS pod version should be updated to match the React Native SDK version (3.1.x) to ensure consistent behavior.
🔗 Analysis chain
Verify XMTP version compatibility
The XMTP pod version has been updated from 3.0.14 to 3.0.19. While this is a minor version bump, please ensure:
- This version matches the corresponding @xmtp/react-native-sdk version in package.json
- There are no breaking changes in the changelog between these versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check package.json for @xmtp/react-native-sdk version
echo "Checking @xmtp/react-native-sdk version in package.json:"
cat package.json | jq '.dependencies["@xmtp/react-native-sdk"]'
# Check for any potential breaking changes in XMTP changelog between 3.0.14 and 3.0.19
echo "Checking XMTP changelog for breaking changes:"
gh api repos/xmtp/xmtp-react-native/releases | jq -r '.[] | select(.tag_name | startswith("v3.0.")) | {tag_name: .tag_name, body: .body}'
Length of output: 3187
alexrisch
force-pushed
the
noe/android-prebuild
branch
from
5fc0967 to
a0e06bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
features/notifications/utils/background/groupMessageNotification.ts (1)
62-64:⚠️ Potential issueHandle potential empty addresses array.
The code still accesses
addresses[0]without proper validation, which could cause runtime errors if the member has no addresses.- const senderAddress = (await conversation.members()).find( - (m) => m.inboxId === message.senderInboxId - )?.addresses[0]; + const member = (await conversation.members()).find( + (m) => m.inboxId === message.senderInboxId + ); + if (!member?.addresses?.length) return; + const senderAddress = member.addresses[0];
🧹 Nitpick comments (4)
features/notifications/utils/background/groupMessageNotification.ts (3)
54-59: Consider improving type safety and threshold clarity.Two suggestions for improvement:
- Add a type guard before casting to GroupWithCodecsType
- Consider using a named constant for the spam score threshold
+ const SPAM_SCORE_THRESHOLD = 0; const spamScore = await computeSpamScoreGroupMessage( xmtpClient, - conversation as GroupWithCodecsType, + isGroup ? (conversation as GroupWithCodecsType) : conversation, message ); - if (spamScore >= 0) return; + if (spamScore >= SPAM_SCORE_THRESHOLD) return;
73-76: Improve type safety in notification content retrieval.Consider adding type validation before casting to GroupWithCodecsType.
const notificationContent = await getNotificationContent( - conversation as GroupWithCodecsType, + isGroup ? (conversation as GroupWithCodecsType) : conversation, message );
107-109: Enhance documentation of Notifee limitations.The current comment about Notifee limitations could be more detailed to help future maintainers.
- // Notifee doesn't handle more complex messages with a group name & image + a person name & image - // so handling it manually by concatenating sender name & message + /** + * Workaround for Notifee limitation: + * The library doesn't support showing both group metadata (name/image) and + * individual sender metadata (name/image) in the same notification. + * Solution: Manually concatenate sender name with message content. + */app.config.ts (1)
Line range hint
3-37: LGTM! Consider extracting environment-specific configurations.The environment-specific constants are well-structured, but consider extracting them into a separate configuration file to improve maintainability.
Consider creating an
environments.config.ts:export const environments = { dev: { scheme: 'converse-dev', androidPackage: 'com.converse.dev', appDomainConverse: 'dev.converse.xyz', appDomainGetConverse: 'dev.getconverse.app', }, // ... preview and prod configs };🧰 Tools
🪛 GitHub Check: lint
[warning] 39-39:
Prefer named exports
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (16)
android/app/src/main/ic_launcher-playstore.pngis excluded by!**/*.pngandroid/app/src/main/ic_launcher_preview-playstore.pngis excluded by!**/*.pngandroid/app/src/main/res/drawable-hdpi/ic_default_expo_notifications.pngis excluded by!**/*.pngandroid/app/src/main/res/drawable-mdpi/ic_default_expo_notifications.pngis excluded by!**/*.pngandroid/app/src/main/res/drawable-xhdpi/ic_default_expo_notifications.pngis excluded by!**/*.pngandroid/app/src/main/res/drawable-xxhdpi/ic_default_expo_notifications.pngis excluded by!**/*.pngandroid/app/src/main/res/drawable-xxxhdpi/ic_default_expo_notifications.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-hdpi/bootsplash_logo.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-mdpi/bootsplash_logo.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xhdpi/bootsplash_logo.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xxhdpi/bootsplash_logo.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xxxhdpi/bootsplash_logo.pngis excluded by!**/*.pngandroid/gradle/wrapper/gradle-wrapper.jaris excluded by!**/*.jarassets/adaptive-icon.pngis excluded by!**/*.pngios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (64)
.github/workflows/build-app-release.yml(5 hunks).github/workflows/build-internal.yml(5 hunks).github/workflows/check-android-build.yml(1 hunks).github/workflows/check-ios-build.yml(1 hunks).github/workflows/create-new-release.yml(1 hunks).gitignore(1 hunks)android/.gitignore(0 hunks)android/app/build.gradle(0 hunks)android/app/proguard-rules.pro(0 hunks)android/app/src/debug/AndroidManifest.xml(0 hunks)android/app/src/main/AndroidManifest.xml(0 hunks)android/app/src/main/google-services.json(0 hunks)android/app/src/main/java/com/converse/MainActivity.kt(0 hunks)android/app/src/main/java/com/converse/MainApplication.kt(0 hunks)android/app/src/main/res/drawable-v24/ic_launcher_background.xml(0 hunks)android/app/src/main/res/drawable/rn_edit_text_material.xml(0 hunks)android/app/src/main/res/mipmap-anydpi-v26/ic_launcher.xml(0 hunks)android/app/src/main/res/mipmap-anydpi-v26/ic_launcher_preview.xml(0 hunks)android/app/src/main/res/mipmap-anydpi-v26/ic_launcher_preview_round.xml(0 hunks)android/app/src/main/res/mipmap-anydpi-v26/ic_launcher_round.xml(0 hunks)android/app/src/main/res/values-night/colors.xml(0 hunks)android/app/src/main/res/values/colors.xml(0 hunks)android/app/src/main/res/values/ic_launcher_background.xml(0 hunks)android/app/src/main/res/values/ic_launcher_preview_background.xml(0 hunks)android/app/src/main/res/values/strings.xml(0 hunks)android/app/src/main/res/values/styles.xml(0 hunks)android/app/src/preview/AndroidManifest.xml(0 hunks)android/app/src/preview/res/values/strings.xml(0 hunks)android/app/src/prod/AndroidManifest.xml(0 hunks)android/app/src/prod/res/values/strings.xml(0 hunks)android/build.gradle(0 hunks)android/gradle.properties(0 hunks)android/gradle/wrapper/gradle-wrapper.properties(0 hunks)android/gradlew(0 hunks)android/gradlew.bat(0 hunks)android/react-settings-plugin/build.gradle.kts(0 hunks)android/react-settings-plugin/src/main/kotlin/expo/plugins/ReactSettingsPlugin.kt(0 hunks)android/sentry.properties(0 hunks)android/settings.gradle(0 hunks)app.config.ts(4 hunks)eas.json(0 hunks)features/conversation-list/hooks/useMessageIsUnread.ts(1 hunks)features/conversation/conversation-composer/conversation-composer-reply-preview.tsx(1 hunks)features/conversation/conversation-message/conversation-message-content-types/conversation-message-reply.tsx(1 hunks)features/conversation/conversation-message/conversation-message.store-context.tsx(1 hunks)features/conversation/conversation-message/conversation-message.utils.tsx(1 hunks)features/conversation/hooks/use-react-on-message.ts(1 hunks)features/conversation/hooks/use-remove-reaction-on-message.ts(1 hunks)features/conversation/utils/has-next-message-in-serie.ts(1 hunks)features/conversation/utils/has-previous-message-in-serie.ts(1 hunks)features/conversation/utils/is-latest-message-settled-from-peer.ts(1 hunks)features/conversation/utils/message-is-from-current-user.ts(1 hunks)features/notifications/utils/background/groupMessageNotification.ts(2 hunks)features/notifications/utils/background/notificationContent.ts(1 hunks)features/notifications/utils/background/notificationSpamScore.ts(1 hunks)ios/Podfile(1 hunks)package.json(4 hunks)queries/useConversationMessages.ts(1 hunks)scripts/build/android/androidDependenciesExpoPlugin.js(1 hunks)scripts/build/android/build.js(0 hunks)scripts/build/android/buildGradleProperties.js(1 hunks)scripts/build/android/google-services/preview.json(1 hunks)scripts/build/android/google-services/production.json(1 hunks)scripts/build/android/notifeeExpoPlugin.js(1 hunks)
💤 Files with no reviewable changes (35)
- android/app/src/main/res/mipmap-anydpi-v26/ic_launcher_preview_round.xml
- android/app/src/main/google-services.json
- android/app/src/main/res/drawable-v24/ic_launcher_background.xml
- android/app/src/debug/AndroidManifest.xml
- android/sentry.properties
- scripts/build/android/build.js
- android/app/src/main/res/mipmap-anydpi-v26/ic_launcher.xml
- android/gradlew
- android/settings.gradle
- android/app/src/main/res/values/ic_launcher_background.xml
- android/build.gradle
- android/gradlew.bat
- android/app/src/preview/AndroidManifest.xml
- android/app/src/main/res/values/ic_launcher_preview_background.xml
- android/app/src/prod/res/values/strings.xml
- android/react-settings-plugin/build.gradle.kts
- android/app/src/main/res/mipmap-anydpi-v26/ic_launcher_round.xml
- eas.json
- android/app/src/main/res/mipmap-anydpi-v26/ic_launcher_preview.xml
- android/react-settings-plugin/src/main/kotlin/expo/plugins/ReactSettingsPlugin.kt
- android/app/src/preview/res/values/strings.xml
- android/gradle/wrapper/gradle-wrapper.properties
- android/app/src/main/res/drawable/rn_edit_text_material.xml
- android/app/proguard-rules.pro
- android/app/src/main/java/com/converse/MainApplication.kt
- android/.gitignore
- android/app/src/main/res/values-night/colors.xml
- android/app/build.gradle
- android/app/src/prod/AndroidManifest.xml
- android/app/src/main/java/com/converse/MainActivity.kt
- android/gradle.properties
- android/app/src/main/res/values/strings.xml
- android/app/src/main/res/values/styles.xml
- android/app/src/main/AndroidManifest.xml
- android/app/src/main/res/values/colors.xml
🚧 Files skipped from review as they are similar to previous changes (27)
- scripts/build/android/google-services/production.json
- .github/workflows/check-android-build.yml
- scripts/build/android/google-services/preview.json
- features/conversation/conversation-message/conversation-message.store-context.tsx
- features/conversation/utils/has-next-message-in-serie.ts
- features/conversation/utils/has-previous-message-in-serie.ts
- queries/useConversationMessages.ts
- .github/workflows/build-app-release.yml
- .github/workflows/check-ios-build.yml
- .github/workflows/build-internal.yml
- features/conversation/conversation-composer/conversation-composer-reply-preview.tsx
- ios/Podfile
- features/notifications/utils/background/notificationSpamScore.ts
- scripts/build/android/notifeeExpoPlugin.js
- features/conversation/conversation-message/conversation-message.utils.tsx
- features/conversation/hooks/use-react-on-message.ts
- features/conversation/hooks/use-remove-reaction-on-message.ts
- features/notifications/utils/background/notificationContent.ts
- features/conversation/utils/message-is-from-current-user.ts
- .gitignore
- scripts/build/android/buildGradleProperties.js
- features/conversation/utils/is-latest-message-settled-from-peer.ts
- .github/workflows/create-new-release.yml
- features/conversation/conversation-message/conversation-message-content-types/conversation-message-reply.tsx
- scripts/build/android/androidDependenciesExpoPlugin.js
- package.json
- features/conversation-list/hooks/useMessageIsUnread.ts
🔇 Additional comments (6)
features/notifications/utils/background/groupMessageNotification.ts (2)
1-24: LGTM! Import statements are well-organized.
The new imports align well with the updated implementation, particularly the addition of conversation-related types and profile utilities.
34-46: Well-structured conversation handling with proper error handling.
The implementation properly handles conversation retrieval with a sync retry mechanism and explicit error handling. The group check using ConversationVersion.GROUP is a clean approach.
app.config.ts (4)
58-63: LGTM! Extra configuration is properly structured.
The extra configuration properly includes EAS project ID and environment information necessary for builds.
82-94: Review and update Android permissions.
Several concerns about the permissions list:
USE_FINGERPRINTis deprecated in favor ofUSE_BIOMETRIC- Some permissions might require careful justification for app store review
95-146: LGTM! Deep linking configuration is well-structured.
The intent filters are properly configured to handle all required deep linking scenarios across different environments.
148-239: Verify Android SDK compatibility with dependencies.
The Android SDK configuration (compileSdkVersion: 34, targetSdkVersion: 34) is up-to-date, but we should verify compatibility with all dependencies.
✅ Verification successful
Android SDK configuration is compatible with dependencies
The Android SDK configuration (compileSdkVersion: 34, targetSdkVersion: 34, minSdkVersion: 26) is appropriate and no conflicting requirements were found in the dependencies or codebase. The configuration:
- Uses the latest stable Android SDK (34/Android 14)
- Sets a reasonable minimum SDK version (26/Android 8.0)
- Is the only SDK version specification in the codebase
- Aligns with the requirements of key dependencies like React Native 0.75.4 and Expo SDK 51
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any SDK version requirements in package.json dependencies
echo "Checking package.json for dependencies with specific Android SDK requirements..."
jq -r '.dependencies' package.json
# Search for any SDK version specifications in the codebase
echo "Searching for Android SDK version specifications in the codebase..."
rg "minSdkVersion|targetSdkVersion|compileSdkVersion" --type ts --type js --type json
Length of output: 7190
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
features/conversation/hooks/use-send-message.ts (1)
Line range hint
115-127: Improve error handling in onSuccess callbackThe current implementation throws an error when the message is not found, but this error might not be properly caught and handled.
Consider this improvement:
onSuccess: async (messageId, _, context) => { if (context && messageId) { // The SDK only returns the messageId const message = await fetchMessageByIdQuery({ account: getCurrentAccount()!, messageId, }); if (!message) { - throw new Error("Message not found"); + // Handle missing message gracefully + captureErrorWithToast(new Error("Failed to fetch sent message")); + // Refetch to ensure consistency + await refetchConversationMessages( + getCurrentAccount()!, + conversation.topic + ); + return; } if (message) { replaceOptimisticMessageWithReal({ tempId: context.generatedMessageId, topic: conversation.topic, account: getCurrentAccount()!, message, }); } } },
🧹 Nitpick comments (4)
ios/ConverseNotificationExtension/Xmtp/Messages.swift (3)
68-71: Reassess overwriting senderInboxId
Overwriting decodedMessage.senderInboxId with the first entry from conversation.members() can create confusion if the group has multiple addresses or if the original inbox ID is needed later. Please confirm that this behavior is intentional.
96-96: Error handling for group image
Try-catch around groupImage retrieval might improve reliability. If group.groupImageUrlSquare() throws or returns nil, you could log or handle that gracefully.
294-295: Confirm concurrency safety when returning decoded message
Consider whether additional concurrency checks or immutable copies of decodedMessage’s properties are required, especially in multi-thread contexts.features/conversation/hooks/use-send-message.ts (1)
Line range hint
74-94: Consider handling all message types optimisticallyThe current implementation only handles simple text messages optimistically. Consider extending the optimistic updates to:
- Messages with remote attachments
- Referenced messages (replies)
This would provide a more consistent user experience across all message types.
Example implementation for handling attachments:
if (variables.content.text && !variables.referencedMessageId) { const generatedMessageId = getRandomId(); + const content = variables.content.remoteAttachment + ? { remoteAttachment: variables.content.remoteAttachment } + : { text: variables.content.text }; const textMessage: DecodedMessage<TextCodec> = { id: generatedMessageId as MessageId, client: conversation.client, contentTypeId: variables.content.text ? contentTypesPrefixes.text : contentTypesPrefixes.remoteAttachment, sentNs: getTodayNs(), fallback: "new-message", deliveryStatus: "sending" satisfies IConversationMessageStatus, topic: conversation.topic, senderAddress: currentUserInboxId, nativeContent: {}, - content: () => { - return variables.content.text!; - }, + content: () => content, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignore(1 hunks)features/conversation/conversation.tsx(1 hunks)features/conversation/hooks/use-send-message.ts(1 hunks)ios/ConverseNotificationExtension/Xmtp/Messages.swift(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🔇 Additional comments (9)
features/conversation/conversation.tsx (1)
198-198: Verify sender identification change impact
The change from senderAddress to senderInboxId for sender identification looks correct, but we should verify a few aspects:
Let's run some checks to ensure the change is consistent and safe:
Additionally, consider adding a null check for senderInboxId to handle cases where it might be undefined:
return messages.ids.find(
(messageId) =>
isAnActualMessage(messages.byId[messageId]) &&
- messages.byId[messageId].senderInboxId === currentAccountInboxId
+ messages.byId[messageId].senderInboxId != null &&
+ messages.byId[messageId].senderInboxId === currentAccountInboxId
);ios/ConverseNotificationExtension/Xmtp/Messages.swift (6)
63-63: Verify conversation existence more robustly
Using "if let" is good, but consider adding logs or error handling if no conversation is found, to help diagnose issues when the conversation is unexpectedly absent.
88-89: Handle fallback for missing profile
When getProfile returns nil or fails, bestAttemptContent.subtitle remains unset. Consider providing a fallback or logging to ensure the user is aware of a missing profile.
99-100: Consistent handling of getProfile
Similar to lines 88-89, ensure you handle failure or nil returns from getProfile. Logging or fallback logic can reduce confusion.
164-164: Ensure consistent identification of sender
You're using (decodedMessage?.senderInboxId ?? "") as the fallback. Returning an empty ID could cause confusion in analytics or logs if the value is missing.
288-288: Reassess default false for forceIgnore
When you encounter an unknown content type, you return false for forceIgnore. Evaluate whether certain unknown content might actually need ignoring, or log a warning here to track unrecognized types.
337-337: Check case sensitivity in inbox ID comparison
Comparing message.decodeOrNull()?.senderInboxId to xmtpClient.inboxID letter-for-letter might cause issues if IDs differ by case only. Determine whether a case-insensitive comparison is needed.
features/conversation/hooks/use-send-message.ts (2)
75-75: LGTM: Type assertion improves type safety
The type assertion ensures that the generated ID conforms to the XMTP SDK's MessageId type, preventing potential type-related issues.
Line range hint 84-84: Verify sender identifier property name
The code uses senderAddress but according to the summary, this should have been changed to senderInboxId across the codebase.
Let's verify this inconsistency:
✅ Verification successful
Property name inconsistency is expected - both are used for different purposes
Based on the search results, both senderAddress and senderInboxId are used in the codebase, but for different purposes:
senderInboxIdis used in message objects from the XMTP protocol for identifying message senderssenderAddressis used in specific contexts like reactions storage and API responses
In the specific file use-send-message.ts, the usage of senderAddress: currentUserInboxId is correct as it's mapping the inbox ID to a property expected by the consuming code.
Key evidence:
features/conversation/hooks/use-send-message.tsusessenderAddressto send messagesfeatures/conversation/hooks/use-react-on-message.tsusessenderInboxIdfor reactions- Both properties coexist in the codebase serving different purposes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for both property names in the codebase
echo "Searching for senderAddress usage:"
rg "senderAddress" --type ts
echo -e "\nSearching for senderInboxId usage:"
rg "senderInboxId" --type ts
# Search for any recent changes to these properties
echo -e "\nChecking recent changes:"
git log -p --grep="sender.*[Aa]ddress|sender.*[Ii]nbox[Ii]d"
Length of output: 6711
| senderAvatar = getPreferredAvatar(socials: senderProfileSocials) | ||
| } | ||
| if let content = decodedMessageResult.content { | ||
| bestAttemptContent.body = content | ||
| } | ||
| messageIntent = getIncoming1v1MessageIntent( | ||
| topic: contentTopic, | ||
| senderId: decodedMessage.senderAddress, | ||
| senderId: decodedMessage.senderInboxId, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify empty string default
When senderInboxId is nil, you default to "". Consider logging or returning early if the inbox ID is unknown, especially if the overall logic relies on it.
| func decodeMessage(xmtpClient: XMTP.Client, envelope: XMTP.Xmtp_MessageApi_V1_Envelope) async throws -> DecodedMessage? { | ||
| // If topic is MLS, the conversation should already be there | ||
| // @todo except if it's new convo => call sync before? | ||
| if (isV3MessageTopic(topic: envelope.contentTopic)) { | ||
| if let conversation = try! xmtpClient.findConversationByTopic(topic: envelope.contentTopic) { | ||
| if let conversation = try! await xmtpClient.findConversationByTopic(topic: envelope.contentTopic) { |
There was a problem hiding this comment.
Avoid force-try to prevent crashes
Applying try! can cause a crash if findConversationByTopic throws. Replace it with a do-catch or optional unwrap to handle errors gracefully.
- if let conversation = try! await xmtpClient.findConversationByTopic(topic: envelope.contentTopic) {
+ if let conversation = try? await xmtpClient.findConversationByTopic(topic: envelope.contentTopic) {
// ...
}📝 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 let conversation = try! await xmtpClient.findConversationByTopic(topic: envelope.contentTopic) { | |
| if let conversation = try? await xmtpClient.findConversationByTopic(topic: envelope.contentTopic) { |
Remove the android folder from the codebase
Configure expo + plugins to make expo generate ~the same Android code & config
Will need to double check that deeplinks & notifications work on a preview build!
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
.gitignoreto include Android development files.Dependencies
@notifee/react-nativeandXMTPpod.Code Improvements
senderInboxIdinstead ofsenderAddress, enhancing clarity and consistency.