[No QA] Add react-native-quick-crypto dependency - #97673
Conversation
|
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de1a7894f7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80a23f6bcc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@marufsharifi Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
| "react-native-plaid-link-sdk": "12.5.3", | ||
| "react-native-qrcode-svg": "6.3.21", | ||
| "react-native-quick-base64": "^3.0.1", | ||
| "react-native-quick-crypto": "^1.1.6", |
There was a problem hiding this comment.
Pin these native modules to exact versions (drop the ^).
"react-native-quick-base64": "^3.0.1",
"react-native-quick-crypto": "^1.1.6",Every other Nitro-based native module in this repo is pinned exactly:
react-native-nitro-modules": "0.36.3"
react-native-nitro-fetch": "1.5.4"
react-native-nitro-sqlite": "9.6.0"
quick-crypto is a Nitro module compiled against a specific NitroModules ABI and OpenSSL-Universal (~> 3.6.2000), and its exact resolutions are frozen in the committed Podfile.lock (QuickCrypto: 1.1.6, react-native-quick-base64: 3.0.1). With a caret range a future npm install can resolve 1.2.x/3.x, diverging from the committed checksums and causing a JS-vs-native mismatch / hard-to-diagnose native link failure. Suggest:
"react-native-quick-base64": "3.0.1",
"react-native-quick-crypto": "1.1.6",
There was a problem hiding this comment.
@JakubKorytko are you doing this? I am not sure if it's really needed, I see no value in using exact versions, given the lockflie exists anyway.
| AppAuth: d4f13a8fe0baf391b2108511793e4b479691fb73 | ||
| AppLogs: 3bc4e9b141dbf265b9464409caaa40416a9ee0e0 | ||
| boost: 7e761d76ca2ce687f7cc98e698152abd03a18f90 | ||
| boost: 659a89341ea4ab3df8259733813b52f26d8be9a5 |
There was a problem hiding this comment.
Please confirm the unrelated Podfile.lock churn is an intentional clean regen.
Beyond the expected OpenSSL-Universal / QuickCrypto / react-native-quick-base64 additions, this lock also bumps checksums unrelated to crypto:
- boost: 7e761d76… + boost: 659a8934…
- hermes-engine: b9fce683… + hermes-engine: 9411dff0…
- React-Core: 821522bb… + React-Core: 4b12e886…
- React-perflogger: 69b12c… + React-perflogger: 5bfe39e…
…and adds React-rendererconsistency to ~14 turbomodule specs. Adding a crypto pod doesn't produce these. It suggests the lock was regenerated against node_modules podspecs that differ from main. If it wasn't a clean "merge main + re-pod," CI's pod install can resolve different checksums than committed → spurious lockfile-dirty failures. Can you confirm main was merged and these 5 checksum changes are expected?
There was a problem hiding this comment.
Yes, this is intended clean-up. I could scrap the unrelated changes but then on each pod-install they would appear as not staged.
There was a problem hiding this comment.
How did this happen that the lockflie is out of date?
| "react-native-plaid-link-sdk": "12.5.3", | ||
| "react-native-qrcode-svg": "6.3.21", | ||
| "react-native-quick-base64": "^3.0.1", | ||
| "react-native-quick-crypto": "^1.1.6", |
There was a problem hiding this comment.
NAB: web has no native crypto.**
react-native-quick-crypto is iOS/Android-native only, but this App also ships web (react-native-web). Nothing imports the library in this diff, so there's no breakage today. But the follow-up that actually uses it must guard web with a platform-specific impl (.web.ts split / Platform check / JS crypto fallback), or the web bundle will fail to resolve the Nitro native module at import time. Flagging now so it isn't missed downstream.
There was a problem hiding this comment.
Yeah, thanks for bringing that up, we are aware and it is noted in design doc.
|
@MelvinBot Review |
ReviewClean, correctly-scoped foundational dependency add — No blockers. Two things worth a second look before merge: 1. The
Those aren't caused by adding a crypto library — they look like the lock picking up React Native drift from a 2. Version pinning uses caret ranges ( Notes I checked and think are fine
Web smoke test — signed-in Home renders normally |
roryabraham
left a comment
roryabraham
left a comment
There was a problem hiding this comment.
Merging since the Mobile-Expensify PR is merged already
|
🚧 roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.4.49-0 🚀
|
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.4.49-3 🚀
Bundle Size Analysis (Sentry): |
7 participants
Explanation of Change
Adds
react-native-quick-crypto(plus itsreact-native-quick-base64native runtime dependency) and updates the iOSPodfile.lock(App and Mobile-Expensify) vianpm run pod-install/npm run pod-install-standalone. Issue #96631 originally proposedexpo-crypto, but @roryabraham commented on Slack thatreact-native-quick-cryptois the better choice: it's Nitro-based (better performance), avoids further Expo lock-in, implements the full Node.js crypto spec (easier to swap out later), we have a closer relationship with Margelo than Expo, and it's Claude's first suggestion for the de-facto standard. This was further endorsed with 10 likes on a related Slack thread.Fixed Issues
$ #96631
PROPOSAL: N/A
Mobile-Expensify PR: https://github.com/Expensify/Mobile-Expensify/pull/14043
Tests
korytko/add-react-native-quick-cryptonpm installandnpm run pod-installnpm run ios)react-native-quick-crypto,react-native-quick-base64, or CocoaPodsOffline tests
N/A
QA Steps
N/A
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari