-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ar/expo 52 #1276
Ar/expo 52 #1276
Conversation
Updates for React Native 75
Added versions for Android Dependency Bumped OTP Entry
WIP
WalkthroughThe changes in this pull request encompass updates to various configuration files across the Android and iOS platforms, including Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
android/settings.gradle (1)
7-23
: Simplify autolinking logic for better readabilityThe autolinking logic involves complex command execution and string interpolation, which can reduce readability and maintainability. Consider refactoring this section to use more straightforward methods or extracting the command construction into a separate script or function.
android/app/src/main/java/com/converse/MainApplication.kt (1)
25-28
: Consider simplifying the package retrievalIf no additional packages are being added manually, you can return the packages directly without assigning them to a variable. This can make the code more concise.
Apply this diff if no manual packages are added:
override fun getPackages(): List<ReactPackage> { - val packages = PackageList(this).packages - // Packages that cannot be autolinked yet can be added manually here, for example: - // packages.add(new MyReactNativePackage()); - return packages + return PackageList(this).packages }If you plan to add packages manually, it's fine to keep the current structure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
ios/Gemfile.lock
is excluded by!**/*.lock
ios/Podfile.lock
is excluded by!**/*.lock
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (14)
.gitignore
(1 hunks)android/app/build.gradle
(7 hunks)android/app/src/main/AndroidManifest.xml
(1 hunks)android/app/src/main/java/com/converse/MainApplication.kt
(3 hunks)android/build.gradle
(1 hunks)android/gradle.properties
(2 hunks)android/gradle/wrapper/gradle-wrapper.properties
(1 hunks)android/gradlew
(3 hunks)android/gradlew.bat
(1 hunks)android/settings.gradle
(1 hunks)babel.config.js
(1 hunks)ios/Converse.xcodeproj/project.pbxproj
(7 hunks)ios/Podfile
(5 hunks)package.json
(5 hunks)
✅ Files skipped from review due to trivial changes (3)
- android/gradlew.bat
- android/gradle/wrapper/gradle-wrapper.properties
- babel.config.js
🧰 Additional context used
🪛 rubocop (1.68.0)
ios/Podfile
[convention] 11-11: Avoid using rescue
in its modifier form.
(Style/RescueModifier)
[convention] 44-44: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
[convention] 64-64: Avoid comma after the last parameter of a method call.
(Style/TrailingCommaInArguments)
🔇 Additional comments (20)
.gitignore (1)
Line range hint 4-11
: Verify that ignoring *.key
doesn't exclude necessary files
Adding *.key
to .gitignore
may unintentionally ignore important files that are essential for the project. Ensure that this pattern does not exclude any required files from version control.
android/build.gradle (1)
6-6
: Confirm that increasing minSdkVersion
to 24 is acceptable
Raising the minSdkVersion
from 23 to 24 means the app will no longer support devices running Android 6.0 (Marshmallow). Verify that this decision aligns with the project's target audience and does not negatively impact user reach.
android/app/src/main/java/com/converse/MainApplication.kt (2)
13-13
: Ensure OpenSourceMergedSoMapping
is available
Verify that com.facebook.react.soloader.OpenSourceMergedSoMapping
is included in your dependencies and compatible with your React Native version to prevent runtime issues.
45-45
: Verify SoLoader
initialization parameters
Updating SoLoader.init
to use OpenSourceMergedSoMapping
changes the initialization behavior. Ensure that this is compatible with your application's requirements and the React Native version you're using.
android/gradle.properties (2)
25-25
: Confirm necessity of enabling PNG crunching in release builds
Setting android.enablePngCrunchInReleaseBuilds=true
can increase build times and is generally disabled for PNG images that don't require processing. Verify if enabling PNG crunching is necessary for your project's assets.
54-54
: Verify impact of expo.useLegacyPackaging=false
Changing expo.useLegacyPackaging
to false
modifies how native libraries are packaged in the APK. Confirm that this setting is compatible with your project dependencies and does not introduce any unexpected behavior.
ios/Podfile (4)
13-14
: New environment variables added for React Native architecture and network inspector.
The addition of these environment variables aligns with modern React Native development practices.
33-46
: Enhanced autolinking configuration with fallback mechanism.
The implementation provides a robust fallback mechanism for community autolinking.
🧰 Tools
🪛 rubocop (1.68.0)
[convention] 44-44: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
90-90
: Consistent deployment target across build configurations.
The deployment target is consistently set to 15.1 across all pod targets.
16-17
: Verify iOS deployment target compatibility with app features.
The deployment target has been updated to iOS 15.1. This change might affect the app's reach to users with older iOS versions.
✅ Verification successful
Let me gather more information to ensure there are no other iOS version-specific API usages.
iOS 15.1 deployment target appears compatible with codebase
No usage of iOS 16+ specific APIs or features was detected in the codebase. The deployment target of iOS 15.1 appears to be safe as:
- No
@available
checks for iOS 16 or higher - No direct references to iOS 16+ versions
- No usage of iOS 16+ specific UIKit features
- No conflicting minimum OS version specified in Info.plist
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any API usage that might be incompatible with iOS 15.1
rg -l "if #available\(iOS 1[6-9]" || echo "No iOS 16+ API usage found"
Length of output: 98
Script:
#!/bin/bash
# Check for iOS version-specific API usage patterns
rg "@available\(iOS 1[6-9]" || echo "No @available iOS 16+ annotations found"
# Check for potential iOS version checks in code
rg "iOS 1[6-9]" || echo "No direct iOS 16+ version references found"
# Look for UIKit features that might be iOS 16+ specific
rg "UICalendarView|UIEditMenuInteraction|UIFindInteraction|UIPageControl\.InteractionState|UIPasteControl" || echo "No iOS 16+ specific UIKit features found"
# Check Info.plist for minimum deployment target
fd "Info.plist" --exec cat {} \; | grep -i "MinimumOSVersion" || echo "No explicit minimum OS version in Info.plist"
Length of output: 593
android/app/src/main/AndroidManifest.xml (2)
58-58
: Application configuration updates with RTL support.
The addition of android:supportsRtl="true"
improves accessibility for RTL languages.
Line range hint 13-57
: Comprehensive URL scheme handling for wallet integrations.
The manifest includes handlers for multiple wallet applications, enhancing interoperability.
✅ Verification successful
URL scheme handlers in AndroidManifest.xml are properly implemented and utilized
The URL schemes declared in AndroidManifest.xml are correctly implemented and actively used in the codebase. The ConnectViaWalletSupportedWallets.ts
file shows proper integration where:
- Each wallet has a corresponding
customScheme
that matches the schemes in AndroidManifest.xml - The code includes functionality to check if these wallet apps are installed via
Linking.canOpenURL
- The implementation supports proper deep linking through these URL schemes for wallet connections
The manifest declarations are necessary and properly utilized for the wallet integration functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any wallet-related code that might use these URL schemes
rg -l "cbwallet|ledgerlive|rainbow|metamask|trust|uniswap|zerion|exodus|oneinch"
Length of output: 344
Script:
#!/bin/bash
# Let's check the actual implementation of wallet-related code to verify proper usage
rg -A 5 "cbwallet|ledgerlive|rainbow|metamask|trust|uniswap|zerion|exodus|oneinch" --type ts --type tsx
Length of output: 133
Script:
#!/bin/bash
# Check the implementation in TypeScript files
rg -A 5 "cbwallet|ledgerlive|rainbow|metamask|trust|uniswap|zerion|exodus|oneinch" "*.ts"
# Also check the content of the wallet-related component
cat "components/Onboarding/ConnectViaWallet/ConnectViaWalletSupportedWallets.ts"
Length of output: 7124
android/gradlew (2)
18-19
: Added SPDX license identifier.
Proper license identification improves compliance tracking.
87-88
: Improved APP_HOME path resolution.
The new implementation provides more robust path resolution and handles edge cases better.
android/app/build.gradle (3)
25-28
: LGTM: Expo CLI integration changes
The added configuration properly integrates the Expo CLI for bundling, which is required for the Expo 52 upgrade.
175-177
: LGTM: Standard resource filtering configuration
The ignoreAssetsPattern configuration properly excludes common unwanted files from resource processing.
204-208
: Verify compatibility of updated dependencies
Several dependencies have been updated or added:
- firebase-messaging-ktx: 24.0.1
- org.xmtp:android: 0.16.2
- org.web3j:crypto: 4.9.4
Please ensure these versions are compatible with the upgraded Expo version.
✅ Verification successful
Let me gather more information about the dependencies and their usage in the codebase.
Let me check the Kotlin and Android SDK versions to ensure compatibility with these dependencies.
Dependencies are compatible with the current setup
The verification shows that:
- Firebase Messaging 24.0.1 has no reported security vulnerabilities
- The dependencies are compatible with the current setup:
- Kotlin 1.9.24 supports all the added libraries
- Android SDK versions (min: 24, target: 34, compile: 35) meet the requirements
- Dependency conflicts are properly managed through resolution strategy
- The new dependencies (XMTP and Web3j) are only used in the app module and don't conflict with existing dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any known compatibility issues with these dependencies
# Expected: No major compatibility issues reported
echo "Checking Firebase Messaging compatibility..."
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "com.google.firebase:firebase-messaging-ktx") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 476
Script:
#!/bin/bash
# Check for usage of these dependencies in the codebase
echo "Checking XMTP usage..."
rg "org\.xmtp" -A 3
echo -e "\nChecking Web3j usage..."
rg "org\.web3j" -A 3
echo -e "\nChecking for other Firebase Messaging versions..."
rg "firebase-messaging.*:" --type gradle
echo -e "\nChecking build.gradle files for conflicts..."
fd build.gradle --exec cat {}
Length of output: 13438
Script:
#!/bin/bash
# Check Kotlin and Android SDK versions
echo "Checking Kotlin version..."
rg "kotlinVersion" --type gradle
echo -e "\nChecking Android SDK versions..."
rg "compileSdkVersion|minSdkVersion|targetSdkVersion" --type gradle
echo -e "\nChecking for potential version conflicts..."
rg "resolutionStrategy" -A 5 --type gradle
Length of output: 1646
package.json (1)
42-44
: Verify compatibility of major version upgrades
Significant version upgrades detected:
-
Expo ecosystem:
- expo: ~52.0.9
- @expo/config-plugins: ~9.0.0
- Multiple expo packages updated to latest versions
-
Core dependencies:
- react: 18.3.1
- react-native: 0.76.3
Please ensure:
- All breaking changes have been addressed
- The upgrade follows the official migration guide
- All native modules are compatible with React Native 0.76.3
Also applies to: 104-134, 150-153
✅ Verification successful
The previous results showed some TODOs related to migrations, so let's gather more context about the upgrade preparations and configurations. This will help us verify if the major version upgrades have been properly handled.
Based on the gathered information, I can now provide a final response:
Major version upgrades appear to be properly configured
The package.json dependencies show a well-coordinated upgrade with compatible versions:
- Expo ecosystem packages are aligned with expo version ~52.0.9
- React Native 0.76.3 and React 18.3.1 are properly matched
- Metro configuration is properly set up with necessary transformers and polyfills
- Dependencies maintain compatible version ranges
The TODOs found are unrelated to the version upgrades:
- RNBootSplash upgrade is a separate enhancement
- Onboarding migration is a feature-specific task
- Message reactions TODO is a feature implementation detail
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential compatibility issues
# Expected: No major compatibility issues reported
# Check for breaking changes in package.json
echo "Checking for breaking changes in dependencies..."
rg -l "breaking|deprecat|remove" "CHANGELOG.md" || true
# Look for any TODO/FIXME comments that might indicate incomplete migration
echo "Checking for migration-related TODOs..."
rg -i "todo.*\b(upgrad|migrat|expo|react)" || true
Length of output: 662
Script:
#!/bin/bash
# Let's check for any migration guides or upgrade-related documentation
echo "Checking for migration documentation..."
fd -t f "README|UPGRADE|MIGRATION" -X cat {} 2>/dev/null
# Check package.json for any potential peer dependency conflicts
echo "Checking package.json for dependencies..."
cat package.json | jq '.dependencies + .devDependencies'
# Look for any expo configuration files that might need updates
echo "Checking for expo configuration files..."
fd -t f "app.json|app.config.js|metro.config.js" -X cat {}
Length of output: 13101
ios/Converse.xcodeproj/project.pbxproj (2)
436-446
: LGTM: Enhanced privacy bundle configuration
Added privacy bundles for various dependencies:
- RCT-Folly_privacy.bundle
- React-Core_privacy.bundle
- React-cxxreact_privacy.bundle
- boost_privacy.bundle
- glog_privacy.bundle
This change improves privacy-related resource management.
Also applies to: 460-470
621-621
: Verify iOS deployment target compatibility
The iOS deployment target has been updated to 15.1 across all configurations. This change aligns with the Expo 52 requirements.
Please ensure this change:
- Matches your app's minimum supported iOS version
- Is consistent with the Podfile configuration
Also applies to: 659-659, 704-704, 749-749
✅ Verification successful
iOS deployment target is correctly configured
The deployment target of 15.1 is consistently set in both the Xcode project configurations and the Podfile, ensuring proper compatibility across the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify iOS deployment target consistency
# Expected: Deployment target should be consistent across configurations
echo "Checking Podfile for deployment target..."
rg "platform :ios" "ios/Podfile"
Length of output: 193
pluginManagement { | ||
includeBuild(new File(["node", "--print", "require.resolve('@react-native/gradle-plugin/package.json')"].execute(null, rootDir).text.trim()).getParentFile().toString()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using .execute()
due to potential security risks
Using .execute()
to run external commands can pose security risks and may lead to inconsistent behavior across different environments. Consider using Gradle's built-in mechanisms or properties to resolve plugin paths without executing external processes.
buildToolsVersion = findProperty('android.buildToolsVersion') ?: '35.0.0' | ||
minSdkVersion = Integer.parseInt(findProperty('android.minSdkVersion') ?: '24') | ||
compileSdkVersion = Integer.parseInt(findProperty('android.compileSdkVersion') ?: '35') | ||
targetSdkVersion = Integer.parseInt(findProperty('android.targetSdkVersion') ?: '34') | ||
kotlinVersion = "1.9.24" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Update Kotlin Gradle plugin to match the Kotlin version
The kotlinVersion
has been updated to "1.9.24"
, but the Kotlin Gradle plugin in the dependencies section is still using version "1.8.0"
. For consistency and to avoid potential compatibility issues, update the plugin version to match the Kotlin version.
Apply this diff to update the plugin version:
dependencies {
classpath('com.android.tools.build:gradle:8.5.0')
classpath('com.facebook.react:react-native-gradle-plugin')
- classpath 'org.jetbrains.kotlin:kotlin-gradle-plugin:1.8.0'
+ classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:$kotlinVersion"
classpath("io.sentry:sentry-android-gradle-plugin:3.11.1")
classpath 'com.google.gms:google-services:4.4.0'
}
Committable suggestion skipped: line range outside the PR's diff.
@@ -47,3 +46,9 @@ expo.webp.enabled=true | |||
# Enable animated webp support (~3.4 MB increase) | |||
# Disabled by default because iOS doesn't support animated webp | |||
expo.webp.animated=false | |||
|
|||
# Enable network inspector | |||
EX_DEV_CLIENT_NETWORK_INSPECTOR=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disable network inspector in production builds
EX_DEV_CLIENT_NETWORK_INSPECTOR=true
enables the network inspector, which is useful during development but can pose security risks and performance overhead in production. Ensure this is disabled or conditionally set based on the build type.
Apply a conditional check to set this property only in debug builds.
shrinkResources (findProperty('android.enableShrinkResourcesInReleaseBuilds')?.toBoolean() ?: false) | ||
minifyEnabled enableProguardInReleaseBuilds | ||
proguardFiles getDefaultProguardFile("proguard-android.txt"), "proguard-rules.pro" | ||
crunchPngs (findProperty('android.enablePngCrunchInReleaseBuilds')?.toBoolean() ?: true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Build configuration properties partially defined
The review requires adjustment as only some of the referenced properties are defined in gradle.properties:
android.enablePngCrunchInReleaseBuilds
is defined and set to trueexpo.useLegacyPackaging
is defined and set to falseandroid.enableShrinkResourcesInReleaseBuilds
is missing from the configuration
The build will use the default fallback values for undefined properties as implemented in build.gradle, but it's recommended to explicitly define all configurable properties for better maintainability.
🔗 Analysis chain
LGTM: Enhanced build configuration flexibility
The changes improve resource management by making shrinkResources, crunchPngs, and useLegacyPackaging configurable through properties. This allows for better control over the build process.
Also applies to: 171-171
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the presence of these properties in gradle.properties
# Expected: These properties should be defined in gradle.properties
echo "Checking for build properties in gradle.properties..."
rg -l "android\.enableShrinkResourcesInReleaseBuilds|android\.enablePngCrunchInReleaseBuilds|expo\.useLegacyPackaging" "android/gradle.properties"
Length of output: 289
Script:
#!/bin/bash
# Let's check the actual content of gradle.properties to verify if these properties are properly defined
echo "Checking the content of gradle.properties..."
cat android/gradle.properties
# Also check if there are any other files that might be overriding these properties
echo -e "\nChecking for other potential property definitions..."
rg "android\.enable(ShrinkResourcesInReleaseBuilds|PngCrunchInReleaseBuilds)" --type properties
Length of output: 2562
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Chores