Conversation
- Fixed openssl issue - Fixed duplicate NFCPassportReader - missing struct issue
Contributor
📝 WalkthroughWalkthroughThe pull request migrates iOS NFC passport reading functionality from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
Contributor
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 (1)
app/ios/Podfile (1)
172-184:⚠️ Potential issue | 🟠 MajorRemove E2E_TESTING guard from OpenSSL bitcode stripping to prevent build failures in E2E testing.
OpenSSL-Universal is installed unconditionally (line 117) and DiditSDK depends on it regardless of build configuration. However, the bitcode stripping at line 173 only runs when
E2E_TESTINGis not set, causing iOS build failures when E2E runs. Unlike SelfNFCPassportReader (conditionally included), OpenSSL cannot be guarded by the same condition.Suggested fix
- # Only strip OpenSSL bitcode if SelfNFCPassportReader is included (not in e2e testing) - unless ENV["E2E_TESTING"] == "1" - framework_paths = [ - "Pods/OpenSSL-Universal/Frameworks/OpenSSL.xcframework/ios-arm64/OpenSSL.framework/OpenSSL", - "Pods/OpenSSL-Universal/Frameworks/OpenSSL.xcframework/ios-arm64_x86_64-maccatalyst/OpenSSL.framework/OpenSSL", - "Pods/OpenSSL-Universal/Frameworks/OpenSSL.xcframework/ios-arm64_x86_64-simulator/OpenSSL.framework/OpenSSL", - "Pods/OpenSSL-Universal/Frameworks/OpenSSL.xcframework/macos-arm64_x86_64/OpenSSL.framework/OpenSSL", - ] - - framework_paths.each do |framework_relative_path| - strip_bitcode_from_framework(bitcode_strip_path, framework_relative_path) - end - end + framework_paths = [ + "Pods/OpenSSL-Universal/Frameworks/OpenSSL.xcframework/ios-arm64/OpenSSL.framework/OpenSSL", + "Pods/OpenSSL-Universal/Frameworks/OpenSSL.xcframework/ios-arm64_x86_64-maccatalyst/OpenSSL.framework/OpenSSL", + "Pods/OpenSSL-Universal/Frameworks/OpenSSL.xcframework/ios-arm64_x86_64-simulator/OpenSSL.framework/OpenSSL", + "Pods/OpenSSL-Universal/Frameworks/OpenSSL.xcframework/macos-arm64_x86_64/OpenSSL.framework/OpenSSL", + ] + + framework_paths.each do |framework_relative_path| + next unless File.exist?(File.join(Dir.pwd, framework_relative_path)) + strip_bitcode_from_framework(bitcode_strip_path, framework_relative_path) + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/ios/Podfile` around lines 172 - 184, Remove the conditional guard around the OpenSSL bitcode stripping so it always runs during pod install even when ENV["E2E_TESTING"] == "1"; specifically, eliminate the surrounding "unless ENV[\"E2E_TESTING\"] == \"1\"" block and keep the framework_paths array and the loop that calls strip_bitcode_from_framework(bitcode_strip_path, framework_relative_path) intact in the Podfile so OpenSSL bitcode is stripped unconditionally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/ios/Podfile`:
- Around line 91-95: The Podfile currently sets nfc_repo_url to a non-existent
placeholder when !is_selfxyz_repo which breaks pod install; update the
nfc_repo_url assignment in the Podfile (search for is_selfxyz_repo and
nfc_repo_url) to either point to a real public NFCPassportReader git URL or read
from an explicit override environment variable (e.g. ENV["NFC_REPO_URL"]) and
fall back to the public repo; if neither a valid public URL nor
ENV["NFC_REPO_URL"] is present, raise a clear error explaining the required env
var so external forks/CI do not silently fail during pod install.
---
Outside diff comments:
In `@app/ios/Podfile`:
- Around line 172-184: Remove the conditional guard around the OpenSSL bitcode
stripping so it always runs during pod install even when ENV["E2E_TESTING"] ==
"1"; specifically, eliminate the surrounding "unless ENV[\"E2E_TESTING\"] ==
\"1\"" block and keep the framework_paths array and the loop that calls
strip_bitcode_from_framework(bitcode_strip_path, framework_relative_path) intact
in the Podfile so OpenSSL bitcode is stripped unconditionally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e9d7271-bbec-4d36-b55e-57c07fc54dc7
⛔ Files ignored due to path filters (2)
app/ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
app/ios/PassportReader.swiftapp/ios/PassportReaderCore.swiftapp/ios/Podfileapp/ios/SelfAnalytics.swiftapp/ios/local-pods/DiditSDK/DiditSDK.podspecapp/ios/local-pods/OpenSSL-Universal/OpenSSL-Universal.podspecapp/package.json
Comment on lines
91
to
95
| if !is_selfxyz_repo | ||
| # External fork - use public NFCPassportReader repository (placeholder) | ||
| # TODO: Replace with actual public NFCPassportReader repository URL | ||
| # External fork - use public SelfNFCPassportReader repository (placeholder) | ||
| # TODO: Replace with actual public SelfNFCPassportReader repository URL | ||
| nfc_repo_url = "https://github.com/PLACEHOLDER/NFCPassportReader.git" | ||
| elsif ENV["GITHUB_ACTIONS"] == "true" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '91,95p' app/ios/Podfile
printf '\nChecking the configured external-fork repository...\n'
git ls-remote https://github.com/PLACEHOLDER/NFCPassportReader.git HEADRepository: selfxyz/self
Length of output: 543
External-fork repo URL is broken and blocks pod install for any non-selfxyz CI run.
Line 94 points to https://github.com/PLACEHOLDER/NFCPassportReader.git, which does not exist. Any external fork or CI environment without the internal selfxyz repo will fail at pod install before the iOS build even starts.
Replace the placeholder with a real public repository URL or require an explicit environment variable override:
Suggested fix
- # External fork - use public SelfNFCPassportReader repository (placeholder)
- # TODO: Replace with actual public SelfNFCPassportReader repository URL
- nfc_repo_url = "https://github.com/PLACEHOLDER/NFCPassportReader.git"
+ nfc_repo_url = ENV["SELF_NFC_PASSPORT_READER_GIT_URL"]
+ raise "Set SELF_NFC_PASSPORT_READER_GIT_URL to a reachable SelfNFCPassportReader repository" if nfc_repo_url.to_s.empty?📝 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.
Suggested change
| if !is_selfxyz_repo | |
| # External fork - use public NFCPassportReader repository (placeholder) | |
| # TODO: Replace with actual public NFCPassportReader repository URL | |
| # External fork - use public SelfNFCPassportReader repository (placeholder) | |
| # TODO: Replace with actual public SelfNFCPassportReader repository URL | |
| nfc_repo_url = "https://github.com/PLACEHOLDER/NFCPassportReader.git" | |
| elsif ENV["GITHUB_ACTIONS"] == "true" | |
| if !is_selfxyz_repo | |
| nfc_repo_url = ENV["SELF_NFC_PASSPORT_READER_GIT_URL"] | |
| raise "Set SELF_NFC_PASSPORT_READER_GIT_URL to a reachable SelfNFCPassportReader repository" if nfc_repo_url.to_s.empty? | |
| elsif ENV["GITHUB_ACTIONS"] == "true" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/ios/Podfile` around lines 91 - 95, The Podfile currently sets
nfc_repo_url to a non-existent placeholder when !is_selfxyz_repo which breaks
pod install; update the nfc_repo_url assignment in the Podfile (search for
is_selfxyz_repo and nfc_repo_url) to either point to a real public
NFCPassportReader git URL or read from an explicit override environment variable
(e.g. ENV["NFC_REPO_URL"]) and fall back to the public repo; if neither a valid
public URL nor ENV["NFC_REPO_URL"] is present, raise a clear error explaining
the required env var so external forks/CI do not silently fail during pod
install.
Collaborator
Author
There was a problem hiding this comment.
Placeholder will be replaced by the CI
Contributor
There was a problem hiding this comment.
@seshanthS, understood — if the CI pipeline substitutes the actual URL into the placeholder before pod install runs, the build concern for CI is addressed. Just worth noting that contributors on external forks who try to run pod install locally (outside CI) would still hit the broken URL. A quick note in the README or CONTRIBUTING.md documenting the required environment variable or local setup step would help them avoid confusion.
Acknowledged the design decision either way. Thanks for clarifying!
🧠 Learnings used
Learnt from: transphorm
Repo: selfxyz/self PR: 1446
File: .github/workflows/mobile-bundle-analysis.yml:117-117
Timestamp: 2025-11-22T23:16:26.719Z
Learning: In the selfxyz/self repository, for mobile workflows (bundle analysis, deployment, CI/CD):
- Both iOS and Android builds now cache Ruby gems at the unified path `app/vendor/bundle`
- The previous separate paths (app/ios/vendor/bundle for iOS) have been deprecated in favor of this unified approach
Learnt from: transphorm
Repo: selfxyz/self PR: 942
File: app/vite.config.ts:170-0
Timestamp: 2025-08-23T02:02:02.556Z
Learning: In the selfxyz/self React Native app, devTools from '@/navigation/devTools' are intentionally shipped to production builds for testing purposes, not excluded as is typical in most applications.
Learnt from: transphorm
Repo: selfxyz/self PR: 1548
File: app/package.json:157-157
Timestamp: 2026-01-06T01:48:39.691Z
Learning: In the selfxyz/self repository, Fabric/New Architecture is explicitly disabled (newArchEnabled=false in app/android/gradle.properties), so dependencies that lack Fabric compatibility are acceptable.
Learnt from: transphorm
Repo: selfxyz/self PR: 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.
Learnt from: CR
Repo: selfxyz/self PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-23T20:57:39.003Z
Learning: Applies to packages/mobile-sdk-alpha/src/!(adapters/react-native)/**/*.{ts,tsx} : Do not import `react-native` in `packages/mobile-sdk-alpha/src/` outside of `src/adapters/react-native/`; keep SDK core platform-agnostic
Learnt from: CR
Repo: selfxyz/self PR: 0
File: app/AGENTS.md:0-0
Timestamp: 2026-03-23T20:58:32.442Z
Learning: For iOS development: Ensure Xcode scheme is set to 'OpenPassport'
Learnt from: CR
Repo: selfxyz/self PR: 0
File: app/AGENTS.md:0-0
Timestamp: 2026-03-23T20:58:32.442Z
Learning: Verify platform-specific code paths are tested (iOS/Android/Web) before PR submission
Learnt from: transphorm
Repo: selfxyz/self PR: 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.
Learnt from: CR
Repo: selfxyz/self PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-23T20:58:10.546Z
Learning: Flag security-sensitive changes for special review in PR description
Learnt from: CR
Repo: selfxyz/self PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-23T20:57:39.003Z
Learning: Every change to `mobile-sdk-alpha` must be backwards-compatible with the existing Self Wallet app
Learnt from: CR
Repo: selfxyz/self PR: 0
File: app/AGENTS.md:0-0
Timestamp: 2026-03-23T20:58:32.442Z
Learning: Verify native modules work correctly if native code was modified in PR
Learnt from: CR
Repo: selfxyz/self PR: 0
File: app/AGENTS.md:0-0
Timestamp: 2026-03-23T20:58:32.442Z
Learning: Complex native module changes must be documented in PR description
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
Native Consolidation Checklist
cd app && yarn jest:run/yarn workspace @selfxyz/rn-sdk-test-app test)Summary by CodeRabbit
Release Notes
Dependencies
Bug Fixes
Improvements