fix: android ci #549
fix: android ci #549
Conversation
WalkthroughAdd a shared constants workspace and new package, switch example imports to it, bump two GitHub Actions in an Android workflow, update a react-native-crypto patch to use react-native-get-random-values with a globalThis.crypto polyfill, and add passphrase UI controls in an example component. No public API signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Example App
participant Shared as @onekey-internal/shared-constants
participant CryptoPatch as react-native-crypto patch
participant RNGetRand as react-native-get-random-values
Note over App,Shared: Example imports shared constants
App->>Shared: import getConnectSrc()
Shared-->>App: CONNECT_SRC
Note over App,CryptoPatch: Crypto shim initialization
App->>CryptoPatch: import crypto shim
alt globalThis.crypto.getRandomValues exists
CryptoPatch-->>App: export crypto APIs (native)
else
CryptoPatch->>RNGetRand: import getRandomValues
RNGetRand-->>CryptoPatch: getRandomValues fn
CryptoPatch->>CryptoPatch: ensure globalThis.crypto and bind getRandomValues
CryptoPatch-->>App: export crypto APIs (polyfilled)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro Cache: Disabled due to data retention organization setting Knowledge Base: Disabled due to data retention organization setting 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (1)
✨ Finishing Touches🧪 Generate unit tests
🪧 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/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
ByteZhang1024
force-pushed
the
fix/androidCi
branch
from
07dac6e to
1e50f43
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
.github/workflows/build-example-android.yml (3)
13-16: Fix Node version mismatch (step name says 20, action installs 22).This can break CI and confuse readers. Align the value with the name. Node 20 is also safer for RN builds today.
Apply:
- - name: Use Node.js 20 - uses: actions/setup-node@v4 - with: - node-version: '22' + - name: Use Node.js 20 + uses: actions/setup-node@v4 + with: + node-version: '20'
19-23: Good upgrade to setup-java v4. Consider enabling its built-in Gradle cache.You can drop the separate cache step and let setup-java handle it.
Option A — add cache here:
- name: Set up JDK 17 uses: actions/setup-java@v4 with: java-version: '17' distribution: 'zulu' + cache: 'gradle'Then remove the dedicated “Cache Gradle packages” step below.
29-36: Prefer gradle/actions/setup-gradle for smarter caching.It’s more reliable than a generic cache step and cleans up old entries.
Replace this step with:
- - name: Cache Gradle packages - uses: actions/cache@v4 - with: - path: | - ~/.gradle/caches - ~/.gradle/wrapper - key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle*', '**/gradle-wrapper.properties') }} - restore-keys: | - ${{ runner.os }}-gradle- + - name: Setup Gradle with caching + uses: gradle/actions/setup-gradle@v4 + with: + gradle-home-cache-includes: | + caches + notifications + wrapper + gradle-home-cache-cleanup: truepatches/react-native-crypto+2.2.0.patch (1)
1-42: Action required — verify polyfill wiring & restore randomBytes compatibility before mergingShort: the patch replaces react-native-randombytes with react-native-get-random-values and sets globalThis.crypto.getRandomValues, but it removed the exported crypto.randomBytes API. That will break callers of crypto.randomBytes and the patch in /patches may not even be applied in CI.
Files/locations to inspect and fix
- ./packages/connect-examples/expo-example/package.json — react-native-get-random-values is present here (@^1.10.0). Add it to any packages that use react-native-crypto or to the repo root if the runtime needs it globally.
- ./package.json (repo root) — currently has no patch-package and no postinstall script (patches under /patches won't be applied). Add patch-package as a devDependency and run it in postinstall (or ensure CI applies patches).
- patches/react-native-crypto+2.2.0.patch — the changed file removed exports.randomBytes; restore a compatibility export or provide an explicit fallback.
- ./packages/connect-examples/expo-example/src/testTools/slip39Test/core/slip39Utils.ts — uses crypto.randomBytes() (multiple sites). Search repo for other crypto.randomBytes callers and update or keep compatibility.
- ./packages/connect-examples/expo-example/shim/crypto/index.native.js — this shim already requires react-native-get-random-values and reassigns randomBytes (so expo-example may still work). Other packages likely lack that shim.
Recommended, minimal fixes
- Ensure patch application: add patch-package to root devDependencies and a postinstall script to run it (or update CI).
- Restore a compatibility randomBytes export in the patched react-native-crypto index.js so crypto.randomBytes(...) continues to work. Example (suggested insertion into the patched file):
- import { getRandomValues } from 'react-native-get-random-values';
- function randomBytes(length) {
- const arr = new Uint8Array(length);
- (globalThis.crypto && globalThis.crypto.getRandomValues
? globalThis.crypto.getRandomValues: getRandomValues)(arr);- return (typeof Buffer !== 'undefined') ? Buffer.from(arr) : arr;
- }
- exports.randomBytes = exports.rng = exports.pseudoRandomBytes = exports.prng = randomBytes;
- Or, alternatively, add react-native-get-random-values to every package that will import react-native-crypto (so getRandomValues is present at runtime) and update callers to use globalThis.crypto.getRandomValues where feasible.
Why this is blocking
- Patches won't be applied in CI unless patch-package is configured at the repo root.
- Removing exports.randomBytes breaks existing code that expects crypto.randomBytes (we found concrete call sites).
- react-native-get-random-values is only declared in the expo-example package.json, not globally.
Next step
- Fix package.json (root or target packages) and either restore the randomBytes export in the patch or update all call sites. I can produce an updated patch snippet if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.github/workflows/build-example-android.yml(2 hunks)package.json(1 hunks)packages/connect-examples/expo-example/src/constants/connect.ts(1 hunks)packages/connect-examples/expo-playground/app/constants/connect.ts(1 hunks)packages/connect-examples/shared-constants/package.json(1 hunks)patches/react-native-crypto+2.2.0.patch(1 hunks)
🧰 Additional context used
🪛 ESLint
packages/connect-examples/expo-playground/app/constants/connect.ts
[error] 1-1: Resolve error: typescript with invalid interface loaded as resolver
(import/namespace)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-and-upload
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: build (22)
- GitHub Check: lint (22)
🔇 Additional comments (2)
package.json (1)
13-13: Workspace entry looks good.This will let Yarn/Lerna bootstrap the new shared-constants package.
packages/connect-examples/expo-example/src/constants/connect.ts (1)
2-4: Import switch to shared package looks good.Keeps the API the same while centralizing constants.
ByteZhang1024
force-pushed
the
fix/androidCi
branch
from
1e50f43 to
ca7b95f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
.github/workflows/build-example-android.yml (3)
13-16: Fix mismatch: step says Node 20 but installs Node 22.Rename the step (or set node-version to '20'). Also enable Yarn cache for faster installs.
- - name: Use Node.js 20 + - name: Use Node.js 22 uses: actions/setup-node@v4 with: - node-version: '22' + node-version: '22' + cache: 'yarn' + cache-dependency-path: 'yarn.lock'
18-23: Use setup-java’s built-in Gradle cache and drop the manual cache step.This simplifies the workflow and avoids duplicate caches.
- name: Set up JDK 17 - uses: actions/setup-java@v4 + uses: actions/setup-java@v4 with: java-version: '17' distribution: 'zulu' + cache: 'gradle' - - name: Cache Gradle packages - uses: actions/cache@v4 - with: - path: | - ~/.gradle/caches - ~/.gradle/wrapper - key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle*', '**/gradle-wrapper.properties') }} - restore-keys: | - ${{ runner.os }}-gradle-Also applies to: 28-36
12-12: Pin GitHub Actions to specific commit SHAsWe’ve found 33 floating refs across eight workflows. To harden our supply chain, replace every
uses: …@v3/@v4with the corresponding full commit SHA.Affected workflows and lines:
- release-web.yml: lines 12, 14, 43
- build-example-android.yml: lines 12, 14, 19, 29, 45
- package-publish.yml: lines 17, 44, 49
- lint.yml: lines 25, 28
- build-packages.yml: lines 25, 28
- github-release.yml: lines 40, 45
- build-example-web.yml: lines 12, 18, 50, 58, 64, 78, 88, 96, 101, 157
- build-example-desktop.yml: lines 11, 13, 33, 41, 49
Actions to pin:
- actions/checkout
- actions/setup-node
- actions/setup-java
- actions/cache
- actions/upload-artifact
- actions/download-artifact
- peaceiris/actions-gh-pages
Please update each occurrence to a fixed commit SHA (e.g.
uses: actions/checkout@<full-sha>).packages/connect-examples/expo-example/src/components/CommonParamsView.tsx (2)
12-14: Use functional state update to avoid stale writesMultiple quick edits can drop changes because
commonParamsis captured by closure. Use an updater.- const handleSetParam = (param: string, value: any) => { - setOptionalParams({ ...commonParams, [param]: value }); - }; + type CommonParamsT = typeof commonParams; + const handleSetParam = <K extends keyof CommonParamsT>(key: K, value: CommonParamsT[K]) => { + setOptionalParams((prev: CommonParamsT) => ({ ...prev, [key]: value })); + };
49-49: Prevent NaN from entering state; parse numbers safelyClearing the input sets
NaN, which then renders as “NaN”. Treat empty asundefinedand set radix.- onChange={value => handleSetParam('retryCount', parseInt(value))} + onChange={value => + handleSetParam('retryCount', value === '' ? undefined : Number.parseInt(value, 10)) + } @@ - onChange={value => handleSetParam('pollIntervalTime', parseInt(value))} + onChange={value => + handleSetParam('pollIntervalTime', value === '' ? undefined : Number.parseInt(value, 10)) + } @@ - onChange={value => handleSetParam('timeout', parseInt(value))} + onChange={value => + handleSetParam('timeout', value === '' ? undefined : Number.parseInt(value, 10)) + }Also applies to: 55-55, 61-61
♻️ Duplicate comments (6)
packages/connect-examples/shared-constants/package.json (1)
1-10: Expose types and an exports map to fix TS/ESLint resolution.Add types, exports, files, and mark sideEffects false. This unblocks the import in the example apps.
{ "name": "@onekey-internal/shared-constants", "version": "1.1.10-alpha.2", "private": true, "author": "OneKey", "homepage": "https://github.com/OneKeyHQ/hardware-js-sdk#readme", "license": "ISC", "type": "module", - "main": "constants.js" + "main": "constants.js", + "types": "constants.d.ts", + "exports": { + ".": { + "types": "./constants.d.ts", + "default": "./constants.js" + } + }, + "files": [ + "constants.js", + "constants.d.ts" + ], + "sideEffects": false }Add the declaration file:
// packages/connect-examples/shared-constants/constants.d.ts export function getConnectSrc(): string;#!/bin/bash # Verify the JS export exists rg -n "export\\s+(const|function)\\s+getConnectSrc\\b" packages/connect-examples/shared-constants/constants.*packages/connect-examples/expo-playground/app/constants/connect.ts (1)
1-1: Import path is correct; ensure shared-constants exposes types/exports.After updating the shared package as suggested, re-run lint to clear the “import/namespace” error.
#!/bin/bash yarn lint --max-warnings=0packages/connect-examples/expo-example/src/constants/connect.ts (1)
2-2: Remove obsolete ESLint disable above.The import is no longer relative; drop the suppression.
-// eslint-disable-next-line import/no-relative-packages import { getConnectSrc } from '@onekey-internal/shared-constants';patches/react-native-crypto+2.2.0.patch (3)
7-7: Avoid ESM import inside a CommonJS module; use side‑effect requirePrevents Metro/CI interop issues.
-'use strict' -import { getRandomValues } from 'react-native-get-random-values'; +'use strict' +// Ensure Web Crypto RNG is registered in RN (Hermes/JSC) +require('react-native-get-random-values');
9-28: Restoring randomBytes/rng/prng exports to avoid breaking consumersRemoving these is a breaking change. Reintroduce a Node‑compatible
randomBytesbuilt oncrypto.getRandomValues.- import { randomBytes } from 'react-native-randombytes' - exports.randomBytes = exports.rng = exports.pseudoRandomBytes = exports.prng = randomBytes + const { Buffer } = require('buffer'); + function randomBytes(size, cb) { + if (typeof size !== 'number' || size < 0) { + const err = new TypeError('size must be a non-negative number'); + if (typeof cb === 'function') { setTimeout(() => cb(err), 0); return; } + throw err; + } + const g = typeof globalThis === 'object' ? globalThis : (typeof global === 'object' ? global : {}); + const gv = g.crypto && typeof g.crypto.getRandomValues === 'function' ? g.crypto.getRandomValues : null; + const noRngErr = new Error('crypto.getRandomValues is unavailable. Ensure react-native-get-random-values is installed and loaded.'); + if (!gv) { + if (typeof cb === 'function') { setTimeout(() => cb(noRngErr), 0); return; } + throw noRngErr; + } + const MAX = 65536; // Web Crypto max per call + const bytes = new Uint8Array(size); + for (let off = 0; off < bytes.length; off += MAX) { + gv.call(g.crypto, bytes.subarray(off, Math.min(off + MAX, bytes.length))); + } + const buf = Buffer.from(bytes.buffer, bytes.byteOffset, bytes.byteLength); + if (typeof cb === 'function') { setTimeout(() => cb(null, buf), 0); return; } + return buf; + } + exports.randomBytes = exports.rng = exports.pseudoRandomBytes = exports.prng = randomBytes
29-36: Harden the global guard; attach polyfill defensivelySupport
globalThisandglobal, and only attach if available.-if (typeof globalThis === 'object') { - if (!globalThis.crypto) { - globalThis.crypto = {}; - } - if (!globalThis.crypto.getRandomValues) { - globalThis.crypto.getRandomValues = getRandomValues; - } -} +const g = typeof globalThis === 'object' ? globalThis : (typeof global === 'object' ? global : undefined); +if (g) { + if (!g.crypto) g.crypto = {}; + // Side-effect import should set this; if not, try to attach safely. + if (typeof g.crypto.getRandomValues !== 'function') { + try { + const maybe = require('react-native-get-random-values'); + const gv = maybe && (maybe.getRandomValues || (maybe.default && maybe.default.getRandomValues)); + if (typeof gv === 'function') g.crypto.getRandomValues = gv; + } catch (_) { + // Leave unset; randomBytes will throw a clear error. + } + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
.github/workflows/build-example-android.yml(2 hunks)package.json(1 hunks)packages/connect-examples/expo-example/src/components/CommonParamsView.tsx(1 hunks)packages/connect-examples/expo-example/src/constants/connect.ts(1 hunks)packages/connect-examples/expo-playground/app/constants/connect.ts(1 hunks)packages/connect-examples/shared-constants/package.json(1 hunks)patches/react-native-crypto+2.2.0.patch(1 hunks)
🧰 Additional context used
🪛 ESLint
packages/connect-examples/expo-playground/app/constants/connect.ts
[error] 1-1: Resolve error: typescript with invalid interface loaded as resolver
(import/namespace)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (22)
- GitHub Check: lint (22)
🔇 Additional comments (1)
package.json (1)
13-17: Workspace entry LGTM.New shared-constants workspace is correctly added.
packages/connect-examples/expo-example/src/components/CommonParamsView.tsx
Show resolved
Hide resolved
packages/connect-examples/expo-example/src/components/CommonParamsView.tsx
Show resolved
Hide resolved
Summary by CodeRabbit