Feat: Lightweight e2e tests for iOS and Android #840
Conversation
WalkthroughThis update introduces robust, automated end-to-end (E2E) testing infrastructure for both Android and iOS platforms, leveraging Maestro flows and CI integration. It adds new test scripts, configuration files, and workflow automation, including conditional build logic and platform-specific test stubs. Numerous code style and quoting consistency improvements are also made across workflow, configuration, and documentation files. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CI as GitHub Actions
participant Android as Android Emulator
participant iOS as iOS Simulator
participant Maestro as Maestro CLI
Dev->>CI: Push/PR triggers workflow
CI->>Android: (If enabled) Build & install app (Release/Debug)
CI->>iOS: Build & install app (Release/Debug)
CI->>Maestro: Run E2E flow (Android/iOS)
Maestro->>Android: Launch app, assert UI elements
Maestro->>iOS: Launch app, assert UI elements
Maestro-->>CI: Return test results
CI->>Dev: Upload artifacts, report outcome
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (32)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
transphorm
changed the title
![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/README.md (1)
344-362: Nit: clarify working directory in the Maestro sectionThe commands assume you are already in
self/app. Readers skimming directly to this section may miss that earlier note. A quick reminder avoids confusion:-```bash -yarn test:e2e:android # Android -yarn test:e2e:ios # iOS -``` +```bash +# from self/app +yarn test:e2e:android # Android +yarn test:e2e:ios # iOS +```
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/mobile-e2e.yml(1 hunks)app/README.md(1 hunks)app/e2e/launch.flow.yaml(1 hunks)app/package.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: when reviewing autogenerated scripts in xcode project files (like react native firebase's embedded s...
Learnt from: transphorm
PR: selfxyz/self#823
File: app/ios/Self.xcodeproj/project.pbxproj:320-332
Timestamp: 2025-08-02T23:53:45.928Z
Learning: When reviewing autogenerated scripts in Xcode project files (like React Native Firebase's embedded shell scripts), avoid suggesting edits since these are regenerated during pod install and cannot be manually modified by users.
Applied to files:
app/package.json
⏰ 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: android
- GitHub Check: analyze-ios
- GitHub Check: analyze-android
- GitHub Check: build
- GitHub Check: android
🔇 Additional comments (1)
.github/workflows/mobile-e2e.yml (1)
27-35: Workflow still points to old path after fixWhen you patch the package.json scripts, the CI job will also need to drop the
app/prefix in its explicitmaestro testcall (or just rely on the script). Otherwise the same path error persists.- yarn test:e2e:android + yarn test:e2e:android(If you leave the script to handle the call, nothing else is required; just make sure no hard-coded
app/e2eremains.)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
.github/workflows/contracts-ci.yml (2)
36-40: Turn the “TODO: Use caching” into an action-cache stepCold-starting Yarn (and the Rust/Cargo crates that get pulled indirectly by build_cpp.sh) adds several minutes to each run.
A minimal improvement:- # TODO(): Use caching + - name: Cache Yarn + uses: actions/cache@v4 + with: + path: | + ~/.cache/yarn + **/.yarn/cache + key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }} + restore-keys: | + ${{ runner.os }}-yarn-You can add a second cache for
$HOME/.cargo/registry+$HOME/.cargo/gitto speed-up Rust compilation.
60-66: Consider a build matrix instead of a serial shell chainRunning four circuit builds serially (
register,register_id,disclose,dsc) leaves CPU cores idle on a 128 GB runner.
Using amatrixstrategy can cut wall-clock time by ≈4× and gives per-circuit visibility..github/workflows/mobile-deploy-auto.yml (1)
33-33: Residual trailing spaces
yaml-lintstill reports trailing whitespace on Line 33. Clean it up to keep the workflow lint-clean.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.coderabbit.yaml(2 hunks).gitguardian.yml(1 hunks).github/actions/get-version/action.yml(1 hunks).github/actions/mobile-setup/action.yml(3 hunks).github/actions/push-changes/action.yml(1 hunks).github/actions/yarn-install/action.yml(1 hunks).github/workflows/circuits-build.yml(1 hunks).github/workflows/circuits.yml(2 hunks).github/workflows/common-ci.yml(1 hunks).github/workflows/contracts-ci.yml(1 hunks).github/workflows/contracts.yml(1 hunks).github/workflows/mobile-bundle-analysis.yml(2 hunks).github/workflows/mobile-ci.yml(3 hunks).github/workflows/mobile-deploy-auto.yml(4 hunks).github/workflows/mobile-deploy.yml(9 hunks).github/workflows/mobile-e2e.yml(1 hunks).github/workflows/npm-publish.yml(1 hunks).github/workflows/web.yml(1 hunks)AGENTS.md(3 hunks)README.md(3 hunks)app/e2e/launch.flow.yaml(1 hunks)app/package.json(1 hunks)package.json(1 hunks)
✅ Files skipped from review due to trivial changes (18)
- .gitguardian.yml
- .github/workflows/common-ci.yml
- .github/workflows/contracts.yml
- .github/actions/get-version/action.yml
- .github/workflows/npm-publish.yml
- .github/workflows/mobile-ci.yml
- .github/workflows/circuits-build.yml
- .github/workflows/mobile-bundle-analysis.yml
- README.md
- .github/actions/yarn-install/action.yml
- .github/actions/mobile-setup/action.yml
- .github/actions/push-changes/action.yml
- .github/workflows/web.yml
- AGENTS.md
- .github/workflows/circuits.yml
- .coderabbit.yaml
- .github/workflows/mobile-deploy.yml
- package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- app/e2e/launch.flow.yaml
- app/package.json
- .github/workflows/mobile-e2e.yml
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/contracts-ci.yml
19-19: label "selfxyz-org" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
19-19: label "ubuntu-22-04" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
19-19: label "128ram" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
.github/workflows/mobile-deploy-auto.yml
29-29: "github.event.pull_request.title" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions for more details
(expression)
🪛 YAMLlint (1.37.1)
.github/workflows/mobile-deploy-auto.yml
[error] 33-33: trailing spaces
(trailing-spaces)
⏰ 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). (2)
- GitHub Check: e2e (android, ubuntu-latest)
- GitHub Check: e2e (ios, macos-latest)
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
.github/workflows/circuits-build.yml (1)
50-56: Binary download lacks integrity verification – add a checksum gateFetching Circom directly with
wgetand executing it without validating a hash opens the door to supply-chain attacks. Pin the exact SHA-256 digest and fail the job on mismatch.- wget -qO /home/runner/work/circom https://github.com/iden3/circom/releases/download/v2.1.9/circom-linux-amd64 - chmod +x /home/runner/work/circom + CIRCOM_URL="https://github.com/iden3/circom/releases/download/v2.1.9/circom-linux-amd64" + EXPECTED_SHA="b0c3c4c9af5a6e4e8d2f1d5d34c8e8d8f3f4e6b7a1c2d3e4f5a6b7c8d9e0f1a2" # ← replace with real digest + wget -qO /home/runner/work/circom "$CIRCOM_URL" + echo "$EXPECTED_SHA /home/runner/work/circom" | sha256sum -c - + chmod +x /home/runner/work/circomIf the checksum fails,
sha256sumexits non-zero and the workflow stops, keeping your build chain trustworthy.
♻️ Duplicate comments (1)
.github/workflows/mobile-deploy-auto.yml (1)
32-38: Still vulnerable to command-injection via PR title/bodyThe step keeps evaluating
github.event.pull_request.titleand.bodydirectly inside a Bash conditional. Branch maintainers (or malicious contributors on forks) can craft titles/bodies containing$(...)or back-ticks to execute arbitrary commands in the runner.This exact issue was raised in a previous review but hasn’t been addressed.
- - name: Check deployment conditions - id: check - run: | - echo "🔍 Checking deployment conditions..." - - # Skip if PR has skip-deploy in title or body - if [[ "${{ github.event.pull_request.title }}" =~ \[skip-deploy\] ]] || - [[ "${{ github.event.pull_request.body }}" =~ \[skip-deploy\] ]]; then + - name: Check deployment conditions + id: check + env: + PR_TITLE: ${{ github.event.pull_request.title }} + PR_BODY: ${{ github.event.pull_request.body }} + run: | + echo "🔍 Checking deployment conditions..." + + # Skip if PR has [skip-deploy] in title or body + if [[ "$PR_TITLE" == *"[skip-deploy]"* ]] || [[ "$PR_BODY" == *"[skip-deploy]"* ]]; then echo "should_deploy=false" >> $GITHUB_OUTPUT echo "⏭️ Skipping deployment due to [skip-deploy] flag" exit 0 fiThis shields the shell from evaluating untrusted input while preserving the existing logic.
🧹 Nitpick comments (2)
.github/workflows/mobile-deploy-auto.yml (1)
33-33: Trailing whitespace flagged by YAMLlintThere’s an extra space at the end of this line. It won’t break the workflow, but cleaning it up silences the linter.
- if [[ "${{ github.event.pull_request.title }}" =~ \[skip-deploy\] ]] ||␠ + if [[ "${{ github.event.pull_request.title }}" =~ \[skip-deploy\] ]] ||.github/workflows/circuits-build.yml (1)
36-46: Leverage caching to cut CI minutes & network trafficThere’s a
# TODO(): Use cachingplaceholder. Yarn installs (~node_modules) and Rust crates are both cache-friendly and dramatically speed repeat runs.- name: Cache Yarn uses: actions/cache@v4 with: path: | ~/.yarn/cache **/node_modules key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }} restore-keys: | ${{ runner.os }}-yarn- - name: Cache Cargo uses: actions/cache@v4 with: path: ~/.cargo/registry key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }} restore-keys: | ${{ runner.os }}-cargo-Drop these steps just before the install/build phases to avoid pulling the world on every push.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.github/actions/get-version/action.yml(1 hunks).github/actions/mobile-setup/action.yml(2 hunks).github/actions/yarn-install/action.yml(1 hunks).github/workflows/circuits-build.yml(2 hunks).github/workflows/circuits.yml(1 hunks).github/workflows/contracts.yml(1 hunks).github/workflows/mobile-bundle-analysis.yml(1 hunks).github/workflows/mobile-ci.yml(1 hunks).github/workflows/mobile-deploy-auto.yml(5 hunks).github/workflows/mobile-deploy.yml(2 hunks).github/workflows/mobile-e2e.yml(1 hunks).github/workflows/npm-publish.yml(4 hunks)app/.codecov.yml(2 hunks)app/.github/workflows/test-coverage.yml(2 hunks)app/.prettierrc(1 hunks)app/.yarnrc.yml(1 hunks)app/e2e/launch.flow.yaml(1 hunks)app/package.json(1 hunks)package.json(1 hunks)
✅ Files skipped from review due to trivial changes (8)
- .github/actions/get-version/action.yml
- .github/workflows/mobile-bundle-analysis.yml
- app/.yarnrc.yml
- app/.github/workflows/test-coverage.yml
- app/.prettierrc
- .github/workflows/npm-publish.yml
- app/.codecov.yml
- .github/workflows/mobile-deploy.yml
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/mobile-ci.yml
- .github/actions/mobile-setup/action.yml
- .github/workflows/circuits.yml
- .github/actions/yarn-install/action.yml
- .github/workflows/contracts.yml
- app/e2e/launch.flow.yaml
- package.json
- app/package.json
- .github/workflows/mobile-e2e.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: when reviewing autogenerated scripts in xcode project files (like react native firebase's embedded s...
Learnt from: transphorm
PR: selfxyz/self#823
File: app/ios/Self.xcodeproj/project.pbxproj:320-332
Timestamp: 2025-08-02T23:53:45.928Z
Learning: When reviewing autogenerated scripts in Xcode project files (like React Native Firebase's embedded shell scripts), avoid suggesting edits since these are regenerated during pod install and cannot be manually modified by users.
Applied to files:
.github/workflows/mobile-deploy-auto.yml
🪛 actionlint (1.7.7)
.github/workflows/circuits-build.yml
19-19: label "selfxyz-org" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
19-19: label "ubuntu-22-04" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
19-19: label "128ram" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
.github/workflows/mobile-deploy-auto.yml
29-29: "github.event.pull_request.title" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions for more details
(expression)
🪛 YAMLlint (1.37.1)
.github/workflows/mobile-deploy-auto.yml
[error] 33-33: trailing spaces
(trailing-spaces)
⏰ 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). (2)
- GitHub Check: e2e (ios, macos-latest)
- GitHub Check: e2e (android, ubuntu-latest)
🔇 Additional comments (1)
.github/workflows/mobile-deploy-auto.yml (1)
8-10: Consistent quoting looks goodSwitching the
pathsglob patterns to double-quoted strings aligns with the new Prettier/YAML rules adopted across the repo. No functional impact—just better consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/tests/e2e/launch.ios.flow.yaml (2)
1-1: Parameterise theappIdrather than hard-coding itHard-coding the bundle identifier ties this flow to a single build flavour and makes parallel testing of dev / staging / prod variants awkward. Use an environment variable or Maestro’s
${APP_ID}placeholder and inject the value from yourmaestro.cloud/ CI job instead.
4-5: Assert by accessibility id / testID instead of UI textText-based assertions break as soon as copy changes or i18n lands. Prefer stable identifiers exported from the React Native component (
testIDon iOS).- - assertVisible: - text: "I have a Passport or Biometric ID" + - assertVisible: + id: passportPrompt # assumes `testID="passportPrompt"` in the JSX
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/tests/e2e/launch.android.flow.yaml(1 hunks)app/tests/e2e/launch.ios.flow.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/tests/e2e/launch.android.flow.yaml
⏰ 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). (4)
- GitHub Check: e2e-ios
- GitHub Check: analyze-android
- GitHub Check: analyze-ios
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/mobile-e2e.yml (1)
61-65: Still executing remote code via unpinnedcurl | bash– supply-chain red flagBoth install steps fetch and execute the latest Maestro installer on every run.
That code path is unauthenticated, mutable and opaque – exactly the stuff SBOM & SLSA audits scream about. We already flagged this in earlier rounds; nothing changed.Pin a specific binary + verify its SHA-256 (or at minimum use GitHub-hosted release assets) so builds are deterministic and the blast-radius is limited.
Also applies to: 136-140
app/scripts/test-e2e-local.sh (1)
68-70: Unverifiedcurl | bash– repeat of earlier security findingSame concern as in the CI workflow: piping arbitrary remote code straight into the shell is a non-starter for reproducible and secure builds.
Adopt the pinned-binary + checksum pattern (or at least prompt the user before execution in local environments).
🧹 Nitpick comments (1)
app/scripts/test-e2e-local.sh (1)
5-6: Harden script preambleConsider the usual defensive trio to fail fast on unset vars and pipe errors.
-set -e +set -euo pipefailIt prevents a surprising amount of foot-guns (typos in var names, silent pipe failures, etc.).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/mobile-e2e.yml(1 hunks)app/scripts/test-e2e-local.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: when reviewing autogenerated scripts in xcode project files (like react native firebase's embedded s...
Learnt from: transphorm
PR: selfxyz/self#823
File: app/ios/Self.xcodeproj/project.pbxproj:320-332
Timestamp: 2025-08-02T23:53:45.928Z
Learning: When reviewing autogenerated scripts in Xcode project files (like React Native Firebase's embedded shell scripts), avoid suggesting edits since these are regenerated during pod install and cannot be manually modified by users.
Applied to files:
.github/workflows/mobile-e2e.ymlapp/scripts/test-e2e-local.sh
📚 Learning: for this react native project, the team prefers build flexibility over fail-fast behavior for releas...
Learnt from: transphorm
PR: selfxyz/self#795
File: app/android/app/build.gradle:157-158
Timestamp: 2025-07-29T01:08:28.530Z
Learning: For this React Native project, the team prefers build flexibility over fail-fast behavior for release builds in app/android/app/build.gradle. They intentionally allow fallback to debug signing for local development runs, relying on Google Play Console validation to catch any improperly signed releases during upload.
Applied to files:
app/scripts/test-e2e-local.sh
🪛 actionlint (1.7.7)
.github/workflows/mobile-e2e.yml
158-158: shellcheck reported issue in this script: SC2181:style:3:6: Check exit code directly with e.g. 'if ! mycmd;', not indirectly with $?
(shellcheck)
158-158: shellcheck reported issue in this script: SC2181:style:59:6: Check exit code directly with e.g. 'if ! mycmd;', not indirectly with $?
(shellcheck)
🪛 Shellcheck (0.10.0)
app/scripts/test-e2e-local.sh
[warning] 412-412: i appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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). (4)
- GitHub Check: e2e-ios
- GitHub Check: analyze-ios
- GitHub Check: analyze-android
- GitHub Check: build
🔇 Additional comments (1)
.github/workflows/mobile-e2e.yml (1)
105-110: Good catch onif-no-files-found– artefact upload now resilientThe step now uses the correct
if-no-files-foundinput so a missingmaestro-results.xmlno longer nukes the job.Also applies to: 248-250
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/scripts/test-e2e-local.sh (1)
68-71: Unverified Maestro install (curl | bash) is still present
Same supply-chain concern flagged earlier – please replace with a pinned, checksum-verified binary as suggested in previous reviews.
🧹 Nitpick comments (1)
.github/workflows/mobile-e2e.yml (1)
158-166: Simplify exit-code handling – avoid the$?anti-patternYou’re already in a
set -eshell. Checkingif [ $? -ne 0 ]is unnecessary and flagged by ShellCheck SC2181. Negate the command directly:- yarn workspace @selfxyz/mobile-app run build:deps --silent; - if [ $? -ne 0 ]; then - echo "❌ Dependency build failed" - exit 1 - fi + if ! yarn workspace @selfxyz/mobile-app run build:deps --silent; then + echo "❌ Dependency build failed" + exit 1 + fi
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/mobile-e2e.yml(1 hunks)app/scripts/test-e2e-local.sh(1 hunks)app/tests/e2e/launch.android.flow.yaml(1 hunks)app/tests/e2e/launch.ios.flow.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/tests/e2e/launch.android.flow.yaml
- app/tests/e2e/launch.ios.flow.yaml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: when reviewing autogenerated scripts in xcode project files (like react native firebase's embedded s...
Learnt from: transphorm
PR: selfxyz/self#823
File: app/ios/Self.xcodeproj/project.pbxproj:320-332
Timestamp: 2025-08-02T23:53:45.928Z
Learning: When reviewing autogenerated scripts in Xcode project files (like React Native Firebase's embedded shell scripts), avoid suggesting edits since these are regenerated during pod install and cannot be manually modified by users.
Applied to files:
.github/workflows/mobile-e2e.ymlapp/scripts/test-e2e-local.sh
📚 Learning: for this react native project, the team prefers build flexibility over fail-fast behavior for releas...
Learnt from: transphorm
PR: selfxyz/self#795
File: app/android/app/build.gradle:157-158
Timestamp: 2025-07-29T01:08:28.530Z
Learning: For this React Native project, the team prefers build flexibility over fail-fast behavior for release builds in app/android/app/build.gradle. They intentionally allow fallback to debug signing for local development runs, relying on Google Play Console validation to catch any improperly signed releases during upload.
Applied to files:
app/scripts/test-e2e-local.sh
🪛 actionlint (1.7.7)
.github/workflows/mobile-e2e.yml
158-158: shellcheck reported issue in this script: SC2181:style:3:6: Check exit code directly with e.g. 'if ! mycmd;', not indirectly with $?
(shellcheck)
158-158: shellcheck reported issue in this script: SC2181:style:59:6: Check exit code directly with e.g. 'if ! mycmd;', not indirectly with $?
(shellcheck)
🪛 Shellcheck (0.10.0)
app/scripts/test-e2e-local.sh
[warning] 412-412: i appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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). (3)
- GitHub Check: analyze-ios
- GitHub Check: analyze-android
- GitHub Check: e2e-ios
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
.github/workflows/mobile-e2e.yml (2)
61-63: Avoidcurl | bash– pin & checksum-verify Maestro binary
Same concern as raised in earlier review: piping arbitrary remote code straight into the shell is a supply-chain risk and breaks reproducibility. Please replace with a pinned binary download + checksum verification (see previous diff suggestion).
181-183: Repeat of insecure installer pattern for iOS job
The iOS section still uses the unpinnedcurl | bashinstaller. Re-use the hardened installation snippet proposed previously.app/scripts/test-e2e-local.sh (2)
69-71: Unpinned Maestro install repeats earlier security concern
The local runner still fetches and executes an arbitrary script every time. Adopt the pinned-binary + checksum approach previously outlined.
110-112: Surface dependency-build failures explicitly
Prior feedback suggested guardingyarn build:depsso a failure is logged before the script exits. Current implementation relies solely onset -e, giving no context.- yarn build:deps + if ! yarn build:deps; then + log_error "Dependency build failed" + exit 1 + fi
🧹 Nitpick comments (3)
.github/workflows/mobile-e2e.yml (1)
263-267: Use direct negation instead of$?check
Indirection via$?is brittle; prefer the standardif ! cmd; then …form which also survivesset -e.- xcrun simctl install "iPhone 15" "$APP_PATH" - if [ $? -ne 0 ]; then + if ! xcrun simctl install "iPhone 15" "$APP_PATH"; then echo "❌ iOS app installation failed" exit 1 fiapp/scripts/test-e2e-local.sh (2)
5-6: Harden script flags for safer Bash execution
set -eaborts on plain command failures but still lets unset variables or pipe errors slip through. Consider the stricter trio below for more predictable behaviour.-set -e +set -euo pipefail
557-558: Potentially empty$FIRST_AVDin cleanup filter
cleanup_android_emulatorunconditionally runspkill -f "emulator.*$FIRST_AVD", butFIRST_AVDis only set when the script booted an emulator itself. If none was started, the pattern degenerates toemulator.*and may kill unrelated processes.Guard with a variable check:
- pkill -f "emulator.*$FIRST_AVD" >/dev/null 2>&1 || true + if [ -n "$FIRST_AVD" ]; then + pkill -f "emulator.*$FIRST_AVD" >/dev/null 2>&1 || true + fi
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/mobile-e2e.yml(1 hunks)app/scripts/test-e2e-local.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: when reviewing autogenerated scripts in xcode project files (like react native firebase's embedded s...
Learnt from: transphorm
PR: selfxyz/self#823
File: app/ios/Self.xcodeproj/project.pbxproj:320-332
Timestamp: 2025-08-02T23:53:45.928Z
Learning: When reviewing autogenerated scripts in Xcode project files (like React Native Firebase's embedded shell scripts), avoid suggesting edits since these are regenerated during pod install and cannot be manually modified by users.
Applied to files:
.github/workflows/mobile-e2e.ymlapp/scripts/test-e2e-local.sh
📚 Learning: for this react native project, the team prefers build flexibility over fail-fast behavior for releas...
Learnt from: transphorm
PR: selfxyz/self#795
File: app/android/app/build.gradle:157-158
Timestamp: 2025-07-29T01:08:28.530Z
Learning: For this React Native project, the team prefers build flexibility over fail-fast behavior for release builds in app/android/app/build.gradle. They intentionally allow fallback to debug signing for local development runs, relying on Google Play Console validation to catch any improperly signed releases during upload.
Applied to files:
.github/workflows/mobile-e2e.ymlapp/scripts/test-e2e-local.sh
🪛 actionlint (1.7.7)
.github/workflows/mobile-e2e.yml
219-219: shellcheck reported issue in this script: SC2181:style:46:6: Check exit code directly with e.g. 'if ! mycmd;', not indirectly with $?
(shellcheck)
🪛 Shellcheck (0.10.0)
app/scripts/test-e2e-local.sh
[warning] 412-412: i appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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). (4)
- GitHub Check: e2e-android
- GitHub Check: analyze-ios
- GitHub Check: analyze-android
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
.github/workflows/mobile-e2e.yml (1)
61-63: Still using un-pinnedcurl | bashMaestro installerEarlier reviews already flagged the supply-chain risk. Pin & checksum-verify a specific Maestro binary instead of executing remote code each run.
app/scripts/test-e2e-local.sh (2)
69-71: Un-pinnedcurl | bashinstaller – same supply-chain concernReuse the pinned / checksum-verified installation logic recommended for the CI workflow.
108-112:yarn build:depserrors are swallowedIf the build fails the script ploughs on and later steps error ambiguously. Guard the call:
- yarn build:deps + if ! yarn build:deps; then + log_error "Dependency build failed" + exit 1 + fi
🧹 Nitpick comments (1)
.github/workflows/mobile-e2e.yml (1)
241-244: Minor style: use direct command check instead of$?Shellcheck SC2181: prefer
if ! xcrun simctl install …; then– shorter and avoids stale exit-code bugs.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/mobile-e2e.yml(1 hunks)app/scripts/test-e2e-local.sh(1 hunks)app/tests/e2e/launch.android.flow.yaml(1 hunks)app/tests/e2e/launch.ios.flow.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/tests/e2e/launch.ios.flow.yaml
- app/tests/e2e/launch.android.flow.yaml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: when reviewing autogenerated scripts in xcode project files (like react native firebase's embedded s...
Learnt from: transphorm
PR: selfxyz/self#823
File: app/ios/Self.xcodeproj/project.pbxproj:320-332
Timestamp: 2025-08-02T23:53:45.928Z
Learning: When reviewing autogenerated scripts in Xcode project files (like React Native Firebase's embedded shell scripts), avoid suggesting edits since these are regenerated during pod install and cannot be manually modified by users.
Applied to files:
app/scripts/test-e2e-local.sh.github/workflows/mobile-e2e.yml
📚 Learning: for this react native project, the team prefers build flexibility over fail-fast behavior for releas...
Learnt from: transphorm
PR: selfxyz/self#795
File: app/android/app/build.gradle:157-158
Timestamp: 2025-07-29T01:08:28.530Z
Learning: For this React Native project, the team prefers build flexibility over fail-fast behavior for release builds in app/android/app/build.gradle. They intentionally allow fallback to debug signing for local development runs, relying on Google Play Console validation to catch any improperly signed releases during upload.
Applied to files:
app/scripts/test-e2e-local.sh.github/workflows/mobile-e2e.yml
🪛 Shellcheck (0.10.0)
app/scripts/test-e2e-local.sh
[warning] 412-412: i appears unused. Verify use (or export if used externally).
(SC2034)
🪛 actionlint (1.7.7)
.github/workflows/mobile-e2e.yml
225-225: shellcheck reported issue in this script: SC2181:style:16:6: Check exit code directly with e.g. 'if ! mycmd;', not indirectly with $?
(shellcheck)
⏰ 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: e2e-ios
- GitHub Check: e2e-android
- GitHub Check: analyze-android
- GitHub Check: analyze-ios
- GitHub Check: build
|
@coderabbitai full review |
|
@coderabbitai summary |
✅ Actions performedFull review triggered. |
✅ Actions performedSummary regeneration triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
.github/workflows/mobile-e2e.yml (2)
61-65: Reuse of remote ‘curl | bash’ installer – still an open supply-chain risk
Previous reviews already asked to pin & checksum-verify Maestro instead of executing a remote script on every run. Nothing has changed here, so the same concern remains.
180-183: Same unsecured Maestro install in the iOS job
Identical issue as above – the iOS lane still pipes an un-pinned script into bash.app/scripts/test-e2e-local.sh (1)
68-71: Unpinned Maestro install – repeat of prior feedback
The local script still uses a rawcurl … | bashinstaller. Pin the binary & verify its checksum (or at least warn the user) to avoid silent supply-chain compromise.
🧹 Nitpick comments (3)
.github/workflows/mobile-e2e.yml (1)
85-92: Un-gatedsdkmanager "ndk;${ANDROID_NDK_VERSION}"downloads on every run
If the NDK was restored from cache, this step needlessly re-downloads 1 GB+ every time, slowing the job and hammering Google’s mirrors. Wrap the install step with an existence check or move it behind the cache condition to skip when${ANDROID_HOME}/ndk/${ANDROID_NDK_VERSION}is already present.app/scripts/test-e2e-local.sh (2)
471-485: Version sort for build-tools paths is lexical, not numeric
sort -rwill rank30.0.0before9.0.0but10.0.0after2.0.0. Use a version sort to reliably pick the newest toolchain:- AAPT_PATH=$(find "$ANDROID_HOME/build-tools" -type f -name "aapt" | sort -r | head -n 1) + AAPT_PATH=$(find "$ANDROID_HOME/build-tools" -type f -name "aapt" | sort -Vr | head -n 1)Apply the same change for the
aapt2lookup.
381-389: Minor: unused loop variable triggers shellcheck SC2034
Thefor i in {1..60}; do … doneprogress loop never references$i. Replace withfor _ in {1..60}or simply awhileloop to silence static-analysis noise.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/mobile-e2e.yml(1 hunks)app/scripts/test-e2e-local.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: when reviewing autogenerated scripts in xcode project files (like react native firebase's embedded s...
Learnt from: transphorm
PR: selfxyz/self#823
File: app/ios/Self.xcodeproj/project.pbxproj:320-332
Timestamp: 2025-08-02T23:53:45.928Z
Learning: When reviewing autogenerated scripts in Xcode project files (like React Native Firebase's embedded shell scripts), avoid suggesting edits since these are regenerated during pod install and cannot be manually modified by users.
Applied to files:
.github/workflows/mobile-e2e.ymlapp/scripts/test-e2e-local.sh
📚 Learning: for this react native project, the team prefers build flexibility over fail-fast behavior for releas...
Learnt from: transphorm
PR: selfxyz/self#795
File: app/android/app/build.gradle:157-158
Timestamp: 2025-07-29T01:08:28.530Z
Learning: For this React Native project, the team prefers build flexibility over fail-fast behavior for release builds in app/android/app/build.gradle. They intentionally allow fallback to debug signing for local development runs, relying on Google Play Console validation to catch any improperly signed releases during upload.
Applied to files:
.github/workflows/mobile-e2e.ymlapp/scripts/test-e2e-local.sh
🪛 Shellcheck (0.10.0)
app/scripts/test-e2e-local.sh
[warning] 415-415: i appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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). (4)
- GitHub Check: analyze-android
- GitHub Check: e2e-ios
- GitHub Check: analyze-ios
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (4)
app/.gitignore (1)
90-91: Broken ignore entry merges two patterns
*-sourcemap.jsonandroid/.kotlin/errors/concatenates two distinct paths, so neither is ignored. Add a newline between them.-*-sourcemap.jsonandroid/.kotlin/errors/ +*-sourcemap.json +android/.kotlin/errors/.github/workflows/circuits-build.yml (2)
36-36: Cache heavy tool-chain to cut minutes off CI
TODO(): Use cachinghas been here a while—cache~/.cargo, Yarn, and Circom build artefacts withactions/cacheto speed subsequent runs.
50-55: Unpinned binary download—supply-chain riskFetching Circom over plain HTTPS without checksum verification invites MITM / tampering.
Add a SHA-256 check or GitHub-release signature validation beforemv:-wget -qO /home/runner/work/circom https://github.com/iden3/circom/releases/download/v2.1.9/circom-linux-amd64 +curl -L -o /home/runner/work/circom https://github.com/iden3/circom/releases/download/v2.1.9/circom-linux-amd64 +echo "d41d8cd98f00b204e9800998ecf8427e /home/runner/work/circom" | sha256sum -c -.github/workflows/npm-publish.yml (1)
110-117: Duplicate checkout wastes one minute of CI timeThis job invokes
actions/checkout@v4twice back-to-back (Line 110 and again Line 116). The second invocation does nothing new, but it still costs ~30-60 s of runtime and extra API calls.110 - uses: actions/checkout@v4 111 - name: Set up Node.js ... 116 - uses: actions/checkout@v4 # second call – removeDelete the redundant step to speed up every publish run.
♻️ Duplicate comments (6)
.github/actions/get-version/action.yml (1)
16-16: Quote-injection risk still unresolved
Past review flagged the single-quotedrequire('${{ inputs.app_path }}/package.json')path. Any single quote inapp_pathwill break the script. Please switch to double quotes inside the Node snippet.-VERSION=$(node -p "require('${{ inputs.app_path }}/package.json').version") +VERSION=$(node -p "require(\"${{ inputs.app_path }}/package.json\").version").github/workflows/circuits-build.yml (1)
19-19: Unknown self-hosted runner labels flagged byactionlint
Same issue was raised earlier and is still unresolved.app/tests/e2e/launch.ios.flow.yaml (1)
3-6: Launch the app with a clean state to avoid flaky testsSame concern as previously flagged: residual keychain / SecureStore data can alter the first screen. Add
clearState: true(orfreshInstall: truefor older Maestro).- - launchApp + - launchApp: + clearState: true.github/workflows/mobile-deploy-auto.yml (1)
31-39: Untrusted PR title/body still interpolated in Bash – RCE vector
github.event.pull_request.title/.bodyare interpolated directly inside the shell script (Lines 32-35). A malicious contributor could smuggle$(curl evil)into the title and get code executed on the runner.
Pass them via environment variables and treat them as inert strings, as recommended earlier..github/workflows/mobile-e2e.yml (1)
61-64: Pin Maestro version for security and reproducibility.Installing Maestro via
curl | bashposes security risks and breaks reproducibility. Consider downloading a specific version and verifying its checksum:- name: Install Maestro if: steps.cache-maestro.outputs.cache-hit != 'true' - run: curl -Ls "https://get.maestro.mobile.dev" | bash + run: | + MAESTRO_URL="https://github.com/mobile-dev-inc/maestro/releases/download/v${{ env.MAESTRO_VERSION }}/maestro-linux-x86_64.zip" + curl -Lf "$MAESTRO_URL" -o maestro.zip + unzip maestro.zip -d "$HOME/.maestro" + chmod +x "$HOME/.maestro/bin/maestro"Apply the same change to the iOS job at lines 179-182.
Also applies to: 179-182
app/scripts/test-e2e-local.sh (1)
62-71: Unchanged insecure “curl | bash” Maestro install
The earlier review already highlighted the supply-chain & TOCTOU risk of piping remote code straight into the shell. The pattern remains.
🧹 Nitpick comments (9)
app/.gitignore (1)
95-97: Broaden ignore-pattern for Maestro artifacts
Maestro can output additional XML/JUnit files (e.g.maestro-results-android.xml, timestamped files, or JSON). Limiting the rule to a single exact filename risks accidental commits of similarly-named artifacts.-# Maestro -maestro-results.xml +# Maestro (all result artifacts) +maestro-results*.xml +maestro-results*.json.github/actions/yarn-install/action.yml (1)
21-26: Consider upgrading the CI Node version to active LTS 22
Line 24 pins to Node 20. According to previous team learnings,actions/setup-node@v4supports Node 22 across all runners. Moving sooner keeps us aligned with LTS security patches and avoids a double upgrade later.- node-version: 20 + node-version: 22app/ios/Podfile (1)
147-165: Consider refactoring to reduce nesting complexity.The E2E_TESTING compilation condition logic is functionally correct and necessary for conditional compilation during testing. However, the deep nesting could be improved for maintainability.
Consider extracting the Xcode project modification into a separate method:
def add_e2e_testing_flag_to_target(installer) self_project_path = File.join(installer.sandbox.project_path, "../Self.xcodeproj") return unless File.exist?(self_project_path) project = Xcodeproj::Project.open(self_project_path) project.targets.each do |target| next unless target.name == "Self" target.build_configurations.each do |config| existing_conditions = config.build_settings["SWIFT_ACTIVE_COMPILATION_CONDITIONS"] || "" unless existing_conditions.to_s.include?("E2E_TESTING") config.build_settings["SWIFT_ACTIVE_COMPILATION_CONDITIONS"] = (existing_conditions.to_s + " E2E_TESTING").strip end end end project.save end # In post_install block: if ENV["E2E_TESTING"] == "1" add_e2e_testing_flag_to_target(installer) endapp/tests/e2e/launch.android.flow.yaml (1)
4-6: Prefer accessibility IDs over visible textWaiting on raw text is brittle (breaks with i18n, copy edits). If possible, surface an
accessibilityLabel/testIDand usevisibleId:instead..github/workflows/mobile-bundle-analysis.yml (1)
18-21: Add concurrency group to avoid duplicate macOS runnersmacOS runners are scarce & costly.
Add:concurrency: group: mobile-bundle-${{ github.ref }} cancel-in-progress: trueto ensure stale runs are cancelled.
.github/workflows/mobile-deploy-auto.yml (1)
64-68: Shell ≠ JSON – platforms array must be JSON-escapedLine 66 echoes
echo 'platforms=["ios", "android"]' >> $GITHUB_OUTPUT.
Shell quotes are fine, but the brackets/quotes need escaping if anyone ever parses this as JSON. Prefer:echo 'platforms=["ios","android"]' >> "$GITHUB_OUTPUT"Also wrap
$GITHUB_OUTPUTin quotes to silence SC2086 warnings.app/package.json (1)
58-59: Consider adding build flags for reliability.The iOS build command could benefit from additional flags to improve CI reliability and reduce output noise:
- "test:e2e:ios": "xcodebuild -workspace ios/OpenPassport.xcworkspace -scheme OpenPassport -configuration Debug -sdk iphonesimulator -derivedDataPath ios/build && maestro test tests/e2e/launch.ios.flow.yaml", + "test:e2e:ios": "xcodebuild -workspace ios/OpenPassport.xcworkspace -scheme OpenPassport -configuration Debug -sdk iphonesimulator -derivedDataPath ios/build -quiet -parallelizeTargets && maestro test tests/e2e/launch.ios.flow.yaml",The
-quietflag reduces verbose output, and-parallelizeTargetscan speed up builds..github/workflows/mobile-e2e.yml (1)
240-245: Simplify exit code check pattern.Check the exit code directly for cleaner code:
echo "Installing app..." - xcrun simctl install "iPhone 15" "$APP_PATH" - if [ $? -ne 0 ]; then - echo "❌ iOS app installation failed" - exit 1 - fi + if ! xcrun simctl install "iPhone 15" "$APP_PATH"; then + echo "❌ iOS app installation failed" + exit 1 + fiapp/scripts/test-e2e-local.sh (1)
473-478: Select the newest build-tools deterministically
sort -ris lexicographic; “8.0.0” outranks “34.0.0”. Use version sort to reliably pick the latestaapt/aapt2.-AAPT_PATH=$(find "$ANDROID_HOME/build-tools" -type f -name "aapt" | sort -r | head -n 1) +AAPT_PATH=$(find "$ANDROID_HOME/build-tools" -type f -name "aapt" | sort -V | tail -n 1)Apply the same to the
aapt2lookup below.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
app/Gemfile.lockis excluded by!**/*.lockapp/ios/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
.cursorignore(0 hunks).github/actions/get-version/action.yml(1 hunks).github/actions/mobile-setup/action.yml(2 hunks).github/actions/yarn-install/action.yml(1 hunks).github/workflows/circuits-build.yml(2 hunks).github/workflows/circuits.yml(1 hunks).github/workflows/common-ci.yml(1 hunks).github/workflows/contracts.yml(1 hunks).github/workflows/mobile-bundle-analysis.yml(1 hunks).github/workflows/mobile-ci.yml(1 hunks).github/workflows/mobile-deploy-auto.yml(5 hunks).github/workflows/mobile-deploy.yml(2 hunks).github/workflows/mobile-e2e.yml(1 hunks).github/workflows/npm-publish.yml(4 hunks)AGENTS.md(3 hunks)README.md(3 hunks)app/.codecov.yml(2 hunks)app/.github/workflows/test-coverage.yml(2 hunks)app/.gitignore(1 hunks)app/.prettierrc(1 hunks)app/.yarnrc.yml(1 hunks)app/README.md(1 hunks)app/android/app/build.gradle(2 hunks)app/ios/PassportReader.swift(4 hunks)app/ios/Podfile(4 hunks)app/package.json(1 hunks)app/scripts/test-e2e-local.sh(1 hunks)app/src/utils/ethers.ts(1 hunks)app/tests/e2e/launch.android.flow.yaml(1 hunks)app/tests/e2e/launch.ios.flow.yaml(1 hunks)app/tests/utils/ethers.test.ts(1 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (1)
- .cursorignore
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{test,spec}.{ts,js,tsx,jsx}
⚙️ CodeRabbit Configuration File
**/*.{test,spec}.{ts,js,tsx,jsx}: Review test files for:
- Test coverage completeness
- Test case quality and edge cases
- Mock usage appropriateness
- Test readability and maintainability
Files:
app/tests/utils/ethers.test.ts
app/src/**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit Configuration File
app/src/**/*.{ts,tsx,js,jsx}: Review React Native TypeScript code for:
- Component architecture and reusability
- State management patterns
- Performance optimizations
- TypeScript type safety
- React hooks usage and dependencies
- Navigation patterns
Files:
app/src/utils/ethers.ts
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/app/build.gradle
app/ios/**/*
⚙️ CodeRabbit Configuration File
app/ios/**/*: Review iOS-specific code for:
- Platform-specific implementations
- Performance considerations
- Security best practices for mobile
Files:
app/ios/Podfileapp/ios/PassportReader.swift
🧠 Learnings (6)
📚 Learning: for this react native project, the team prefers build flexibility over fail-fast behavior for releas...
Learnt from: transphorm
PR: selfxyz/self#795
File: app/android/app/build.gradle:157-158
Timestamp: 2025-07-29T01:08:28.530Z
Learning: For this React Native project, the team prefers build flexibility over fail-fast behavior for release builds in app/android/app/build.gradle. They intentionally allow fallback to debug signing for local development runs, relying on Google Play Console validation to catch any improperly signed releases during upload.
Applied to files:
.github/workflows/mobile-ci.ymlapp/android/app/build.gradleapp/package.jsonapp/README.md.github/workflows/mobile-e2e.ymlapp/scripts/test-e2e-local.sh
📚 Learning: node.js 22 is supported by actions/setup-node@v4 and has been in active lts since october 2024. it w...
Learnt from: aaronmgdr
PR: selfxyz/self#763
File: app/.github/workflows/test-coverage.yml:0-0
Timestamp: 2025-07-14T09:03:08.292Z
Learning: Node.js 22 is supported by actions/setup-nodev4 and has been in Active LTS since October 2024. It works on Linux, macOS, and Windows runners (Windows issues were resolved after version 22.8.0).
Applied to files:
.github/actions/yarn-install/action.ymlapp/.github/workflows/test-coverage.yml.github/workflows/npm-publish.yml
📚 Learning: the import path '@selfxyz/common/utils/scope' is valid when the common package has a "./utils/*" exp...
Learnt from: transphorm
PR: selfxyz/self#795
File: app/src/screens/prove/ProveScreen.tsx:5-5
Timestamp: 2025-07-28T19:18:48.270Z
Learning: The import path 'selfxyz/common/utils/scope' is valid when the common package has a "./utils/*" export mapping in package.json, even if ESLint shows resolution errors before the package is built. The package.json exports field properly maps "./utils/*" to the compiled files in dist/.
Applied to files:
app/.codecov.yml
📚 Learning: when reviewing autogenerated scripts in xcode project files (like react native firebase's embedded s...
Learnt from: transphorm
PR: selfxyz/self#823
File: app/ios/Self.xcodeproj/project.pbxproj:320-332
Timestamp: 2025-08-02T23:53:45.928Z
Learning: When reviewing autogenerated scripts in Xcode project files (like React Native Firebase's embedded shell scripts), avoid suggesting edits since these are regenerated during pod install and cannot be manually modified by users.
Applied to files:
app/tests/e2e/launch.ios.flow.yaml.github/actions/mobile-setup/action.ymlapp/ios/Podfileapp/package.jsonapp/README.md.github/workflows/mobile-deploy-auto.ymlapp/ios/PassportReader.swift.github/workflows/mobile-e2e.ymlapp/scripts/test-e2e-local.sh
📚 Learning: react native 0.80 supports ios 15.1 as the minimum deployment target, not ios 16.0. this allows for ...
Learnt from: transphorm
PR: selfxyz/self#636
File: app/ios/Podfile:14-14
Timestamp: 2025-06-30T15:27:13.795Z
Learning: React Native 0.80 supports iOS 15.1 as the minimum deployment target, not iOS 16.0. This allows for broader device compatibility while still being compatible with the React Native 0.80 upgrade.
Applied to files:
app/ios/Podfile
📚 Learning: explicit export mappings in package.json exports field provide better tree shaking than wildcard pat...
Learnt from: transphorm
PR: selfxyz/self#823
File: common/package.json:21-60
Timestamp: 2025-08-01T17:11:39.232Z
Learning: Explicit export mappings in package.json exports field provide better tree shaking than wildcard patterns because they enable easier static analysis by bundlers, allowing for more precise dependency graph understanding and better dead code elimination.
Applied to files:
package.json
🪛 actionlint (1.7.7)
.github/workflows/circuits-build.yml
19-19: label "selfxyz-org" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
19-19: label "ubuntu-22-04" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
19-19: label "128ram" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
.github/workflows/mobile-deploy-auto.yml
29-29: shellcheck reported issue in this script: SC2086:info:6:33: Double quote to prevent globbing and word splitting
(shellcheck)
29-29: shellcheck reported issue in this script: SC2086:info:13:41: Double quote to prevent globbing and word splitting
(shellcheck)
29-29: shellcheck reported issue in this script: SC2086:info:16:39: Double quote to prevent globbing and word splitting
(shellcheck)
29-29: shellcheck reported issue in this script: SC2086:info:23:32: Double quote to prevent globbing and word splitting
(shellcheck)
29-29: shellcheck reported issue in this script: SC2086:info:26:32: Double quote to prevent globbing and word splitting
(shellcheck)
29-29: shellcheck reported issue in this script: SC2086:info:29:32: Double quote to prevent globbing and word splitting
(shellcheck)
29-29: shellcheck reported issue in this script: SC2086:info:32:32: Double quote to prevent globbing and word splitting
(shellcheck)
29-29: shellcheck reported issue in this script: SC2086:info:37:40: Double quote to prevent globbing and word splitting
(shellcheck)
29-29: shellcheck reported issue in this script: SC2086:info:38:30: Double quote to prevent globbing and word splitting
(shellcheck)
29-29: "github.event.pull_request.title" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions for more details
(expression)
.github/workflows/mobile-e2e.yml
225-225: shellcheck reported issue in this script: SC2181:style:16:6: Check exit code directly with e.g. 'if ! mycmd;', not indirectly with $?
(shellcheck)
🪛 RuboCop (1.76.1)
app/ios/Podfile
[convention] 157-159: Avoid more than 3 levels of block nesting.
(Metrics/BlockNesting)
🪛 YAMLlint (1.37.1)
.github/workflows/mobile-deploy-auto.yml
[error] 33-33: trailing spaces
(trailing-spaces)
🪛 Shellcheck (0.10.0)
app/scripts/test-e2e-local.sh
[warning] 415-415: i appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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). (1)
- GitHub Check: e2e-ios
🔇 Additional comments (23)
app/.yarnrc.yml (1)
2-2: LGTM – stylistic quote switch only
No functional impact; configuration remains correct..github/actions/get-version/action.yml (1)
17-17: Whitespace tidy-up acknowledged
Trailing space removed – good cleanup.README.md (1)
65-65: Minor markdown win
Angle-bracketed URL improves rendering; no further action needed.AGENTS.md (1)
1-57: LGTM! Documentation formatting improvements enhance readability.The added blank lines between sections and improved spacing make the documentation more scannable and easier to follow. The cleanup of the extra space in the test command comment is also good attention to detail.
app/src/utils/ethers.ts (1)
4-5: LGTM! Import order ensures proper ethers initialization.Moving the ethers import to the top with the ESLint disable is the correct approach to ensure ethers is available before registering the crypto polyfills. This follows the React Native cookbook pattern and prevents initialization order issues.
app/tests/utils/ethers.test.ts (1)
4-7: LGTM! Consistent import order and comprehensive test coverage.The import order change aligns with the source file changes, maintaining consistency. The test suite provides excellent coverage of all crypto polyfill functions with proper test vectors.
.github/actions/mobile-setup/action.yml (1)
55-103: LGTM! Clean formatting and robust dependency installation logic.The removal of extraneous blank lines improves readability while maintaining the excellent error handling and platform-specific logic. The detailed error messages for lockfile mismatches provide clear guidance for developers.
app/.prettierrc (1)
11-18: LGTM! Thoughtful YAML formatting configuration.Adding the YAML-specific override to use double quotes is excellent practice. This aligns with YAML conventions and ensures consistent formatting across CI/CD files while maintaining single quotes for TypeScript/JavaScript files.
app/.codecov.yml (1)
7-54: LGTM - Formatting standardization applied consistently.The quote consistency changes from single to double quotes improve YAML formatting standards across the repository without affecting any coverage behavior or ignore patterns.
app/android/app/build.gradle (2)
133-138: Smart optimization for E2E testing builds.Conditionally limiting ABI filters to x86_64 for E2E builds is an excellent performance optimization that reduces build time and APK size for emulator-based testing while maintaining full architecture support for production builds.
154-160: Appropriate fallback for flexible development workflow.The debug keystore fallback for release builds when upload store credentials aren't available supports the team's established preference for build flexibility over fail-fast behavior, particularly useful for CI/local E2E testing scenarios.
app/ios/Podfile (2)
30-33: Excellent hardware dependency management for E2E testing.Excluding NFCPassportReader during E2E testing is the right approach since NFC functionality requires physical hardware unavailable in simulators. This prevents build issues and reduces dependencies for testing.
86-98: Consistent dependency handling for E2E builds.Skipping OpenSSL bitcode stripping when NFCPassportReader is excluded maintains build consistency and avoids unnecessary processing during E2E testing scenarios.
.github/workflows/mobile-ci.yml (1)
1-1: Good naming standardization for workflow clarity.The name change from "App CI" to "Mobile CI" improves consistency with the new Mobile E2E workflow and provides clearer identification of mobile-specific CI processes.
.github/workflows/common-ci.yml (1)
1-1: Clear and concise workflow naming improvement.Changing from "General Self CI" to "Common CI" makes the workflow purpose more immediately clear and aligns with the repository-wide workflow naming standardization effort.
.github/workflows/circuits.yml (1)
1-1: Workflow rename & quoting look good.github/workflows/contracts.yml (1)
1-1: Consistent naming 👍.github/workflows/npm-publish.yml (1)
55-56: Consider upgrading the CI Node version / keep versions consistentOther workflows (e.g.
test-coverage.yml) now run on Node 22, while this one is still pinned to 18. Mixing major Node versions across jobs can surface “it works on CI A but not CI B” divergences (ES-modules flags, npm lockfile format, etc.).
Either bump to the current Active LTS (20) or 22, or document the reason this job must stay on 18.app/.github/workflows/test-coverage.yml (1)
25-26: Node-version drift vs. publish workflowNice to see tests on v22, but the publish workflow still targets v18. If the codebase eventually uses APIs only available since Node 20+, the publish job will silently fail.
Double-check that all workflows intentionally use different versions or align them.app/README.md (1)
356-359: 👍 Clear, platform-specific E2E instructionsThe new Maestro section makes it trivial for devs to run tests locally and references the correct flow files. Nice touch!
.github/workflows/mobile-deploy.yml (1)
1-1: LGTM! Clean formatting improvements.The workflow name change and trailing space removal improve consistency and code hygiene.
Also applies to: 12-16
app/ios/PassportReader.swift (1)
12-14: Well-structured conditional compilation for E2E testing.The conditional compilation approach cleanly separates production NFC functionality from the E2E testing stub. The stub implementation correctly matches the production interface and provides appropriate error messaging.
Also applies to: 17-29, 38-458
package.json (1)
13-15: Good separation of formatting concerns.The refactored formatting scripts provide better organization and ensure consistent YAML formatting across the codebase.
| set -e | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add -u -o pipefail for safer Bash execution
set -e alone won’t catch unset variables or pipeline failures. Hard-fail early on both to avoid silent mis-behaviour.
-set -e
+set -euo pipefail🤖 Prompt for AI Agents
In app/scripts/test-e2e-local.sh at lines 5 to 6, the script currently uses 'set
-e' which only stops execution on command errors but does not catch unset
variables or pipeline failures. Update the set command to 'set -euo pipefail' to
enable strict error handling by exiting on unset variables and any failure in a
pipeline, ensuring safer and more predictable script execution.
| # Use the dynamic simulator ID | ||
| SIMULATOR_ID="${IOS_SIMULATOR_ID:-iPhone 15}" | ||
| log_info "Installing on simulator: $SIMULATOR_ID" | ||
|
|
There was a problem hiding this comment.
Fallback to simulator name breaks simctl install
simctl expects a UDID. When $IOS_SIMULATOR_ID is missing the default "iPhone 15" causes:
An error was encountered processing the command (domain=NSPOSIXErrorDomain, code=2)
Fail fast instead of defaulting to an invalid identifier.
-SIMULATOR_ID="${IOS_SIMULATOR_ID:-iPhone 15}"
+if [ -z "$IOS_SIMULATOR_ID" ]; then
+ log_error "Simulator UDID not set – setup_ios_simulator must have failed."
+ exit 1
+fi
+SIMULATOR_ID="$IOS_SIMULATOR_ID"📝 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.
| # Use the dynamic simulator ID | |
| SIMULATOR_ID="${IOS_SIMULATOR_ID:-iPhone 15}" | |
| log_info "Installing on simulator: $SIMULATOR_ID" | |
| # Use the dynamic simulator ID | |
| if [ -z "$IOS_SIMULATOR_ID" ]; then | |
| log_error "Simulator UDID not set – setup_ios_simulator must have failed." | |
| exit 1 | |
| fi | |
| SIMULATOR_ID="$IOS_SIMULATOR_ID" | |
| log_info "Installing on simulator: $SIMULATOR_ID" |
🤖 Prompt for AI Agents
In app/scripts/test-e2e-local.sh around lines 286 to 289, the fallback value for
SIMULATOR_ID is set to a simulator name "iPhone 15" instead of a UDID, causing
simctl install to fail. Remove the default fallback and instead check if
IOS_SIMULATOR_ID is set; if not, exit the script immediately with an error
message to fail fast rather than using an invalid simulator identifier.
| # Set up trap to cleanup emulator on script exit | ||
| trap cleanup_android_emulator EXIT | ||
|
|
||
| # Only check Metro if not in workflow match mode | ||
| if [ "$WORKFLOW_MATCH" != "true" ]; then | ||
| check_metro_running | ||
| fi | ||
|
|
There was a problem hiding this comment.
Metro check still triggered for Android even though only Release APK is built
build_android_app always assembles a Release build (JS bundled ⇒ no Metro). Yet lines 602-604 abort when Metro isn’t running, causing needless failures for local runs that don’t pass --workflow-match.
- # Only check Metro if not in workflow match mode
- if [ "$WORKFLOW_MATCH" != "true" ]; then
- check_metro_running
- fi
+ # Release builds never need Metro; skip the check altogether
+ # If you ever add a Debug-build path, guard it explicitly.🤖 Prompt for AI Agents
In app/scripts/test-e2e-local.sh around lines 599 to 606, the script checks if
Metro is running even when only a Release APK is built, which does not require
Metro and causes unnecessary failures. Modify the condition to skip the Metro
check not only when WORKFLOW_MATCH is true but also when building a Release APK,
ensuring check_metro_running is only called when a Metro server is actually
needed.
| @@ -0,0 +1,6 @@ | |||
| appId: com.proofofpassportapp | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hard-coded appId reduces portability
Baking the bundle ID into the flow means the same YAML cannot be reused for debug / staging variants or white-label builds. Consider parameterising the value via ${MAESTRO_APP_ID} (Env var) in local runs / CI.
🤖 Prompt for AI Agents
In app/tests/e2e/launch.android.flow.yaml at line 1, the appId is hard-coded
which reduces portability across different environments. Replace the fixed appId
value with a parameterized environment variable reference like ${MAESTRO_APP_ID}
so that the YAML can be reused for debug, staging, or white-label builds by
setting the environment variable accordingly.
goal: give us confidence with the upcoming import / export refactor in
commonandsdkthis pull request contains the following:
Summary
Testing
yarn lintyarn buildyarn workspace @selfxyz/contracts build(fails: Invalid account for network)yarn typesyarn workspace @selfxyz/common testyarn workspace @selfxyz/circuits test(fails: Unsupported signature algorithm)yarn workspace @selfxyz/mobile-app testhttps://chatgpt.com/codex/tasks/task_b_6890145be06c832d8821ecc7b51e4e37
Summary by CodeRabbit
New Features
Chores
Documentation
Style