chore: android studio agp assistant; 16kb pagesize fix #1107
Conversation
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on October 17. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
WalkthroughUpgrades Android build tooling and Gradle wrapper, sets Java/Kotlin to Java 17 toolchain, adjusts native build/linker flags and modernizes native packaging (jniLibs/pickFirst for libc++_shared), and standardizes NDK version references across modules and CI. No runtime app-source logic changes. Changes
Sequence Diagram(s)(omitted — changes are build/configuration edits, not runtime control-flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/android/app/build.gradle (1)
112-119: Java/Kotlin 17 OK — remove or guard release→debug keystore fallback
- Java 17 toolchain + kotlin jvmTarget=17 are configured.
- signingConfigs.release currently falls back to debug.keystore when MYAPP_UPLOAD_* props are missing (app/android/app/build.gradle — signingConfigs.release, ~lines 157–167). Fastlane maps these env vars (app/fastlane/Fastfile ~lines 408–411).
- Action (choose one): (A) hard‑fail releases when creds are absent (throw GradleException in the else branch), (B) gate fallback behind an explicit local‑only flag so CI releases fail fast, or (C) keep fallback but ensure CI/Fastlane always supplies MYAPP_UPLOAD_* and add a CI check that signing is correct before publishing.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/android/app/build.gradle(3 hunks)app/android/app/src/main/AndroidManifest.xml(1 hunks)app/android/build.gradle(1 hunks)app/android/gradle/wrapper/gradle-wrapper.properties(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/android/**/*
⚙️ CodeRabbit configuration file
app/android/**/*: Review Android-specific code for:
- Platform-specific implementations
- Performance considerations
- Security best practices for mobile
Files:
app/android/gradle/wrapper/gradle-wrapper.propertiesapp/android/build.gradleapp/android/app/src/main/AndroidManifest.xmlapp/android/app/build.gradle
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-deps
- GitHub Check: e2e-ios
- GitHub Check: android-build-test
- GitHub Check: analyze-android
- GitHub Check: analyze-ios
🔇 Additional comments (2)
app/android/gradle/wrapper/gradle-wrapper.properties (1)
4-4: Gradle 8.13 is compatible with AGP 8.11.x — good upgrade. Confirm CI JDK.Looks good. Please ensure your CI/Android Studio Gradle JVM is ≥17 (prefer 21 if your environment already standardized on it) to avoid toolchain/runtime mismatches.
app/android/app/build.gradle (1)
234-238: Toolchain block LGTM; ensure Gradle daemon JVM matches.Keep the Gradle JVM (in CI/Studio) at ≥17 to avoid running Gradle on an older JDK while compiling with 17.
| pickFirsts += ['**/libc++_shared.so'] | ||
| } | ||
| resources { | ||
| excludes += ['META-INF/LICENSE', 'META-INF/NOTICE', 'META-INF/versions/9/OSGI-INF/MANIFEST.MF'] | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Don’t pickFirst libc++_shared; enforce a single known source.
pickFirst on libc++_shared.so can silently mask version skew from transitive AARs and cause hard‑to‑trace native crashes. Since you already link c++_shared via CMake (ANDROID_STL=c++_shared) and useLegacyPackaging=false, exclude all transitive copies and ship exactly one from your app.
jniLibs {
useLegacyPackaging false
- pickFirsts += ['**/libc++_shared.so']
+ excludes += ['**/libc++_shared.so']
}Also verify the merged manifest contains android:extractNativeLibs="false" (see Manifest comment) to keep mmap loading and APK size benefits.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Grep for libc++_shared / c++_shared / ANDROID_STL occurrences =="
rg -n --hidden --no-ignore -S "libc\+\+_shared|c\+\+_shared|ANDROID_STL" -C3 || true
echo
echo "== Grep for pickFirsts / jniLibs / useLegacyPackaging in android app dir =="
rg -n --hidden --no-ignore -S "pickFirsts|jniLibs|useLegacyPackaging|excludes \+= \['\*\*/libc\+\+_shared.so'\]" -C4 android || true
echo
echo "== Locate CMakeLists.txt files =="
fd -HI --type f CMakeLists.txt || true
echo
echo "== Show android/app/src/main/AndroidManifest.xml (first 200 lines) if present =="
if [ -f android/app/src/main/AndroidManifest.xml ]; then
sed -n '1,200p' android/app/src/main/AndroidManifest.xml || true
else
echo "android/app/src/main/AndroidManifest.xml not found"
fi
echo
echo "== Search for android:extractNativeLibs across repository =="
rg -n --hidden --no-ignore -S "extractNativeLibs" -C3 || true
echo
echo "== Search for externalNativeBuild / cmake / stl references in android dir =="
rg -n --hidden --no-ignore -S "externalNativeBuild|cmake|stl" -C5 android || true
echo
echo "== Print matching build.gradle snippet around any jniLibs occurrences =="
rg -n --hidden --no-ignore -S "jniLibs" -C8 android || trueLength of output: 2976
Remove pickFirst for libc++_shared — exclude transitive copies and ship a single c++_shared
pickFirst masks version skew in transitive AARs and can cause hard‑to‑trace native crashes; you build with ANDROID_STL=c++_shared and useLegacyPackaging=false, so exclude transitive copies and ship exactly one libc++_shared from the app.
- app/android/app/build.gradle — change in the jniLibs block:
jniLibs {
useLegacyPackaging false
- pickFirsts += ['**/libc++_shared.so']
+ excludes += ['**/libc++_shared.so']
}- packages/mobile-sdk-alpha/android/build.gradle (around line 97) — remove/replace packagingOptions pickFirst '**/libc++_shared.so' so the SDK does not ship its own copy.
- Verify the merged app manifest has android:extractNativeLibs="false" (check app/android/app/src/main/AndroidManifest.xml and the merged manifest).
📝 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.
| pickFirsts += ['**/libc++_shared.so'] | |
| } | |
| resources { | |
| excludes += ['META-INF/LICENSE', 'META-INF/NOTICE', 'META-INF/versions/9/OSGI-INF/MANIFEST.MF'] | |
| } | |
| jniLibs { | |
| useLegacyPackaging false | |
| excludes += ['**/libc++_shared.so'] | |
| } | |
| resources { | |
| excludes += ['META-INF/LICENSE', 'META-INF/NOTICE', 'META-INF/versions/9/OSGI-INF/MANIFEST.MF'] | |
| } |
🤖 Prompt for AI Agents
In app/android/app/build.gradle lines 186-190, remove the pickFirst rule that
accepts '**/libc++_shared.so' and instead exclude transitive copies so the app
ships exactly one c++_shared: delete the pickFirst entry and add a
jniLibs/packagingOptions exclusion for '**/libc++_shared.so' in library modules
that currently include it (also remove/replace the same pickFirst in
packages/mobile-sdk-alpha/android/build.gradle around line 97), ensure the app
module provides the single libc++_shared (or depends on the NDK/CMake-provided
shared lib) and finally verify android:extractNativeLibs="false" is set in the
merged app manifest (check app/src/main/AndroidManifest.xml and the merged
manifest) to avoid duplicate native extraction.
| android:roundIcon="@mipmap/ic_launcher" | ||
| android:extractNativeLibs="false" | ||
| tools:replace="android:icon, android:roundIcon, android:name, android:extractNativeLibs" | ||
| tools:replace="android:icon, android:roundIcon, android:name, android:extractNativeLibs" |
There was a problem hiding this comment.
🧩 Analysis chain
Harden manifest: drop privileged permission, restrict cleartext, and verify native libs extraction.
- android.permission.ACCESS_SURFACE_FLINGER (Line 8) is system-only; requesting it can trigger Play policy review and offers no benefit. Remove it.
- android:usesCleartextTraffic="true" (Line 23) weakens transport security. Gate it to debug or switch to a Network Security Config that whitelists only development hosts; keep false for release.
- You removed android:extractNativeLibs while enabling modern JNI packaging in Gradle. That’s fine if the AGP merger injects extractNativeLibs="false"; please verify it’s present in the merged manifest to retain mmap performance and APK size wins.
Suggested minimal changes:
- <uses-permission android:name="android.permission.ACCESS_SURFACE_FLINGER" />
...
- android:usesCleartextTraffic="true"
+ android:usesCleartextTraffic="false"
+ android:networkSecurityConfig="@xml/network_security_config"
+ <!-- If the merger doesn't add it, keep this explicitly: -->
+ <!-- android:extractNativeLibs="false" -->🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Listing AndroidManifest.xml files (repo):"
rg -n --hidden --no-ignore -S "AndroidManifest.xml" || true
echo
MANIFEST="app/android/app/src/main/AndroidManifest.xml"
if [ -f "$MANIFEST" ]; then
echo "Showing $MANIFEST (first 200 lines with numbers):"
nl -ba -w3 -s': ' "$MANIFEST" | sed -n '1,200p'
else
echo "$MANIFEST not found; listing any manifest files found:"
rg -n --hidden --no-ignore -S '<manifest' --glob '!**/node_modules/**' || true
fi
echo
echo "Searching repo for suspicious entries:"
rg -n --hidden --no-ignore -S "ACCESS_SURFACE_FLINGER|usesCleartextTraffic|extractNativeLibs|android:extractNativeLibs|networkSecurityConfig" || true
echo
echo "Searching for network security config xml (res/xml):"
rg -n --hidden --no-ignore -S '<network-security-config|res/xml' --glob '!**/node_modules/**' || true
echo
echo "Searching Gradle files for packaging / legacy packaging settings:"
rg -n --hidden --no-ignore -S 'useLegacyPackaging|packagingOptions|extractNativeLibs|jniLibs|jni' || true
echo
echo "Done."Length of output: 814
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
MANIFEST="app/android/app/src/main/AndroidManifest.xml"
if [ -f "$MANIFEST" ]; then
echo "=== $MANIFEST ==="
awk '{printf "%4d: %s\n", NR, $0}' "$MANIFEST" | sed -n '1,200p'
else
echo "Manifest not found: $MANIFEST"
fi
echo
echo "Searching repo for target strings:"
rg -n --hidden --no-ignore -S "ACCESS_SURFACE_FLINGER|usesCleartextTraffic|android:extractNativeLibs|extractNativeLibs|networkSecurityConfig" || true
echo
echo "Searching for network security config XML files under res/xml:"
rg -n --hidden --no-ignore -S "res/xml|network_security_config" || true
echo
echo "Searching Gradle files for packaging / JNI settings:"
rg -n --hidden --no-ignore -S "useLegacyPackaging|packagingOptions|extractNativeLibs|jniLibs|jni" || trueLength of output: 318519
Harden manifest: drop privileged permission, restrict cleartext, and verify native libs extraction.
- Remove android.permission.ACCESS_SURFACE_FLINGER — present in app/android/app/src/main/AndroidManifest.xml (line 8) and debug manifests; system-only and can trigger Play policy action.
- Disable cleartext in release — android:usesCleartextTraffic="true" is set in app/android/app/src/main/AndroidManifest.xml (line 23) (also in debug manifests); set false for release and use a Network Security Config that only whitelists dev hosts — keep true only in debug.
- Verify native libs extraction — main manifest’s tools:replace includes android:extractNativeLibs (line 20). app/android/app/build.gradle sets jniLibs/useLegacyPackaging false; confirm the merged RELEASE manifest contains android:extractNativeLibs="false" and, if it does not, add it explicitly.
Suggested minimal changes:
- <uses-permission android:name="android.permission.ACCESS_SURFACE_FLINGER" />
...
- android:usesCleartextTraffic="true"
+ android:usesCleartextTraffic="false"
+ android:networkSecurityConfig="@xml/network_security_config"
+ <!-- If the merger doesn't add it, keep this explicitly: -->
+ <!-- android:extractNativeLibs="false" -->Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/android/app/src/main/AndroidManifest.xml around lines 8 and 20–23, remove
the system-only permission android.permission.ACCESS_SURFACE_FLINGER from the
manifest and any debug manifests; set android:usesCleartextTraffic="false" in
the release manifest (keep true only in debug) and reference a Network Security
Config that whitelists dev hosts for debug builds; ensure the merged RELEASE
manifest contains android:extractNativeLibs="false" (since build.gradle sets
jniLibs/useLegacyPackaging=false) and if the merged release manifest does not
include it, add android:extractNativeLibs="false" explicitly in the release
manifest or via tools:replace in the main manifest so the release APK will not
extract native libs.
| } | ||
| dependencies { | ||
| classpath("com.android.tools.build:gradle:8.6.0") | ||
| classpath('com.android.tools.build:gradle:8.11.2') |
There was a problem hiding this comment.
🧩 Analysis chain
AGP 8.11.2 OK; remove jcenter() and the forced firebase-iid to avoid supply‑chain and compat risks.
- jcenter() (Lines 36) is read‑only and a supply‑chain/availability risk. Remove unless you have a hard requirement and mirror artifacts.
- Forcing com.google.firebase:firebase-iid:21.1.0 (Lines 53–55) is obsolete and can conflict with modern Firebase Messaging. Let the Firebase BoM manage versions.
Suggested edits:
allprojects {
repositories {
google()
mavenCentral()
- jcenter()
+ // jcenter() removed (read-only; avoid supply-chain risk)
maven {
url("$rootDir/../../node_modules/react-native/android")
}
...
- configurations.all {
- resolutionStrategy {
- force 'com.google.firebase:firebase-iid:21.1.0'
- }
- }
+ // Do not force firebase-iid; rely on the Firebase BoM.
}🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Searching repo for jcenter(), firebase-iid and forced resolutionStrategy..."
rg -n --hidden --no-ignore -S "jcenter\(|com.google.firebase:firebase-iid|resolutionStrategy|configurations\.all" || true
echo
echo "2) Show likely android build.gradle files if present:"
for f in app/android/build.gradle android/build.gradle android/app/build.gradle; do
if [ -f "$f" ]; then
echo "----- $f -----"
nl -ba "$f" | sed -n '1,240p'
echo
fi
done
echo "3) Show any gradle files that contain 'firebase-iid' or 'jcenter' (full context)"
rg -n --hidden --no-ignore -S "jcenter\(|com.google.firebase:firebase-iid" -g '!*node_modules/*' || true
echo
echo "4) Search for firebase BoM usage"
rg -n --hidden --no-ignore -S "firebase-bom|com.google.firebase:firebase-bom" || trueLength of output: 938
AGP 8.11.2 OK — remove jcenter() and stop forcing firebase‑iid
- app/android/build.gradle: remove jcenter() (line 36) — jcenter is read‑only and a supply‑chain/availability risk.
- app/android/build.gradle: remove the configurations.all resolutionStrategy forcing 'com.google.firebase:firebase-iid:21.1.0' (line 54). Let the Firebase BoM manage versions to avoid conflicts with modern Firebase Messaging.
- Also note: resolutionStrategy.force 'com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava' (line 50) — confirm this shim is still required.
Suggested edits:
allprojects {
repositories {
google()
mavenCentral()
- jcenter()
+ // jcenter() removed (read-only; avoid supply-chain risk)
maven {
url("$rootDir/../../node_modules/react-native/android")
}
...
- configurations.all {
- resolutionStrategy {
- force 'com.google.firebase:firebase-iid:21.1.0'
- }
- }
+ // Do not force firebase-iid; rely on the Firebase BoM.
}📝 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.
| classpath('com.android.tools.build:gradle:8.11.2') | |
| buildscript { | |
| repositories { | |
| google() | |
| mavenCentral() | |
| } | |
| dependencies { | |
| classpath('com.android.tools.build:gradle:8.11.2') | |
| } | |
| } | |
| allprojects { | |
| repositories { | |
| google() | |
| mavenCentral() | |
| // jcenter() removed (read-only; avoid supply-chain risk) | |
| maven { | |
| url("$rootDir/../../node_modules/react-native/android") | |
| } | |
| } | |
| // Do not force firebase-iid; rely on the Firebase BoM. | |
| } |
🤖 Prompt for AI Agents
In app/android/build.gradle around lines 23–60, remove the deprecated jcenter()
repository entry at line 36 and delete the configurations.all resolutionStrategy
that force‑pins 'com.google.firebase:firebase-iid:21.1.0' at line 54 so Firebase
versions are governed by the Firebase BoM; also inspect the
resolutionStrategy.force entry for
'com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava' at
line 50 and either keep it with a comment explaining why the shim is still
required or remove it if no longer necessary.
transphorm
changed the title
| cmake { | ||
| cppFlags += "-fexceptions -frtti -std=c++11" | ||
| arguments += "-DANDROID_STL=c++_shared" | ||
| arguments += "-Wl,-z,max-page-size=16384" |
There was a problem hiding this comment.
fyi @seshanthS
supports using 16kb page sizes
https://developer.android.com/guide/practices/page-sizes#update-packaging
| } | ||
| dependencies { | ||
| classpath("com.android.tools.build:gradle:8.6.0") | ||
| classpath('com.android.tools.build:gradle:8.11.2') |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/android/react-native-passport-reader/android/build.gradle (3)
17-20: AGP DSL change: replace packagingOptions with android.packaging.resources.excludes.AGP 8+ deprecates/renames this block; old DSL may break the build.
Suggested diff:
- packagingOptions { - exclude 'META-INF/LICENSE' - exclude 'META-INF/NOTICE' - } + packaging { + resources { + excludes += ['META-INF/LICENSE', 'META-INF/NOTICE'] + } + }
27-29: lintOptions is deprecated; use the lint block.Keeps the build forward‑compatible with AGP 8+.
- lintOptions { - warning 'InvalidPackage' - } + lint { + warning 'InvalidPackage' + }
32-48: Pin React Native dependency; remove dynamic 'com.facebook.react:react-native:+' (blocking)app/android/react-native-passport-reader/android/build.gradle (lines 32–48) uses implementation 'com.facebook.react:react-native:+', which creates non-reproducible builds and can pull incompatible RN/artifacts — replace with a pinned RN version or the RN BOM that matches your app.
- Replace the dynamic '+' with either:
- implementation(platform("com.facebook.react:react-native-bom:<TARGET_RN_VERSION>")) + implementation "com.facebook.react:react-native"
- OR implementation "com.facebook.react:react-native:<TARGET_RN_VERSION>"
- Also pin and update other outdated libs in the same file (okhttp 4.9.0, gson 2.8.9, commons-io 2.8.0, androidx.core:core-ktx 1.9.0) to maintained patch lines to address security/perf/stability risk.
- Do not merge until RN is pinned and these libraries are pinned/upgraded or verified safe.
Provide your target RN and AGP versions and I will produce a compatible dependency matrix and exact version pins.
🧹 Nitpick comments (1)
app/android/react-native-passport-reader/android/build.gradle (1)
21-26: ProGuard config: use modern default or R8 rules only.
getDefaultProguardFile('proguard-android.txt')is obsolete; prefer'proguard-android-optimize.txt'or remove if minifyDisabled.- proguardFiles getDefaultProguardFile('proguard-android.txt') + proguardFiles getDefaultProguardFile('proguard-android-optimize.txt')
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/README.md(3 hunks)app/android/app/build.gradle(4 hunks)app/android/app/src/main/AndroidManifest.xml(1 hunks)app/android/build.gradle(2 hunks)app/android/react-native-passport-reader/android/build.gradle(1 hunks)app/docs/MOBILE_DEPLOYMENT.md(2 hunks)packages/mobile-sdk-alpha/android/build.gradle(1 hunks)packages/mobile-sdk-alpha/demo-app/android/build.gradle(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/README.md
- packages/mobile-sdk-alpha/android/build.gradle
🚧 Files skipped from review as they are similar to previous changes (3)
- app/android/app/src/main/AndroidManifest.xml
- app/android/build.gradle
- app/android/app/build.gradle
🧰 Additional context used
📓 Path-based instructions (1)
app/android/**/*
⚙️ CodeRabbit configuration file
app/android/**/*: Review Android-specific code for:
- Platform-specific implementations
- Performance considerations
- Security best practices for mobile
Files:
app/android/react-native-passport-reader/android/build.gradle
🧠 Learnings (5)
📚 Learning: 2025-08-24T18:52:25.796Z
Learnt from: CR
PR: selfxyz/self#0
File: .cursorrules:0-0
Timestamp: 2025-08-24T18:52:25.796Z
Learning: Applies to native/android/**/*.{kt,kts} : Android NFC: implement RNPassportReaderModule in Kotlin
Applied to files:
app/android/react-native-passport-reader/android/build.gradle
📚 Learning: 2025-06-30T15:51:11.193Z
Learnt from: transphorm
PR: selfxyz/self#636
File: app/android/app/build.gradle:76-76
Timestamp: 2025-06-30T15:51:11.193Z
Learning: For React Native 0.80, the correct JSC version is `io.github.react-native-community:jsc-android:2026004.+`. This version was specifically built with Android NDK r23 to align with React Native 0.80's NDK requirements and prevent compatibility issues. The `2026004` version pattern is the official version for RN 0.80, not an outdated pin.
Applied to files:
app/android/react-native-passport-reader/android/build.gradle
📚 Learning: 2025-08-24T18:54:04.809Z
Learnt from: CR
PR: selfxyz/self#0
File: .cursor/rules/mobile-sdk-migration.mdc:0-0
Timestamp: 2025-08-24T18:54:04.809Z
Learning: Update the app to consume the mobile-sdk-alpha package and replace local modules with SDK imports
Applied to files:
packages/mobile-sdk-alpha/demo-app/android/build.gradle
📚 Learning: 2025-08-29T15:30:12.210Z
Learnt from: CR
PR: selfxyz/self#0
File: app/AGENTS.md:0-0
Timestamp: 2025-08-29T15:30:12.210Z
Learning: Document complex native module changes for AI review
Applied to files:
app/docs/MOBILE_DEPLOYMENT.md
📚 Learning: 2025-08-24T18:54:04.809Z
Learnt from: CR
PR: selfxyz/self#0
File: .cursor/rules/mobile-sdk-migration.mdc:0-0
Timestamp: 2025-08-24T18:54:04.809Z
Learning: Applies to packages/mobile-sdk-alpha/README.md : Document new/updated SDK modules and usage in packages/mobile-sdk-alpha/README.md
Applied to files:
app/docs/MOBILE_DEPLOYMENT.md
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test
- GitHub Check: build-android
- GitHub Check: build-ios
- GitHub Check: analyze-ios
- GitHub Check: analyze-android
- GitHub Check: e2e-ios
- GitHub Check: android-build-test
🔇 Additional comments (4)
app/docs/MOBILE_DEPLOYMENT.md (1)
217-218: Doc cross‑reference: confirm app/README.md matches this NDK pin.Avoid drift between docs; mismatches lead to cache misses and flaky builds.
app/android/react-native-passport-reader/android/build.gradle (2)
9-15: Library minSdk set to 21: verify compatibility under app’s minSdk and transitive deps.If the app targets a higher minSdk (e.g., 24), keep it there; libraries can be lower but must avoid calling APIs gated above 21 inside this module.
7-7: NDK pin consistent — 27.0.12077973.
All Android modules either set ndkVersion = "27.0.12077973" or reference rootProject.ext.ndkVersion; no mismatch found. Files: ./app/android/build.gradle, ./app/android/app/build.gradle, ./app/android/react-native-passport-reader/android/build.gradle, ./packages/mobile-sdk-alpha/android/build.gradle, ./packages/mobile-sdk-alpha/demo-app/android/build.gradle.packages/mobile-sdk-alpha/demo-app/android/build.gradle (1)
7-9: Align Android SDK/toolchain on CI or fall back to API 35NDK pin (27.0.12077973) is fine; verification failed because sdkmanager is not available in the verification environment, so I could not confirm Android 36 / Build Tools 36.0.0 on your CI.
- If CI image lacks Android 36, use compileSdkVersion = 35 and targetSdkVersion = 35 and remove explicit buildToolsVersion (AGP 8+ manages build tools). File: packages/mobile-sdk-alpha/demo-app/android/build.gradle (lines ~7–9).
- Verify on CI/local: sdkmanager --list | rg 'platforms;android-35|platforms;android-36|build-tools;36' or install with sdkmanager 'platforms;android-36' 'build-tools;36.0.0'.
Proposed diff (if 36 is unavailable):
- buildToolsVersion = "36.0.0" - compileSdkVersion = 36 - targetSdkVersion = 36 + // AGP manages build tools; keep SDKs on a stable, available API + compileSdkVersion = 35 + targetSdkVersion = 35
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/mobile-ci.yml (1)
195-199: Gate secret-dependent mobile builds to protect tokens and keep fork PRs greenBuild steps/jobs requiring SELFXYZ_INTERNAL_REPO_PAT will fail on forked PRs (secrets not exposed) and risk accidental exposure if triggers change. Gate the jobs.
Apply this diff to skip full iOS/Android builds on forks:
build-ios: + if: ${{ !github.event.pull_request.head.repo.fork }} runs-on: macos-latest-large @@ build-android: + if: ${{ !github.event.pull_request.head.repo.fork }} runs-on: ubuntu-latestIf you prefer step-level gating, additionally guard these steps:
- - name: Install iOS Dependencies + - name: Install iOS Dependencies + if: ${{ !github.event.pull_request.head.repo.fork && secrets.SELFXYZ_INTERNAL_REPO_PAT != '' }} @@ - - name: Clone android-passport-reader + - name: Clone android-passport-reader + if: ${{ !github.event.pull_request.head.repo.fork && secrets.SELFXYZ_INTERNAL_REPO_PAT != '' }}Also applies to: 384-388, 314-316, 452-456
🧹 Nitpick comments (3)
.github/workflows/mobile-ci.yml (3)
272-289: Avoid direct actions/cache; wrap in repo composite actions for consistencyRepo guideline: prefer ./.github/actions/* cache composites over actions/cache. Please create wrappers (e.g., cache-xcode-build, cache-xcode-index, cache-ndk) mirroring your existing cache-* actions and switch these three sites to use them. This centralizes keys and eviction policy.
Also applies to: 433-437
38-39: Use Ubuntu for non-iOS jobs to cut cost and queue timeThese jobs only run Node/Yarn and don’t require macOS. Move to ubuntu-latest.
Apply:
- runs-on: macos-latest-large + runs-on: ubuntu-latestRepeat for the test job.
Also applies to: 94-96
7-7: Gate NDK install on cache miss and include GH_CACHE_VERSION in cache keyPrevents redundant ndk downloads and provides global cache-invalidation; confirmed all modules pin NDK 27.0.12077973.
- - name: Cache NDK - uses: actions/cache@v4 - with: - path: ${{ env.ANDROID_HOME }}/ndk/${{ env.ANDROID_NDK_VERSION }} - key: ${{ runner.os }}-ndk-${{ env.ANDROID_NDK_VERSION }} + - name: Cache NDK + id: ndk-cache + uses: actions/cache@v4 + with: + path: ${{ env.ANDROID_HOME }}/ndk/${{ env.ANDROID_NDK_VERSION }} + key: ${{ runner.os }}-ndk-${{ env.GH_CACHE_VERSION }}-${{ env.ANDROID_NDK_VERSION }} - - name: Install NDK - run: sdkmanager "ndk;${{ env.ANDROID_NDK_VERSION }}" + - name: Install NDK (cache miss) + if: steps.ndk-cache.outputs.cache-hit != 'true' + run: sdkmanager "ndk;${{ env.ANDROID_NDK_VERSION }}"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/mobile-ci.yml(1 hunks).github/workflows/mobile-deploy.yml(1 hunks).github/workflows/mobile-e2e.yml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/mobile-deploy.yml
- .github/workflows/mobile-e2e.yml
🧰 Additional context used
📓 Path-based instructions (1)
.github/workflows/**/*.{yml,yaml}
📄 CodeRabbit inference engine (AGENTS.md)
.github/workflows/**/*.{yml,yaml}: In GitHub Actions workflows, use shared composite caching actions from .github/actions (cache-yarn, cache-bundler, cache-gradle, cache-pods)
Do not call actions/cache directly; rely on the shared composite caching actions
When using cache actions, optionally pass cache-version (often with GH_CACHE_VERSION and tool version) for stable keys
Files:
.github/workflows/mobile-ci.yml
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build-ios
- GitHub Check: test
- GitHub Check: build-android
- GitHub Check: android-build-test
- GitHub Check: e2e-ios
- GitHub Check: analyze-android
- GitHub Check: analyze-ios
Summary by CodeRabbit
New Features
Performance/Optimization
Compatibility
Chores